fix(search): reject a surfaced reference that declares no ref typeName - #742
Closed
ddeboer wants to merge 2 commits into
Closed
fix(search): reject a surfaced reference that declares no ref typeName#742ddeboer wants to merge 2 commits into
ddeboer wants to merge 2 commits into
Conversation
`typeName` became optional when `idOnly` landed, since that strategy emits no type and so needs no name to emit one under. Every other surfaced strategy IS served as a named type, but nothing checked that it had a name: `validateSearchType` only checked that `ref` existed at all. TypeScript's union already forbids it, so this only bites a declaration built outside it – plain JS, or a generator – which is exactly what this validation exists to guard. The failure was silent rather than loud: the GraphQL surface skipped registering the type, then printed `undefined` as the field's type, publishing a contract that is not a schema. - add a `missing-ref-type-name` issue, raised for an `output` reference whose strategy is not `idOnly` and whose `ref` declares no `typeName` - leave a filter-only reference alone: nothing has to be emitted for it, so it falls back to the target-less IRI filter
The documented “everything related to Van Gogh” query no longer validates. It declared one `PersonFilter` variable and fed it to `creator`, `about` and `contentLocation` – three fields whose filters now carry different input types – and GraphQL checks variable usage nominally, so the query is rejected before it runs. The same section also filtered `material` on `"oil"`, a bare token on a field that keys on identity, which now fails coercion. - draw the alternatives from one target (`creator`, `contributor`, `publisher`), which is what the refined discovery strategy actually returns for an agent IRI, and is ADR 18's own motivating example - filter `material` on an AAT IRI - state why the alternatives share a variable, and that fields pointing at different targets each need their own
Member
Author
|
Superseded by #740, which landed both fixes verbatim: |
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
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.
Two code-review findings against #723 / #728 that were not addressed before that PR merged. Both are consequences of the per-target filter typing it introduced.
A surfaced reference with no
ref.typeNamepublishes an invalid contracttypeNamebecame optional whenidOnlylanded, because that strategy emits no type and so needs no name to emit one under. Every other surfaced strategy is served as a named type – but nothing checked it had a name:validateSearchTypeonly checked thatrefexisted at all.The failure is silent rather than loud.
registerReferenceTypeskips the field,outputFieldTypethen resolvesreferenceTypes.get('')toundefined, and the SDL comes out as:A published contract that is not a schema. Now:
TypeScript's union already forbids this, so it only bites a declaration built outside it – plain JS, or a generator – which is exactly the case
validateSearchTypeexists to guard. A filter-only reference is deliberately left alone: nothing has to be emitted for it, so it falls back to the target-lessIRIFilter.Same issue name as the fix on
feat/search-lookup-strategy(#731), with the predicate inverted: therelookupreplaceslabelOnly, whereas hereidOnlyis precisely the strategy that legitimately carries notypeName.The documented cross-field
orexample no longer validatesdocs/reference/search.mddeclared onePersonFiltervariable and fed it tocreator,aboutandcontentLocation– three fields whose filters now carry different input types. GraphQL checks variable usage nominally, so the documented query is rejected before it runs.The same section also filtered
materialon"oil"– a bare token on a field that keys on identity, which now fails coercion. That one was not in the review.Fixed by drawing the alternatives from a single target (
creator,contributor,publisher) – which is both ADR 18's own motivating example and what the refined discovery strategy actually returns for an agent IRI – filteringmaterialon an AAT IRI, and stating why the alternatives can share a variable while fields pointing at different targets each need their own.Notes
packages/search/vite.config.tsmoves the branch threshold 99.49 → 99.5, anautoUpdatere-anchor from the new covered branch.fix, notfeat!: a declaration this now rejects was already emitting an invalid schema, so nothing that worked stops working.mainafter feat(search)!: type a filter by what its field keys on #728 merged, so it does not overlap the projection-side IRI hardening still in flight.