Skip to content

feat(graphql): unify Binary, File, and Image field queries via DotBinaryLike #34540#35363

Draft
nollymar wants to merge 2 commits intomainfrom
issue-34540-graphql-unified-binary-interface
Draft

feat(graphql): unify Binary, File, and Image field queries via DotBinaryLike #34540#35363
nollymar wants to merge 2 commits intomainfrom
issue-34540-graphql-unified-binary-interface

Conversation

@nollymar
Copy link
Copy Markdown
Member

Summary

Refs #34540.

Today BinaryField maps to DotBinary and FileField/ImageField map to DotFileasset in the GraphQL schema. The two object types expose completely different sub-fields, so a client cannot write a uniform query across the three dotCMS field types:

mybinary { versionPath name size mime }   # works
myfile   { versionPath name size mime }   # fails — fields aren't on DotFileasset
myimage  { versionPath name size mime }   # fails — same

This PR introduces a shared DotBinaryLike GraphQL interface implemented by both DotBinary and DotFileasset. It declares the 12 common binary properties:

name, title, size, mime, versionPath, idPath, path, sha256, isImage, width, height, modDate

  • DotBinary — resolves them from the binary map already produced by BinaryToMapTransformer. No behavior change.
  • DotFileasset — new FileAssetBinaryPropertyDataFetcher runs the same transformer against the underlying FileAsset/DotAsset Contentlet, so the values returned are identical to those a client gets from the equivalent mybinary query.

Type-specific extras stay on the concrete type:

  • DotBinary keeps focalPoint.
  • DotFileasset keeps fileName, description, fileAsset, metaData, showOnMenu, sortOrder.

A TypeResolver on the interface dispatches on the source Java type: MapDotBinary, ContentletDotFileasset.

Backward compatibility

  • All existing DotBinary queries: unchanged.
  • All existing DotFileasset queries (myfile { fileAsset { ... } }, fileName, description, etc.): unchanged.
  • Change is purely additive.

Test plan

  • Unit tests: CustomFieldTypeBinaryLikeTest (8 tests, all green) — asserts interface presence on both concrete types, common field set, consistent scalar types, and backward-compat fields.
  • Regression: full com.dotcms.graphql.** unit-test package passes (24 tests).
  • Manual integration smoke via GraphiQL on a content type with Binary, File, and Image fields — verify all three respond to the same sub-field query.
  • Postman collection for GraphQL binary-field queries (follow-up if missing).

Commits

  1. test(graphql): failing tests for unified DotBinaryLike interface #34540 — TDD red step.
  2. feat(graphql): unify Binary, File, and Image field queries via DotBinaryLike #34540 — implementation.

🤖 Generated with Claude Code

nollymar and others added 2 commits April 16, 2026 16:28
Adds a unit test suite that pins down the target contract for unifying
GraphQL queries across BinaryField, FileField, and ImageField: both
DotBinary and DotFileasset must implement a shared DotBinaryLike interface
and expose the same 12 common properties (name, title, size, mime,
versionPath, idPath, path, sha256, isImage, width, height, modDate).

Tests currently red — implementation follows in a subsequent commit.
Existing DotBinary assertions and DotFileasset backward-compat assertions
are already green and guard against regressions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…aryLike #34540

BinaryField maps to DotBinary and FileField/ImageField map to DotFileasset.
The two object types exposed completely different sub-fields, forcing
clients to know which dotCMS field type backed a property and to write
queries like `myfile { fileAsset { versionPath } }` while the equivalent
`mybinary { versionPath }` worked directly.

Introduce a shared DotBinaryLike GraphQL interface implemented by both
DotBinary and DotFileasset. It declares the 12 common binary properties
(name, title, size, mime, versionPath, idPath, path, sha256, isImage,
width, height, modDate). DotBinary resolves them from the binary map
already produced by BinaryToMapTransformer (no behavior change). DotFileasset
gets a new FileAssetBinaryPropertyDataFetcher that runs the same transformer
against the FileAsset/DotAsset contentlet, so the values returned are
identical across all three field types.

Type-specific extras stay on the concrete type — DotBinary still exposes
focalPoint, DotFileasset still exposes fileName, description, fileAsset,
metaData, showOnMenu, sortOrder. Existing queries continue to work.

Includes:
- New TypeUtil.createObjectType overloads accepting GraphQLInterfaceType
  varargs.
- DotBinaryLike registered via ContentAPIGraphQLTypesProvider so
  introspection sees it.
- TypeResolver dispatches on source type: Map -> DotBinary,
  Contentlet -> DotFileasset.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 16, 2026

Claude finished @nollymar's task in 2m 6s —— View job


PR Review

  • Read changed files
  • Analyze for issues

Issues Found

1. Performance regression — N transformer invocations per contentlet (FileAssetBinaryPropertyDataFetcher.java)

FileAssetBinaryPropertyDataFetcher.get() creates a new BinaryToMapTransformer on every field call. Because the 12 DotBinaryLike fields are each wired to the same fetcher instance but each invocation rebuilds the transformer, a single query that selects all 12 common fields on a DotFileasset will call DotTransformerBuilder().binaryToMapTransformer().content(contentlet).build().toMaps() 12 times for the same contentlet — instead of once.

// FileAssetBinaryPropertyDataFetcher.java:49–54
private Map<String, Object> resolveBinaryMap(final Contentlet contentlet) {
    final String var = contentlet.getContentType().baseType() == DOTASSET
            ? "asset" : FileAssetContentType.FILEASSET_FILEASSET_FIELD_VAR;
    final BinaryToMapTransformer transformer = new BinaryToMapTransformer(contentlet);  // ← rebuilt every call
    return (Map<String, Object>) transformer.asMap().get(var + "Map");
}

Compare with BinaryFieldDataFetcher, which returns the whole map once and lets MapFieldPropertiesDataFetcher extract individual keys from it. The new fetcher inverts this: it extracts one key per call but reconstructs the whole map each time.

The simplest fix is to cache the resolved map in DataFetchingEnvironment.getLocalContext() keyed by contentlet inode, so the 12 field calls for the same contentlet share one transformer invocation. Fix this →

2. Missing @Override on get() (FileAssetBinaryPropertyDataFetcher.java:30)

// Should be:
@Override
public Object get(final DataFetchingEnvironment environment) throws Exception {

3. TypeResolver returns null silently (CustomFieldType.java:98–107)

If source is neither Map nor Contentlet, the resolver returns null, which causes GraphQL-Java to throw a schema error at runtime with no log context. It won't happen today, but a defensive log before returning null would make future debugging much easier.

final TypeResolver dotBinaryLikeResolver = env -> {
    final Object source = env.getObject();
    if (source instanceof Map) { return customFieldTypes.get("BINARY"); }
    if (source instanceof Contentlet) { return customFieldTypes.get("FILEASSET"); }
    // ← no log, null return will surface as an opaque runtime error
    return null;
};

4. Tests cover schema structure only

CustomFieldTypeBinaryLikeTest verifies static schema wiring (interface presence, field names, scalar types) — all unit tests with no I/O. There are no tests for FileAssetBinaryPropertyDataFetcher.get() with an actual DataFetchingEnvironment mock, so the map key resolution logic ("asset" vs "fileAsset") and null-safety paths are untested. The manual integration test is marked pending.


Minor Observations

  • fileAssetTypeFields uses HashMap (unordered) while binaryTypeFields uses HashMap and commonBinaryFields uses LinkedHashMap. Inconsistent ordering may affect schema introspection output for DotFileasset fields.
  • The unchecked cast (Map<String, Object>) transformer.asMap().get(var + "Map") in resolveBinaryMap has no @SuppressWarnings annotation but it's on the method, not the cast. Minor.
  • The cast in the test at line 41 — ((GraphQLInterfaceType) i) — is unsafe if future code adds a non-interface GraphQLNamedType to these types. Using instanceof first would be safer.

The performance issue (#1) is the only one I'd block on. The rest are polish. The additive-only compatibility guarantee and the TypeResolver logic are sound.

@github-actions github-actions bot added the Area : Backend PR changes Java/Maven backend code label Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant