Skip to content

Downgrade missing required CSV column from exception to warning #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
claudear wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Downgrade missing required CSV column from exception to warning #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions src/Migration/Sources/CSV.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,26 +479,24 @@ private function validateCSVHeaders(array $headers, array $columnTypes, array $r

$messages = [];

// If there are missing required columns, throw an exception
// If there are missing required columns, log a warning (the database layer will enforce constraints per-row)
if (!empty($missingRequired)) {
$label = \count($missingRequired) === 1 ? 'Missing required column' : 'Missing required columns';
$messages[] = "$label: '" . \implode("', '", $missingRequired) . "'";
}
if (!empty($missingRequired)) {
throw new \Exception('CSV header validation failed: ' . \implode('. ', $messages), Exception::CODE_VALIDATION);
}

// If there are unknown columns but no missing required columns, just log a warning
// If there are unknown columns, log a warning
$unknown = \array_diff($headers, $allKnown, $internals);
if (!empty($unknown)) {
$label = \count($unknown) === 1 ? 'Unknown column' : 'Unknown columns';
$messages[] = "$label: '" . \implode("', '", $unknown) . "' (will be ignored)";
}
if (!empty($unknown)) {

if (!empty($messages)) {
$this->addWarning(new Warning(
UtopiaResource::TYPE_ROW,
Transfer::GROUP_DATABASES,
\implode(', ', $messages),
\implode('. ', $messages),
$this->resourceId
));
}
Expand Down
56 changes: 56 additions & 0 deletions tests/Migration/Unit/General/CSVTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,62 @@ public function testCSVExportImportCompatibility()
}
}

/**
* @throws \ReflectionException
*/
public function testValidateCSVHeadersMissingRequiredColumnIsWarning(): void
{
$reflection = new \ReflectionClass(CSV::class);
$instance = $reflection->newInstanceWithoutConstructor();

// Set resourceId to avoid uninitialized property error
$resourceIdProp = $reflection->getProperty('resourceId');
$resourceIdProp->setAccessible(true);
$resourceIdProp->setValue($instance, 'testdb:testtable');

$refMethod = $reflection->getMethod('validateCSVHeaders');
$refMethod->setAccessible(true);

$headers = ['name', 'age'];
$columnTypes = ['name' => 'string', 'age' => 'integer', 'texte' => 'string'];
$requiredColumns = ['texte' => true];

// Should NOT throw an exception — missing required columns should be a warning
$refMethod->invoke($instance, $headers, $columnTypes, $requiredColumns);

// Verify a warning was added
$this->assertNotEmpty($instance->warnings, 'A warning should be added for missing required columns');
$this->assertStringContainsString('texte', $instance->warnings[0]->getMessage());
}

/**
* @throws \ReflectionException
*/
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');
Comment on lines +450 to +472
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}

private function recursiveDelete(string $dir): void
{
if (is_dir($dir)) {
Expand Down
Loading