Fix csv parser - #106
Fix csv parser#106
Conversation
added new key for the resources to have a parent child relationship # Conflicts: # src/Migration/Cache.php
updated the bug of resource key setting
CSV allow internals
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe CSV source export logic was updated to improve handling of required and unknown columns, array parsing, and value interpretation. Required columns are now explicitly tracked and enforced, unknown columns are logged as warnings instead of causing errors, and array-type fields use enhanced parsing with JSON and fallback mechanisms. Empty values are handled more precisely. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CSV
participant Logger
User->>CSV: Provide CSV file for export
CSV->>CSV: Track required columns
CSV->>CSV: Validate headers (enforce required, warn on unknown)
alt Unknown columns found
CSV->>Logger: Log warning
end
loop For each row
CSV->>CSV: Parse row
alt Array-type column
CSV->>CSV: Try JSON decode
alt JSON fails
CSV->>CSV: Fallback to comma-separated parsing
end
end
alt Unknown column in row
CSV->>CSV: Skip column
end
CSV->>CSV: Interpret empty values by type
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Migration/Sources/CSV.php (1)
250-264: Well-implemented array parsing with JSON support.The dual-format support with fallback is excellent for compatibility. Consider limiting the exposed value in the error message to prevent potential sensitive data leakage:
- throw new \Exception("Invalid array format for column '$key': $parsedValue"); + throw new \Exception("Invalid array format for column '$key'");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Migration/Sources/CSV.php(8 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: in the utopia-php/migration codebase, during the terminology swap from collection/attribute/document...
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: in the utopia-php/migration codebase, the `fromarray` method is not used on row objects, so mismatch...
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: in the utopia-php/migration codebase, during the terminology swap from collection/attribute/document...
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: in the utopia-php/migration codebase, the utopia database package does not have a memory adapter. wh...
Learnt from: abnegate
PR: utopia-php/migration#0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: in the utopia-php/migration codebase, invalid numeric values (like null) in parsed data are acceptab...
Learnt from: ItzNotABug
PR: utopia-php/migration#81
File: src/Migration/Sources/CSV.php:215-233
Timestamp: 2025-07-02T06:24:50.844Z
Learning: In the utopia-php/migration codebase, invalid numeric values (like null) in parsed data are acceptable because the underlying database structure validator will catch and handle these validation errors, so explicit filtering during parsing is not required.
Applied to files:
src/Migration/Sources/CSV.php
🧬 Code Graph Analysis (1)
src/Migration/Sources/CSV.php (2)
src/Migration/Resources/Database/Column.php (3)
isArray(113-116)isRequired(103-106)Column(8-151)src/Migration/Exception.php (1)
Exception(5-65)
🔇 Additional comments (6)
src/Migration/Sources/CSV.php (6)
5-5: LGTM!The Console import is correctly added and used for warning messages later in the code.
161-182: Good implementation of required column tracking.The use of array keys for storing required columns is efficient for O(1) lookups during validation.
197-203: Correct propagation of required columns.The $requiredColumns parameter is properly passed through the closure and to the validation method.
227-229: Good improvement to error handling.Skipping unknown columns instead of throwing errors makes the CSV import more resilient while still warning users during header validation.
278-295: Excellent handling of empty values vs nulls.The distinction between empty strings and null values based on column type is semantically correct and follows database best practices.
399-433: Well-designed validation logic.The distinction between critical errors (missing required columns) and warnings (unknown columns) is appropriate. The error messages are clear and properly pluralized.
# Conflicts: # src/Migration/Sources/CSV.php
# Conflicts: # src/Migration/Sources/CSV.php
# Conflicts: # composer.lock
# Conflicts: # src/Migration/Cache.php # src/Migration/Sources/CSV.php
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Migration/Sources/CSV.php (2)
246-260:json_decode()corner-case: literalnulltrips the array parserWhen the cell contains the JSON literal
null,json_decode()returnsnull
andjson_last_error()isJSON_ERROR_NONE, so the fallback branch is skipped.
is_array()then fails and an exception is thrown even though the value is
perfectly valid.Quick fix – treat
nullas an empty list:- if (!\is_array($arrayValues)) { - throw new \Exception("Invalid array format for column '$key': $parsedValue"); - } + if ($arrayValues === null) { + $arrayValues = []; + } + if (!\is_array($arrayValues)) { + throw new \Exception("Invalid array format for column '$key': $parsedValue"); + }This prevents unnecessary hard-stops on otherwise acceptable input.
406-409: Duplicate list of internal columns
validateCSVHeaders()declares its own$internalslist instead of re-using
self::ALLOWED_INTERNALS. Updating one but not the other will cause subtle
mismatches.- $internals = ['$id', '$permissions', '$createdAt', '$updatedAt']; + $internals = \array_keys(self::ALLOWED_INTERNALS);Keeps the list single-sourced and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
composer.json(1 hunks)src/Migration/Sources/CSV.php(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- composer.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
PR: utopia-php/migration#0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Applied to files:
src/Migration/Sources/CSV.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.
Applied to files:
src/Migration/Sources/CSV.php
🔇 Additional comments (1)
src/Migration/Sources/CSV.php (1)
522-526: Potential CSV parsing issue with identical enclosure and escape characters
str_getcsv($line, $delimiter, '"', '"')(and the analogousfgetcsvcall)
sets both enclosure and escape to". PHP treats the escape character
literally, so embedded quotes like"He said ""hi"""will no longer be parsed
correctly.Please confirm this deliberately deviates from RFC 4180; otherwise restore the
default backslash escape:- $fields = \str_getcsv($line, $delimiter, '"', '"'); + $fields = \str_getcsv($line, $delimiter, '"'); // escape char defaults to \
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Other Improvements