-
Notifications
You must be signed in to change notification settings - Fork 54
feat: Add preserveSequence flag for upserts #792
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.
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
premtsd-code
wants to merge
5
commits into
utopia-php:main
Choose a base branch
from
premtsd-code:feat-upsert-preserve-sequence
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+178
−2
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ea6861c
feat: Add preserveSequence flag for upserts
premtsd-code f418802
fix: Add getSupportForAttributes guard to test
premtsd-code 33af097
Merge branch 'main' into feat-upsert-preserve-sequence
premtsd-code b0586bd
refactor: Move test to DocumentTests, fix schemaless support
premtsd-code cf11809
Add setPreserveSequence to Mirror and test invalid sequence validation
premtsd-code File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1178,6 +1178,144 @@ public function testUpsertMixedPermissionDelta(): void | |
| ], $db->getDocument(__FUNCTION__, 'b')->getPermissions()); | ||
| } | ||
|
|
||
| public function testPreserveSequenceUpsert(): void | ||
| { | ||
| /** @var Database $database */ | ||
| $database = $this->getDatabase(); | ||
|
|
||
| if (!$database->getAdapter()->getSupportForUpserts()) { | ||
| $this->expectNotToPerformAssertions(); | ||
| return; | ||
| } | ||
|
|
||
| $collectionName = 'preserve_sequence_upsert'; | ||
|
|
||
| $database->createCollection($collectionName); | ||
|
|
||
| if ($database->getAdapter()->getSupportForAttributes()) { | ||
| $database->createAttribute($collectionName, 'name', Database::VAR_STRING, 128, true); | ||
| } | ||
|
|
||
| // Create initial documents | ||
| $doc1 = $database->createDocument($collectionName, new Document([ | ||
| '$id' => 'doc1', | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ], | ||
| 'name' => 'Alice', | ||
| ])); | ||
|
|
||
| $doc2 = $database->createDocument($collectionName, new Document([ | ||
| '$id' => 'doc2', | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ], | ||
| 'name' => 'Bob', | ||
| ])); | ||
|
|
||
| $originalSeq1 = $doc1->getSequence(); | ||
| $originalSeq2 = $doc2->getSequence(); | ||
|
|
||
| $this->assertNotEmpty($originalSeq1); | ||
| $this->assertNotEmpty($originalSeq2); | ||
|
|
||
| // Test: Without preserveSequence (default), $sequence should be ignored | ||
| $database->setPreserveSequence(false); | ||
|
|
||
| $database->upsertDocuments($collectionName, [ | ||
| new Document([ | ||
| '$id' => 'doc1', | ||
| '$sequence' => 999, // Try to set a different sequence | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ], | ||
| 'name' => 'Alice Updated', | ||
| ]), | ||
| ]); | ||
|
|
||
| $doc1Updated = $database->getDocument($collectionName, 'doc1'); | ||
| $this->assertEquals('Alice Updated', $doc1Updated->getAttribute('name')); | ||
| $this->assertEquals($originalSeq1, $doc1Updated->getSequence()); // Sequence unchanged | ||
|
|
||
| // Test: With preserveSequence=true, $sequence from document should be used | ||
| $database->setPreserveSequence(true); | ||
|
|
||
| $database->upsertDocuments($collectionName, [ | ||
| new Document([ | ||
| '$id' => 'doc2', | ||
| '$sequence' => $originalSeq2, // Keep original sequence | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ], | ||
| 'name' => 'Bob Updated', | ||
| ]), | ||
| ]); | ||
|
|
||
| $doc2Updated = $database->getDocument($collectionName, 'doc2'); | ||
| $this->assertEquals('Bob Updated', $doc2Updated->getAttribute('name')); | ||
| $this->assertEquals($originalSeq2, $doc2Updated->getSequence()); // Sequence preserved | ||
|
|
||
| // Test: withPreserveSequence helper | ||
| $database->setPreserveSequence(false); | ||
|
|
||
| $doc1 = $database->getDocument($collectionName, 'doc1'); | ||
| $currentSeq1 = $doc1->getSequence(); | ||
|
|
||
| $database->withPreserveSequence(function () use ($database, $collectionName, $currentSeq1) { | ||
| $database->upsertDocuments($collectionName, [ | ||
| new Document([ | ||
| '$id' => 'doc1', | ||
| '$sequence' => $currentSeq1, | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ], | ||
| 'name' => 'Alice Final', | ||
| ]), | ||
| ]); | ||
| }); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @premtsd-code can we add one more case here , where externally setting a very invalid sequence like 'abc' is throwing error |
||
| $doc1Final = $database->getDocument($collectionName, 'doc1'); | ||
| $this->assertEquals('Alice Final', $doc1Final->getAttribute('name')); | ||
| $this->assertEquals($currentSeq1, $doc1Final->getSequence()); | ||
|
|
||
| // Verify flag was reset after withPreserveSequence | ||
| $this->assertFalse($database->getPreserveSequence()); | ||
|
|
||
| // Test: With preserveSequence=true, invalid $sequence should throw error (SQL adapters only) | ||
| $database->setPreserveSequence(true); | ||
|
|
||
| try { | ||
| $database->upsertDocuments($collectionName, [ | ||
| new Document([ | ||
| '$id' => 'doc1', | ||
| '$sequence' => 'abc', // Invalid sequence value | ||
| '$permissions' => [ | ||
| Permission::read(Role::any()), | ||
| Permission::update(Role::any()), | ||
| ], | ||
| 'name' => 'Alice Invalid', | ||
| ]), | ||
| ]); | ||
| // Schemaless adapters may not validate sequence type, so only fail for schemaful | ||
| if ($database->getAdapter()->getSupportForAttributes()) { | ||
| $this->fail('Expected StructureException for invalid sequence'); | ||
| } | ||
| } catch (Throwable $e) { | ||
ArnabChatterjee20k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ($database->getAdapter()->getSupportForAttributes()) { | ||
| $this->assertInstanceOf(StructureException::class, $e); | ||
| $this->assertStringContainsString('sequence', $e->getMessage()); | ||
| } | ||
| } | ||
|
|
||
| $database->setPreserveSequence(false); | ||
| $database->deleteCollection($collectionName); | ||
| } | ||
|
|
||
| public function testRespectNulls(): Document | ||
| { | ||
| /** @var Database $database */ | ||
|
|
||
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a valid (UUID7-safe) sequence value in the “ignored” branch.
'$sequence' => 999can be invalid for UUID7 adapters; if validation happens before$sequenceis stripped, this test will fail or become adapter‑order dependent. Prefer a valid sequence (e.g., the other document’s sequence) to keep the test robust and still verify “ignore” behavior.🔧 Suggested adjustment
$originalSeq1 = $doc1->getSequence(); $originalSeq2 = $doc2->getSequence(); $this->assertNotEmpty($originalSeq1); $this->assertNotEmpty($originalSeq2); + $this->assertNotEquals($originalSeq1, $originalSeq2); // Test: Without preserveSequence (default), $sequence should be ignored $database->setPreserveSequence(false); $database->upsertDocuments($collectionName, [ new Document([ '$id' => 'doc1', - '$sequence' => 999, // Try to set a different sequence + '$sequence' => $originalSeq2, // Valid but different sequence '$permissions' => [ Permission::read(Role::any()), Permission::update(Role::any()), ], 'name' => 'Alice Updated', ]), ]);🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the setPreserveSequence is set false so getting ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArnabChatterjee20kTotally—preserveSequence=false means we intend to ignore $sequence. The concern is test robustness: some adapters may validate the payload before the stripping happens, and 999 is not a valid UUID7 sequence. Using a known-valid, different sequence keeps the test adapter-agnostic while still proving the “ignore” behavior.Proposed small tweak:
$originalSeq1 = $doc1->getSequence(); $originalSeq2 = $doc2->getSequence(); $this->assertNotEmpty($originalSeq1); $this->assertNotEmpty($originalSeq2); + $this->assertNotEquals($originalSeq1, $originalSeq2); // Test: Without preserveSequence (default), $sequence should be ignored $database->setPreserveSequence(false); $database->upsertDocuments($collectionName, [ new Document([ '$id' => 'doc1', - '$sequence' => 999, // Try to set a different sequence + '$sequence' => $originalSeq2, // Valid but different sequence; should still be ignored '$permissions' => [ Permission::read(Role::any()), Permission::update(Role::any()), ], 'name' => 'Alice Updated', ]), ]);This keeps the intent intact (sequence must remain $originalSeq1) and avoids adapter-order/format issues.
🧠 Learnings used