Skip to content

feat(graphql)!: type asset-pointing fields by interface so clients can narrow to the real content type (#34540) - #37594

Open
fabrizzio-dotCMS wants to merge 5 commits into
34540-graphql-asset-subtype-fieldsfrom
34540-impl-graphql-asset-subtypes
Open

fabrizzio-dotCMS wants to merge 5 commits into
34540-graphql-asset-subtype-fieldsfrom
34540-impl-graphql-asset-subtypes

Conversation

@fabrizzio-dotCMS

@fabrizzio-dotCMS fabrizzio-dotCMS commented Sep 17, 2026

Copy link
Copy Markdown
Member

PR 2 of 2 — implementation. Based on the spec branch (approved in #37537); retarget to main once 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's identifier were 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:

image {
  fileName
  ... on DotFileasset     { fileName }
  ... on DotAssetBaseType { asset     { size mime } }
  ... on FileBaseType     { fileName  fileAsset { size mime } }
  ... on Images           { tags }
  ... on BannerImages     { campaignName adSize }
}

Type hierarchy

Four independent interfaces. None implements another — createInterfaceType is called without a parent in every case, and graphql-java's interface-implements-interface support is not used:

Interface Applies to
DotContentlet every content type
ContentBaseType CONTENT-derived
DotAssetBaseType DOTASSET-derived
FileBaseType FILEASSET-derived
DotFileasset every asset content type, both base types

Each object type declares the ones that apply, side by side:

type Images       implements DotContentlet & DotAssetBaseType & DotFileasset
type BannerImages implements DotContentlet & DotAssetBaseType & DotFileasset
type FileAsset    implements DotContentlet & FileBaseType     & DotFileasset
type PDFDocuments implements DotContentlet & FileBaseType     & DotFileasset
type Blog         implements DotContentlet & ContentBaseType

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 DotFileasset does not give it to DotAssetBaseType, 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 .vtl FileAsset). 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, 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 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.

What Why it could not be avoided
image { description } no longer resolves The one property whose meaning differs between the flat view (the contentlet title) and the content answering it (a stored description). Declaring it on a shared interface would have returned a different value without failing — measured on a real instance: populated for 57 of 57 images before, 2 of 57 after. It stays reachable through a narrowing clause on the concrete type, where it returns the correct value.
An asset field pointing at non-asset content resolves to null rather than the flat view A contentlet outside the interface cannot be handed on; doing so raises UnresolvedTypeException, 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) 5/5 — 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, that description is on none of them
AssetSubtypeAccessTest 9/9
AssetFieldValueContractTest 3/3 — the surviving flat properties still return exactly what they returned before
GraphqlAPITest 51/51

GraphqlQueryRunner remains the first harness executing real GraphQL queries from an integration test; before it, only the Postman collection asserted values. All new classes are registered in MainSuite1b.

Still open

🤖 Generated with Claude Code

…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

claude Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 3m 6s —— View job


Code review — PR #37594 (GraphQL asset subtype fields)

Reviewed the diff against origin/34540-graphql-asset-subtype-fields. The interface restructuring is coherent and the non-asset-content guard in FileFieldDataFetcher (from b2da0d27) is correct. Two non-blocking items below.

New Issues

  • 🟡 Medium: dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java:114-117 (with CustomFieldType.java:50) — Image/File fields now map to new GraphQLTypeReference("DotFileasset"), while the registered custom object type was renamed to "DotFileassetFlat". As a result isCustomFieldType() no longer recognizes the asset field type (its GraphQLTypeReference branch matches names against getCustomFieldTypes(), which now contains DotFileassetFlat, not DotFileasset). This changes isFieldVariableGraphQLCompatible() (called from FieldFactoryImpl.isFieldVariableValid, field-save validation): when a new Image/File field's variable collides with a non-custom inherited content field (e.g. contentType, baseType, urlMap, creationDate), the old code hit the equals/name check and rejected it; the new code takes the (!isCustom(inherited) && !isCustom(field)) branch and returns true — the field is now accepted. At schema generation the object type would then receive two definitions for that name (the user field as the asset interface + the inherited content field), which duplicate-field-definition would reject at build time.

    • Assumption: at least one non-custom inherited field name in ContentFields.getContentFields() is not also in RESERVED_FIELD_VARS, so it can actually be used as a field variable.
    • What to verify: create an Image field whose variable equals a non-custom inherited field name and confirm it is still rejected on save (or that the resulting schema still builds). If it now passes validation but breaks the schema, restore the previous behavior — e.g. have isCustomFieldType recognize the DotFileasset interface reference.
    • Fix this →
  • 🟡 Medium: dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java:100 and datafetcher/FileFieldDataFetcher.java:30-31 — leftovers from the abandoned companion-field design. ASSET_CONTENT_FIELD_SUFFIX = "Content" is never referenced anywhere, and FileFieldDataFetcher.resolve(environment, var) was split out with a javadoc claiming reuse by "a companion field [that] sits beside the asset field" — but no companion field is created and the only caller is get(), passing the field's own name. Dead constant + misleading doc. (You already list DotFileassetFlat cleanup as open; fold this in.)

Resolved

  • dotCMS/src/main/java/com/dotcms/graphql/datafetcher/FileFieldDataFetcher.java:58-73 — the prior Medium (asset field pointing at non-asset content failing the whole request via UnresolvedTypeException) is fixed: the fetcher now returns null when the resolved content's base type is not an asset base type, and AssetSubtypeAccessTest.test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError covers it.

Note: the breaking changes (description no longer resolvable on the shared interface; non-asset content → null) are intentional and documented; flagging for reviewer awareness only, not as defects. The PR correctly calls out that FR-012 re-approval and spec/data-model.md/contracts updates are still outstanding.
· 34540-impl-graphql-asset-subtypes

…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>
@fabrizzio-dotCMS

Copy link
Copy Markdown
Member Author

The 🟡 Medium finding was real. Confirmed, then fixed in b2da0d270a.

Verified rather than assumed. The premise holds in the code — FileFieldDataFetcher ends with getOrElse(fileAsContent), so when FileAssetAPI.fromContentlet cannot convert the target the raw contentlet is handed on whatever its base type is, with no filter anywhere. I then wrote a test that points an Image field at ordinary content and selects the new field:

UnresolvedTypeError{path=[...Collection, 0, subtypeImage, content],
  exception=graphql.execution.UnresolvedTypeException:
  Runtime Object type 'testVarname...' is not a possible type for 'DotAssetContent'.}

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 AssetContentDataFetcher, not in ContentResolver. The review offered either; the resolver is shared by every other base-type interface, so changing it there is the wider blast radius for no gain. The fetcher now returns null when the source's base type is not an asset base type. 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, as the review notes.

Test added: test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError.

AssetSubtypeAccessTest 6/6
AssetFieldValueContractTest 3/3
GraphqlAPITest 51/51

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>
@fabrizzio-dotCMS fabrizzio-dotCMS changed the title feat(graphql): expose the real content type behind Image and File fields (#34540) feat(graphql)!: type asset-pointing fields by interface so clients can narrow to the real content type (#34540) Sep 17, 2026
…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>
@claude

claude Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

SDK Breaking Change Detected!!!

  • Category: G-1 — Removing or Renaming a Reachable GraphQL Type/Field

  • Why it breaks compatibility: Every Image/File field (e.g. a banner/avatar field on a Blog, Product, etc.) is now typed by the DotFileasset interface instead of a flat GraphQLObjectType, and description is not carried on that interface, on DotAssetBaseType, or on FileBaseType — it's only reachable through an inline fragment on the concrete asset type. Any already-deployed customer query (built via the page API's graphql.page/graphql.content extension) that selects image { description } or someFileField { description } without narrowing now fails GraphQL validation entirely. Per page-api.ts's "BAD QUERY" branch, that's not a degraded field — the whole query returns data: null and the page load throws DotErrorPage, exactly the failure mode this category describes. The PR body itself confirms the value dropped from 57/57 to 2/57 images in their own test, though "populated" undersells it: for the plain (non-fragment) selection it isn't a different value, it's a validation error.

  • Code that makes it breaking:

    • dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java:170-174FILEASSET_DESCRIPTION_FIELD_VAR fetcher still exists but is stripped out via assetFlatFields.remove(FILEASSET_DESCRIPTION_FIELD_VAR) (line ~197) before being reused as the shared field set for the new interfaces.
    • dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java — new ASSET_INTERFACE_NAME = "DotFileasset" interface built from CustomFieldType.getAssetFlatFields() (no description); DOTASSET_INTERFACE_NAME/FILE_INTERFACE_NAME likewise gain the flat fields minus description.
    • dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.javaImageField.class/FileField.class are now mapped to new GraphQLTypeReference(InterfaceType.ASSET_INTERFACE_NAME) instead of CustomFieldType.FILEASSET.getType() (a flat object type that did expose description), i.e. the field's declared GraphQL type changed kind and shrank its field set.
    • Confirmed in the PR's own description: "image { description } no longer resolves... populated for 57 of 57 images before, 2 of 57 after."
  • Safer alternative: Keep description declared on the shared interface(s) resolving to the pre-existing flat value (the contentlet title) for backward compatibility, and additionally expose the "real" stored description under a differently-named field (e.g. assetDescription) reachable via narrowing — additive, not a replacement of the existing selectable field. If the semantic collision is truly unavoidable without a value lie, this needs a deprecation window (keep description behaviorally as-is for at least one release, call out the new narrowed name as fixing the record, then remove/repoint description after consumers have migrated) rather than breaking it in the same release the interface ships.

  • Category: G-1 — Removing or Renaming a Reachable GraphQL Type/Field (secondary instance — type kind change)

  • Why it breaks compatibility: DotFileasset changes from a GraphQLObjectType to a GraphQLInterfaceType (same name, different kind) at dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java:50 (FILEASSET("DotFileassetFlat"), i.e. the old object type is renamed/orphaned and never registered — see the removed customFieldTypes.put("FILEASSET", ...) call) combined with InterfaceType.java's new ASSET_INTERFACE_NAME = "DotFileasset". Plain field selections that stayed on the flat 5 properties keep working (they're carried on the interface), but this is a wire-visible kind change: any SDK-adjacent tooling that does introspection-based codegen against DotFileasset (as the PR body itself flags: "anyone with generated types must regenerate") will generate code that no longer matches the runtime schema kind, and any query using ... on DotFileasset where the position is not itself typed as (or compatible with) the new interface's possible-types set could behave differently under interface resolution (e.g. UnresolvedTypeException risk called out and partially mitigated in FileFieldDataFetcher.java, but only for the "points at non-asset content" case).

  • Code that makes it breaking: dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java:50 and the removed object-type registration; dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java (ASSET_INTERFACE_NAME); dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java (ImageField/FileFieldGraphQLTypeReference(InterfaceType.ASSET_INTERFACE_NAME)).

  • Safer alternative: As the reference doc prescribes for renames: keep the old object type available as a deprecated alias for a release cycle where practical, and treat the object→interface kind change on a name generated clients hard-code as equivalent to a rename for compatibility purposes — call it out explicitly in release notes as requiring SDK/codegen regeneration, and consider bumping MinSdkVersion.VALUE alongside this change.

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.

@claude claude Bot added the SDK Breaking Change Breaks @dotcms SDK compatibility. AI adds it; only a human removes it. label Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SDK Breaking Change Breaks @dotcms SDK compatibility. AI adds it; only a human removes it.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant