feat(sync)!: URL/query template mechanism for sync endpoints - #116
Open
Gabriel-Pereira1788 wants to merge 12 commits into
Open
feat(sync)!: URL/query template mechanism for sync endpoints#116Gabriel-Pereira1788 wants to merge 12 commits into
Gabriel-Pereira1788 wants to merge 12 commits into
Conversation
New minimal, non-RFC6570 `{token}` parser/renderer (cpp/http/UrlTemplate)
with a closed vocabulary per context: item paths see {basePath}/{id},
list queries see {since}/{limit}/{cursorField}. Handles {{ }} escaping,
rejects illegal raw characters (quotes, angle brackets, backtick, control
chars, '#', non-ASCII) at parse time, and percent-encodes substituted
values except {basePath} (structural, schema-author-controlled, may
legitimately contain '/').
HttpUrlBuilder gains a build(baseUrl, path, renderedQuery) overload that
appends an already-rendered query string verbatim; the old QueryParams-pair
overload is removed now that nothing constructs a query from loose pairs.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
SyncEndpoint drops the sinceParam/limitParam string pair in favor of
itemPathTemplate (optional, defaults to "{basePath}/{id}") and
listQueryTemplate (required) — both UrlTemplate instances parsed once in
SyncContract::fromDefinition, rendered per call in SyncHttpCaller's
list()/update()/remove().
Covers shapes the old two-loose-params model couldn't express: composed
filters ($filter=updatedAt gt {since}&$top={limit}) and non-slash item
addressing ({basePath}({id})).
Validation closes two gaps a naive port would leave open: itemPathTemplate
must reference {id} (or PATCH/DELETE silently target the collection route)
and listQueryTemplate must reference both {since} and {limit} (or pull
either never filters or the tied-timestamp escalation loop silently stalls
every session). A pre-#115 persisted _salve_sync_definitions row (legacy
sinceParam/limitParam, no listQueryTemplate) is synthesized into an
equivalent template rather than rejected, since the headless background-wake
path reads that row before JS ever gets to re-register the schema.
BREAKING CHANGE: sync.endpoint.sinceParam/limitParam are removed. Existing
schemas must declare listQueryTemplate explicitly, e.g.
listQueryTemplate: "updatedAfter={since}&limit={limit}".
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
MigrationEngine::parseSchemaJson now calls SyncContract::fromDefinition for the sync block instead of re-implementing conflict.strategy validation by hand — the two were already silently duplicating that check. This also makes register() validate endpoint templates eagerly (previously deferred to the first triggerSync). The lastWriteWins column check stays in MigrationEngine: it's the one check that needs schema.columns, which SyncContract has no access to. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Mechanical: every sync-enabled fixture across the cpp test suite that
declared endpoint.sinceParam/limitParam (or a bare "sync": {"enabled":
true} relying on register()-time validation not touching endpoint) now
declares listQueryTemplate, and gets an endpoint block where it was missing
one entirely — required now that MigrationEngine eagerly validates the
full sync contract at register() time.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Mirrors the native contract: IEndpointDefinition drops sinceParam/
limitParam in favor of itemPathTemplate?/listQueryTemplate, matching
cpp/http/UrlTemplate's {token} vocabulary. IPaginationDefinition.pageSize's
doc comment updated to reference the new {limit} token instead of the
removed <limitParam> placeholder.
BREAKING CHANGE: sync.endpoint.sinceParam/limitParam are removed from
IEndpointDefinition. See the sync/SyncContract native commit for the
equivalent runtime change.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Mechanical, following the IEndpointDefinition breaking change. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
UserSchema/ProductSchema and the two inline harness fixtures that declared endpoint config move to listQueryTemplate, keeping each module's distinct query-param naming (updatedAfter/limit vs. modified_since/page_size) as a listQueryTemplate string instead of two separate fields — still proving per-module config, now via #115's mechanism. Podfile.lock/project.pbxproj regenerated (`pod install`) to pick up the new cpp/http/UrlTemplate.cpp source file via SalveDb.podspec's glob. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Adds a breaking-change addendum to sync-rest-contract.md pointing at the new UrlTemplate mechanism, and updates the interface blocks, REST table, and full example in that doc plus the README quickstart to itemPathTemplate/listQueryTemplate. The doc's own historical #84 narrative is left as-is (marked as superseded by the addendum) rather than rewritten line by line. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
with-harness-server.mjs's cleanup only signaled the spawned shell wrapper, not the grandchild `tsx src/index.ts` process actually holding port 4000 — detached: true plus a process-group SIGTERM (and a bounded SIGKILL escalation) reaches it. stopServer() now awaits the real exit instead of racing process.exit() against a SIGTERM that pglite/tsx may take a moment to honor, and both spawned children get 'error' listeners so a failed spawn surfaces as a clean message instead of an uncaught exception that skips cleanup entirely. The hand-rolled fetch-poll readiness check is replaced by shelling out to `npx --yes wait-on`, the same tool the CI workflows already use — one implementation of "wait for salve-db-server", not two that can drift apart (and --yes avoids an interactive install prompt blocking the timeout on a first run with no local wait-on). Signal handlers now also stop the inner harness command, not just the server, so a programmatic `kill -TERM <script pid>` (not just interactive Ctrl-C) doesn't leave the harness runner and its simulator session orphaned. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
test:harness:ios built with CODE_SIGNING_ALLOWED=NO (no entitlements at all) instead of CI's CODE_SIGN_IDENTITY=- CODE_SIGNING_REQUIRED=NO (ad-hoc signed), and built both arm64 and x86_64 for iphonesimulator without a -destination, hitting a React Native Codegen script-phase race that's unique to the multi-arch case on this SDK/Xcode version. Passing ONLY_ACTIVE_ARCH=YES plus an explicit simulator -destination builds only the arch the simulator actually needs. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 7
🧹 Nitpick comments (1)
docs/sync-rest-contract.md (1)
7-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the legacy
sinceParam/limitParamfallback parsing.This note states that upsert-on-register alone handles the migration, with no manual step needed. The cohort context indicates
SyncContract::fromDefinitionalso implements a legacysinceParam/limitParamfallback, used for background wake-ups that read a persisted definition before the nextDatabase.register()call rewrites it. Mention this fallback here so readers understand whySyncContract.cppstill parses the old field names after the breaking change.🤖 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 `@docs/sync-rest-contract.md` around lines 7 - 8, Atualize a nota de migração em docs/sync-rest-contract.md para documentar que SyncContract::fromDefinition também interpreta temporariamente sinceParam/limitParam legados como fallback. Esclareça que esse caminho atende wake-ups em background que leem uma definição persistida antes do próximo Database.register() executar o upsert, enquanto o restante da migração continua automático e sem etapa manual.
🤖 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 `@cpp/database/MigrationEngine.cpp`:
- Around line 589-603: Update SyncContract::fromDefinition and the
MigrationEngine::parseSchemaJson registration path to use strict parsing for new
schema JSON, preventing absent listQueryTemplate from being synthesized through
legacyListQueryTemplate(endpoint) and rejecting legacy sinceParam/limitParam
fields. Preserve legacy-field compatibility only when loading persisted
background-wake definitions, and add a regression test confirming schemas using
those legacy fields cannot register or persist.
In `@cpp/http/UrlTemplate.cpp`:
- Around line 34-44: Update UrlTemplate::parse() to validate every literal
percent sign as the start of a complete two-hex-digit escape, rejecting trailing
%, %G0, and %0G during parsing before rendering reaches HttpUrlBuilder. Preserve
valid %HH escapes and existing placeholder parsing behavior, and add coverage
for the three invalid cases.
In `@cpp/sync/SyncContract.cpp`:
- Around line 96-101: Restrict the listQueryTemplate fallback in
SyncContract::fromDefinition to persisted-definition loading, using a dedicated
legacy conversion entry point or explicit source mode invoked by background
wake-up. Keep normal registration strict so missing or explicitly empty
listQueryTemplate is rejected even when sinceParam and limitParam exist, and
update the legacy test to exercise the persisted path while adding strict
registration coverage.
- Around line 31-37: Update legacyListQueryTemplate to percent-encode
legacySince and legacyLimit after validating they are non-empty and before
concatenating them into the query template, preserving the existing placeholders
and error behavior. Add a compatibility test covering reserved characters in
both persisted parameter names and verify the generated template preserves the
intended query structure.
In `@scripts/with-harness-server.mjs`:
- Around line 22-32: Update the server startup flow around spawn and
waitForServer so both the ChildProcess error event and any exit occurring before
readiness reject a startup-failure promise. Race that promise with
waitForServer(), and ensure either failure stops the child process, reaches the
existing wrapper failure path, and prevents the harness from running against
another process serving READY_URL.
- Around line 42-45: Update the shutdown handling around the visible stop logic
and the signal routes to also terminate the readiness subprocess created by
waitForServer(). Kill that waiter and await its completion, with a bounded
timeout fallback, before process.exit(130) or process.exit(143); preserve the
existing stopped guard and server SIGTERM behavior.
- Line 75: Declare wait-on as a locked development dependency in package.json
and package-lock.json, then update the spawn call in with-harness-server to
execute the locally installed binary rather than invoking npx --yes. Preserve
the existing URL and timeout arguments.
---
Nitpick comments:
In `@docs/sync-rest-contract.md`:
- Around line 7-8: Atualize a nota de migração em docs/sync-rest-contract.md
para documentar que SyncContract::fromDefinition também interpreta
temporariamente sinceParam/limitParam legados como fallback. Esclareça que esse
caminho atende wake-ups em background que leem uma definição persistida antes do
próximo Database.register() executar o upsert, enquanto o restante da migração
continua automático e sem etapa manual.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c296b143-0b95-49bf-bc96-4de94dc5e3bc
⛔ Files ignored due to path filters (1)
example/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
README.mdandroid/CMakeLists.txtcpp/database/MigrationEngine.cppcpp/http/HttpUrlBuilder.cppcpp/http/HttpUrlBuilder.hppcpp/http/SyncHttpCaller.cppcpp/http/UrlTemplate.cppcpp/http/UrlTemplate.hppcpp/sync/SyncContract.cppcpp/sync/SyncContract.hppcpp/tests/CMakeLists.txtcpp/tests/HybridSalveDatabaseResetTests.cppcpp/tests/HybridSalveDatabaseSyncTests.cppcpp/tests/database/DatabaseResetterTests.cppcpp/tests/database/MigrationEngineTests.cppcpp/tests/http/HttpUrlBuilderTests.cppcpp/tests/http/SyncHttpCallerTests.cppcpp/tests/http/SyncHttpRequesterTests.cppcpp/tests/http/UrlTemplateTests.cppcpp/tests/query/QueryExecutorTests.cppcpp/tests/sync/RelationCascadeRewriterTests.cppcpp/tests/sync/SalveMetadataManagerTests.cppcpp/tests/sync/SyncApplyGuardTests.cppcpp/tests/sync/SyncContractTests.cppcpp/tests/sync/SyncDefinitionStoreTests.cppcpp/tests/sync/SyncNativeEntryPointTests.cppcpp/tests/sync/SyncOperationApplierTests.cppcpp/tests/sync/SyncOrchestratorTests.cppcpp/tests/sync/SyncQueueStoreTests.cppdocs/sync-rest-contract.mdexample/ios/SalveDbExample.xcodeproj/project.pbxprojexample/package.jsonexample/src/__harness__/SalveDb.core.harness.tsexample/src/__harness__/SalveDb.reset.harness.tsxexample/src/schemas/ProductSchema.tsexample/src/schemas/UserSchema.tsexample/src/screens/SyncTestScreen.tsxscripts/with-harness-server.mjssrc/database/classes/QueryDb/__tests__/QueryDb.writeSync.test.tssrc/database/classes/QueryDb/classes/DeleteQueryBuilder/__tests__/DeleteQueryBuilder.test.tssrc/database/classes/QueryDb/classes/InsertQueryBuilder/__tests__/InsertQueryBuilder.test.tssrc/database/classes/QueryDb/classes/UpdateQueryBuilder/__tests__/UpdateQueryBuilder.test.tssrc/hooks/useQuery/__tests__/useQuery.readSync.test.tsxsrc/types/sync/IEndpointDefinition.tssrc/types/sync/IPaginationDefinition.ts
CodeRabbit review on PR #116 caught a real gap: the legacy sinceParam/ limitParam fallback added to SyncContract::fromDefinition was reachable from MigrationEngine::parseSchemaJson (register()) too, so a freshly authored schema could still pass registration using the removed fields — silently defeating the #115 breaking change. fromDefinition gains an allowLegacyEndpointFallback parameter, false by default (MigrationEngine's register() path — strict, rejects legacy fields). Only SyncOrchestrator::runSyncSession passes true, since it's the one path reading a possibly pre-#115 persisted _salve_sync_definitions row (including the headless background-wake path, which runs before JS gets a chance to re-register and rewrite the row). Also closes two smaller correctness gaps UrlTemplate::parse missed: literal text containing a bare '%' or a non-hex escape (e.g. "%GG") now throws instead of reaching HttpUrlBuilder as an unparseable URL, and the legacy sinceParam/limitParam names are percent-encoded before being spliced into the synthesized template's literal text. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Second CodeRabbit pass on PR #116. Three fixes to with-harness-server.mjs: - The spawned server's 'error' listener only logged; a failed spawn or an exit before readiness went unnoticed for the full 30s timeout, and if some other process happened to already answer READY_URL, the harness could end up running against the wrong server entirely. waitForServer() now races against a startup-failure promise fed by both events. - The wait-on readiness subprocess wasn't tracked, so a signal arriving while still waiting for readiness could leave it running after this script exits. Tracked alongside the harness child and killed in stopServer(). - `npx --yes wait-on` depended on registry access on every clean run. wait-on is now a locked root devDependency; the script resolves and spawns its bin directly from node_modules/.bin. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Member
Author
|
Addressed all 7 actionable CodeRabbit findings (2 commits: c0655ff, 03f12aa): Real bug, fixed: the legacy Also fixed: malformed All green: |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #115.
What
Replaces
sync.endpoint.sinceParam/limitParamwith a{token}URL/query template mechanism:itemPathTemplate?(optional, defaults to{basePath}/{id}) — item route forPATCH/DELETElistQueryTemplate(required) — pull query string, e.g."updatedAfter={since}&limit={limit}", or a composed filter like"$filter={cursorField} gt {since}&$top={limit}"New
cpp/http/UrlTemplateengine (not RFC 6570): closed vocabulary per context,{{/}}escaping, illegal-raw-char rejection at parse time, percent-encoding on substituted values except{basePath}(structural).MigrationEngine::parseSchemaJsonis consolidated to callSyncContract::fromDefinitionfor the sync block (was duplicatingconflict.strategyvalidation) —register()now validates the full endpoint contract eagerly instead of deferring to the firsttriggerSync.Breaking change
sinceParam/limitParamare removed fromIEndpointDefinitionand the native contract. Existing schemas must declarelistQueryTemplateexplicitly. A pre-#115 persisted_salve_sync_definitionsrow (read by the headless background-wake path before JS re-registers) is synthesized into an equivalent template instead of rejected, so background sync doesn't silently stall across the upgrade.Review
A
reviewersubagent pass on the full diff found 2 real correctness bugs (both fixed):itemPathTemplateaccepted an empty/non-string value and silently rendered an empty path, sendingPATCH/DELETEto the bare API rootsinceParam/limitParam) would throw instead of falling back, silently stalling background sync until the next foregroundregister()Plus hardening:
listQueryTemplatemust reference both{since}/{limit},itemPathTemplatemust reference{id}, a leading?in a rendered query no longer produces??, widened illegal-raw-char blocklist (#, non-ASCII), and a couple of perf/robustness fixes inscripts/with-harness-server.mjs(zombie process cleanup, unhandled spawn errors).Verification
npm run test:native: 819 assertions / 341 cases, greennpm test: 192/192, 22 suitesnpm run typecheck: cleanreact-native-harness, iPhone 16 simulator) against the realpackages/salve-db-server: thelistQueryTemplate-exercising suite passes end-to-end across multiple runsCo-Authored-By: Claude Haiku 4.5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation