removed schema enforcement during createRow in destination #122
Conversation
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
Contributor
WalkthroughThe composer.json require constraint for the package "utopia-php/database" was changed from "4." to "3.". No other files or code changes are present in this diff. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
composer.json(1 hunks)src/Migration/Destinations/Appwrite.php(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
Learnt from: abnegate
Repo: utopia-php/migration PR: 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.
Learnt from: abnegate
Repo: utopia-php/migration PR: 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.
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
Repo: utopia-php/migration PR: 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:
composer.json
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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/Destinations/Appwrite.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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/Destinations/Appwrite.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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/Destinations/Appwrite.php
🔇 Additional comments (1)
composer.json (1)
29-29: Clarify and document the development branch dependency.The
utopia-php/databasedependency uses dev branchdat-851(pinned to today's commit) while all other dependencies use stable semantic versions. This lacks any documentation explaining the rationale or when it will transition to a stable release.Before merging:
- Clarify whether this dependency is intentional and required for this release, or if it's temporary pending PR #751 finalization
- Document the reason in your PR description or commit message
- Specify the timeline for updating to a stable version, or confirm this is the planned release state
Comment on lines
952
to
954
| // bypassing schema constraints to create data without enforcing schema limits | ||
| // data created in source should be in destination | ||
| $this->database->getAdapter()->setSupportForAttributes(false); |
Contributor
There was a problem hiding this comment.
Critical: Inconsistent schema enforcement across batches.
Schema validation is only bypassed when $isLast is true, meaning only the final batch of rows avoids schema checks. If earlier batches contain null values for required columns, they will fail validation while the last batch will not. This creates unpredictable migration behavior.
Consider one of these approaches:
- Apply consistently to all rows: Disable schema enforcement for the entire
createRowmethod, not just the last batch.
protected function createRow(Row $resource, bool $isLast): bool
{
+ static $schemaDisabled = false;
+
+ if (!$schemaDisabled) {
+ $this->database->getAdapter()->setSupportForAttributes(false);
+ $schemaDisabled = true;
+ }
+
if ($resource->getId() == 'unique()') {Then restore it when the transfer completes (outside this method).
- Validate at source: Add validation at the source to ensure data meets destination schema requirements before migration begins, eliminating the need to bypass schema checks.
Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
998
to
997
| } finally { | ||
| $this->database->getAdapter()->setSupportForAttributes(true); | ||
| $this->rowBuffer = []; | ||
| } |
Contributor
There was a problem hiding this comment.
Data loss risk: rowBuffer cleared on failure.
The rowBuffer is cleared in the finally block regardless of success or failure. If createDocuments throws an exception (lines 993-996), the buffered rows are lost and won't be retried, resulting in incomplete migration without clear indication.
Consider this safer approach:
} finally {
$this->database->getAdapter()->setSupportForAttributes(true);
- $this->rowBuffer = [];
}
+
+ // Only clear buffer after successful creation
+ $this->rowBuffer = [];
}Alternatively, add explicit error handling to mark failed rows:
try {
// ... existing code ...
$this->database->skipRelationshipsExistCheck(fn () => $this->database->createDocuments(
'database_' . $databaseInternalId . '_collection_' . $tableInternalId,
$this->rowBuffer
));
+
+ $this->rowBuffer = [];
} finally {
$this->database->getAdapter()->setSupportForAttributes(true);
- $this->rowBuffer = [];
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Migration/Destinations/Appwrite.php around lines 998 to 1001, the finally
block unconditionally clears $this->rowBuffer which discards buffered rows when
createDocuments throws; instead only clear the buffer after a successful write
or explicitly record/mark failed rows for retry. Change the flow so that
setSupportForAttributes(true) remains in finally, but move $this->rowBuffer = []
into the success path (after createDocuments completes), or on exception push
the failed rows into a retry/failed list and log the error before rethrowing;
ensure exceptions are not swallowed so failures are visible and retriable.
ArnabChatterjee20k
force-pushed
the
dat-851
branch
from
4c2edfd to
f9ee52b
Compare
Contributor
Author
|
closing this (not required) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Allow inserting null for required columns on restoration
Because it must have come from the original data, we should allow inserting it
Screencast.from.2025-11-04.11-33-39.webm
Summary by CodeRabbit