Skip to content

Refetch documents by $sequence#916

Open
fogelito wants to merge 5 commits into
mainfrom
refetch-documents-by-sequence
Open

Refetch documents by $sequence#916
fogelito wants to merge 5 commits into
mainfrom
refetch-documents-by-sequence

Conversation

@fogelito

@fogelito fogelito commented Jul 7, 2026

Copy link
Copy Markdown
Contributor
  • Refetch using primary key ($sequence)
  • Refetch using find method has a limit of default 25 rows max
  • Added chunking for large documents list
  • Refetch using find did not preserve select queries, always returning all attributes as doing select (*)
  • Fix decode issue, since find method already decoded documents

Summary by CodeRabbit

  • Bug Fixes

    • Improved operator-driven document refresh by re-matching updated results using each document’s sequence value.
    • Operator-computed updates now consistently preserve the original field projection, ensuring returned documents include only the expected attributes.
    • Operator refresh for single and batch updates now occurs after transaction commit to prevent timing-related inconsistencies.
  • Tests

    • Added end-to-end tests for operator updates with selected projections and for batch updates larger than the default limit.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Database now refetches operator-computed documents after commit, matches refreshed documents by $sequence, preserves selection context, and adds e2e coverage for select projections and larger batch updates.

Changes

Refetch and replay operator-computed documents

Layer / File(s) Summary
Refetch by sequence with selections
src/Database/Database.php
refetchDocuments() accepts selection context, queries by $sequence, and remaps refreshed documents back to the original input order.
Commit-time refetch wiring
src/Database/Database.php
updateDocument() records operator presence, moves refetching after commit, and skips the extra decode path when operator results are returned.
Batch refetch and e2e coverage
src/Database/Database.php, tests/e2e/Adapter/Scopes/OperatorTests.php
updateDocuments() forwards grouped selections into refetchDocuments(), skips the extra decode path for operator results, and new e2e tests cover select projections and large batch updates.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • utopia-php/database#907: Changes operator update handling in the adapter layer, which affects the operator-computed values this PR refetches.

Suggested reviewers: abnegate

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: refetching documents by $sequence.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refetch-documents-by-sequence

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes several bugs in the operator-driven document refetch path: switching from $id to $sequence as the lookup key, chunking large batches that previously hit find()'s 25-row default limit, preserving the caller's select projection, and skipping a redundant decode call since find() already decodes the returned documents.

  • refetchDocuments rewritten: uses Query::equal('$sequence', ...) instead of Query::equal('$id', ...), chunks the sequence list by maxQueryValues, and forwards the caller's $selections so the refetch honors the original projection.
  • Decode guard added: both updateDocument and updateDocuments now skip the post-write decode() call when operators were used, since find() inside refetchDocuments already returns fully decoded documents. Three targeted e2e tests cover the select-projection case, the >25-document batch case, and a non-idempotent-filter regression for double-decode.

Confidence Score: 5/5

Safe to merge; the core logic is correct for all current adapters and the three new e2e tests exercise the precise failure modes the PR addresses.

All known adapters handle the one remaining inconsistency (castingAfter called twice on refetched batch documents) idempotently — SQL's castingAfter is an identity function and Mongo's casts are safe to repeat. No data-corrupting path was found.

src/Database/Database.php — specifically the castingAfter call in the batch operator loop, which is architecturally inconsistent with the explicit decode guard added in the same PR.

Important Files Changed

Filename Overview
src/Database/Database.php Refetch now uses $sequence as the lookup key with chunking and selection preservation; decode is correctly skipped for the operator path; castingAfter is still applied twice in the batch operator case
tests/e2e/Adapter/Scopes/OperatorTests.php Three new e2e tests covering projection preservation during operator refetch, large-batch (>25) correctness, and the double-decode regression; all test patterns are sound

Reviews (5): Last reviewed commit: "check double decode test. Add refetch in..." | Re-trigger Greptile

Comment thread src/Database/Database.php

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Database/Database.php (1)

812-842: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Add an explicit limit to the refetch query. find() falls back to a 25-row limit when no Query::limit() is present, so bulk calls to refetchDocuments() can return stale pre-operator documents for everything past the first 25. Use Query::limit(count($sequences)) here, and chunk the sequence list if needed for adapter limits.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/Database/Database.php` around lines 812 - 842, The refetchDocuments()
flow currently calls find() with only a Query::equal('$sequence', $sequences)
filter, so it can silently stop at the default 25-result cap. Update the refetch
query in Database::refetchDocuments() to include
Query::limit(count($sequences)), and if the sequence list can exceed adapter
query limits, split the lookup into chunks and merge the results before
rebuilding the refetchedMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/Database/Database.php`:
- Around line 812-842: The refetchDocuments() flow currently calls find() with
only a Query::equal('$sequence', $sequences) filter, so it can silently stop at
the default 25-result cap. Update the refetch query in
Database::refetchDocuments() to include Query::limit(count($sequences)), and if
the sequence list can exceed adapter query limits, split the lookup into chunks
and merge the results before rebuilding the refetchedMap.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 91a5b9c5-ff4c-40bf-97eb-ffa188eb1459

📥 Commits

Reviewing files that changed from the base of the PR and between a090699 and 5803460.

📒 Files selected for processing (1)
  • src/Database/Database.php

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/Database/Database.php`:
- Around line 827-833: The refetch logic in refetchDocuments() is still relying
on the default find() limit, so batches larger than 25 can leave later documents
using stale computed/operator values. Update the query passed to
Database::find() to explicitly bound it to the current batch size (or the number
of requested sequence IDs) using Query::limit(), alongside the existing
Query::equal('$sequence', $sequences) and $selections so every document in the
batch is actually refetched.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0b45f00f-1ddd-49aa-b7e4-a4c42b4548ca

📥 Commits

Reviewing files that changed from the base of the PR and between 5803460 and c67b757.

📒 Files selected for processing (1)
  • src/Database/Database.php

Comment thread src/Database/Database.php Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants