-
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
base: main
Are you sure you want to change the base?
Changes from all commits
ea6861c
f418802
33af097
b0586bd
cf11809
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) { | ||
|
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. what if not support for attributes? what is the issue coming then? check processException |
||
| 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 */ | ||
|
|
||
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