Downgrade missing required CSV column from exception to warning#159
Open
Downgrade missing required CSV column from exception to warning#159
Conversation
When a CSV file is missing columns marked as required in the database schema, the import now logs a warning instead of throwing an exception. This allows the import to proceed and lets the database layer enforce constraints per-row, rather than blocking the entire import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughThis pull request modifies CSV header validation in the migration source to shift from exception-based to warning-based signaling. Missing required headers and unknown columns are now logged as warnings instead of throwing exceptions, deferring constraint enforcement to the database layer. Additionally, two new unit tests are added to verify the warning behavior of the private validateCSVHeaders method using reflection. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Migration/Unit/General/CSVTest.php`:
- Around line 450-472: The test testValidateCSVHeadersAllRequiredPresent is too
loose: tighten it by initializing the instance->resourceId (like the other test)
before invoking validateCSVHeaders, and assert exactly that there are no
warnings emitted (e.g., assertEmpty or assertCount(0) on $instance->warnings)
rather than scanning for a specific message; this ensures any unexpected warning
(including unknown-header or other regressions) fails the test and avoids false
positives. Reference: CSV::validateCSVHeaders,
CSVTest::testValidateCSVHeadersAllRequiredPresent and the
instance->warnings/resourceId fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fe540a4-6ddf-47d2-90ba-265bf0dc7ef4
📒 Files selected for processing (2)
src/Migration/Sources/CSV.phptests/Migration/Unit/General/CSVTest.php
Comment on lines
+450
to
+472
| public function testValidateCSVHeadersAllRequiredPresent(): void | ||
| { | ||
| $reflection = new \ReflectionClass(CSV::class); | ||
| $instance = $reflection->newInstanceWithoutConstructor(); | ||
|
|
||
| $refMethod = $reflection->getMethod('validateCSVHeaders'); | ||
| $refMethod->setAccessible(true); | ||
|
|
||
| $headers = ['name', 'age', 'texte']; | ||
| $columnTypes = ['name' => 'string', 'age' => 'integer', 'texte' => 'string']; | ||
| $requiredColumns = ['texte' => true]; | ||
|
|
||
| // Should not throw and not add warnings for required columns | ||
| $refMethod->invoke($instance, $headers, $columnTypes, $requiredColumns); | ||
|
|
||
| // No warnings about missing required columns (may have unknown column warnings) | ||
| $hasRequiredWarning = false; | ||
| foreach ($instance->warnings as $warning) { | ||
| if (str_contains($warning->getMessage(), 'Missing required')) { | ||
| $hasRequiredWarning = true; | ||
| } | ||
| } | ||
| $this->assertFalse($hasRequiredWarning, 'No warning should be added when all required columns are present'); |
Contributor
There was a problem hiding this comment.
Tighten this “no false warnings” test.
With these inputs there are no unknown headers either, so any warning is a regression. The current assertion would still pass if validateCSVHeaders() started emitting some other warning. Also, initialize resourceId here like the previous test so an unexpected warning path fails as an assertion, not via uninitialized state.
Suggested test hardening
public function testValidateCSVHeadersAllRequiredPresent(): void
{
$reflection = new \ReflectionClass(CSV::class);
$instance = $reflection->newInstanceWithoutConstructor();
+
+ $resourceIdProp = $reflection->getProperty('resourceId');
+ $resourceIdProp->setAccessible(true);
+ $resourceIdProp->setValue($instance, 'testdb:testtable');
$refMethod = $reflection->getMethod('validateCSVHeaders');
$refMethod->setAccessible(true);
$headers = ['name', 'age', 'texte'];
$columnTypes = ['name' => 'string', 'age' => 'integer', 'texte' => 'string'];
$requiredColumns = ['texte' => true];
- // Should not throw and not add warnings for required columns
+ // Should not throw and should not add warnings
$refMethod->invoke($instance, $headers, $columnTypes, $requiredColumns);
- // No warnings about missing required columns (may have unknown column warnings)
- $hasRequiredWarning = false;
- foreach ($instance->warnings as $warning) {
- if (str_contains($warning->getMessage(), 'Missing required')) {
- $hasRequiredWarning = true;
- }
- }
- $this->assertFalse($hasRequiredWarning, 'No warning should be added when all required columns are present');
+ $this->assertEmpty($instance->warnings, 'No warning should be added when all headers are valid');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Migration/Unit/General/CSVTest.php` around lines 450 - 472, The test
testValidateCSVHeadersAllRequiredPresent is too loose: tighten it by
initializing the instance->resourceId (like the other test) before invoking
validateCSVHeaders, and assert exactly that there are no warnings emitted (e.g.,
assertEmpty or assertCount(0) on $instance->warnings) rather than scanning for a
specific message; this ensures any unexpected warning (including unknown-header
or other regressions) fails the test and avoids false positives. Reference:
CSV::validateCSVHeaders, CSVTest::testValidateCSVHeadersAllRequiredPresent and
the instance->warnings/resourceId fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
requiredin the database schema (e.g., a column namedtexte), the CSV importer was throwing a hard exception that blocked the entire import and generated Sentry errors.validateCSVHeadersto log a warning instead of throwing an exception for missing required columns. The import proceeds and the database layer enforces constraints per-row, providing better error granularity.Fixes: https://appwrite.sentry.io/issues/7326827048/
Test plan
testValidateCSVHeadersMissingRequiredColumnIsWarningverifies missing required columns produce a warning (not an exception)testValidateCSVHeadersAllRequiredPresentverifies no false warnings when all required columns exist🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests