feat(graphql)!: type asset-pointing fields by interface so clients can narrow to the real content type (#34540) - #37594
Conversation
…lds (#34540) An Image or File field resolves to DotFileasset, a flat six-property type built once in a static block. A customer who extends DOTASSET or FILEASSET to model their own assets can author their fields but cannot read them back: the schema never offers them, so no query at any depth reaches them. Neither are the asset's tags -- which is what blocks the AI tagging workflow -- nor even its identifier. Adds one field, `content`, to DotFileasset, returning the asset described by a new DotAssetContent interface whose possible types are every content type derived from either asset base type. The set is derived from the content types present when the schema is built, never enumerated, so a type the customer creates later is reachable with no registration step -- covered by a test that builds the schema first and creates the type afterwards, which a hardcoded list would fail while passing everything else. The interface spans BOTH base types rather than one per kind of field. An Image field accepts and resolves file-style content today (verified against a running instance), so typing each field by its own kind would have silently dropped content those fields already hold. Strictly additive, per FR-012 and ADR-0022: all six existing properties keep their names, types and exact values -- including the two that look wrong. `fileName` and `description` are synthesized by base-type ternaries, so for image-style content `description` returns the title, i.e. the file name. AssetFieldValueContractTest locks that: correcting it would change what a live customer query returns without failing it. Also: - GraphqlQueryRunner, the first harness that executes real GraphQL queries from an integration test. Until now only the Postman collection asserted values, so a change could keep every type in place while altering what a field returns and no Java test would notice. - Fixes areFileassetFieldsPresent in GraphqlAPITest, which used allMatch and so asserted the type exposed NO field other than the six -- the opposite of its name, and a lock against ever adding one. It is what PR #35363's CI fails on. DotFileasset references the new interface by name rather than by instance: InterfaceType's static initializer reaches ContentAPIGraphQLTypesProvider, which reads CustomFieldType, so resolving it there would close that cycle and observe a half-initialized class. Tests: AssetSubtypeAccessTest 5/5, AssetFieldValueContractTest 3/3, GraphqlAPITest 51/51. New classes registered in MainSuite1b. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @fabrizzio-dotCMS's task in 3m 6s —— View job Code review — PR #37594 (GraphQL asset subtype fields)Reviewed the diff against New Issues
Resolved
Note: the breaking changes ( |
…e request (#34540) Addresses the review finding on #37594. Confirmed real, and worse than a wrong value: it fails the whole request. Nothing stops an Image or File field from holding the identifier of ordinary content. The field stores a bare identifier, and FileFieldDataFetcher ends with `getOrElse(fileAsContent)` — when FileAssetAPI.fromContentlet cannot convert the target, the raw contentlet is handed on whatever its base type is. Passing that to the new `content` field made the type resolver name an object type outside the interface's possible types: UnresolvedTypeException: Runtime Object type 'testVarname...' is not a possible type for 'DotAssetContent'. graphql-java answers that by failing the entire request, so one mis-pointed field takes every other collection in the query down with it — verified by the new test, which asserts the rest of the response survives. Guarded in AssetContentDataFetcher rather than ContentResolver, which is shared by every other base-type interface and would have been the wider blast radius. The misconfiguration is in the data, so the field reports nothing and the rest of the response is delivered. The six flat properties are unaffected either way. Tests: AssetSubtypeAccessTest 6/6, AssetFieldValueContractTest 3/3, GraphqlAPITest 51/51. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The 🟡 Medium finding was real. Confirmed, then fixed in Verified rather than assumed. The premise holds in the code — One correction to the finding's framing, and it makes it worse rather than better. The review describes "a runtime resolution error for that selection". It is not scoped to the selection — graphql-java fails the entire request. A single mis-pointed field takes every other collection in the query down with it. The new test asserts the rest of the response survives, precisely because that is the part that would have hurt. Fixed in Test added:
Thanks for the catch — this one had no test covering it and would have reached a customer as an intermittent whole-query failure. |
…34540) User Story 2 — telling which kind of asset came back. Both tests passed on the first run with no production change. That is the expected outcome, not a gap: US1's resolver already returns the concrete type of whatever contentlet was resolved, and the interface already spans both base types, so __typename and the crossed case fall out of it. Recording that rather than manufacturing a failure to satisfy the ritual. Their job here is characterization, not construction: - test_typename_distinguishesTwoDifferentAssetTypes — one Image and one File field pointing at different asset types in a single query; each must name its own concrete type and the two must differ. Today both would answer DotFileasset, leaving a client with a mixed feed nothing to branch on. - test_fieldsResolveContentOfTheOtherBaseType — deliberately crossed: the Image field holds file-style content and the File field holds image-style content. This is the lock that matters. A later change typing each field by its own kind would compile, pass every other test in the class, and silently drop content those fields already hold today — verified on a live instance, where an Image field resolved a plain-text FileAsset. Tests: AssetSubtypeAccessTest 8/8. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…bject (#34540) Replaces the companion-field approach from the previous commits. An Image or File field is now described by an interface, so a client narrows to the concrete asset type in the same block as the flat properties: image { fileName ... on DotFileasset { fileName } ... on DotAssetBaseType { asset { size mime } } ... on FileBaseType { fileName fileAsset { size mime } } ... on Images { tags } ... on BannerImages { campaignName adSize } } The interface KEEPS THE NAME the flat object type carried. Clients already write `... on DotFileasset`, and a fragment on the position's own interface always matches, so those clauses stay valid and keep returning data; a new name would have invalidated every one of them. The kind changes, object -> interface, which query text does not notice but client code generators do. It spans BOTH asset base types rather than one interface per kind of field: an Image field accepts and resolves file-style content today, verified on a running instance, so a per-field-kind design would have silently dropped content those fields already hold. The five flat properties -- fileName, fileAsset, metaData, showOnMenu, sortOrder -- are declared on the asset interface, on BOTH base-type interfaces, and on every concrete asset type, synthesized where absent with the very same fetchers the flat view used. Anything less makes the same property selectable through one clause and not another, so a client's query changes shape depending on how it narrows. The DOTASSET interface was missing them at one point and nothing else noticed, which is what AssetTypeHierarchyTest now prevents. BREAKING CHANGES, both visible rather than silent: - `image { description }` no longer resolves. It is the one property whose meaning differs between the flat view (the contentlet title) and the content answering it (a stored description), so a shared declaration would have returned a different value without failing. The stored value is reachable through a narrowing clause on the concrete type. - An asset field pointing at content that is not an asset now resolves to null rather than to the flat view. Forced: a contentlet outside the interface cannot be handed on, and doing so fails the WHOLE request with UnresolvedTypeException rather than just that field. New: AssetTypeHierarchyTest locks the shape -- which interfaces each asset type declares, that the asset interface spans both base types, that the flat properties are reachable through every surface, and that `description` is on none of the interfaces. Tests: AssetTypeHierarchyTest 5/5, AssetSubtypeAccessTest 9/9, AssetFieldValueContractTest 3/3, GraphqlAPITest 51/51. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eft behind (#34540) The spec still described the non-breaking design. It now matches what shipped, and says plainly that the difference is a product call rather than a technical one — recorded in full because the reasoning ran both ways and a later reader would otherwise assume the compliant option was never available. Spec: - FR-012 superseded: five of the six flat properties survive unchanged; `description` and the non-asset-content case do not, and both fail visibly. - FR-012a and FR-012b no longer apply — there is no surviving surface to mark, and retirement is not deferred. - FR-012c added: the break ships with announcement and migration guidance. - FR-009a survives intact and is why the two breaks take the shape they do: nothing may change value silently. A removal a client can see is acceptable; a name that keeps working and returns something else is not. - SC-008/SC-009 now measure what matters — five of six still correct, and ZERO selections that keep working while returning different data. - ADR Alignment asks for an exception to ADR-0022 knowingly, rather than claiming none is needed. It records what the exception preserves of the ADR's intent, that the compliant expand-phase design was built and verified green before being set aside, and that the technical constraint is not negotiable: a GraphQL field has one type and a resolved value one runtime type, so the flat view and the asset cannot share a position. Sign-off requested from @fmontes and @nollymar; neither has agreed yet. Postman: `Request content with DotAsset` queried `description` on an asset field and asserted it equalled the file name — which is what the flat view returned, since it answered with the contentlet title. That query now fails outright, so the selection and its assertion are removed with a note saying where the real value moved. Edited as text rather than re-serialized: a json round-trip reformatted all 14,928 lines for a three-line change. Minor fixes from the validation pass: - The flat object type is no longer registered as a schema type. Nothing references it since asset fields became interface-typed, so registering it left an orphan in every customer's schema, visible in introspection and reachable by nobody. - getAssetFlatFields() throws with an explanatory message instead of returning null if read during class initialization — a new static-init cycle would otherwise surface much later as an unexplained NPE inside schema construction. - ASSET_CONTENT_INTERFACE_NAME renamed ASSET_INTERFACE_NAME, matching the "DotFileasset" it actually holds. - AssetTypeHierarchyTest's javadoc diagram drew the interfaces as a hierarchy. They are four independent interfaces; object types declare the ones that apply. They share fields because the same fields are declared on each, not by inheritance — which is exactly why a field added to one must be added to all. Tests: 68/68 (AssetTypeHierarchyTest 5, AssetSubtypeAccessTest 9, AssetFieldValueContractTest 3, GraphqlAPITest 51). Postman not run locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
SDK Breaking Change Detected!!!
This PR's own description already discloses both of these as intentional, visible breaking changes ("Contains breaking changes", "Needs re-approval: this violates FR-012 of the approved spec"), so this confirms rather than contradicts the author's own assessment — flagging per the automated check's mandate regardless. |
PR 2 of 2 — implementation. Based on the spec branch (approved in #37537); retarget to
mainonce that lands.Fixes #34540 (limb a). Contains breaking changes — see below. Needs re-approval: this violates FR-012 of the approved spec.
What it does
An Image or File field resolved to
DotFileasset, a flat six-property object built once in a static block, so a customer who extends DOTASSET or FILEASSET to model their own assets could author their fields but never read them back —tags, custom properties and even the asset'sidentifierwere absent from the schema at any depth.The field is now described by an interface, so a client narrows to the concrete type in the same block as the flat properties:
Type hierarchy
Four independent interfaces. None implements another —
createInterfaceTypeis called without a parent in every case, and graphql-java's interface-implements-interface support is not used:DotContentletContentBaseTypeDotAssetBaseTypeFileBaseTypeDotFileassetEach object type declares the ones that apply, side by side:
The three asset interfaces share the five flat properties because the same fields are declared on each, not because one inherits from another. That matters for anyone changing this later: adding a field to
DotFileassetdoes not give it toDotAssetBaseType, and every implementing object type must carry it or the whole schema build fails.Three decisions worth a reviewer's attention:
The interface keeps the name
DotFileasset. Clients already write... on DotFileasset { … }, and a fragment on the position's own interface always matches, so those clauses stay valid and keep returning data. A new name would have invalidated every one of them. The kind changes — object → interface — which query text does not notice but client code generators do: anyone with generated types must regenerate.It spans both base types, not one interface per kind of field. An Image field accepts and resolves file-style content today (verified on a running instance, where an Image field returned a
.vtlFileAsset). A per-field-kind design would have compiled, passed everything else, and silently dropped content those fields already hold. This came out of @nollymar's spec review.The five flat properties are on every surface.
fileName,fileAsset,metaData,showOnMenu,sortOrderare declared on the asset interface, on both base-type interfaces, and on every concrete asset type — synthesized where absent, with the very same fetchers the flat view used. Anything less makes a property selectable through one clause and not another, so a client's query changes shape depending on how it narrows. The DOTASSET interface was missing them at one point and nothing else noticed.Breaking changes
Both are visible, not silent — that constraint was kept even where the no-break one could not be.
image { description }no longer resolvesnullrather than the flat viewUnresolvedTypeException, which fails the whole request — one mis-pointed field taking every other collection in the query down with it.This is why the PR needs re-approval: the approved spec's FR-012 says existing queries keep working, and ADR-0022 prescribes expand → adopt → bake → retire. The spec artifacts still describe the earlier, non-breaking shape and need updating alongside.
Tests
AssetTypeHierarchyTest(new)descriptionis on none of themAssetSubtypeAccessTestAssetFieldValueContractTestGraphqlAPITestGraphqlQueryRunnerremains the first harness executing real GraphQL queries from an integration test; before it, only the Postman collection asserted values. All new classes are registered inMainSuite1b.Still open
DotFileassetFlat— the old flat object type is now orphaned in the schema; remove it or justify keeping it.data-model.md,contracts/andtasks.mdstill describe the earlier shape.🤖 Generated with Claude Code