feat: Add preserveSequence flag for upserts #792
Conversation
- Add preserveSequence property to Database class - Add getPreserveSequence(), setPreserveSequence(), withPreserveSequence() methods - Conditionally remove $sequence in upsertDocuments based on flag - Add testPreserveSequenceUpsert test in GeneralTests.php
📝 WalkthroughWalkthroughThe changes introduce a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 305-314: The test currently only guards using
getSupportForUpserts(), but then calls createAttribute() which will fail on
schemaless adapters; update the guard to also check the adapter's attribute
support (e.g., call getAdapter()->getSupportForAttributes() or equivalent) and
if attributes aren't supported call $this->expectNotToPerformAssertions() and
return before invoking createAttribute(); ensure the new check is placed before
createAttribute() and references the existing getSupportForUpserts(),
createCollection(), and createAttribute() symbols so the test skips for adapters
that support upserts but not attributes.
|
|
||
| public function testPreserveSequenceUpsert(): void | ||
| { | ||
| $this->getDatabase()->getAuthorization()->disable(); |
There was a problem hiding this comment.
I dont think authorization here is required to disable
|
|
||
| $database->deleteCollection($collectionName); | ||
|
|
||
| $this->getDatabase()->getAuthorization()->reset(); |
There was a problem hiding this comment.
same here like the above
| return; | ||
| } | ||
|
|
||
| if (!$database->getAdapter()->getSupportForAttributes()) { |
There was a problem hiding this comment.
we should not skip this case here.
For context -> getSupportForAttributes() controls between schemaful and schemaless in mongodb
And mongodb supports sequence in both
There was a problem hiding this comment.
lets move them in the e2e/documenttests.php
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 1218-1237: The test uses an invalid literal sequence 999 which can
fail UUID7 adapters; change the upsert payload in the ignored-branch to supply a
valid sequence value (e.g., reuse $originalSeq1 or $originalSeq2) instead of 999
so validation passes even though setPreserveSequence(false) should ignore it;
update the Document creation passed to Database::upsertDocuments (the new
Document([... '$sequence' => ...])) to use the captured
$originalSeq1/$originalSeq2 while keeping the rest of the assertions intact.
- Around line 1225-1290: The test mutates the shared preserveSequence flag via
Database::setPreserveSequence() and leaves it false, risking cross-test state
bleed; modify the test to capture the prior state by calling $previous =
$database->getPreserveSequence() before changing it, wrap the operations that
change the flag (setPreserveSequence, withPreserveSequence, upsertDocuments,
deleteCollection) in a try/finally, and in the finally restore the original
value via $database->setPreserveSequence($previous) so the flag is always reset
even on assertion failures.
| $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', | ||
| ]), | ||
| ]); |
There was a problem hiding this comment.
Use a valid (UUID7-safe) sequence value in the “ignored” branch.
'$sequence' => 999 can be invalid for UUID7 adapters; if validation happens before $sequence is 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
In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 1218 - 1237, The
test uses an invalid literal sequence 999 which can fail UUID7 adapters; change
the upsert payload in the ignored-branch to supply a valid sequence value (e.g.,
reuse $originalSeq1 or $originalSeq2) instead of 999 so validation passes even
though setPreserveSequence(false) should ignore it; update the Document creation
passed to Database::upsertDocuments (the new Document([... '$sequence' => ...]))
to use the captured $originalSeq1/$originalSeq2 while keeping the rest of the
assertions intact.
There was a problem hiding this comment.
the setPreserveSequence is set false so getting ignored
There was a problem hiding this comment.
@ArnabChatterjee20k Totally—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:
- Ensure the two initial sequences differ.
- Provide a valid-but-different sequence in the “ignored” branch.
$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
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Learnt from: fogelito
Repo: utopia-php/database PR: 733
File: src/Database/Adapter/MariaDB.php:1801-1806
Timestamp: 2025-10-16T09:37:33.531Z
Learning: In the MariaDB adapter (src/Database/Adapter/MariaDB.php), only duplicate `_uid` violations should throw `DuplicateException`. All other unique constraint violations, including `PRIMARY` key collisions on the internal `_id` field, should throw `UniqueException`. This is the intended design to distinguish between user-facing document duplicates and internal/user-defined unique constraint violations.
| $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, $sequence should be preserved | ||
| $database->setPreserveSequence(true); | ||
|
|
||
| $database->upsertDocuments($collectionName, [ | ||
| new Document([ | ||
| '$id' => 'doc2', | ||
| '$sequence' => $originalSeq2, // Explicitly set the same 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', | ||
| ]), | ||
| ]); | ||
| }); | ||
|
|
||
| $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()); | ||
|
|
||
| $database->setPreserveSequence(false); | ||
| $database->deleteCollection($collectionName); |
There was a problem hiding this comment.
Restore preserveSequence to its prior state to avoid cross‑test bleed.
The test mutates a shared flag and leaves it false. If another test depends on a different prior value, this can leak state. Capture the previous value and restore it (ideally in finally) so failures don’t cascade.
🔧 Suggested adjustment
- // Test: Without preserveSequence (default), $sequence should be ignored
- $database->setPreserveSequence(false);
+ $previousPreserveSequence = $database->getPreserveSequence();
+ try {
+ // Test: Without preserveSequence (default), $sequence should be ignored
+ $database->setPreserveSequence(false);
$database->upsertDocuments($collectionName, [
new Document([
'$id' => 'doc1',
'$sequence' => $originalSeq2, // Valid but different sequence
@@
// Verify flag was reset after withPreserveSequence
$this->assertFalse($database->getPreserveSequence());
- $database->setPreserveSequence(false);
- $database->deleteCollection($collectionName);
+ } finally {
+ $database->setPreserveSequence($previousPreserveSequence);
+ }
+ $database->deleteCollection($collectionName);🤖 Prompt for AI Agents
In `@tests/e2e/Adapter/Scopes/DocumentTests.php` around lines 1225 - 1290, The
test mutates the shared preserveSequence flag via
Database::setPreserveSequence() and leaves it false, risking cross-test state
bleed; modify the test to capture the prior state by calling $previous =
$database->getPreserveSequence() before changing it, wrap the operations that
change the flag (setPreserveSequence, withPreserveSequence, upsertDocuments,
deleteCollection) in a try/finally, and in the finally restore the original
value via $database->setPreserveSequence($previous) so the flag is always reset
even on assertion failures.
There was a problem hiding this comment.
false is the default state
There was a problem hiding this comment.
@ArnabChatterjee20k Totally—today’s default is false. The concern is isolation and failure-safety: this test flips the flag to true mid-way. If an assertion throws before the last reset, preserveSequence could leak as true into subsequent tests that reuse the same Database instance. Capturing the prior value and restoring it in a finally block prevents that and also future-proofs the test if the default ever changes.
Here’s a minimal, failure-safe tweak that keeps the test logic identical while guaranteeing restoration:
@@
- // Test: Without preserveSequence (default), $sequence should be ignored
- $database->setPreserveSequence(false);
+ $previousPreserveSequence = $database->getPreserveSequence();
+ try {
+ // Test: Without preserveSequence (default), $sequence should be ignored
+ $database->setPreserveSequence(false);
@@
- // Verify flag was reset after withPreserveSequence
- $this->assertFalse($database->getPreserveSequence());
-
- $database->setPreserveSequence(false);
- $database->deleteCollection($collectionName);
+ // Verify flag was reset after withPreserveSequence
+ $this->assertFalse($database->getPreserveSequence());
+ } finally {
+ // Always restore whatever the flag was before this test started
+ $database->setPreserveSequence($previousPreserveSequence);
+ }
+ $database->deleteCollection($collectionName);Benefits:
- Prevents cross-test bleed on early failures.
- Keeps the assertion that withPreserveSequence resets to the pre-call value (false within this test).
- Makes the test robust to future default changes and suite reordering.
If you prefer to keep it as-is, I’ll drop the suggestion. Otherwise, I can prep a follow-up patch with this change.
🧠 Learnings used
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
premtsd-code
force-pushed
the
feat-upsert-preserve-sequence
branch
from
b091840 to
b0586bd
Compare
| ]), | ||
| ]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
@premtsd-code can we add one more case here , where externally setting a very invalid sequence like 'abc' is throwing error
- Add setPreserveSequence delegation to Mirror.php - Add test case for invalid $sequence value throwing StructureException
premtsd-code
force-pushed
the
feat-upsert-preserve-sequence
branch
from
faf92c8 to
cf11809
Compare
| if ($database->getAdapter()->getSupportForAttributes()) { | ||
| $this->fail('Expected StructureException for invalid sequence'); | ||
| } | ||
| } catch (Throwable $e) { |
There was a problem hiding this comment.
what if not support for attributes? what is the issue coming then? check processException
2 participants
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.