Skip to content

fix(search)!: drop an unselectable bucket instead of failing the response - #740

Merged
ddeboer merged 1 commit into
mainfrom
fix/iri-outbound-coercion
Aug 14, 2026
Merged

fix(search)!: drop an unselectable bucket instead of failing the response#740
ddeboer merged 1 commit into
mainfrom
fix/iri-outbound-coercion

Conversation

@ddeboer

@ddeboer ddeboer commented Aug 14, 2026

Copy link
Copy Markdown
Member

Follow-up to #728, from a review of that PR that landed after it merged. Two of its findings are runtime faults in the released 0.21.0, one is a missed guard, and one is a naming inconsistency that only gets more expensive to correct.

A stale bucket nulled the whole response

The outbound IRI coercion raised wherever it sat. On a facet bucket that is the wrong call: value: IRI! is inside [IRIBucket!]! inside Facets! inside ‹Type›SearchResult! inside a non-null root field, so GraphQL’s null propagation carried the error all the way to data: null. One unusable bucket destroyed items and pagination – precisely the collateral ADR 5’s degradation contract exists to prevent a supplementary sidebar count from causing.

Buckets are now filtered instead: a bucket that cannot be sent back as the filter that selects it is not worth serving, and dropping it keeps IRI’s promise without the collateral. Raising is kept where the value is identity (id, a reference’s id) – there the document genuinely cannot be selected, so serving it would hand back something unusable. Both halves have a test; #728’s test asserted only the error message, which is why it passed while the response was being destroyed.

The invariant that justified raising was not true yet

#728 argued outbound validation was safe because “the projection applies the same rule at the source, so the only way to reach this error is an index written before it existed”. Three routes wrote a reference value without ever passing isAbsoluteIri:

  • applyField – a derive assigned its return value straight to the document;
  • applyProjectionValue – a from projection value was written unchecked;
  • applyFacet – a transform ran after iriString had validated, so it could undo it.

The first is not hypothetical: ADR 19 tells deployments to convert IRI-valued keyword derive fields to kind: 'reference', which routes straight through it. Such a value indexed cleanly and then failed every response carrying it, under an error instructing the operator to reindex – which reproduced the value exactly.

applyFacet now holds from values and transform results to the same rule the graph path passes, and a derive on a reference keeps only the absolute IRIs it returned, leaving the field absent when none survive – the same outcome the graph path gives a reference it cannot key on. The claim in ADR 19 and the scalar’s docblock is rewritten to say what is now true, rather than what was assumed.

Two smaller things

The collision guard was incomplete. It checked the new reserved names only on the targetFilter path, while takenTypeNames was seeded from root type names alone – so a type named IRI, ValueBucket or KeywordFilter passed every check and failed at new GraphQLSchema with graphql-js’s generic “Schema must contain uniquely named types”, naming neither the declaration nor the field. That is the opaque failure the guard exists to replace. It is now seeded with every shared built-in name.

Also folded in: ref.typeName is validated on every surfaced strategy but idOnly. idOnly is what made typeName optional in the first place, and without this a labelOnly reference missing a name prints undefined as its field’s type – a published contract that is not a schema.

IriBucketIRIBucket. One concept, three names it minted – IRI, IRIFilter, IriBucket – and they did not agree. #728 was the moment to fix this for free and the review reached it too late, so this is a second breaking release for a rename. Worth it: this is a surface ADR 4 freezes, the cost only grows with every consumer that adopts 0.21.0, and the inconsistency is in the one place a generic consumer reads structurally.

Notes for review

  • The coverage thresholds in both vite.config.ts files are autoUpdate re-anchors from the new covered branches.
  • docs/reference/search.md’s or example now uses real IRIs and same-target fields; the previous one filtered material on "oil", which the input coercion shipped in 0.21.0 now rejects, and spread one PersonFilter variable across fields with three different targets.

…onse

The outbound IRI coercion shipped in 0.21.0 raised on any value it could
not accept, wherever it sat. On a facet bucket that is the wrong call:
`value: IRI!` is inside `[IRIBucket!]!` inside `Facets!` inside
`‹Type›SearchResult!` inside a non-null root field, so one stale bucket
nulled the WHOLE response and took `items` and `pagination` with it -
exactly the collateral ADR 5’s degradation contract exists to prevent a
supplementary sidebar count from causing.

The justification given for raising was also not true yet. It rested on
the projection applying the same rule at the source, but three routes
wrote a reference value without ever passing `isAbsoluteIri`, and the
migration ADR 19 prescribes - an IRI-valued `keyword` `derive` converted
to `kind: 'reference'` - runs straight through the busiest of them. Such
a value indexed cleanly and then failed every response carrying it, under
an error telling the operator to reindex, which reproduced it exactly.

- drop unselectable buckets in the facet resolvers rather than raising:
  a bucket that cannot be sent back as the filter selecting it is not
  worth serving, and dropping it keeps the promise without the collateral
- keep raising where the value IS identity (`id`, a reference’s `id`),
  where the document genuinely cannot be selected
- guard the three unguarded routes: `applyFacet` now holds a `from`
  projection value and a `transform` result to the same rule the graph
  path passes, and a `derive` on a `reference` keeps only the absolute
  IRIs it returned, leaving the field absent when none survive
- validate `ref.typeName` on every surfaced strategy but `idOnly`, which
  emits no type and so needs no name to emit one under
- seed the type-name collision guard with the shared built-in names, so a
  type named `IRI` or `ValueBucket` is named as the cause instead of
  failing at schema assembly with graphql-js’s generic message
- rename `IriBucket` to `IRIBucket`, so the three names this surface
  minted for one concept agree on their casing

BREAKING CHANGE: a reference facet’s bucket type is `IRIBucket`, not
`IriBucket`. Renaming it needs a second breaking release because 0.21.0
published the inconsistent spelling; the three names for one concept
(`IRI`, `IRIFilter`, `IRIBucket`) are worth more on a surface ADR 4
freezes than the cost of correcting it now, which only grows.
@ddeboer
ddeboer force-pushed the fix/iri-outbound-coercion branch from 0ed8dcc to 70e2087 Compare August 14, 2026 12:56
@ddeboer
ddeboer merged commit 2eec619 into main Aug 14, 2026
4 checks passed
@ddeboer
ddeboer deleted the fix/iri-outbound-coercion branch August 14, 2026 13:07
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.

1 participant