Match type filters through re-export spellings via canonical code refs - #5800
Conversation
Preview deploymentsHost Test Results 1 files 1 suites 1h 13m 58s ⏱️ Results for commit b1ea516. Realm Server Test Results 1 files ±0 1 suites ±0 15m 22s ⏱️ +5s Results for commit 2a58eb1. ± Comparison against earlier commit 7921aea. |
bcbbc1f to
7921aea
Compare
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review focus: the two-sided canonical-ref contract — that the server's types-membership compile and the host's client-side matcher agree on what a re-export spelling matches, and that the new deferred type-condition node composes exactly as the inline any() it replaces.
Bottom line: the design is correct — I found no correctness defect in the compiled SQL or in the server/matcher agreement. Two non-blocking findings: a stale-source race in the host's async canonicalization, and a swallow-all error path in typeKeysFor. Details inline. No blocking issues.
What lands right.
- The mechanism checks out end-to-end. Index rows stamp
typesviaidentifyCard(the defining-module ref —makeDefinitioninhost/app/routes/module.tssetscodeRef = identifyCard(cardOrFieldDef)). The matcher'sinstanceIsTypewalks the instance's prototype chain comparingidentifyCard(klass)throughcodeRefEquals, which tolerates URL-spelling variance (RRI / real-URL / virtual-alias) but not module-identity variance. So a rawfile-api/FileDeffilter makesinstanceIsTypereturn false and the reconciler strips the server's correct rows. Canonicalizing the filter's refs to thecard-apidefining-module ref on both sides is exactly the right fix, and doing it upstream insearch.tsrather than inside the matcher keeps that single call site as the only consumer needing it (verified:search.tsis the only non-test caller ofmatchInstanceAgainstFilter/isClientEvaluable). - The deferred
type-conditionnode is structurally equivalent to the old inline path.filterConditionstill wrapsthis.typeCondition(ref)(now[typeConditionNode]) throughevery/any, whoseaddExplicitParensparenthesizes the single-element array; pass 1 spliceshandleTypeCondition'sany(keys.map(typesContains))— the same token shape the old code emitted — inside those parens. All three type-key sites (handleTypeCondition,hasFileType,hasInstanceType) route throughtypeKeysFor, so no twin is left on the old inline path. typeKeysFormutatinginternalKeysFor's return with.pushis safe —internalKeysForreturns a fresh.map(...)array per call.- The canonicalization walker's single-flight memo is correct:
canonicalRefrunsmemo.has/memo.setsynchronously before its firstawait, so concurrent sibling walks can't both miss (theduplicate refs resolve oncetest pins it), and the per-node{ ...node }shallow clone keeps the input tree immutable (also pinned).
Recommendations.
- Guard the host
.thenagainst a stale filter so an out-of-orderloader.importresolution can't stickily disable client reconciliation — see theloadCanonicalizedFilterthread. (non-blocking) - Consider distinguishing "definition not found" from a transient lookup failure in
typeKeysFor, so a valid re-export ref doesn't silently under-match under load — see thetypeKeysForthread. (non-blocking)
Adjacent, out of scope. The realm test-results status comment reports realm-server tests as ±0 against the base even though this PR adds two tests to search-entries-engine-test.ts — most likely a status-comment/base-comparison artifact rather than the tests not running, but worth a glance to confirm they executed in CI.
2a58eb1 to
6ddc33d
Compare
A filter whose `type`/`on` ref named a type through a re-exporting module (e.g. file-api's FileDef, re-exported from card-api) silently matched nothing: index rows stamp `types` with the canonical (defining-module) key from identifyCard, so only the canonical spelling joined the membership keys. Filtering on base FileDef via its documented file-api module returned zero rows while subtype filters matched. The query engine now resolves each type condition's ref through the definition lookup and unions the canonical ref's spelling-tolerant keys with the as-given ref's, so both spellings compile to the same `types` membership predicates. The type condition becomes a deferred expression node (pass-1-resolved) since the definition lookup is async. The host's search resource applies the same rewrite before client-side matching — the matcher compares against identifyCard of the instance's class, so an uncanonicalized re-export spelling would strip the server's correct results during reconciliation. Until the rewrite settles for the active filter (or when a ref doesn't resolve), the search stays a server-only passthrough. The shared canonicalization walker single-flights duplicate ref spellings by memoizing the in-flight promise: sibling filter nodes are walked concurrently, and a value-memo would let both siblings miss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… errors Two robustness fixes from review of the canonical-code-ref type matching: - SearchResource.loadCanonicalizedFilter guards its `.then` against a stale filter. Sibling canonicalizations race and a cache-warm loader import can resolve ahead of a cold one, so an earlier filter's resolution could land last and overwrite `canonicalizedFilter` with a `source` no longer matching the active query. That left `isClientFilterEligible` false and the search stuck in server-only passthrough until the next query change. The guard drops a resolution whose filter is no longer the active one. - IndexQueryEngine.typeKeysFor no longer swallows every lookup error. A ref that resolves to no known type still falls through to the spelling-based keys (matching nothing unless rows were stamped under that spelling), matching how the engine's top-level catch treats a nonexistent type as an empty result; any other, unexpected error now propagates instead of silently narrowing the match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6ddc33d to
b1ea516
Compare
What this does
A
filter.type/oncode ref that names a type through a re-exporting module — the canonical example being baseFileDefaddressed via its documentedfile-apimodule, which re-exports it fromcard-api— silently matched nothing. Index rows stamptypeswith the canonical (defining-module) key fromidentifyCard, so only the canonical spelling ever joined the membership keys: filtering on baseFileDefreturned zero rows while concrete subtype filters matched, making the base-type filter a silent dead end.Two halves, kept in agreement:
typesmembership predicates union the canonical ref's spelling-tolerant keys (RRI / real-URL / virtual-alias) with the as-given ref's. Because the definition lookup is async, the type condition becomes a deferred expression node resolved in pass 1, alongside the existing deferred kinds. A ref whose definition doesn't resolve keeps only its spelling-based keys and matches nothing, exactly as before.identifyCard) before client-side matching — the matcher compares refs againstidentifyCardof an instance's class, so an uncanonicalized re-export spelling would strip the server's correct results during live reconciliation. Until the rewrite settles for the active filter, or when a ref doesn't resolve, the search stays a server-only passthrough so the two evaluations can't disagree.The shared walker (
runtime-common/query-canonicalization.ts) rewritestype/onrefs throughany/every/notwithout mutating the input tree, and single-flights duplicate ref spellings by memoizing the in-flight promise — sibling nodes are walked concurrently, so a value-memo would double-resolve.Test plan
packages/realm-server/tests/search-entries-engine-test.ts: a pure base-FileDef filter matches every file row across subtypes, and thefile-apire-export spelling matches the same rows as the canonicalcard-apispelling (36/36 pass locally).packages/host/tests/unit/query-canonicalization-test.ts: rewrite of top-level and nested refs, input-tree immutability, unresolvable refs marking the result incomplete, and duplicate refs resolving once (8/8 pass locally via the dev test page).🤖 Generated with Claude Code