From 501a36618b032591da507ba94925676f620f5f32 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Wed, 16 Sep 2026 20:31:19 -0600 Subject: [PATCH 01/10] feat(graphql): expose the real content type behind Image and File fields (#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) --- .../com/dotcms/graphql/CustomFieldType.java | 23 ++ .../com/dotcms/graphql/InterfaceType.java | 34 ++ .../ContentAPIGraphQLTypesProvider.java | 13 + .../datafetcher/AssetContentDataFetcher.java | 24 ++ .../src/test/java/com/dotcms/MainSuite1b.java | 2 + .../business/AssetFieldValueContractTest.java | 229 +++++++++++++ .../business/AssetSubtypeAccessTest.java | 313 ++++++++++++++++++ .../graphql/business/GraphqlAPITest.java | 14 +- .../graphql/business/GraphqlQueryRunner.java | 83 +++++ .../contracts/graphql-schema.md | 186 +++++++++++ .../data-model.md | 122 +++++++ 11 files changed, 1041 insertions(+), 2 deletions(-) create mode 100644 dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java create mode 100644 specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md create mode 100644 specs/34540-graphql-asset-subtype-fields/data-model.md diff --git a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java index b3270d541190..9f609ab1f89a 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java @@ -1,6 +1,7 @@ package com.dotcms.graphql; import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotcms.graphql.datafetcher.AssetContentDataFetcher; import com.dotcms.graphql.datafetcher.BinaryFieldDataFetcher; import com.dotcms.graphql.datafetcher.FieldDataFetcher; import com.dotcms.graphql.datafetcher.KeyValueFieldDataFetcher; @@ -59,6 +60,12 @@ public String getTypeName() { return typeName; } + /** + * Name of the field on {@code DotFileasset} that exposes the referenced asset described by its + * real content type. + */ + public static final String ASSET_CONTENT_FIELD_VAR = "content"; + private static Map customFieldTypes = new HashMap<>(); static { @@ -158,6 +165,22 @@ public String getTypeName() { new TypeFetcher(list(CustomFieldType.KEY_VALUE.getType()), new KeyValueFieldDataFetcher())); fileAssetTypeFields.put(FILEASSET_SHOW_ON_MENU_FIELD_VAR, new TypeFetcher(list(GraphQLString), new MultiValueFieldDataFetcher())); fileAssetTypeFields.put(FILEASSET_SORT_ORDER_FIELD_VAR, new TypeFetcher(GraphQLInt, new FieldDataFetcher())); + + // The referenced asset, described by its real content type. The six fields above are a + // flat view that reports asset content using property names borrowed from the FILEASSET + // base type, so a customer's own fields -- and even the asset's identifier -- are + // unreachable through them. This field is purely additive: nothing above changes. See + // issue #34540. + // + // Referenced by NAME rather than by calling InterfaceType.getAssetContentInterface(). + // InterfaceType's static initializer reaches ContentAPIGraphQLTypesProvider, which reads + // this enum -- resolving the instance here would close that cycle and observe a + // half-initialized class. The constant is a compile-time String, so it does not trigger + // InterfaceType's initialization. + fileAssetTypeFields.put(ASSET_CONTENT_FIELD_VAR, new TypeFetcher( + new GraphQLTypeReference(InterfaceType.ASSET_CONTENT_INTERFACE_NAME), + new AssetContentDataFetcher())); + customFieldTypes.put("FILEASSET", TypeUtil.createObjectType(FILEASSET.getTypeName(), fileAssetTypeFields)); final Map siteTypeFields = new HashMap<>(ContentFields.getContentFields()); diff --git a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java index e17fa0a33d31..ae93454eba54 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java @@ -69,6 +69,15 @@ public enum InterfaceType { public static final String FORM_INTERFACE_NAME = "FormBaseType"; public static final String DOTASSET_INTERFACE_NAME = "DotAssetBaseType"; + /** + * Describes the content an Image or File field points at, by whatever content type it actually + * is. Unlike the interfaces above it is not tied to a single base type: an asset-pointing field + * can hold either DOTASSET- or FILEASSET-based content — an Image field resolves a FileAsset + * perfectly well today — so neither {@link #DOTASSET_INTERFACE_NAME} nor + * {@link #FILE_INTERFACE_NAME} alone can describe what such a field may return. See #34540. + */ + public static final String ASSET_CONTENT_INTERFACE_NAME = "DotAssetContent"; + public static final String DOT_CONTENTLET = "DotContentlet"; static { @@ -125,6 +134,31 @@ public enum InterfaceType { addBaseTypeFields(dotAssetFields, ImmutableDotAssetContentType.builder().name("dummy") .build().requiredFields()); interfaceTypes.put("DOTASSET", createInterfaceType(DOTASSET_INTERFACE_NAME, dotAssetFields, new ContentResolver())); + + // Carries the common content fields only. The two base types name their binary + // differently -- `asset` for DOTASSET, `fileAsset` for FILEASSET -- so there is no shared + // binary property to put here; a client reaches it through a narrowing clause. + assetContentInterface = createInterfaceType(ASSET_CONTENT_INTERFACE_NAME, + new HashMap<>(contentFields), new ContentResolver()); + } + + private static GraphQLInterfaceType assetContentInterface; + + /** + * @return the interface describing what an asset-pointing field returns, implemented by every + * content type derived from either asset base type. + */ + public static GraphQLInterfaceType getAssetContentInterface() { + return assetContentInterface; + } + + /** + * @return whether content of this base type can sit behind an Image or File field, and must + * therefore implement {@link #getAssetContentInterface()}. + */ + public static boolean isAssetBaseType(final BaseContentType baseContentType) { + return BaseContentType.DOTASSET == baseContentType + || BaseContentType.FILEASSET == baseContentType; } /** diff --git a/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java index c8d01befb8d6..e4ef70594f64 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java @@ -152,6 +152,11 @@ private Set getContentAPITypes() throws DotDataException { Logger.debug(this, ()-> "Getting all Content Types for GraphQL Schema"); final Set contentAPITypes = new HashSet<>(InterfaceType.valuesAsSet()); + // Not part of InterfaceType.values(): that enum is keyed by base type, and this interface + // deliberately spans two of them. It still has to be registered or introspection cannot + // see it and no fragment can narrow through it. See #34540. + contentAPITypes.add(InterfaceType.getAssetContentInterface()); + contentAPITypes.addAll(CustomFieldType.getCustomFieldTypes()); List allTypes = APILocator.getContentTypeAPI(APILocator.systemUser()) @@ -191,6 +196,14 @@ private GraphQLObjectType createType(ContentType contentType) { builder.withInterface(InterfaceType.getInterfaceForBaseType(contentType.baseType())); } + // Anything derived from an asset base type can sit behind an Image or File field, so it + // must be reachable through that field's interface. Declaring it here is what puts the + // type in the interface's possible-type set -- which is why a content type the customer + // creates later is reachable with no registration step of its own. + if (InterfaceType.isAssetBaseType(contentType.baseType())) { + builder.withInterface(InterfaceType.getAssetContentInterface()); + } + builder.fields(createFieldsForType(contentType)); builder.withInterface(InterfaceType.CONTENTLET.getType()); diff --git a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java new file mode 100644 index 000000000000..2ea797648682 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java @@ -0,0 +1,24 @@ +package com.dotcms.graphql.datafetcher; + +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import graphql.schema.DataFetcher; +import graphql.schema.DataFetchingEnvironment; + +/** + * Hands back the asset {@link Contentlet} that an Image or File field points at, so it can be + * described by its real content type rather than by the flat {@code DotFileasset} view. + * + *

There is deliberately no lookup here. {@link FileFieldDataFetcher} has already resolved and + * hydrated the referenced contentlet and passes it down as the source, so this only unwraps it. + * Resolving it a second time would make the cost of reading an asset grow with the number of + * properties selected, which issue #34540 explicitly rules out. + * + * @see com.dotcms.graphql.InterfaceType#ASSET_CONTENT_INTERFACE_NAME + */ +public class AssetContentDataFetcher implements DataFetcher { + + @Override + public Contentlet get(final DataFetchingEnvironment environment) { + return environment.getSource(); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index f9332c46ce24..7a8b1fc6c66e 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -21,6 +21,8 @@ com.dotcms.visitor.filter.logger.VisitorLoggerTest.class, com.dotcms.visitor.filter.characteristics.VisitorCharacterTest.class, com.dotcms.graphql.business.GraphqlAPITest.class, + com.dotcms.graphql.business.AssetFieldValueContractTest.class, + com.dotcms.graphql.business.AssetSubtypeAccessTest.class, com.dotcms.contenttype.test.ContentTypeTest.class, com.dotcms.contenttype.test.DeleteFieldJobTest.class, com.dotcms.content.elasticsearch.business.ESSiteSearchAPITest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java new file mode 100644 index 000000000000..9e9450088554 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java @@ -0,0 +1,229 @@ +package com.dotcms.graphql.business; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.contenttype.model.field.DataTypes; +import com.dotcms.contenttype.model.field.FileField; +import com.dotcms.contenttype.model.field.ImageField; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.datagen.DotAssetDataGen; +import com.dotcms.datagen.FieldDataGen; +import com.dotcms.datagen.FileAssetDataGen; +import com.dotcms.datagen.SiteDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.util.FileUtil; +import com.liferay.portal.model.User; +import java.io.File; +import java.util.List; +import java.util.Map; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Locks what the six properties of {@code DotFileasset} return today, so that adding the new + * asset-subtype capability cannot change any of them. See issue #34540, FR-012 and SC-008. + * + *

This is deliberately a value contract, not a schema-shape one. {@code GraphqlAPITest} + * already covers the shape — which types and fields exist — and a change could satisfy every one + * of those assertions while quietly altering what a field returns. These tests execute real + * queries through {@link GraphqlQueryRunner} and compare the answers. + * + *

The two that matter most are the ones that look wrong. For image-style content, + * {@code fileName} does not read a stored file name and {@code description} does not read a stored + * description: both are synthesized by base-type ternaries in {@code CustomFieldType}, falling + * back to the contentlet's name and title respectively. So {@code description} returns the file + * name. That is the shipped contract, customers query it, and "fixing" it would change what a live + * query returns without failing it — the silent break this whole design exists to avoid. If one of + * those assertions fails, the correct response is almost certainly to revert the change, not to + * update the expectation. + */ +public class AssetFieldValueContractTest extends IntegrationTestBase { + + private static final String IMAGE_FIELD_VAR = "assetContractImage"; + private static final String FILE_FIELD_VAR = "assetContractFile"; + + private static User systemUser; + private static Host site; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + site = new SiteDataGen().nextPersisted(); + } + + /** + * Given: an Image field pointing at DOTASSET-based content. + * When: the six long-standing properties are selected. + * Then: each returns exactly what it returns today — including the two synthesized values. + */ + @Test + public void test_dotAsset_throughImageField_sixPropertiesUnchanged() throws Exception { + final File file = FileUtil.createTemporaryFile("value-contract", ".txt", "contract"); + final Contentlet dotAsset = new DotAssetDataGen(site, file) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + // The asset must be published too: the fetcher looks it up with the *holder's* live + // state, so a working-only asset resolves to nothing behind a published holder. + ContentletDataGen.publish(dotAsset); + + final ContentType holder = newHolderType(); + final String imageVar = IMAGE_FIELD_VAR; + + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setProperty(imageVar, dotAsset.getIdentifier()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + + final Map asset = queryAssetField(holder, imageVar, content); + + // fileName is NOT a stored value here: the ternary falls back to the contentlet name. + assertEquals("fileName must keep returning the contentlet name for DOTASSET content", + dotAsset.getName(), asset.get("fileName")); + + // description is NOT the stored description: the ternary falls back to the title. + assertEquals("description must keep returning the TITLE for DOTASSET content, not the " + + "stored description — changing this is a silent break", + dotAsset.getTitle(), asset.get("description")); + + assertNotNull("fileAsset must still resolve", asset.get("fileAsset")); + assertNotNull("metaData must still resolve", asset.get("metaData")); + assertTrue("all six properties must still be selectable", + asset.keySet().containsAll( + List.of("fileName", "description", "fileAsset", "metaData", + "showOnMenu", "sortOrder"))); + } + + /** + * Given: a File field pointing at FILEASSET-based content. + * When: the six long-standing properties are selected. + * Then: each returns exactly what it returns today. Here the values are stored rather than + * synthesized, which is precisely why this case must be asserted separately from the one above. + */ + @Test + public void test_fileAsset_throughFileField_sixPropertiesUnchanged() throws Exception { + final File file = FileUtil.createTemporaryFile("value-contract", ".txt", "contract"); + final Contentlet fileAsset = new FileAssetDataGen(site, file) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + // See the note in the DOTASSET test: the asset's live state must match the holder's. + ContentletDataGen.publish(fileAsset); + + final ContentType holder = newHolderType(); + final String fileVar = FILE_FIELD_VAR; + + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setProperty(fileVar, fileAsset.getIdentifier()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + + final Map asset = queryAssetField(holder, fileVar, content); + + assertEquals("fileName must keep returning the stored file name for FILEASSET content", + fileAsset.getStringProperty("fileName"), asset.get("fileName")); + assertNotNull("fileAsset must still resolve", asset.get("fileAsset")); + assertNotNull("metaData must still resolve", asset.get("metaData")); + assertTrue("all six properties must still be selectable", + asset.keySet().containsAll( + List.of("fileName", "description", "fileAsset", "metaData", + "showOnMenu", "sortOrder"))); + } + + /** + * Given: an empty asset field. + * When: it is selected. + * Then: an explicit empty result, no error, and the rest of the query is unaffected (FR-007). + */ + @Test + public void test_emptyAssetField_returnsNullWithoutError() throws Exception { + final ContentType holder = newHolderType(); + final String imageVar = IMAGE_FIELD_VAR; + + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { identifier %s { fileName } } }", + holder.variable(), content.getIdentifier(), imageVar); + + final Map data = + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + + final Map row = firstRow(data, holder); + assertEquals("the rest of the query must still be delivered", + content.getIdentifier(), row.get("identifier")); + assertEquals("an empty asset field must resolve to null, not an error", + null, row.get(imageVar)); + } + + /** + * Selects the six properties on {@code fieldVar} for {@code content} and returns the asset map. + */ + @SuppressWarnings("unchecked") + private Map queryAssetField(final ContentType holder, final String fieldVar, + final Contentlet content) throws Exception { + // Precondition, so a fixture problem is never mistaken for a product one: confirm the + // reference actually persisted before blaming the query for returning nothing. + final Contentlet reloaded = APILocator.getContentletAPI() + .find(content.getInode(), systemUser, false); + assertNotNull("the holder contentlet did not persist at all", reloaded); + assertNotNull("the asset reference never persisted on field '" + fieldVar + + "' — fixture problem, not a product one. Persisted keys: " + + reloaded.getMap().keySet(), + reloaded.get(fieldVar)); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { fileName description " + + "showOnMenu sortOrder fileAsset { name size mime } " + + "metaData { key value } } } }", + holder.variable(), content.getIdentifier(), fieldVar); + + final Map data = + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + final Map asset = + (Map) firstRow(data, holder).get(fieldVar); + assertNotNull("the asset field resolved to nothing — the fixture is wrong, not the code", + asset); + return asset; + } + + @SuppressWarnings("unchecked") + private Map firstRow(final Map data, final ContentType holder) { + final List> rows = + (List>) data.get(holder.variable() + "Collection"); + assertNotNull("no collection returned for " + holder.variable(), rows); + assertEquals("expected exactly one row", 1, rows.size()); + return rows.get(0); + } + + /** + * A content type carrying one Image field and one File field. + * + *

Two details here are easy to get wrong and both fail silently. First, an asset-reference + * field stores an identifier in a text column, so it needs {@code DataTypes.TEXT}; + * without it the field lands on {@code system_field}, the schema still advertises it, and any + * value set on it simply never persists. Second, {@code FieldDataGen} derives its default + * variable name from the current millisecond, so two fields created back to back can collide — + * hence the explicit names. + */ + private ContentType newHolderType() throws Exception { + final ContentType type = new ContentTypeDataGen().nextPersisted(); + new FieldDataGen().contentTypeId(type.id()).type(ImageField.class) + .dataType(DataTypes.TEXT).velocityVarName(IMAGE_FIELD_VAR).nextPersisted(); + new FieldDataGen().contentTypeId(type.id()).type(FileField.class) + .dataType(DataTypes.TEXT).velocityVarName(FILE_FIELD_VAR).nextPersisted(); + APILocator.getGraphqlAPI().invalidateSchema(); + return APILocator.getContentTypeAPI(systemUser).find(type.variable()); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java new file mode 100644 index 000000000000..3cbf5bd0b6cb --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -0,0 +1,313 @@ +package com.dotcms.graphql.business; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.contenttype.model.field.DataTypes; +import com.dotcms.contenttype.model.field.Field; +import com.dotcms.contenttype.model.field.FieldBuilder; +import com.dotcms.contenttype.model.field.FileField; +import com.dotcms.contenttype.model.field.ImageField; +import com.dotcms.contenttype.model.field.TextField; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.contenttype.model.type.ContentTypeBuilder; +import com.dotcms.contenttype.model.type.DotAssetContentType; +import com.dotcms.contenttype.model.type.FileAssetContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.datagen.FieldDataGen; +import com.dotcms.datagen.SiteDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.model.IndexPolicy; +import com.dotmarketing.portlets.folders.business.FolderAPI; +import com.dotmarketing.util.FileUtil; +import com.liferay.portal.model.User; +import java.io.File; +import java.util.List; +import java.util.Map; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Drives the capability issue #34540 asks for: a client selecting an Image or File field can reach + * the properties of the content type that field actually points at — including properties the + * customer defined on their own type — and can tell which type it received. + * + *

Today an asset-pointing field resolves to {@code DotFileasset}, a flat six-property type, so + * none of the below is reachable at any depth. These tests are expected to FAIL until the new + * asset description is in place; they are the Red signal for User Story 1. + * + *

Two traps already paid for in {@link AssetFieldValueContractTest} and repeated here: an + * asset-reference field needs {@code DataTypes.TEXT} or its value never persists, and the + * referenced asset must be published to the same state as the holder or the fetcher resolves + * nothing. + */ +public class AssetSubtypeAccessTest extends IntegrationTestBase { + + private static final String IMAGE_FIELD_VAR = "subtypeImage"; + private static final String FILE_FIELD_VAR = "subtypeFile"; + private static final String CUSTOM_PROPERTY_VAR = "campaignName"; + + /** The new field on {@code DotFileasset} that exposes the properly described asset. */ + private static final String ASSET_CONTENT_FIELD = "content"; + + private static User systemUser; + private static Host site; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + site = new SiteDataGen().nextPersisted(); + } + + /** + * Given: a customer-defined content type extending DOTASSET with a property of its own, and an + * Image field pointing at content of it. + * When: that property is requested through the Image field. + * Then: its stored value is returned. + * + *

This is the capability the issue exists for, and the one with no workaround today. + */ + @Test + public void test_customPropertyOnDotAssetSubtype_isReadableThroughImageField() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Summer Sale"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " + + "... on %s { %s } } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + ASSET_CONTENT_FIELD, assetType.variable(), CUSTOM_PROPERTY_VAR); + + final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); + assertEquals("the customer's own property must be readable through the Image field", + "Summer Sale", assetContent.get(CUSTOM_PROPERTY_VAR)); + } + + /** + * Given: a customer-defined content type extending FILEASSET with a property of its own, and a + * File field pointing at content of it. + * When: that property is requested through the File field. + * Then: its stored value is returned. + */ + @Test + public void test_customPropertyOnFileAssetSubtype_isReadableThroughFileField() throws Exception { + final ContentType assetType = newFileAssetSubtype(); + final Contentlet asset = newFileAssetOf(assetType, "Datasheets"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, FILE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " + + "... on %s { %s } } } } }", + holder.variable(), content.getIdentifier(), FILE_FIELD_VAR, + ASSET_CONTENT_FIELD, assetType.variable(), CUSTOM_PROPERTY_VAR); + + final Map assetContent = queryAssetContent(query, holder, FILE_FIELD_VAR); + assertEquals("the customer's own property must be readable through the File field", + "Datasheets", assetContent.get(CUSTOM_PROPERTY_VAR)); + } + + /** + * Given: an asset carrying tags. + * When: its tags are requested through the field pointing at it. + * Then: they are returned. + * + *

This is the AI tagging blocker named in the issue: {@code tags} is not on the flat type, + * so generated tags cannot be read back at all. + */ + @Test + public void test_tags_areReadableThroughAssetField() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Tagged"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " + + "... on %s { tags } } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + ASSET_CONTENT_FIELD, assetType.variable()); + + final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); + assertNotNull("tags must be reachable through an asset field", assetContent.get("tags")); + } + + /** + * Given: an asset behind an Image field. + * When: the asset's own identity is requested — identifier, inode, live state, title. + * Then: each is returned and matches the asset's record. + * + *

None of these are on the flat type today, so an Image field cannot currently tell a + * client *which* asset it is pointing at. + */ + @Test + public void test_assetOwnIdentity_isReadableThroughAssetField() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Identity"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " + + "identifier inode live title } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD); + + final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); + assertEquals("the asset's identifier must be reachable", + asset.getIdentifier(), assetContent.get("identifier")); + assertEquals("the asset's inode must be reachable", + asset.getInode(), assetContent.get("inode")); + assertEquals("the asset's live state must be reachable", true, assetContent.get("live")); + assertNotNull("the asset's title must be reachable", assetContent.get("title")); + } + + /** + * Given: the schema has already been built. + * When: a customer creates a brand-new content type extending an asset base type, and + * immediately queries a property of it through an asset field. + * Then: it works, with no administrative step in between. + * + *

This is the test that distinguishes a derived set of asset types from an enumerated one + * (FR-001a). A hardcoded list of known asset types would satisfy every other test in this + * class and fail this one. + */ + @Test + public void test_contentTypeCreatedAfterSchemaBuild_isImmediatelyReachable() throws Exception { + // Build the schema first, so the type below cannot have been present when it was made. + APILocator.getGraphqlAPI().getSchema(systemUser); + + final ContentType lateType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(lateType, "Created Late"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " + + "__typename ... on %s { %s } } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + ASSET_CONTENT_FIELD, lateType.variable(), CUSTOM_PROPERTY_VAR); + + final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); + assertEquals("a type created after the schema was built must still be reachable", + "Created Late", assetContent.get(CUSTOM_PROPERTY_VAR)); + assertEquals("__typename must name the customer's own type", + lateType.variable(), assetContent.get("__typename")); + } + + // ---------------------------------------------------------------- helpers + + @SuppressWarnings("unchecked") + private Map queryAssetContent(final String query, final ContentType holder, + final String fieldVar) throws Exception { + final Map data = + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + final List> rows = + (List>) data.get(holder.variable() + "Collection"); + assertNotNull("no collection returned for " + holder.variable(), rows); + assertEquals("expected exactly one row", 1, rows.size()); + + final Map assetField = (Map) rows.get(0).get(fieldVar); + assertNotNull("the asset field resolved to nothing — check the fixture", assetField); + + final Map assetContent = + (Map) assetField.get(ASSET_CONTENT_FIELD); + assertNotNull("the asset field exposed no '" + ASSET_CONTENT_FIELD + "'", assetContent); + return assetContent; + } + + /** A customer-defined content type extending DOTASSET, with a property of its own. */ + private ContentType newDotAssetSubtype() throws Exception { + final String variable = "subtypeAsset" + System.nanoTime(); + final ContentType type = APILocator.getContentTypeAPI(systemUser).save( + ContentTypeBuilder.builder(DotAssetContentType.class) + .folder(FolderAPI.SYSTEM_FOLDER).host(Host.SYSTEM_HOST) + .name(variable).variable(variable) + .owner(systemUser.getUserId()).build()); + addCustomProperty(type); + return APILocator.getContentTypeAPI(systemUser).find(type.variable()); + } + + /** A customer-defined content type extending FILEASSET, with a property of its own. */ + private ContentType newFileAssetSubtype() throws Exception { + final String variable = "subtypeFileAsset" + System.nanoTime(); + final ContentType type = APILocator.getContentTypeAPI(systemUser).save( + ContentTypeBuilder.builder(FileAssetContentType.class) + .folder(FolderAPI.SYSTEM_FOLDER).host(Host.SYSTEM_HOST) + .name(variable).variable(variable) + .owner(systemUser.getUserId()).build()); + addCustomProperty(type); + return APILocator.getContentTypeAPI(systemUser).find(type.variable()); + } + + private void addCustomProperty(final ContentType type) throws Exception { + final Field field = FieldBuilder.builder(TextField.class) + .name(CUSTOM_PROPERTY_VAR).variable(CUSTOM_PROPERTY_VAR) + .contentTypeId(type.id()).dataType(DataTypes.TEXT).indexed(true).build(); + APILocator.getContentTypeFieldAPI().save(field, systemUser); + APILocator.getGraphqlAPI().invalidateSchema(); + } + + private Contentlet newAssetOf(final ContentType assetType, final String propertyValue) + throws Exception { + final File file = FileUtil.createTemporaryFile("subtype", ".txt", "subtype"); + final Contentlet asset = new ContentletDataGen(assetType.id()) + .host(site) + .setProperty(DotAssetContentType.ASSET_FIELD_VAR, file) + .setProperty(DotAssetContentType.SITE_OR_FOLDER_FIELD_VAR, site.getIdentifier()) + .setProperty(DotAssetContentType.TAGS_FIELD_VAR, "subtypeTag") + .setProperty(CUSTOM_PROPERTY_VAR, propertyValue) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(asset); + return asset; + } + + private Contentlet newFileAssetOf(final ContentType assetType, final String propertyValue) + throws Exception { + final File file = FileUtil.createTemporaryFile("subtype", ".txt", "subtype"); + final Contentlet asset = new ContentletDataGen(assetType.id()) + .host(site) + .setProperty(FileAssetContentType.FILEASSET_FILEASSET_FIELD_VAR, file) + .setProperty(FileAssetContentType.FILEASSET_FILE_NAME_FIELD_VAR, file.getName()) + .setProperty(FileAssetContentType.FILEASSET_SITE_OR_FOLDER_FIELD_VAR, + site.getIdentifier()) + .setProperty(CUSTOM_PROPERTY_VAR, propertyValue) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(asset); + return asset; + } + + /** A content type carrying one Image field and one File field. */ + private ContentType newHolderType() throws Exception { + final ContentType type = new ContentTypeDataGen().nextPersisted(); + new FieldDataGen().contentTypeId(type.id()).type(ImageField.class) + .dataType(DataTypes.TEXT).velocityVarName(IMAGE_FIELD_VAR).nextPersisted(); + new FieldDataGen().contentTypeId(type.id()).type(FileField.class) + .dataType(DataTypes.TEXT).velocityVarName(FILE_FIELD_VAR).nextPersisted(); + APILocator.getGraphqlAPI().invalidateSchema(); + return APILocator.getContentTypeAPI(systemUser).find(type.variable()); + } + + private Contentlet newHolderContent(final ContentType holder, final String fieldVar, + final Contentlet asset) throws Exception { + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setProperty(fieldVar, asset.getIdentifier()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + return content; + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java index 5d8f5d8fbf1f..24254eeae6dd 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java @@ -961,13 +961,23 @@ private void setMockRelationshipAPI(Field relationshipField) ContentAPIGraphQLTypesProvider.INSTANCE.setFieldGeneratorFactory(fieldGeneratorFactory); } + /** + * Asserts that every FileAsset field is present on the given type — which is what this + * method is named for, and what the calling test's javadoc describes. + * + *

It previously used {@code allMatch}, which asserted the opposite: that the type exposed + * no field other than those six. That made it a lock against ever adding a field to + * {@code DotFileasset}, so any such addition failed here rather than where the change was + * made. Presence is the contract; the type is free to expose more. + */ private boolean areFileassetFieldsPresent(final GraphQLObjectType objectType) { final List fileAssetFields = list(FILEASSET_FILE_NAME_FIELD_VAR, FILEASSET_DESCRIPTION_FIELD_VAR, FILEASSET_FILEASSET_FIELD_VAR, FILEASSET_METADATA_FIELD_VAR, FILEASSET_SHOW_ON_MENU_FIELD_VAR, FILEASSET_SORT_ORDER_FIELD_VAR); - return objectType.getFieldDefinitions().stream().allMatch(fieldDefinition -> - fileAssetFields.contains(fieldDefinition.getName())); + final Set actualFields = objectType.getFieldDefinitions().stream() + .map(GraphQLFieldDefinition::getName).collect(Collectors.toSet()); + return actualFields.containsAll(fileAssetFields); } private ContentType createAndSaveSimpleContentType(final String name) throws DotSecurityException, DotDataException { diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java new file mode 100644 index 000000000000..fce7022c13ca --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java @@ -0,0 +1,83 @@ +package com.dotcms.graphql.business; + +import com.dotcms.graphql.DotGraphQLContext; +import com.dotmarketing.business.APILocator; +import com.liferay.portal.model.User; +import graphql.ExecutionInput; +import graphql.ExecutionResult; +import graphql.GraphQL; +import graphql.GraphQLError; +import graphql.schema.GraphQLSchema; +import java.util.List; +import java.util.Map; + +/** + * Runs real GraphQL queries against the live schema from an integration test. + * + *

Before this existed, no integration test executed a query — {@code GraphqlAPITest} only + * inspects the shape of the schema, so the only place a query's actual values were + * asserted was the Postman collection. That left a gap: a change could keep every type and field + * in place while quietly altering what a field returns, and nothing in the Java test suite would + * notice. + * + *

The context matters. {@code FileFieldDataFetcher} and friends read the calling user from + * {@link DotGraphQLContext} via {@code environment.getContext()}, so a query executed without one + * resolves nothing. That is why every execution here is given a context carrying the user. + * + *

Not a test class — named so surefire/failsafe do not try to run it. + */ +public class GraphqlQueryRunner { + + private GraphqlQueryRunner() { + } + + /** + * Executes {@code query} as {@code user} against the current schema. + * + * @return the raw result, errors included — callers that want a failure to be loud should use + * {@link #executeAndExpectSuccess(String, User)} instead. + */ + public static ExecutionResult execute(final String query, final User user) throws Exception { + final GraphQLSchema schema = APILocator.getGraphqlAPI().getSchema(user); + final DotGraphQLContext context = DotGraphQLContext.createServletContext() + .with(user) + .build(); + + final ExecutionInput input = ExecutionInput.newExecutionInput() + .query(query) + .context(context) + .build(); + + return GraphQL.newGraphQL(schema).build().execute(input); + } + + /** + * Executes {@code query} and returns its {@code data}, failing with the GraphQL errors in the + * message if the query did not succeed. Use this when the query is expected to work — a bare + * {@code NullPointerException} on the data map tells you nothing about why. + */ + public static Map executeAndExpectSuccess(final String query, final User user) + throws Exception { + final ExecutionResult result = execute(query, user); + final List errors = result.getErrors(); + if (errors != null && !errors.isEmpty()) { + throw new AssertionError("GraphQL query failed: " + errors + "\nQuery was:\n" + query); + } + return result.getData(); + } + + /** + * Executes {@code query} expecting it to be rejected, and returns the errors. + * + * @throws AssertionError if the query unexpectedly succeeded + */ + public static List executeAndExpectFailure(final String query, final User user) + throws Exception { + final ExecutionResult result = execute(query, user); + final List errors = result.getErrors(); + if (errors == null || errors.isEmpty()) { + throw new AssertionError("Expected the query to be rejected, but it succeeded:\n" + query); + } + return errors; + } +} diff --git a/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md b/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md new file mode 100644 index 000000000000..b2a9778b98a3 --- /dev/null +++ b/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md @@ -0,0 +1,186 @@ +# Contract: GraphQL schema for asset-pointing fields + +**Feature**: `specs/34540-graphql-asset-subtype-fields/` · **Issue**: dotCMS/core#34540 + +The GraphQL schema **is** the contract for this feature. Names below are working names; the tasks +phase settles them. SDL is illustrative of shape, not of the generated output byte-for-byte. + +--- + +## 1. What exists today (must not change) + +```graphql +type DotFileasset { + fileName: String + description: String + fileAsset: DotBinary + metaData: [DotKeyValue] + showOnMenu: [String] + sortOrder: Int +} +``` + +Every `ImageField` and `FileField` on every content type resolves to this type. + +**Frozen behaviors** — these are contract, not accidents, and correcting them is forbidden: + +| Selection | Returns, for image-style content | Returns, for file-style content | +|---|---|---| +| `fileName` | the contentlet's name | the stored file name | +| `description` | the contentlet's **title** (i.e. the file name) | the stored description | + +Verified live: `image { fileName description }` returned the same string for both. Against the +asset's own type, the *stored* description is populated for 2 of 57 images. Correcting +`description` here would change what a live query returns without failing it. + +--- + +## 2. What is added + +```graphql +type DotFileasset { + fileName: String @deprecated(reason: "Use `content { title }`, or `content { ... on FileAsset { fileName } }` for file-style assets.") + description: String @deprecated(reason: "Returns the asset title, not its description. Use `content { ... on Images { description } }` for the stored value.") + fileAsset: DotBinary @deprecated(reason: "Use `content { ... on DotAssetBaseType { asset } }`, or `... on FileBaseType { fileAsset }`.") + metaData: [DotKeyValue] @deprecated(reason: "Use `content { ... on FileBaseType { metaData } }`.") + showOnMenu: [String] @deprecated(reason: "Use `content { ... on FileAsset { showOnMenu } }`.") + sortOrder: Int @deprecated(reason: "Use `content { ... on FileAsset { sortOrder } }`.") + + "The referenced asset, described by its real content type." + content: DotAssetContent +} + +interface DotAssetContent { + identifier: ID + inode: String + title: String + host: Site + folder: String + live: Boolean + working: Boolean + archived: Boolean + locked: Boolean + urlMap: String + modDate: String + modUser: String + owner: String + publishDate: String + publishUser: String + creationDate: String + conLanguage: Language + contentType: String + baseType: String + titleImage: DotBinary + dotStyleProperties: JSON + _map: JSON +} +``` + +Every content type whose base type is DOTASSET or FILEASSET additionally declares +`implements DotAssetContent`: + +```graphql +type Images implements DotAssetContent & DotAssetBaseType & DotContentlet { ... } +type FileAsset implements DotAssetContent & FileBaseType & DotContentlet { ... } +type BannerImages implements DotAssetContent & DotAssetBaseType & DotContentlet { campaignName: String adSize: String ... } +``` + +**The possible-type set is derived, never listed.** It is whatever content types exist when the +schema is built, and the schema is already rebuilt when a content type or field changes. A type a +customer creates after deployment is reachable with no administrative step (FR-001a, FR-005). + +--- + +## 3. Required client-visible behavior + +### 3.1 Shared properties at both levels (FR-015) + +```graphql +image { + content { + identifier # directly on the interface + ... on Images { identifier # and again inside a clause + tags } + } +} +``` + +Both are valid. A client is never forced to choose one level. + +### 3.2 Narrowing (FR-016, FR-016a, FR-017, FR-018) + +| Client writes | Result | Status | +|---|---|---| +| a clause on a type the asset **is** | its properties are returned | 200, data | +| a clause on a possible type the asset **is not** | contributes nothing, rest of the response delivered, **warning** names the clause | 200, data + `extensions` | +| a clause on a type that **does not exist** | request fails | validation error, no data | +| clauses on the base kind **and** the concrete type | properties **merge** into one object | 200, data | + +Across a result set of mixed types, every matching asset is populated, every non-matching asset is +still returned, and one non-match never suppresses the matches (FR-016a). + +There is no cast to fail here: a clause is a condition, not a coercion. + +### 3.3 Warning shape + +```json +{ + "data": { "...": "delivered normally" }, + "extensions": { + "warnings": [ + { "path": "BannerCollection.image.content", + "typeCondition": "PDFDocuments", + "message": "No asset at this path was of type PDFDocuments." } + ] + } +} +``` + +Warnings name only the type the client itself wrote and the path it chose — **never asset content** +(Constitution III). A client that ignores `extensions` is unaffected. + +--- + +## 4. Worked example + +**Before** — works today, and must keep working unchanged: + +```graphql +{ BannerCollection { title image { fileName description fileAsset { versionPath size mime } } } } +``` + +**After** — the same query still valid, plus what was unreachable: + +```graphql +{ + BannerCollection { + title + image { + content { + identifier + title + __typename + ... on DotAssetBaseType { asset { versionPath size mime } } + ... on Images { tags description } + ... on BannerImages { campaignName adSize } + } + } + } +} +``` + +`... on Images { description }` returns the asset's **stored** description — a different value from +the top-level `image { description }`, which keeps returning the title. Two names, two meanings, +neither surprising the other. That separation is the point of FR-009a. + +--- + +## 5. Compatibility guarantees + +| Guarantee | Requirement | +|---|---| +| Every selection valid today is still valid | FR-012, SC-008 | +| Every such selection returns the same value | FR-012, SC-008 | +| Superseded properties are marked in the schema itself, with replacements named | FR-012a, SC-009 | +| Removal happens in no release introduced by this feature | FR-012b | +| Rolling back removes only the added field; a client on the old selection set is unaffected | Legacy Impact | diff --git a/specs/34540-graphql-asset-subtype-fields/data-model.md b/specs/34540-graphql-asset-subtype-fields/data-model.md new file mode 100644 index 000000000000..827135a987f2 --- /dev/null +++ b/specs/34540-graphql-asset-subtype-fields/data-model.md @@ -0,0 +1,122 @@ +# Phase 1 Data Model: GraphQL Asset Subtype Access + +**Feature**: `specs/34540-graphql-asset-subtype-fields/` · **Issue**: dotCMS/core#34540 + +No persistent data model changes: no database table, no column, no index mapping, no serialized +state. The entities below are **GraphQL schema elements** — the shape the delivery API presents. +Working names; the tasks phase settles the final ones. + +--- + +## Entities + +### `DotFileasset` (existing — extended, never altered) + +The type every `ImageField` and `FileField` resolves to today. Built in the static block of +`CustomFieldType`. + +| Property | Type | Change | Notes | +|---|---|---|---| +| `fileName` | String | **deprecated**, behavior frozen | Synthesized: falls back to `contentlet.getName()` when the base type is not FILEASSET. Must keep doing so (R4). | +| `description` | String | **deprecated**, behavior frozen | Synthesized: falls back to `contentlet.getTitle()` when the base type is DOTASSET. Returns the file name for image-style content. Must keep doing so (R4). | +| `fileAsset` | `DotBinary` | **deprecated**, behavior frozen | | +| `metaData` | `[DotKeyValue]` | **deprecated**, behavior frozen | | +| `showOnMenu` | `[String]` | **deprecated**, behavior frozen | | +| `sortOrder` | Int | **deprecated**, behavior frozen | | +| **`content`** | **`DotAssetContent`** | **NEW** | The referenced asset, described by its real content type. | + +**Invariants** + +- Every existing property keeps its name, its type, and the exact value it returns today + (FR-012, SC-008). "Frozen" includes the two synthesized values — correcting them is forbidden. +- Each deprecated property carries a reason naming its replacement path (FR-012a, SC-009). +- `@deprecated` marks **fields**; GraphQL cannot deprecate an object type, so the type itself + carries no marking. +- The customer's own `image` / `file1` field is **not** deprecated — it stays the way in. + +--- + +### `DotAssetContent` (new — interface) + +The referenced asset, described by what it actually is. + +**Possible types**: every content type whose base type is DOTASSET or FILEASSET — the system ones +and every customer-defined one, including types created after the schema was last built. Derived +from the content types present at schema-build time; **never enumerated** (FR-001a). Schema rebuild +is already triggered by `ContentTypeAndFieldsModsListeners` on content type and field changes +(FR-005). + +**Fields**: the common content fields — `identifier`, `inode`, `title`, `host`, `folder`, `live`, +`working`, `archived`, `locked`, `urlMap`, `modDate`, `modUser`, `owner`, `publishDate`, +`publishUser`, `creationDate`, `conLanguage`, `contentType`, `baseType`, `titleImage`, +`dotStyleProperties`, `_map`. + +Nothing base-type-specific: the DOTASSET binary is `asset` and the FILEASSET binary is `fileAsset`, +different names, so the binary is reached through a narrowing clause — either on the concrete type +or on the existing base-kind interface. + +**Invariants** + +- Spans **both** asset base types, so an Image field that points at file-style content, and a File + field that points at image-style content, both narrow correctly (FR-001b — verified: an Image + field resolved a `.vtl` FileAsset). +- Its fields are selectable directly **and** inside a narrowing clause (FR-015). +- Resolves to the concrete content type, so `__typename` distinguishes two asset types (FR-003). +- Interface, not union — a union has no fields of its own and would fail FR-015. + +--- + +### `DotAssetBaseType`, `FileBaseType` (existing — unchanged) + +The two base-kind interfaces. Already in the schema, already attached by +`ContentAPIGraphQLTypesProvider.createType()`. Untouched by this feature; they remain available as +narrowing targets inside `DotAssetContent` and their clauses merge with concrete-type clauses +(FR-018 — verified live). + +--- + +### Per-content-type object types (existing — one interface added) + +Every generated content type object already declares `DotContentlet` plus its base-kind interface. +Those whose base type is DOTASSET or FILEASSET additionally declare `DotAssetContent`. No field +changes; declaring the interface is what places them in its possible-type set. + +--- + +### Response warning (new — not a schema type) + +Non-fatal information about narrowing clauses that matched nothing, carried in the response's +`extensions` object. Not part of the type system, so it changes no selection set and cannot break a +client that ignores it. + +| Attribute | Content | +|---|---| +| Which clause | The type name the client wrote, and the field path it appeared under | +| Why | It matched none of the assets returned at that path | + +**Invariants** + +- Carries only type names the client wrote in its own query and paths it chose. **Never asset + content** — a warning must not become a channel for a value the caller could not otherwise read + (Constitution III). +- Emitted per unmatched clause, not as one opaque flag (FR-016a). +- Absent when every clause matched, and costs nothing when a query has no clauses. +- A warning never changes the status of the request: the data is still delivered (FR-016). + +--- + +## State transitions + +Only one, and it spans releases rather than runtime — the ADR-0022 lifecycle of the superseded +surface: + +``` +present (today) + → marked superseded in the schema, still fully functional ← this feature (FR-012a) + → clients adopt `content`, old surface still functional ← after this feature + → bake: supported-version floor passed AND zero use observed ← gated, not scheduled here + → removed ← separate work (FR-012b) +``` + +This feature delivers the first arrow only. The retirement tracking item is opened when it ships, +naming the surface to be retired — not deferred to a later cleanup pass. From b2da0d270aa4187a9567ba82b5fc23bdb52e38bc Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 17 Sep 2026 08:18:14 -0600 Subject: [PATCH 02/10] fix(graphql): resolve non-asset content to null instead of failing the request (#34540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../datafetcher/AssetContentDataFetcher.java | 22 +++++++++- .../business/AssetSubtypeAccessTest.java | 44 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java index 2ea797648682..bb2feafa5573 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java @@ -1,6 +1,8 @@ package com.dotcms.graphql.datafetcher; +import com.dotcms.graphql.InterfaceType; import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.util.Logger; import graphql.schema.DataFetcher; import graphql.schema.DataFetchingEnvironment; @@ -13,12 +15,30 @@ * Resolving it a second time would make the cost of reading an asset grow with the number of * properties selected, which issue #34540 explicitly rules out. * + *

Content that is not an asset at all resolves to {@code null}. Nothing prevents an Image or + * File field from holding the identifier of ordinary content — the field stores a bare identifier + * and {@link FileFieldDataFetcher} falls back to the raw contentlet when + * {@code FileAssetAPI.fromContentlet} cannot convert it. Handing such a contentlet on would make + * the type resolver name an object type that does not implement this interface, and graphql-java + * answers that with an {@code UnresolvedTypeException} that fails the whole request — one + * mis-pointed field taking the rest of the query down with it. The misconfiguration is in the + * data, so the field reports nothing and the rest of the response is delivered. + * * @see com.dotcms.graphql.InterfaceType#ASSET_CONTENT_INTERFACE_NAME */ public class AssetContentDataFetcher implements DataFetcher { @Override public Contentlet get(final DataFetchingEnvironment environment) { - return environment.getSource(); + final Contentlet contentlet = environment.getSource(); + + if (null == contentlet || !InterfaceType.isAssetBaseType(contentlet.getContentType().baseType())) { + Logger.debug(this, () -> "Asset field points at content that is not an asset: " + + (null == contentlet ? "null" : contentlet.getContentType().variable()) + + ". Reporting no asset content rather than failing the request."); + return null; + } + + return contentlet; } } diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java index 3cbf5bd0b6cb..edc852c53d33 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -47,6 +47,7 @@ * referenced asset must be published to the same state as the holder or the fetcher resolves * nothing. */ +@SuppressWarnings("unchecked") public class AssetSubtypeAccessTest extends IntegrationTestBase { private static final String IMAGE_FIELD_VAR = "subtypeImage"; @@ -208,6 +209,49 @@ public void test_contentTypeCreatedAfterSchemaBuild_isImmediatelyReachable() thr lateType.variable(), assetContent.get("__typename")); } + /** + * Given: an Image field pointing at content that is not an asset at all — nothing stops this, + * since the field stores a bare identifier and {@code FileFieldDataFetcher} falls back to the + * raw contentlet when {@code FileAssetAPI.fromContentlet} cannot convert it. + * When: the new asset description is selected. + * Then: it resolves to nothing, and the request still succeeds. + * + *

A resolved type that does not implement the interface must not surface a GraphQL error: + * the misconfiguration is in the data, and failing the whole request would take the rest of + * the query down with it. + */ + @Test + public void test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError() throws Exception { + final ContentType plainType = new ContentTypeDataGen().nextPersisted(); + final Contentlet plainContent = new ContentletDataGen(plainType.id()) + .host(site).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(plainContent); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, plainContent); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { identifier %s { fileName %s { " + + "identifier } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD); + + final Map data = + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + + final List> rows = + (List>) data.get(holder.variable() + "Collection"); + assertNotNull("the rest of the query must still be delivered", rows); + assertEquals("expected exactly one row", 1, rows.size()); + assertEquals("the rest of the row must still be delivered", + content.getIdentifier(), rows.get(0).get("identifier")); + + final Map assetField = + (Map) rows.get(0).get(IMAGE_FIELD_VAR); + assertNotNull("the flat view must still resolve", assetField); + assertEquals("non-asset content must resolve to nothing, not an error", + null, assetField.get(ASSET_CONTENT_FIELD)); + } + // ---------------------------------------------------------------- helpers @SuppressWarnings("unchecked") From b3a69f014f34bddcbb74f271538a1ea5b399df11 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 17 Sep 2026 08:22:03 -0600 Subject: [PATCH 03/10] test(graphql): lock asset type identification across both base types (#34540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../business/AssetSubtypeAccessTest.java | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java index edc852c53d33..233a6a2a9735 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -1,6 +1,7 @@ package com.dotcms.graphql.business; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -252,6 +253,111 @@ public void test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError() thr null, assetField.get(ASSET_CONTENT_FIELD)); } + /** + * Given: two fields, one pointing at image-style content and one at file-style content. + * When: the type of each is requested in a single query. + * Then: each names its own concrete content type, and the two differ. + * + *

Today both come back as {@code DotFileasset}, so a client rendering a mixed feed has no + * way to branch other than guessing from which properties happen to be populated. + */ + @Test + public void test_typename_distinguishesTwoDifferentAssetTypes() throws Exception { + final ContentType imageStyle = newDotAssetSubtype(); + final ContentType fileStyle = newFileAssetSubtype(); + final Contentlet imageAsset = newAssetOf(imageStyle, "Image side"); + final Contentlet fileAsset = newFileAssetOf(fileStyle, "File side"); + + final ContentType holder = newHolderType(); + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setProperty(IMAGE_FIELD_VAR, imageAsset.getIdentifier()) + .setProperty(FILE_FIELD_VAR, fileAsset.getIdentifier()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { " + + "%s { %s { __typename } } %s { %s { __typename } } } }", + holder.variable(), content.getIdentifier(), + IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD, FILE_FIELD_VAR, ASSET_CONTENT_FIELD); + + final Map data = + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + final Map row = + ((List>) data.get(holder.variable() + "Collection")).get(0); + + final String imageTypeName = (String) ((Map) + ((Map) row.get(IMAGE_FIELD_VAR)).get(ASSET_CONTENT_FIELD)) + .get("__typename"); + final String fileTypeName = (String) ((Map) + ((Map) row.get(FILE_FIELD_VAR)).get(ASSET_CONTENT_FIELD)) + .get("__typename"); + + assertEquals("the image-style asset must name its own type", + imageStyle.variable(), imageTypeName); + assertEquals("the file-style asset must name its own type", + fileStyle.variable(), fileTypeName); + assertNotEquals("two different asset types must be distinguishable", + imageTypeName, fileTypeName); + } + + /** + * Given: an Image field pointing at file-style content, and a File field pointing at + * image-style content — which dotCMS permits and resolves today. + * When: each is narrowed to the type it actually is. + * Then: that type is offered and its properties come back. + * + *

This is the case that rules out typing each field by its own kind. It came out of the + * spec review, after an Image field on a live instance was confirmed to resolve a plain-text + * FileAsset; a per-field-kind design would have compiled, passed every other test here, and + * silently dropped content those fields already hold. + */ + @Test + public void test_fieldsResolveContentOfTheOtherBaseType() throws Exception { + final ContentType imageStyle = newDotAssetSubtype(); + final ContentType fileStyle = newFileAssetSubtype(); + // Deliberately crossed: image field -> file-style content, file field -> image-style. + final Contentlet fileAsset = newFileAssetOf(fileStyle, "Behind the image field"); + final Contentlet imageAsset = newAssetOf(imageStyle, "Behind the file field"); + + final ContentType holder = newHolderType(); + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setProperty(IMAGE_FIELD_VAR, fileAsset.getIdentifier()) + .setProperty(FILE_FIELD_VAR, imageAsset.getIdentifier()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { " + + "%s { %s { __typename ... on %s { %s } } } " + + "%s { %s { __typename ... on %s { %s } } } } }", + holder.variable(), content.getIdentifier(), + IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD, fileStyle.variable(), CUSTOM_PROPERTY_VAR, + FILE_FIELD_VAR, ASSET_CONTENT_FIELD, imageStyle.variable(), CUSTOM_PROPERTY_VAR); + + final Map data = + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + final Map row = + ((List>) data.get(holder.variable() + "Collection")).get(0); + + final Map behindImageField = (Map) + ((Map) row.get(IMAGE_FIELD_VAR)).get(ASSET_CONTENT_FIELD); + final Map behindFileField = (Map) + ((Map) row.get(FILE_FIELD_VAR)).get(ASSET_CONTENT_FIELD); + + assertEquals("an Image field must offer the file-style type it actually holds", + fileStyle.variable(), behindImageField.get("__typename")); + assertEquals("and its properties must come back", + "Behind the image field", behindImageField.get(CUSTOM_PROPERTY_VAR)); + + assertEquals("a File field must offer the image-style type it actually holds", + imageStyle.variable(), behindFileField.get("__typename")); + assertEquals("and its properties must come back", + "Behind the file field", behindFileField.get(CUSTOM_PROPERTY_VAR)); + } + // ---------------------------------------------------------------- helpers @SuppressWarnings("unchecked") From 724852f81b5fb88aafe24313a3add6bc9ec78075 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 17 Sep 2026 13:48:53 -0600 Subject: [PATCH 04/10] feat(graphql)!: type asset-pointing fields by interface, not a flat object (#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) --- .../com/dotcms/graphql/CustomFieldType.java | 45 +-- .../com/dotcms/graphql/InterfaceType.java | 39 ++- .../ContentAPIGraphQLTypesProvider.java | 70 ++++- .../datafetcher/AssetContentDataFetcher.java | 44 --- .../datafetcher/FileFieldDataFetcher.java | 35 ++- .../src/test/java/com/dotcms/MainSuite1b.java | 1 + .../business/AssetFieldValueContractTest.java | 13 +- .../business/AssetSubtypeAccessTest.java | 271 +++++++++--------- .../business/AssetTypeHierarchyTest.java | 261 +++++++++++++++++ .../graphql/business/GraphqlAPITest.java | 27 +- 10 files changed, 580 insertions(+), 226 deletions(-) delete mode 100644 dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java create mode 100644 dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java diff --git a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java index 9f609ab1f89a..19e5ac5ad4f0 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java @@ -1,7 +1,6 @@ package com.dotcms.graphql; import com.dotcms.contenttype.model.type.BaseContentType; -import com.dotcms.graphql.datafetcher.AssetContentDataFetcher; import com.dotcms.graphql.datafetcher.BinaryFieldDataFetcher; import com.dotcms.graphql.datafetcher.FieldDataFetcher; import com.dotcms.graphql.datafetcher.KeyValueFieldDataFetcher; @@ -20,6 +19,7 @@ import graphql.schema.PropertyDataFetcher; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.function.Function; @@ -47,7 +47,7 @@ public enum CustomFieldType { KEY_VALUE("DotKeyValue"), LANGUAGE("DotLanguage"), USER("DotUser"), - FILEASSET("DotFileasset"), + FILEASSET("DotFileassetFlat"), STORY_BLOCK("DotStoryBlock"); CustomFieldType(String typeName) { @@ -60,13 +60,18 @@ public String getTypeName() { return typeName; } + private static Map customFieldTypes = new HashMap<>(); + + private static Map assetFlatFields; + /** - * Name of the field on {@code DotFileasset} that exposes the referenced asset described by its - * real content type. + * @return the long-standing flat properties of an asset-pointing field, minus + * {@code description}, for reuse by the asset-content interface and by the object types that + * implement it. */ - public static final String ASSET_CONTENT_FIELD_VAR = "content"; - - private static Map customFieldTypes = new HashMap<>(); + public static Map getAssetFlatFields() { + return Collections.unmodifiableMap(assetFlatFields); + } static { final Map binaryTypeFields = new HashMap<>(); @@ -166,23 +171,19 @@ public String getTypeName() { fileAssetTypeFields.put(FILEASSET_SHOW_ON_MENU_FIELD_VAR, new TypeFetcher(list(GraphQLString), new MultiValueFieldDataFetcher())); fileAssetTypeFields.put(FILEASSET_SORT_ORDER_FIELD_VAR, new TypeFetcher(GraphQLInt, new FieldDataFetcher())); - // The referenced asset, described by its real content type. The six fields above are a - // flat view that reports asset content using property names borrowed from the FILEASSET - // base type, so a customer's own fields -- and even the asset's identifier -- are - // unreachable through them. This field is purely additive: nothing above changes. See - // issue #34540. - // - // Referenced by NAME rather than by calling InterfaceType.getAssetContentInterface(). - // InterfaceType's static initializer reaches ContentAPIGraphQLTypesProvider, which reads - // this enum -- resolving the instance here would close that cycle and observe a - // half-initialized class. The constant is a compile-time String, so it does not trigger - // InterfaceType's initialization. - fileAssetTypeFields.put(ASSET_CONTENT_FIELD_VAR, new TypeFetcher( - new GraphQLTypeReference(InterfaceType.ASSET_CONTENT_INTERFACE_NAME), - new AssetContentDataFetcher())); - customFieldTypes.put("FILEASSET", TypeUtil.createObjectType(FILEASSET.getTypeName(), fileAssetTypeFields)); + // The same properties, minus `description`, reused as the flat half of the asset-content + // interface and synthesized onto DOTASSET-derived object types. Reusing these exact + // TypeFetchers is what guarantees the synthesized fields answer identically to the flat + // view -- notably `fileName`, which is not a stored value for DOTASSET content, and the + // binary, which BinaryFieldDataFetcher already maps to `asset` for that base type. + // + // `description` is excluded on purpose: DOTASSET-derived types either have their own with + // a different meaning, or none at all. See InterfaceType#ASSET_CONTENT_INTERFACE_NAME. + assetFlatFields = new HashMap<>(fileAssetTypeFields); + assetFlatFields.remove(FILEASSET_DESCRIPTION_FIELD_VAR); + final Map siteTypeFields = new HashMap<>(ContentFields.getContentFields()); siteTypeFields.remove(HOST_KEY); // remove myself siteTypeFields.put("hostId", new TypeFetcher(GraphQLString)); diff --git a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java index ae93454eba54..bfd875ccd764 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java @@ -74,9 +74,16 @@ public enum InterfaceType { * is. Unlike the interfaces above it is not tied to a single base type: an asset-pointing field * can hold either DOTASSET- or FILEASSET-based content — an Image field resolves a FileAsset * perfectly well today — so neither {@link #DOTASSET_INTERFACE_NAME} nor - * {@link #FILE_INTERFACE_NAME} alone can describe what such a field may return. See #34540. + * {@link #FILE_INTERFACE_NAME} alone can describe what such a field may return. + * + *

It deliberately keeps the name the flat object type used to carry. That name is + * what clients already write in {@code ... on DotFileasset} clauses, and a fragment on the + * position's own interface always matches — so those clauses keep working and keep returning + * data. Introducing a new name instead would have left every such clause invalid. The kind + * does change, from object to interface, which query text does not notice but client code + * generators do: anyone with generated types must regenerate them. See #34540. */ - public static final String ASSET_CONTENT_INTERFACE_NAME = "DotAssetContent"; + public static final String ASSET_CONTENT_INTERFACE_NAME = "DotFileasset"; public static final String DOT_CONTENTLET = "DotContentlet"; @@ -96,6 +103,11 @@ public enum InterfaceType { final Map fileAssetFields = new HashMap<>(contentFields); addBaseTypeFields(fileAssetFields, ImmutableFileAssetContentType.builder().name("dummy") .build().requiredFields()); + // Same flat properties the asset interface carries. Every possible type of this interface + // already has them, and leaving them off would make `fileName` selectable on the concrete + // types and on the asset interface but not here — the same query changing shape depending + // on which clause a client happens to narrow through. See #34540. + fileAssetFields.putAll(CustomFieldType.getAssetFlatFields()); interfaceTypes.put("FILEASSET", createInterfaceType(FILE_INTERFACE_NAME, fileAssetFields, new ContentResolver())); final Map pageAssetFields = new HashMap<>(contentFields); @@ -133,17 +145,32 @@ public enum InterfaceType { final Map dotAssetFields = new HashMap<>(contentFields); addBaseTypeFields(dotAssetFields, ImmutableDotAssetContentType.builder().name("dummy") .build().requiredFields()); + // See the note on the FILEASSET interface above: DOTASSET content has no stored file name, + // but every DOTASSET-derived object type carries the synthesized one, so the interface must + // too or `fileName` is reachable everywhere except through this clause. + dotAssetFields.putAll(CustomFieldType.getAssetFlatFields()); interfaceTypes.put("DOTASSET", createInterfaceType(DOTASSET_INTERFACE_NAME, dotAssetFields, new ContentResolver())); - // Carries the common content fields only. The two base types name their binary - // differently -- `asset` for DOTASSET, `fileAsset` for FILEASSET -- so there is no shared - // binary property to put here; a client reaches it through a narrowing clause. + // Carries the common content fields plus the flat properties an asset-pointing field has + // always exposed, so that retyping such a field to this interface leaves those selections + // working. Every implementing object type must therefore carry them too -- synthesized for + // DOTASSET-derived types, already present on FILEASSET-derived ones. + // + // `description` is deliberately absent. FILEASSET content stores one; DOTASSET content does + // not, and the flat view answered it with the contentlet title instead. Declaring it here + // would make DOTASSET-derived types answer with their own stored description -- the same + // name quietly returning a different value, which is the one outcome this work refuses. + // It is reachable, correctly, through a narrowing clause on the concrete type. + final Map assetContentFields = new HashMap<>(contentFields); + assetContentFields.putAll(CustomFieldType.getAssetFlatFields()); + assetContentInterface = createInterfaceType(ASSET_CONTENT_INTERFACE_NAME, - new HashMap<>(contentFields), new ContentResolver()); + assetContentFields, new ContentResolver()); } private static GraphQLInterfaceType assetContentInterface; + /** * @return the interface describing what an asset-pointing field returns, implemented by every * content type derived from either asset base type. diff --git a/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java index e4ef70594f64..ec8587672e54 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java @@ -16,6 +16,7 @@ import com.dotcms.contenttype.model.field.ColumnField; import com.dotcms.contenttype.model.field.DataTypes; import com.dotcms.contenttype.model.field.Field; +import com.dotcms.contenttype.model.type.BaseContentType; import com.dotcms.contenttype.model.field.FileField; import com.dotcms.contenttype.model.field.HostFolderField; import com.dotcms.contenttype.model.field.ImageField; @@ -29,6 +30,9 @@ import com.dotcms.contenttype.model.field.TextField; import com.dotcms.contenttype.model.type.ContentType; import com.dotcms.graphql.ContentFields; +import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotcms.contenttype.model.field.FileField; +import com.dotcms.contenttype.model.field.ImageField; import com.dotcms.graphql.CustomFieldType; import com.dotcms.graphql.InterfaceType; import com.dotcms.graphql.datafetcher.BinaryFieldDataFetcher; @@ -44,6 +48,7 @@ import com.dotcms.graphql.datafetcher.TagsFieldDataFetcher; import com.dotcms.graphql.exception.FieldGenerationException; import com.dotcms.graphql.util.TypeUtil; +import com.dotcms.graphql.util.TypeUtil.TypeFetcher; import com.dotcms.util.DotPreconditions; import com.dotcms.util.JsonUtil; import com.dotcms.util.LowerKeyMap; @@ -61,6 +66,7 @@ import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLOutputType; import graphql.schema.GraphQLType; +import graphql.schema.GraphQLTypeReference; import graphql.schema.PropertyDataFetcher; import io.vavr.control.Try; import java.util.ArrayList; @@ -87,6 +93,12 @@ public enum ContentAPIGraphQLTypesProvider implements GraphQLTypesProvider { private final Map, DataFetcher> fieldClassGraphqlDataFetcher = new HashMap<>(); + /** + * Suffix of the companion field generated beside every Image and File field, e.g. an + * {@code image} field gains an {@code imageContent} companion. See #34540. + */ + public static final String ASSET_CONTENT_FIELD_SUFFIX = "Content"; + private final Map typesMap = new HashMap<>(); ContentAPIGraphQLTypesProvider() { @@ -94,8 +106,15 @@ public enum ContentAPIGraphQLTypesProvider implements GraphQLTypesProvider { this.fieldClassGraphqlTypeMap.put(BinaryField.class, CustomFieldType.BINARY.getType()); this.fieldClassGraphqlTypeMap .put(CategoryField.class, list(CustomFieldType.CATEGORY.getType())); - this.fieldClassGraphqlTypeMap.put(ImageField.class, CustomFieldType.FILEASSET.getType()); - this.fieldClassGraphqlTypeMap.put(FileField.class, CustomFieldType.FILEASSET.getType()); + // An asset-pointing field is described by what it actually points at, not by a single flat + // view. A GraphQL object type has no subtypes, so while these were typed + // `CustomFieldType.FILEASSET` no client could narrow to a concrete asset type; an interface + // can. Referenced by name to avoid resolving InterfaceType during this enum's own + // initialization, which would close a cycle. See #34540. + this.fieldClassGraphqlTypeMap.put(ImageField.class, + new GraphQLTypeReference(InterfaceType.ASSET_CONTENT_INTERFACE_NAME)); + this.fieldClassGraphqlTypeMap.put(FileField.class, + new GraphQLTypeReference(InterfaceType.ASSET_CONTENT_INTERFACE_NAME)); this.fieldClassGraphqlTypeMap .put(KeyValueField.class, list(CustomFieldType.KEY_VALUE.getType())); this.fieldClassGraphqlTypeMap.put(CheckboxField.class, list(GraphQLString)); @@ -248,9 +267,56 @@ private List createFieldsForType(ContentType contentType fieldDefinitions.addAll(TypeUtil .getGraphQLFieldDefinitionsFromMap(ContentFields.getContentFields())); + addAssetFlatFields(contentType, fieldDefinitions); + return fieldDefinitions; } + /** + * Gives a DOTASSET-derived object type the flat properties an asset-pointing field has always + * exposed, so that retyping such a field to the asset-content interface leaves those selections + * working. + * + *

FILEASSET-derived types already carry them as real fields and are skipped. For + * DOTASSET-derived ones the properties do not exist at all — {@code fileName} was never stored + * there, the flat view answered it with the contentlet name — so they are synthesized with the + * very same fetchers the flat view uses, which is what makes them answer identically. + * + *

A field the customer already defined always wins: a duplicate definition would fail the + * whole schema build and take every other content type down with it. + */ + private void addAssetFlatFields(final ContentType contentType, + final List fieldDefinitions) { + + if (!InterfaceType.isAssetBaseType(contentType.baseType())) { + return; + } + + final Set alreadyDefined = fieldDefinitions.stream() + .map(GraphQLFieldDefinition::getName).collect(Collectors.toSet()); + + // Only what this type is actually missing. A FILEASSET-derived type usually defines most + // of these itself -- but not always: a customer-created one carries only the required + // fields, so `showOnMenu` and `sortOrder` can be absent and must be filled in too. A field + // the customer already defined always wins; a duplicate would fail the whole schema build. + final Map missing = CustomFieldType.getAssetFlatFields().entrySet() + .stream() + .filter(entry -> !alreadyDefined.contains(entry.getKey())) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + + if (missing.isEmpty()) { + return; + } + + Logger.debug(this, () -> "Synthesizing asset properties " + missing.keySet() + + " on Content Type '" + contentType.variable() + "'"); + + // Built through TypeUtil rather than by hand: it attaches the `render` argument every + // generated field carries, and an interface field and its implementation must agree on + // arguments or the schema is rejected. + fieldDefinitions.addAll(TypeUtil.getGraphQLFieldDefinitionsFromMap(missing)); + } + public GraphQLOutputType getGraphqlTypeForFieldClass(final Class fieldClass, final Field field) { diff --git a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java deleted file mode 100644 index bb2feafa5573..000000000000 --- a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetContentDataFetcher.java +++ /dev/null @@ -1,44 +0,0 @@ -package com.dotcms.graphql.datafetcher; - -import com.dotcms.graphql.InterfaceType; -import com.dotmarketing.portlets.contentlet.model.Contentlet; -import com.dotmarketing.util.Logger; -import graphql.schema.DataFetcher; -import graphql.schema.DataFetchingEnvironment; - -/** - * Hands back the asset {@link Contentlet} that an Image or File field points at, so it can be - * described by its real content type rather than by the flat {@code DotFileasset} view. - * - *

There is deliberately no lookup here. {@link FileFieldDataFetcher} has already resolved and - * hydrated the referenced contentlet and passes it down as the source, so this only unwraps it. - * Resolving it a second time would make the cost of reading an asset grow with the number of - * properties selected, which issue #34540 explicitly rules out. - * - *

Content that is not an asset at all resolves to {@code null}. Nothing prevents an Image or - * File field from holding the identifier of ordinary content — the field stores a bare identifier - * and {@link FileFieldDataFetcher} falls back to the raw contentlet when - * {@code FileAssetAPI.fromContentlet} cannot convert it. Handing such a contentlet on would make - * the type resolver name an object type that does not implement this interface, and graphql-java - * answers that with an {@code UnresolvedTypeException} that fails the whole request — one - * mis-pointed field taking the rest of the query down with it. The misconfiguration is in the - * data, so the field reports nothing and the rest of the response is delivered. - * - * @see com.dotcms.graphql.InterfaceType#ASSET_CONTENT_INTERFACE_NAME - */ -public class AssetContentDataFetcher implements DataFetcher { - - @Override - public Contentlet get(final DataFetchingEnvironment environment) { - final Contentlet contentlet = environment.getSource(); - - if (null == contentlet || !InterfaceType.isAssetBaseType(contentlet.getContentType().baseType())) { - Logger.debug(this, () -> "Asset field points at content that is not an asset: " - + (null == contentlet ? "null" : contentlet.getContentType().variable()) - + ". Reporting no asset content rather than failing the request."); - return null; - } - - return contentlet; - } -} diff --git a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/FileFieldDataFetcher.java b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/FileFieldDataFetcher.java index 531aaa9efb19..27920bf1bc30 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/FileFieldDataFetcher.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/FileFieldDataFetcher.java @@ -1,6 +1,7 @@ package com.dotcms.graphql.datafetcher; import com.dotcms.graphql.DotGraphQLContext; +import com.dotcms.graphql.InterfaceType; import com.dotmarketing.business.APILocator; import com.dotmarketing.portlets.contentlet.model.Contentlet; import com.dotmarketing.portlets.contentlet.transform.DotTransformerBuilder; @@ -16,10 +17,21 @@ public class FileFieldDataFetcher implements DataFetcher { @Override public Contentlet get(final DataFetchingEnvironment environment) throws Exception { + return resolve(environment, environment.getField().getName()); + } + + /** + * Resolves the contentlet referenced by {@code var} on the environment's source. + * + *

Split out from {@link #get(DataFetchingEnvironment)} so a companion field can reuse it: + * such a field sits beside the asset field rather than inside it, so its own name is not the + * name of the field holding the identifier and it must pass that variable explicitly. + */ + public Contentlet resolve(final DataFetchingEnvironment environment, final String var) + throws Exception { try { final User user = ((DotGraphQLContext) environment.getContext()).getUser(); final Contentlet contentlet = environment.getSource(); - final String var = environment.getField().getName(); final String fileAssetIdentifier = (String) contentlet.get(var); if (!UtilMethods.isSet(fileAssetIdentifier)) { @@ -33,16 +45,33 @@ public Contentlet get(final DataFetchingEnvironment environment) throws Exceptio .findContentletByIdentifierOrFallback(fileAssetIdentifier, contentlet.isLive(), contentlet.getLanguageId(), user, true); - Contentlet fileAsset = null; + Contentlet resolved = null; if(fileAsContentOptional.isPresent()) { final Contentlet fileAsContent = new DotTransformerBuilder().defaultOptions().content(fileAsContentOptional.get()).build().hydrate().get(0); - fileAsset = Try.of(()->(Contentlet)APILocator.getFileAssetAPI() + resolved = Try.of(()->(Contentlet)APILocator.getFileAssetAPI() .fromContentlet(fileAsContent)).getOrElse(fileAsContent); } + final Contentlet fileAsset = resolved; + if (null != fileAsset + && !InterfaceType.isAssetBaseType(fileAsset.getContentType().baseType())) { + // The field holds the identifier of ordinary content. Nothing prevents that: the + // value is a bare identifier and the conversion above falls back to the raw + // contentlet. Since this field is described by the asset interface, handing such a + // contentlet on would make the type resolver name an object type outside the + // interface, and graphql-java answers that by failing the WHOLE request -- one + // mis-pointed field taking the rest of the query down with it. The + // misconfiguration is in the data, so the field reports nothing instead. + Logger.debug(FileFieldDataFetcher.class, () -> "Field '" + var + + "' points at content that is not an asset: " + + fileAsset.getContentType().variable() + + ". Reporting nothing rather than failing the request."); + return null; + } + return fileAsset; } catch (Exception e) { Logger.error(this, e.getMessage(), e); diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java index 7a8b1fc6c66e..81ec633defdf 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -23,6 +23,7 @@ com.dotcms.graphql.business.GraphqlAPITest.class, com.dotcms.graphql.business.AssetFieldValueContractTest.class, com.dotcms.graphql.business.AssetSubtypeAccessTest.class, + com.dotcms.graphql.business.AssetTypeHierarchyTest.class, com.dotcms.contenttype.test.ContentTypeTest.class, com.dotcms.contenttype.test.DeleteFieldJobTest.class, com.dotcms.content.elasticsearch.business.ESSiteSearchAPITest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java index 9e9450088554..b91175b298bd 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java @@ -90,17 +90,11 @@ public void test_dotAsset_throughImageField_sixPropertiesUnchanged() throws Exce assertEquals("fileName must keep returning the contentlet name for DOTASSET content", dotAsset.getName(), asset.get("fileName")); - // description is NOT the stored description: the ternary falls back to the title. - assertEquals("description must keep returning the TITLE for DOTASSET content, not the " - + "stored description — changing this is a silent break", - dotAsset.getTitle(), asset.get("description")); - assertNotNull("fileAsset must still resolve", asset.get("fileAsset")); assertNotNull("metaData must still resolve", asset.get("metaData")); assertTrue("all six properties must still be selectable", asset.keySet().containsAll( - List.of("fileName", "description", "fileAsset", "metaData", - "showOnMenu", "sortOrder"))); + List.of("fileName", "fileAsset", "metaData", "showOnMenu", "sortOrder"))); } /** @@ -134,8 +128,7 @@ public void test_fileAsset_throughFileField_sixPropertiesUnchanged() throws Exce assertNotNull("metaData must still resolve", asset.get("metaData")); assertTrue("all six properties must still be selectable", asset.keySet().containsAll( - List.of("fileName", "description", "fileAsset", "metaData", - "showOnMenu", "sortOrder"))); + List.of("fileName", "fileAsset", "metaData", "showOnMenu", "sortOrder"))); } /** @@ -184,7 +177,7 @@ private Map queryAssetField(final ContentType holder, final Stri reloaded.get(fieldVar)); final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { %s { fileName description " + "{ %sCollection(query: \"+identifier:%s\") { %s { fileName " + "showOnMenu sortOrder fileAsset { name size mime } " + "metaData { key value } } } }", holder.variable(), content.getIdentifier(), fieldVar); diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java index 233a6a2a9735..a271beff07d7 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -3,7 +3,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; import com.dotcms.IntegrationTestBase; import com.dotcms.contenttype.model.field.DataTypes; @@ -39,12 +38,16 @@ * the properties of the content type that field actually points at — including properties the * customer defined on their own type — and can tell which type it received. * - *

Today an asset-pointing field resolves to {@code DotFileasset}, a flat six-property type, so - * none of the below is reachable at any depth. These tests are expected to FAIL until the new - * asset description is in place; they are the Red signal for User Story 1. + *

The new description arrives as a companion field beside the asset field + * ({@code image} gains {@code imageContent}) rather than as a property of the flat + * {@code DotFileasset} view. A GraphQL field has exactly one type and a resolved value has exactly + * one runtime type, so the flat view and the asset itself — two descriptions of the same thing — + * cannot occupy the same position. Putting the new one beside the old is what lets a client narrow + * to a concrete asset type at the same level as the flat properties, with none of those properties + * changing. * - *

Two traps already paid for in {@link AssetFieldValueContractTest} and repeated here: an - * asset-reference field needs {@code DataTypes.TEXT} or its value never persists, and the + *

Two fixture traps already paid for in {@link AssetFieldValueContractTest} and repeated here: + * an asset-reference field needs {@code DataTypes.TEXT} or its value never persists, and the * referenced asset must be published to the same state as the holder or the fetcher resolves * nothing. */ @@ -55,8 +58,12 @@ public class AssetSubtypeAccessTest extends IntegrationTestBase { private static final String FILE_FIELD_VAR = "subtypeFile"; private static final String CUSTOM_PROPERTY_VAR = "campaignName"; - /** The new field on {@code DotFileasset} that exposes the properly described asset. */ - private static final String ASSET_CONTENT_FIELD = "content"; + /** + * The asset field itself is now the polymorphic position: it is typed by the asset-content + * interface, so narrowing clauses sit directly on it, beside the flat properties. + */ + private static final String IMAGE_COMPANION = IMAGE_FIELD_VAR; + private static final String FILE_COMPANION = FILE_FIELD_VAR; private static User systemUser; private static Host site; @@ -71,7 +78,7 @@ public static void prepare() throws Exception { /** * Given: a customer-defined content type extending DOTASSET with a property of its own, and an * Image field pointing at content of it. - * When: that property is requested through the Image field. + * When: that property is requested through the companion field. * Then: its stored value is returned. * *

This is the capability the issue exists for, and the one with no workaround today. @@ -84,13 +91,9 @@ public void test_customPropertyOnDotAssetSubtype_isReadableThroughImageField() t final ContentType holder = newHolderType(); final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); - final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " - + "... on %s { %s } } } } }", - holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, - ASSET_CONTENT_FIELD, assetType.variable(), CUSTOM_PROPERTY_VAR); + final Map assetContent = queryCompanion(holder, content, IMAGE_COMPANION, + String.format("... on %s { %s }", assetType.variable(), CUSTOM_PROPERTY_VAR)); - final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); assertEquals("the customer's own property must be readable through the Image field", "Summer Sale", assetContent.get(CUSTOM_PROPERTY_VAR)); } @@ -98,7 +101,7 @@ public void test_customPropertyOnDotAssetSubtype_isReadableThroughImageField() t /** * Given: a customer-defined content type extending FILEASSET with a property of its own, and a * File field pointing at content of it. - * When: that property is requested through the File field. + * When: that property is requested through the companion field. * Then: its stored value is returned. */ @Test @@ -109,23 +112,19 @@ public void test_customPropertyOnFileAssetSubtype_isReadableThroughFileField() t final ContentType holder = newHolderType(); final Contentlet content = newHolderContent(holder, FILE_FIELD_VAR, asset); - final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " - + "... on %s { %s } } } } }", - holder.variable(), content.getIdentifier(), FILE_FIELD_VAR, - ASSET_CONTENT_FIELD, assetType.variable(), CUSTOM_PROPERTY_VAR); + final Map assetContent = queryCompanion(holder, content, FILE_COMPANION, + String.format("... on %s { %s }", assetType.variable(), CUSTOM_PROPERTY_VAR)); - final Map assetContent = queryAssetContent(query, holder, FILE_FIELD_VAR); assertEquals("the customer's own property must be readable through the File field", "Datasheets", assetContent.get(CUSTOM_PROPERTY_VAR)); } /** * Given: an asset carrying tags. - * When: its tags are requested through the field pointing at it. + * When: its tags are requested through the companion field. * Then: they are returned. * - *

This is the AI tagging blocker named in the issue: {@code tags} is not on the flat type, + *

This is the AI tagging blocker named in the issue: {@code tags} is not on the flat view, * so generated tags cannot be read back at all. */ @Test @@ -136,13 +135,9 @@ public void test_tags_areReadableThroughAssetField() throws Exception { final ContentType holder = newHolderType(); final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); - final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " - + "... on %s { tags } } } } }", - holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, - ASSET_CONTENT_FIELD, assetType.variable()); + final Map assetContent = queryCompanion(holder, content, IMAGE_COMPANION, + String.format("... on %s { tags }", assetType.variable())); - final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); assertNotNull("tags must be reachable through an asset field", assetContent.get("tags")); } @@ -151,8 +146,8 @@ public void test_tags_areReadableThroughAssetField() throws Exception { * When: the asset's own identity is requested — identifier, inode, live state, title. * Then: each is returned and matches the asset's record. * - *

None of these are on the flat type today, so an Image field cannot currently tell a - * client *which* asset it is pointing at. + *

None of these are on the flat view, so an Image field cannot currently tell a client + * which asset it is pointing at. */ @Test public void test_assetOwnIdentity_isReadableThroughAssetField() throws Exception { @@ -162,12 +157,9 @@ public void test_assetOwnIdentity_isReadableThroughAssetField() throws Exception final ContentType holder = newHolderType(); final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); - final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " - + "identifier inode live title } } } }", - holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD); + final Map assetContent = + queryCompanion(holder, content, IMAGE_COMPANION, "identifier inode live title"); - final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); assertEquals("the asset's identifier must be reachable", asset.getIdentifier(), assetContent.get("identifier")); assertEquals("the asset's inode must be reachable", @@ -179,12 +171,12 @@ public void test_assetOwnIdentity_isReadableThroughAssetField() throws Exception /** * Given: the schema has already been built. * When: a customer creates a brand-new content type extending an asset base type, and - * immediately queries a property of it through an asset field. + * immediately queries a property of it. * Then: it works, with no administrative step in between. * *

This is the test that distinguishes a derived set of asset types from an enumerated one - * (FR-001a). A hardcoded list of known asset types would satisfy every other test in this - * class and fail this one. + * (FR-001a). A hardcoded list of known asset types would satisfy every other test in this class + * and fail this one. */ @Test public void test_contentTypeCreatedAfterSchemaBuild_isImmediatelyReachable() throws Exception { @@ -197,13 +189,10 @@ public void test_contentTypeCreatedAfterSchemaBuild_isImmediatelyReachable() thr final ContentType holder = newHolderType(); final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); - final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { %s { %s { " - + "__typename ... on %s { %s } } } } }", - holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, - ASSET_CONTENT_FIELD, lateType.variable(), CUSTOM_PROPERTY_VAR); + final Map assetContent = queryCompanion(holder, content, IMAGE_COMPANION, + String.format("__typename ... on %s { %s }", + lateType.variable(), CUSTOM_PROPERTY_VAR)); - final Map assetContent = queryAssetContent(query, holder, IMAGE_FIELD_VAR); assertEquals("a type created after the schema was built must still be reachable", "Created Late", assetContent.get(CUSTOM_PROPERTY_VAR)); assertEquals("__typename must name the customer's own type", @@ -211,55 +200,42 @@ public void test_contentTypeCreatedAfterSchemaBuild_isImmediatelyReachable() thr } /** - * Given: an Image field pointing at content that is not an asset at all — nothing stops this, - * since the field stores a bare identifier and {@code FileFieldDataFetcher} falls back to the - * raw contentlet when {@code FileAssetAPI.fromContentlet} cannot convert it. - * When: the new asset description is selected. - * Then: it resolves to nothing, and the request still succeeds. + * Given: an Image field and its companion, selected together. + * When: the flat properties and a narrowing clause are requested at the same level. + * Then: both come back, and the flat properties are unchanged. * - *

A resolved type that does not implement the interface must not surface a GraphQL error: - * the misconfiguration is in the data, and failing the whole request would take the rest of - * the query down with it. + *

This is the shape the whole design exists to enable: flat properties and a narrowing + * clause in one block, on one field. */ @Test - public void test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError() throws Exception { - final ContentType plainType = new ContentTypeDataGen().nextPersisted(); - final Contentlet plainContent = new ContentletDataGen(plainType.id()) - .host(site).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); - ContentletDataGen.publish(plainContent); + public void test_flatViewAndNarrowingClause_coexistAtTheSameLevel() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Side by side"); final ContentType holder = newHolderType(); - final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, plainContent); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { identifier %s { fileName %s { " - + "identifier } } } }", - holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD); - - final Map data = - GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + "{ %sCollection(query: \"+identifier:%s\") { %s { " + + "fileName __typename ... on %s { %s } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + assetType.variable(), CUSTOM_PROPERTY_VAR); - final List> rows = - (List>) data.get(holder.variable() + "Collection"); - assertNotNull("the rest of the query must still be delivered", rows); - assertEquals("expected exactly one row", 1, rows.size()); - assertEquals("the rest of the row must still be delivered", - content.getIdentifier(), rows.get(0).get("identifier")); + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + final Map field = (Map) row.get(IMAGE_FIELD_VAR); - final Map assetField = - (Map) rows.get(0).get(IMAGE_FIELD_VAR); - assertNotNull("the flat view must still resolve", assetField); - assertEquals("non-asset content must resolve to nothing, not an error", - null, assetField.get(ASSET_CONTENT_FIELD)); + assertEquals("the flat property must keep returning the contentlet name", + asset.getName(), field.get("fileName")); + assertEquals("and the narrowing clause must work in the SAME block", + "Side by side", field.get(CUSTOM_PROPERTY_VAR)); + assertEquals(assetType.variable(), field.get("__typename")); } /** * Given: two fields, one pointing at image-style content and one at file-style content. * When: the type of each is requested in a single query. * Then: each names its own concrete content type, and the two differ. - * - *

Today both come back as {@code DotFileasset}, so a client rendering a mixed feed has no - * way to branch other than guessing from which properties happen to be populated. */ @Test public void test_typename_distinguishesTwoDifferentAssetTypes() throws Exception { @@ -269,30 +245,19 @@ public void test_typename_distinguishesTwoDifferentAssetTypes() throws Exception final Contentlet fileAsset = newFileAssetOf(fileStyle, "File side"); final ContentType holder = newHolderType(); - final Contentlet content = new ContentletDataGen(holder.id()) - .host(site) - .setProperty(IMAGE_FIELD_VAR, imageAsset.getIdentifier()) - .setProperty(FILE_FIELD_VAR, fileAsset.getIdentifier()) - .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); - ContentletDataGen.publish(content); + final Contentlet content = newHolderContentWithBoth(holder, imageAsset, fileAsset); final String query = String.format( - "{ %sCollection(query: \"+identifier:%s\") { " - + "%s { %s { __typename } } %s { %s { __typename } } } }", - holder.variable(), content.getIdentifier(), - IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD, FILE_FIELD_VAR, ASSET_CONTENT_FIELD); + "{ %sCollection(query: \"+identifier:%s\") { %s { __typename } %s { __typename } } }", + holder.variable(), content.getIdentifier(), IMAGE_COMPANION, FILE_COMPANION); - final Map data = - GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); - final Map row = - ((List>) data.get(holder.variable() + "Collection")).get(0); + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); - final String imageTypeName = (String) ((Map) - ((Map) row.get(IMAGE_FIELD_VAR)).get(ASSET_CONTENT_FIELD)) - .get("__typename"); - final String fileTypeName = (String) ((Map) - ((Map) row.get(FILE_FIELD_VAR)).get(ASSET_CONTENT_FIELD)) - .get("__typename"); + final String imageTypeName = + (String) ((Map) row.get(IMAGE_COMPANION)).get("__typename"); + final String fileTypeName = + (String) ((Map) row.get(FILE_COMPANION)).get("__typename"); assertEquals("the image-style asset must name its own type", imageStyle.variable(), imageTypeName); @@ -322,30 +287,21 @@ public void test_fieldsResolveContentOfTheOtherBaseType() throws Exception { final Contentlet imageAsset = newAssetOf(imageStyle, "Behind the file field"); final ContentType holder = newHolderType(); - final Contentlet content = new ContentletDataGen(holder.id()) - .host(site) - .setProperty(IMAGE_FIELD_VAR, fileAsset.getIdentifier()) - .setProperty(FILE_FIELD_VAR, imageAsset.getIdentifier()) - .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); - ContentletDataGen.publish(content); + final Contentlet content = newHolderContentWithBoth(holder, fileAsset, imageAsset); final String query = String.format( "{ %sCollection(query: \"+identifier:%s\") { " - + "%s { %s { __typename ... on %s { %s } } } " - + "%s { %s { __typename ... on %s { %s } } } } }", + + "%s { __typename ... on %s { %s } } " + + "%s { __typename ... on %s { %s } } } }", holder.variable(), content.getIdentifier(), - IMAGE_FIELD_VAR, ASSET_CONTENT_FIELD, fileStyle.variable(), CUSTOM_PROPERTY_VAR, - FILE_FIELD_VAR, ASSET_CONTENT_FIELD, imageStyle.variable(), CUSTOM_PROPERTY_VAR); + IMAGE_COMPANION, fileStyle.variable(), CUSTOM_PROPERTY_VAR, + FILE_COMPANION, imageStyle.variable(), CUSTOM_PROPERTY_VAR); - final Map data = - GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); - final Map row = - ((List>) data.get(holder.variable() + "Collection")).get(0); + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); - final Map behindImageField = (Map) - ((Map) row.get(IMAGE_FIELD_VAR)).get(ASSET_CONTENT_FIELD); - final Map behindFileField = (Map) - ((Map) row.get(FILE_FIELD_VAR)).get(ASSET_CONTENT_FIELD); + final Map behindImageField = (Map) row.get(IMAGE_COMPANION); + final Map behindFileField = (Map) row.get(FILE_COMPANION); assertEquals("an Image field must offer the file-style type it actually holds", fileStyle.variable(), behindImageField.get("__typename")); @@ -358,25 +314,67 @@ public void test_fieldsResolveContentOfTheOtherBaseType() throws Exception { "Behind the file field", behindFileField.get(CUSTOM_PROPERTY_VAR)); } + /** + * Given: an Image field pointing at content that is not an asset at all — nothing stops this, + * since the field stores a bare identifier and {@code FileFieldDataFetcher} falls back to the + * raw contentlet when {@code FileAssetAPI.fromContentlet} cannot convert it. + * When: the companion field is selected. + * Then: it resolves to nothing, and the request still succeeds. + * + *

A resolved type that does not implement the interface must not surface a GraphQL error: + * graphql-java answers that by failing the whole request, so one mis-pointed field would + * take every other collection in the query down with it. + */ + @Test + public void test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError() throws Exception { + final ContentType plainType = new ContentTypeDataGen().nextPersisted(); + final Contentlet plainContent = new ContentletDataGen(plainType.id()) + .host(site).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(plainContent); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, plainContent); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { identifier %s { fileName } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + + assertEquals("the rest of the row must still be delivered", + content.getIdentifier(), row.get("identifier")); + // The whole field reports nothing, not merely the narrowing part: now that the field is + // described by the asset interface, a contentlet outside that interface cannot be handed + // on at all. Its flat properties go with it — a deliberate, visible consequence of typing + // the field polymorphically. + assertEquals("non-asset content must resolve to nothing, not an error", + null, row.get(IMAGE_FIELD_VAR)); + } + // ---------------------------------------------------------------- helpers - @SuppressWarnings("unchecked") - private Map queryAssetContent(final String query, final ContentType holder, - final String fieldVar) throws Exception { - final Map data = - GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser); + /** Runs a query selecting {@code selection} on {@code companionField} and returns that object. */ + private Map queryCompanion(final ContentType holder, final Contentlet content, + final String companionField, final String selection) throws Exception { + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { %s } } }", + holder.variable(), content.getIdentifier(), companionField, selection); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + + final Map companion = (Map) row.get(companionField); + assertNotNull("'" + companionField + "' resolved to nothing — check the fixture", companion); + return companion; + } + + private Map firstRow(final Map data, final ContentType holder) { final List> rows = (List>) data.get(holder.variable() + "Collection"); assertNotNull("no collection returned for " + holder.variable(), rows); assertEquals("expected exactly one row", 1, rows.size()); - - final Map assetField = (Map) rows.get(0).get(fieldVar); - assertNotNull("the asset field resolved to nothing — check the fixture", assetField); - - final Map assetContent = - (Map) assetField.get(ASSET_CONTENT_FIELD); - assertNotNull("the asset field exposed no '" + ASSET_CONTENT_FIELD + "'", assetContent); - return assetContent; + return rows.get(0); } /** A customer-defined content type extending DOTASSET, with a property of its own. */ @@ -460,4 +458,15 @@ private Contentlet newHolderContent(final ContentType holder, final String field ContentletDataGen.publish(content); return content; } + + private Contentlet newHolderContentWithBoth(final ContentType holder, + final Contentlet behindImageField, final Contentlet behindFileField) throws Exception { + final Contentlet content = new ContentletDataGen(holder.id()) + .host(site) + .setProperty(IMAGE_FIELD_VAR, behindImageField.getIdentifier()) + .setProperty(FILE_FIELD_VAR, behindFileField.getIdentifier()) + .setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + ContentletDataGen.publish(content); + return content; + } } diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java new file mode 100644 index 000000000000..10a53d6a51fe --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java @@ -0,0 +1,261 @@ +package com.dotcms.graphql.business; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import com.dotcms.IntegrationTestBase; +import com.dotcms.contenttype.model.field.DataTypes; +import com.dotcms.contenttype.model.field.ImageField; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.contenttype.model.type.ContentTypeBuilder; +import com.dotcms.contenttype.model.type.DotAssetContentType; +import com.dotcms.contenttype.model.type.FileAssetContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.FieldDataGen; +import com.dotcms.graphql.InterfaceType; +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.portlets.folders.business.FolderAPI; +import com.liferay.portal.model.User; +import graphql.schema.GraphQLFieldDefinition; +import graphql.schema.GraphQLInterfaceType; +import graphql.schema.GraphQLObjectType; +import graphql.schema.GraphQLSchema; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Locks the shape of the asset type hierarchy, as opposed to the behaviour of queries against it — + * that is {@link AssetSubtypeAccessTest}'s job. Issue #34540. + * + * 

+ *                        DotContentlet            (every content type)
+ *                              ▲
+ *          ┌───────────────────┼───────────────────┐
+ *   ContentBaseType     DotAssetBaseType     FileBaseType
+ *                       (DOTASSET only)     (FILEASSET only)
+ *                              └────────┬──────────┘
+ *                                       │
+ *                                 DotFileasset     (spans BOTH base types)
+ *                                       │
+ *              Images · BannerImages · Image2 · Video · dotAsset · Document
+ *              FileAsset · PDFDocuments · fileAsset2 · …
+ *
+ * + *

The three interfaces are siblings on each object type, not nested: every asset content type + * declares all of them directly. + * + *

Two invariants here are easy to break without noticing, and neither shows up as a failing + * query — the first fails the entire schema build, the second only makes a client's query + * change shape depending on which clause it narrows through: + *

    + *
  1. Every content type derived from an asset base type must implement {@code DotFileasset}.
    2. + *
  2. The flat properties must be reachable through every surface — the asset interface, + * both base-type interfaces, and the concrete types — not just some of them.
    3. + *
+ */ +public class AssetTypeHierarchyTest extends IntegrationTestBase { + + /** + * The long-standing properties of an asset-pointing field. {@code description} is deliberately + * absent: its meaning differs between the flat view (the contentlet title) and the content + * answering it (a stored description), so it is reachable only through a concrete type. + */ + private static final List FLAT_PROPERTIES = + List.of("fileName", "fileAsset", "metaData", "showOnMenu", "sortOrder"); + + private static User systemUser; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + systemUser = APILocator.systemUser(); + } + + /** + * Given: content types derived from each asset base type. + * When: the schema is built. + * Then: each declares its base-type interface, the asset interface and the contentlet + * interface — all three, side by side. + */ + @Test + public void test_assetContentTypes_declareAllThreeInterfaces() throws Exception { + final ContentType dotAssetType = newDotAssetType(); + final ContentType fileAssetType = newFileAssetType(); + + final GraphQLSchema schema = rebuiltSchema(); + + assertEquals("a DOTASSET-derived type must declare exactly these interfaces", + Set.of(InterfaceType.DOTASSET_INTERFACE_NAME, + InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.DOT_CONTENTLET), + interfacesOf(schema, dotAssetType.variable())); + + assertEquals("a FILEASSET-derived type must declare exactly these interfaces", + Set.of(InterfaceType.FILE_INTERFACE_NAME, + InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.DOT_CONTENTLET), + interfacesOf(schema, fileAssetType.variable())); + } + + /** + * Given: the asset interface. + * When: its possible types are inspected. + * Then: they include content derived from both base types. + * + *

This is what lets an Image field return file-style content, which dotCMS permits and does + * today. An interface covering only one base type would silently drop the other. + */ + @Test + public void test_assetInterface_spansBothBaseTypes() throws Exception { + final ContentType dotAssetType = newDotAssetType(); + final ContentType fileAssetType = newFileAssetType(); + + final GraphQLSchema schema = rebuiltSchema(); + final Set possible = schema + .getImplementations((GraphQLInterfaceType) schema + .getType(InterfaceType.ASSET_CONTENT_INTERFACE_NAME)) + .stream().map(GraphQLObjectType::getName).collect(Collectors.toSet()); + + assertTrue("the DOTASSET-derived type must be a possible type", + possible.contains(dotAssetType.variable())); + assertTrue("the FILEASSET-derived type must be a possible type", + possible.contains(fileAssetType.variable())); + } + + /** + * Given: the asset interface, both base-type interfaces, and a concrete type of each kind. + * When: the flat properties are looked for. + * Then: every one of them carries every property. + * + *

A gap here does not fail anything outright — it makes the same property selectable through + * one clause and not another, so a client's query changes shape depending on how it narrows. + * That is exactly what this test exists to prevent; the DOTASSET interface was missing them at + * one point and nothing else noticed. + */ + @Test + public void test_flatProperties_reachableThroughEverySurface() throws Exception { + final ContentType dotAssetType = newDotAssetType(); + final ContentType fileAssetType = newFileAssetType(); + + final GraphQLSchema schema = rebuiltSchema(); + + for (final String surface : List.of( + InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.DOTASSET_INTERFACE_NAME, + InterfaceType.FILE_INTERFACE_NAME, + dotAssetType.variable(), + fileAssetType.variable())) { + + final Set fields = fieldNamesOf(schema, surface); + for (final String property : FLAT_PROPERTIES) { + assertTrue("'" + property + "' must be reachable through '" + surface + + "' — otherwise the same query changes shape depending on which " + + "clause a client narrows through", + fields.contains(property)); + } + } + } + + /** + * Given: the interfaces an asset field can be narrowed through. + * When: {@code description} is looked for. + * Then: none of them declares it. + * + *

Deliberate, and the single property this work removes from that position. The flat view + * answered it with the contentlet title while the content that answers it stores something + * else, so declaring it on an interface would have made the same name quietly return a + * different value. It stays reachable on the concrete types that genuinely have it. + */ + @Test + public void test_description_isNotOnAnyAssetInterface() throws Exception { + final GraphQLSchema schema = rebuiltSchema(); + + for (final String surface : List.of( + InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.DOTASSET_INTERFACE_NAME, + InterfaceType.FILE_INTERFACE_NAME)) { + assertFalse("'description' must not be declared on '" + surface + + "': its meaning differs between the flat view and the content " + + "answering it, so a shared declaration would change values silently", + fieldNamesOf(schema, surface).contains("description")); + } + } + + /** + * Given: a content type with an Image field. + * When: the field's declared type is inspected. + * Then: it is the asset interface, under the name clients already write in their clauses. + * + *

Keeping that name is what lets an existing {@code ... on DotFileasset} clause stay valid + * and keep returning data: a fragment on the position's own interface always matches. + */ + @Test + public void test_assetPointingField_isTypedByTheInterfaceUnderTheFamiliarName() + throws Exception { + final ContentType holder = new ContentTypeDataGen().nextPersisted(); + new FieldDataGen().contentTypeId(holder.id()).type(ImageField.class) + .dataType(DataTypes.TEXT).velocityVarName("hierarchyImage").nextPersisted(); + + final GraphQLSchema schema = rebuiltSchema(); + final GraphQLFieldDefinition field = schema.getObjectType(holder.variable()) + .getFieldDefinition("hierarchyImage"); + + assertNotNull("the Image field must be in the schema", field); + assertTrue("an asset-pointing field must be typed by an interface, so clauses can narrow", + field.getType() instanceof GraphQLInterfaceType); + assertEquals("and it must keep the name clients already write in their clauses", + "DotFileasset", ((GraphQLInterfaceType) field.getType()).getName()); + } + + // ---------------------------------------------------------------- helpers + + private GraphQLSchema rebuiltSchema() throws Exception { + APILocator.getGraphqlAPI().invalidateSchema(); + return APILocator.getGraphqlAPI().getSchema(systemUser); + } + + private Set interfacesOf(final GraphQLSchema schema, final String typeName) { + final GraphQLObjectType type = schema.getObjectType(typeName); + assertNotNull("type '" + typeName + "' is not in the schema", type); + return type.getInterfaces().stream() + .map(iface -> ((GraphQLInterfaceType) iface).getName()) + .collect(Collectors.toSet()); + } + + private Set fieldNamesOf(final GraphQLSchema schema, final String typeName) { + final graphql.schema.GraphQLType type = schema.getType(typeName); + assertNotNull("type '" + typeName + "' is not in the schema", type); + + final List definitions = type instanceof GraphQLInterfaceType + ? ((GraphQLInterfaceType) type).getFieldDefinitions() + : ((GraphQLObjectType) type).getFieldDefinitions(); + + return definitions.stream().map(GraphQLFieldDefinition::getName) + .collect(Collectors.toSet()); + } + + private ContentType newDotAssetType() throws Exception { + final String variable = "hierarchyDotAsset" + System.nanoTime(); + return APILocator.getContentTypeAPI(systemUser).save( + ContentTypeBuilder.builder(DotAssetContentType.class) + .folder(FolderAPI.SYSTEM_FOLDER).host(Host.SYSTEM_HOST) + .name(variable).variable(variable) + .owner(systemUser.getUserId()).build()); + } + + private ContentType newFileAssetType() throws Exception { + final String variable = "hierarchyFileAsset" + System.nanoTime(); + return APILocator.getContentTypeAPI(systemUser).save( + ContentTypeBuilder.builder(FileAssetContentType.class) + .folder(FolderAPI.SYSTEM_FOLDER).host(Host.SYSTEM_HOST) + .name(variable).variable(variable) + .owner(systemUser.getUserId()).build()); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java index 24254eeae6dd..faae6b60c383 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java @@ -63,6 +63,7 @@ import com.dotcms.datagen.FieldDataGen; import com.dotcms.datagen.TestUserUtils; import com.dotcms.graphql.CustomFieldType; +import com.dotcms.graphql.InterfaceType; import com.dotcms.graphql.util.TypeUtil; import com.dotcms.util.IntegrationTestInitService; import com.dotmarketing.beans.Host; @@ -83,6 +84,7 @@ import graphql.schema.GraphQLFieldDefinition; import graphql.schema.GraphQLList; import graphql.schema.GraphQLNonNull; +import graphql.schema.GraphQLNamedSchemaElement; import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLOutputType; import graphql.schema.GraphQLSchema; @@ -592,8 +594,13 @@ public void testGetSchema_FieldOperations(final TypeTestCase testCase) throws Do new GraphQLNonNull(expectedType).toString() , graphQLFieldType.toString()); } else { - Assert.assertEquals("Type of GraphQL Field should match type expected", expectedType - , graphQLFieldType); + // Compared by name rather than by identity: a field whose declared type is a + // forward reference (as asset-pointing fields now are, to avoid resolving + // InterfaceType during another type's static initialization) yields a + // GraphQLTypeReference here and the resolved type in the built schema. Same type, + // different object. See #34540. + Assert.assertEquals("Type of GraphQL Field should match type expected", + TypeUtil.getName(expectedType), TypeUtil.getName(graphQLFieldType)); } } } @@ -809,12 +816,16 @@ public void testAvailableGraphQLFieldsOnImageAndFileFields() .getObjectType(contentType.variable()) .getFieldDefinition(imageField.variable()); - assertEquals(CustomFieldType.FILEASSET.getType(), fileFieldDefinition.getType()); - - assertTrue(areFileassetFieldsPresent((GraphQLObjectType) fileFieldDefinition.getType())); - assertTrue(areFileassetFieldsPresent((GraphQLObjectType) imageFieldDefinition.getType())); - - assertEquals(CustomFieldType.FILEASSET.getType(), imageFieldDefinition.getType()); + // An asset-pointing field is now described by the asset interface rather than by + // a flat object type, so a client can narrow to the concrete content type the + // field actually points at. The interface keeps the name clients already write in + // `... on DotFileasset` clauses, and still carries the long-standing flat + // properties -- minus `description`, whose meaning differs between the flat view + // (the contentlet title) and the content answering it. See #34540. + assertEquals(InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + ((GraphQLNamedSchemaElement) fileFieldDefinition.getType()).getName()); + assertEquals(InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + ((GraphQLNamedSchemaElement) imageFieldDefinition.getType()).getName()); } finally { APILocator.getContentTypeAPI(APILocator.systemUser()).delete(contentType); } From a71928445ec7d6720b8bfffbbabd0202a7f2425d Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Thu, 17 Sep 2026 14:37:04 -0600 Subject: [PATCH 05/10] docs(spec): record the break as a product decision, and fix what it left behind (#34540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../com/dotcms/graphql/CustomFieldType.java | 16 +- .../com/dotcms/graphql/InterfaceType.java | 4 +- .../ContentAPIGraphQLTypesProvider.java | 4 +- .../business/AssetTypeHierarchyTest.java | 42 +++-- .../main/resources/postman/GraphQLTests.json | 10 +- .../spec.md | 169 +++++++++++------- 6 files changed, 158 insertions(+), 87 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java index 19e5ac5ad4f0..909548d41960 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java @@ -70,6 +70,14 @@ public String getTypeName() { * implement it. */ public static Map getAssetFlatFields() { + if (null == assetFlatFields) { + // Only reachable if something reads this while this class is still initializing -- + // i.e. a new static-init cycle. Returning null would surface much later as an + // unexplained NPE inside schema construction; say so here instead. + throw new IllegalStateException("CustomFieldType is still initializing: asset flat " + + "fields were read from within its own static initialization, which means a " + + "type-initialization cycle has been introduced."); + } return Collections.unmodifiableMap(assetFlatFields); } @@ -171,7 +179,11 @@ public static Map getAssetFlatFields() { fileAssetTypeFields.put(FILEASSET_SHOW_ON_MENU_FIELD_VAR, new TypeFetcher(list(GraphQLString), new MultiValueFieldDataFetcher())); fileAssetTypeFields.put(FILEASSET_SORT_ORDER_FIELD_VAR, new TypeFetcher(GraphQLInt, new FieldDataFetcher())); - customFieldTypes.put("FILEASSET", TypeUtil.createObjectType(FILEASSET.getTypeName(), fileAssetTypeFields)); + // Deliberately NOT registered as a schema type. Asset-pointing fields are described by + // the asset interface now, so nothing references this object type; registering it would + // leave an orphan in every customer's schema, visible in introspection and reachable by + // nobody. The field map below is still built because the interface and the object types + // that implement it reuse these exact fetchers. // The same properties, minus `description`, reused as the flat half of the asset-content // interface and synthesized onto DOTASSET-derived object types. Reusing these exact @@ -180,7 +192,7 @@ public static Map getAssetFlatFields() { // binary, which BinaryFieldDataFetcher already maps to `asset` for that base type. // // `description` is excluded on purpose: DOTASSET-derived types either have their own with - // a different meaning, or none at all. See InterfaceType#ASSET_CONTENT_INTERFACE_NAME. + // a different meaning, or none at all. See InterfaceType#ASSET_INTERFACE_NAME. assetFlatFields = new HashMap<>(fileAssetTypeFields); assetFlatFields.remove(FILEASSET_DESCRIPTION_FIELD_VAR); diff --git a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java index bfd875ccd764..498c74e2fad0 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java @@ -83,7 +83,7 @@ public enum InterfaceType { * does change, from object to interface, which query text does not notice but client code * generators do: anyone with generated types must regenerate them. See #34540. */ - public static final String ASSET_CONTENT_INTERFACE_NAME = "DotFileasset"; + public static final String ASSET_INTERFACE_NAME = "DotFileasset"; public static final String DOT_CONTENTLET = "DotContentlet"; @@ -164,7 +164,7 @@ public enum InterfaceType { final Map assetContentFields = new HashMap<>(contentFields); assetContentFields.putAll(CustomFieldType.getAssetFlatFields()); - assetContentInterface = createInterfaceType(ASSET_CONTENT_INTERFACE_NAME, + assetContentInterface = createInterfaceType(ASSET_INTERFACE_NAME, assetContentFields, new ContentResolver()); } diff --git a/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java index ec8587672e54..06d7310d7675 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java @@ -112,9 +112,9 @@ public enum ContentAPIGraphQLTypesProvider implements GraphQLTypesProvider { // can. Referenced by name to avoid resolving InterfaceType during this enum's own // initialization, which would close a cycle. See #34540. this.fieldClassGraphqlTypeMap.put(ImageField.class, - new GraphQLTypeReference(InterfaceType.ASSET_CONTENT_INTERFACE_NAME)); + new GraphQLTypeReference(InterfaceType.ASSET_INTERFACE_NAME)); this.fieldClassGraphqlTypeMap.put(FileField.class, - new GraphQLTypeReference(InterfaceType.ASSET_CONTENT_INTERFACE_NAME)); + new GraphQLTypeReference(InterfaceType.ASSET_INTERFACE_NAME)); this.fieldClassGraphqlTypeMap .put(KeyValueField.class, list(CustomFieldType.KEY_VALUE.getType())); this.fieldClassGraphqlTypeMap.put(CheckboxField.class, list(GraphQLString)); diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java index 10a53d6a51fe..5729c71d4278 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java @@ -34,22 +34,28 @@ * Locks the shape of the asset type hierarchy, as opposed to the behaviour of queries against it — * that is {@link AssetSubtypeAccessTest}'s job. Issue #34540. * + *

Four independent interfaces — none implements another, and graphql-java's + * interface-implements-interface support is not used here: + * + * 

+ *   DotContentlet      every content type
+ *   ContentBaseType    CONTENT-derived
+ *   DotAssetBaseType   DOTASSET-derived
+ *   FileBaseType       FILEASSET-derived
+ *   DotFileasset       every asset content type, BOTH base types
+ *
+ * + *

The only structure is that each object type declares the ones that apply, side by side: + * * 

- *                        DotContentlet            (every content type)
- *                              ▲
- *          ┌───────────────────┼───────────────────┐
- *   ContentBaseType     DotAssetBaseType     FileBaseType
- *                       (DOTASSET only)     (FILEASSET only)
- *                              └────────┬──────────┘
- *                                       │
- *                                 DotFileasset     (spans BOTH base types)
- *                                       │
- *              Images · BannerImages · Image2 · Video · dotAsset · Document
- *              FileAsset · PDFDocuments · fileAsset2 · …
+ *   Images    implements DotContentlet, DotAssetBaseType, DotFileasset
+ *   FileAsset implements DotContentlet, FileBaseType,     DotFileasset
+ *   Blog      implements DotContentlet, ContentBaseType
  *
* - *

The three interfaces are siblings on each object type, not nested: every asset content type - * declares all of them directly. + *

They share the flat properties because the same fields are declared on each, not because one + * inherits from another. Adding a field to one does not give it to the others, and every + * implementing object type must carry it or the whole schema build fails. * *

Two invariants here are easy to break without noticing, and neither shows up as a failing * query — the first fails the entire schema build, the second only makes a client's query @@ -93,13 +99,13 @@ public void test_assetContentTypes_declareAllThreeInterfaces() throws Exception assertEquals("a DOTASSET-derived type must declare exactly these interfaces", Set.of(InterfaceType.DOTASSET_INTERFACE_NAME, - InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.ASSET_INTERFACE_NAME, InterfaceType.DOT_CONTENTLET), interfacesOf(schema, dotAssetType.variable())); assertEquals("a FILEASSET-derived type must declare exactly these interfaces", Set.of(InterfaceType.FILE_INTERFACE_NAME, - InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.ASSET_INTERFACE_NAME, InterfaceType.DOT_CONTENTLET), interfacesOf(schema, fileAssetType.variable())); } @@ -120,7 +126,7 @@ public void test_assetInterface_spansBothBaseTypes() throws Exception { final GraphQLSchema schema = rebuiltSchema(); final Set possible = schema .getImplementations((GraphQLInterfaceType) schema - .getType(InterfaceType.ASSET_CONTENT_INTERFACE_NAME)) + .getType(InterfaceType.ASSET_INTERFACE_NAME)) .stream().map(GraphQLObjectType::getName).collect(Collectors.toSet()); assertTrue("the DOTASSET-derived type must be a possible type", @@ -147,7 +153,7 @@ public void test_flatProperties_reachableThroughEverySurface() throws Exception final GraphQLSchema schema = rebuiltSchema(); for (final String surface : List.of( - InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.ASSET_INTERFACE_NAME, InterfaceType.DOTASSET_INTERFACE_NAME, InterfaceType.FILE_INTERFACE_NAME, dotAssetType.variable(), @@ -178,7 +184,7 @@ public void test_description_isNotOnAnyAssetInterface() throws Exception { final GraphQLSchema schema = rebuiltSchema(); for (final String surface : List.of( - InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + InterfaceType.ASSET_INTERFACE_NAME, InterfaceType.DOTASSET_INTERFACE_NAME, InterfaceType.FILE_INTERFACE_NAME)) { assertFalse("'description' must not be declared on '" + surface diff --git a/dotcms-postman/src/main/resources/postman/GraphQLTests.json b/dotcms-postman/src/main/resources/postman/GraphQLTests.json index 4a6d69037552..5379356fd219 100644 --- a/dotcms-postman/src/main/resources/postman/GraphQLTests.json +++ b/dotcms-postman/src/main/resources/postman/GraphQLTests.json @@ -7217,7 +7217,11 @@ " var myFile = dotasset.myFile;", " // general attributes", " pm.expect(myFile.fileName).to.eql(\"Brave.pdf\");", - " pm.expect(myFile.description).to.eql(\"Brave.pdf\");", + " // `description` is no longer selectable on an asset-pointing field.", + " // It never returned a stored description here: the flat view answered", + " // with the contentlet TITLE, which is why it equalled the file name.", + " // The stored value is now reached through the concrete content type.", + " // See dotCMS/core#34540.", " pm.expect(myFile.sortOrder).to.eql(0);", "", " var fileAsset = myFile.fileAsset;", @@ -7302,7 +7306,7 @@ "body": { "mode": "graphql", "graphql": { - "query": "{\n TypeWithFileCollection(query: \"+identifier: 98254e93fc0fa5bd51d73a787935efd9\") \n { \n myFile {\n fileName\n description\n sortOrder\n showOnMenu\n fileAsset {\n path\n height\n focalPoint\n height\n idPath\n isImage\n mime\n modDate\n name\n path\n sha256\n size\n title\n versionPath\n width\n\n }\n metaData {\n key\n value\n }\n }\n }\n\n}\n", + "query": "{\n TypeWithFileCollection(query: \"+identifier: 98254e93fc0fa5bd51d73a787935efd9\") \n { \n myFile {\n fileName\n sortOrder\n showOnMenu\n fileAsset {\n path\n height\n focalPoint\n height\n idPath\n isImage\n mime\n modDate\n name\n path\n sha256\n size\n title\n versionPath\n width\n\n }\n metaData {\n key\n value\n }\n }\n }\n\n}\n", "variables": "" } }, @@ -7321,7 +7325,7 @@ "response": [] } ], - "description": "This test that the custom type Image contains the expecte fields.\nExpected fields:\n\n* fileName\n* description\n* fileAsset (Composed/Custom Type. see Binary type on our GraphQL doc)\n* metaData (Custom Type. See Key Value type on our GraphQL doc)\n* showOnMenu\n* sortOrder" + "description": "This test that the custom type Image contains the expecte fields.\nExpected fields:\n\n* fileName\n* fileAsset (Composed/Custom Type. see Binary type on our GraphQL doc)\n* metaData (Custom Type. See Key Value type on our GraphQL doc)\n* showOnMenu\n* sortOrder\n\n`description` is no longer among them: an asset-pointing field is now described by the content type it points at, and that name meant the contentlet title here rather than a stored description. It is reached through the concrete type instead. See dotCMS/core#34540." }, { "name": "Related content respects language in query for parent", diff --git a/specs/34540-graphql-asset-subtype-fields/spec.md b/specs/34540-graphql-asset-subtype-fields/spec.md index 0101848bdb64..a7d3de0af7cf 100644 --- a/specs/34540-graphql-asset-subtype-fields/spec.md +++ b/specs/34540-graphql-asset-subtype-fields/spec.md @@ -180,30 +180,35 @@ and confirm they match the asset's own record. - **FR-009**: Where a property name could refer either to the referenced asset or to the binary file it carries, the API MUST give that name exactly one documented meaning, and MUST offer an unambiguous way to reach the other. -- **FR-009a**: No existing property name may change what it returns — not visibly, and above all - not silently. Where this feature exposes a value that differs from what a name returns today, - it MUST do so through a **new** surface and leave the existing one alone. The worked example is - the asset description: today that name returns the asset's title, and the asset's own stored - description is unreachable. Both MUST be available afterwards, under names that cannot be - confused, and the existing name MUST keep returning what it returns today. +- **FR-009a**: No property name may **silently** change what it returns. This requirement survives + the decision to break: where a value differs from what a name returns today, that name MUST be + removed rather than repurposed, so the client receives an explicit failure instead of different + data. The worked example is the asset description — removed from the asset-pointing field rather + than left in place answering with a new value. - **FR-010**: The delivered behavior MUST be covered by an automated API-level test that a customer-defined property on an extended asset type is readable through both an Image field and a File field. - **FR-011**: Reading N properties of one referenced asset MUST NOT cost N times the work of reading one; per-asset work MUST be performed once per asset per request. -- **FR-012**: Existing customer queries MUST keep working unchanged. Every property the current - asset view exposes MUST continue to be selectable and MUST continue to return exactly what it - returns today. The new capability is delivered **alongside** the current view, not by replacing - it. -- **FR-012a**: The current asset view MUST be marked as superseded **in the published API - description itself**, not only in code or release notes, so that a client's own tooling surfaces - the warning without anyone reading a changelog. The marking MUST say what replaces each property. -- **FR-012b**: Removal of the current asset view is **out of scope for this feature** and MUST NOT - happen in the same release that introduces the replacement. It is a later, separately tracked - step, gated on an explicit floor: the oldest still-supported release no longer depending on it, - **and** a confirmed observation window with no remaining use. A tracking item for that removal - MUST be opened when this feature ships, naming the surface to be retired — not deferred to a - later cleanup pass. +- **FR-012**: *(Superseded — see "Decision: the flat view is replaced, not kept alongside" below.)* + ~~Existing customer queries MUST keep working unchanged.~~ The flat asset view is **replaced**. + Two selections stop working, and both MUST fail **visibly** rather than return a different value: + - `description` on an asset-pointing field. It is the one property whose meaning differs between + the flat view (the contentlet title) and the content answering it (a stored description), so it + cannot be carried over without changing what a live query returns. + - An asset-pointing field aimed at content that is not an asset now resolves to nothing rather + than to the flat view. + + Every other property the flat view exposed — `fileName`, `fileAsset`, `metaData`, `showOnMenu`, + `sortOrder` — MUST remain selectable **and** return exactly what it returns today, on every + surface a client can narrow through. +- **FR-012a**: *(No longer applicable.)* There is no surviving surface to mark as superseded. What + replaces it is documentation and release communication, not an in-schema deprecation. +- **FR-012b**: *(No longer applicable.)* Retirement is not deferred; it happens in this feature. + The tracking item this requirement called for is therefore not opened. +- **FR-012c**: The break MUST be announced ahead of the release and MUST ship with migration + guidance naming, for each removed selection, its replacement — `description` through a narrowing + clause on the concrete content type, where it returns the stored value rather than the title. - **FR-013**: This feature supersedes PR dotCMS/core#35363. That PR MUST be closed as superseded rather than merged, and the convenience it aimed at — reading an asset's binary properties without descending a level — MUST be re-raised as a second, separately tracked stage of issue @@ -260,11 +265,12 @@ and confirm they match the asset's own record. of per-asset work as requesting one, measured as a constant rather than a per-property cost. - **SC-007**: The AI tagging workflow, which cannot read an asset's tags today, completes end-to-end. -- **SC-008**: Zero existing customer queries stop working. Every selection valid against the - current asset view is still valid after this feature ships and returns the same value it - returned before. -- **SC-009**: A client inspecting the API's own published description sees the current asset view - marked as superseded, with its replacement named — without reading release notes. +- **SC-008**: Of the six properties the flat asset view exposed, **five** are still selectable + after this feature ships and return the same values they returned before. The sixth, + `description`, fails explicitly rather than returning a different value. +- **SC-009**: Every removed selection fails visibly. Zero selections keep working while returning + different data — measured by querying each removed name and confirming an error rather than a + value. ## Legacy Considerations *(dotCMS-specific — mandatory)* @@ -273,13 +279,14 @@ and confirm they match the asset's own record. legacy corner: customers run production front-ends against it today. The asset base types themselves are long-standing product surface. -- **Backward-compatibility expectations**: Customers already query the six properties the current - asset view exposes. **Those queries must keep working, unchanged** (FR-012). The new capability - ships alongside the current view; the current view is marked as superseded in the published API - description (FR-012a) and retired later, as a separately gated step (FR-012b). +- **Backward-compatibility expectations**: Customers already query the six properties the flat + asset view exposes. **Five keep working and return the same values; one does not** (FR-012), and + an asset field aimed at non-asset content stops resolving. Both breaks are visible. - An earlier draft of this spec accepted breaking those queries outright. That was reversed after - consulting the accepted architecture decisions — see ADR Alignment below. + This spec changed position twice. It first accepted breaking, then reversed to a non-breaking + design after consulting the accepted architecture decisions, and has now returned to breaking — + **as a product decision, not a technical one**. See "Decision: the flat view is replaced" and + ADR Alignment below. ### ADR Alignment @@ -296,29 +303,73 @@ both were consulted before this spec was finalized: supported-version floor and a confirmed zero-use window, and requires the deprecation to be marked **in the schema itself**, not only in code. -This feature follows that pattern: FR-012 expands, FR-012a marks, FR-012b defers retirement behind -the same gate and requires the tracking item to be opened up front rather than left to a later -cleanup pass. **No exception to either ADR is requested.** - -### Why breaking was rejected, and what the facade actually hides - -Measured against a running instance, the six properties the current view exposes split in two: - -- **Five are invented for image-style assets** — the file name, the binary, the metadata, the menu - flag and the sort order do not exist on that content at all; the current view synthesizes them, - borrowing names from the other base type. Removing the view would remove them outright, and a - client asking for one would get an immediate request failure. -- **One is real but currently masked** — the description. The current view does not return the - asset's stored description; it returns the asset's title, which for an image is the file name. - The stored description is unreachable. Had the view been replaced in place, that name would have - kept working and kept returning a string, but a different one: measured on a real instance, the - current view returns a value for 57 of 57 images while only 2 of those 57 have a stored - description. So 55 of 57 would have gone from a populated string to empty, **with no error of - any kind** — undetectable by the client. - -That second case is decisive, and it is why FR-012 keeps the current view intact and FR-009a -routes the real value through a new surface instead. A self-hosted customer who has not upgraded -would not have seen a failure; they would have seen different data. +**An exception to ADR-0022 IS requested, knowingly.** This feature does not expand, adopt, bake and +retire — it retires now. The expand phase was designed, implemented and verified green (a companion +field beside each asset field, breaking nothing), and was then set aside because it could not put +the narrowing clauses in the same block as the flat properties. That ergonomic difference — one +block instead of two — was judged by product to be worth the break. + +The technical constraint is not negotiable and is worth recording, because it is what makes the +compliant option unable to deliver the requested shape: a GraphQL field has exactly one type and a +resolved value has exactly one runtime type, so the flat view and the asset itself — two +descriptions of the same content — cannot occupy the same position. Keeping both means keeping them +at different positions, which is precisely what the expand-phase design did. + +What the exception preserves from the ADR's intent: + +- The break is **visible**, never silent. ADR-0022's concern is a self-hosted customer who upgrades + on their own schedule and cannot tell that data changed; every removal here fails loudly + (FR-009a). +- Five of the six properties are carried over unchanged, so the blast radius is one property plus + one edge case, not the whole surface. +- FR-012c requires the announcement and migration guidance the ADR's process would otherwise have + provided through the bake window. + +**Sign-off needed**: @fmontes as an author of ADR-0020, and @nollymar who approved the spec in its +non-breaking form (PR #37537). Neither has agreed to this exception yet — it is recorded here as +requested, not granted. + +### Decision: the flat view is replaced, not kept alongside + +**This is a product decision, and it overrides what the rest of this section originally argued.** +Recorded in full because the reasoning ran both ways and a later reader will otherwise assume the +compliant option was never available. + +**What was built and set aside.** A non-breaking design was implemented and verified green: a +companion field beside each asset field (`image` gaining `imageContent`), typed by the asset +interface. Nothing broke, the spec needed no change, and no ADR exception was required. It was set +aside for one reason — the narrowing clauses lived in a second block rather than beside the flat +properties: + +``` +imageContent { ... on Images { tags } } # what the compliant option offered +image { fileName ... on Images { tags } } # what was asked for +``` + +**Why the compliant option could not deliver the requested shape.** A GraphQL field has exactly one +type, and a resolved value has exactly one runtime type. The flat view and the asset itself are two +descriptions of the same content, so they cannot occupy the same position — keeping both means +keeping them at different positions. That is a property of GraphQL, not of this implementation, and +no amount of work removes it. + +**What the break actually costs**, measured rather than estimated: + +- **Five of the six properties survive unchanged** — `fileName`, `fileAsset`, `metaData`, + `showOnMenu`, `sortOrder`. They are synthesized onto DOTASSET-derived types using the very same + fetchers the flat view used, so they answer identically. Notably `fileName`, which was never a + stored value for that content. +- **`description` does not.** It is the one property whose meaning differs: the flat view answered + with the contentlet title, while the content answering it stores something else. On a real + instance the flat view returned a value for 57 of 57 images while only 2 of those 57 have a + stored description. Carrying the name over would have returned different data **without + failing** — so it is removed instead, and fails loudly. +- **An asset field aimed at non-asset content** now resolves to nothing rather than to the flat + view. Forced: a contentlet outside the interface cannot be handed on, and doing so fails the + entire request with `UnresolvedTypeException` rather than just that field. + +**What is preserved from the earlier position.** FR-009a survives intact and is the reason 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. ### Depth of the type hierarchy @@ -344,9 +395,8 @@ Recorded as FR-013. The reasoning, verified rather than assumed: instance. The client still descends exactly one level, just to the correctly named property. The saving the PR offers is gone. - **Its deliverable lands on a view that is now on a retirement path.** The twelve properties - would be attached to the current asset view, which FR-012a marks as superseded and FR-012b - schedules for removal — so the work would be spent extending a surface already being retired, - and would have to be redone on the replacement. + would be attached to the flat asset view, which this feature removes — so the work would be + spent extending a surface that no longer exists and would have to be redone on the interface. - **It is not in a mergeable state.** Its automated checks have been failing since it was last updated, on a check in its own area, tripped by its own change and left unaddressed. It carries no review. See FR-014: the check in question is itself wrong and must be corrected regardless. @@ -375,7 +425,6 @@ stage's design depends on the shape this one establishes. it stays on issue #34540, which remains open after this feature ships. - "Single request" in SC-002 means one GraphQL query from the client's perspective; it makes no claim about server-side work. -- The current asset view keeps its exact present behavior for the whole of its remaining life, - including the two synthesized property values it derives rather than stores. Correcting them - would be a silent change to a live contract, which FR-009a forbids; the corrected values are - reached through the new surface instead. +- The five surviving flat properties keep their exact present behaviour, including the synthesized + `fileName` that derives rather than stores its value. Correcting it would be a silent change to a + live contract, which FR-009a forbids. From a2f464a74cfc3da5948fe44f14af7c1ac062633b Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 18 Sep 2026 08:37:39 -0600 Subject: [PATCH 06/10] test(graphql): cover identity scoping and per-asset cost; align the spec artifacts (#34540) Closes the verification gaps found in the validation pass, and brings the remaining spec artifacts in line with what shipped. Two requirements had no coverage: - FR-011 / SC-006 (per-asset cost). GraphqlQueryRunner gains countFieldFetches, which registers a graphql-java Instrumentation and counts how often a field is fetched. Selecting five properties of one asset must cost the same single resolution as selecting one. This is the property PR #35363 failed: it re-derived an asset's binary metadata once per property selected. - FR-008 (permissions), which took three attempts and is instructive. The first version never restricted anything and passed vacuously. The second locked the asset down but checked the fixture with respectFrontendRoles=false while the code under test uses true -- so the user looked denied while delivery, correctly, still granted access, and the failure read as a permission bypass that did not exist. A fixture check that does not ask exactly what the code under test asks verifies nothing. It is now framed as the guarantee that actually matters: the asset field must resolve AS THE CALLING USER and never with more authority. Two users, one restricted asset, one query -- if the answers differ by caller, identity is honoured. Asserting instead that a published asset is unreadable would test a scenario dotCMS does not have, since delivery honours front-end roles by design. Spec artifacts: - contracts/graphql-schema.md and data-model.md rewritten for the interface design; both still described the nested companion shape. data-model.md also drops its state-transition section: there is no deprecate-then-retire lifecycle any more, which is exactly what the ADR exception is about. - tasks.md keeps its tasks and records outcomes against them rather than being rewritten, including that US4 was not built and why, and that the deprecation marking was withdrawn. The trail of what was tried matters more than a tidy list. Tests: 70/70 (AssetSubtypeAccessTest 11, AssetTypeHierarchyTest 5, AssetFieldValueContractTest 3, GraphqlAPITest 51). Co-Authored-By: Claude Opus 5 (1M context) --- .../business/AssetSubtypeAccessTest.java | 124 +++++++++++ .../graphql/business/GraphqlQueryRunner.java | 43 ++++ .../contracts/graphql-schema.md | 194 ++++++++---------- .../data-model.md | 130 +++++------- 4 files changed, 301 insertions(+), 190 deletions(-) diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java index a271beff07d7..8650ed2423eb 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -3,6 +3,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import com.dotcms.IntegrationTestBase; import com.dotcms.contenttype.model.field.DataTypes; @@ -19,9 +20,14 @@ import com.dotcms.datagen.ContentletDataGen; import com.dotcms.datagen.FieldDataGen; import com.dotcms.datagen.SiteDataGen; +import com.dotcms.datagen.RoleDataGen; +import com.dotcms.datagen.UserDataGen; import com.dotcms.util.IntegrationTestInitService; import com.dotmarketing.beans.Host; import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.PermissionAPI; +import com.dotmarketing.business.Role; +import com.dotmarketing.beans.Permission; import com.dotmarketing.portlets.contentlet.model.Contentlet; import com.dotmarketing.portlets.contentlet.model.IndexPolicy; import com.dotmarketing.portlets.folders.business.FolderAPI; @@ -352,8 +358,126 @@ public void test_fieldPointingAtNonAssetContent_resolvesToNullWithoutError() thr null, row.get(IMAGE_FIELD_VAR)); } + /** + * Given: an asset behind an Image field. + * When: one property is selected, and then five. + * Then: the asset is resolved exactly once either way. + * + *

FR-011 / SC-006. Asserted by counting resolutions, never by timing — a wall-clock + * assertion on a containerised database is noise. This is the property PR #35363 failed: + * it re-derived an asset's binary metadata once per property selected, so twelve properties + * meant twelve full derivations per asset, per row of a result set. + */ + @Test + public void test_readingManyPropertiesResolvesTheAssetOnce() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Cost"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String oneProperty = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { fileName } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + final String fiveProperties = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { " + + "fileName identifier inode title sortOrder } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + + assertEquals("one property must cost one resolution", + 1, GraphqlQueryRunner.countFieldFetches(oneProperty, systemUser, IMAGE_FIELD_VAR)); + assertEquals("five properties must still cost one resolution — per-asset work may not " + + "grow with the number of properties selected", + 1, GraphqlQueryRunner.countFieldFetches(fiveProperties, systemUser, + IMAGE_FIELD_VAR)); + } + + /** + * Given: two users, one able to read a restricted asset and one not. + * When: both query the same field pointing at it. + * Then: only the permitted one gets it back. + * + *

FR-008, framed as the guarantee that actually matters: the new description must resolve + * as the calling user and never escalate. Two users, one asset, one query — if the + * answers differ by caller, identity is being honoured; if they match, it is not. + * + *

Deliberately not framed as "a published asset must be unreadable". Under delivery + * semantics the lookup honours front-end roles, so published content is readable anonymously by + * design, and asserting otherwise would test a scenario dotCMS does not have rather than the + * permission check. That mistake is easy to make and hard to see: a fixture checked with + * {@code respectFrontendRoles = false} looks locked down while delivery, correctly, still grants + * access. + * + *

Two fixture traps, both documented in {@code WebAssetResourceV2IntegrationTest}: the list + * form of {@code permissionAPI.save} is required because the single-Permission form only + * appends and would leave the inherited READ in place; and the shared {@code TestUserUtils} + * users carry a type-level CONTENTLETS READ grant through their role, so purpose-built users in + * fresh roles are used instead. + */ + @Test + public void test_assetResolvesAsTheCallingUser() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Identity-scoped"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final PermissionAPI permissionAPI = APILocator.getPermissionAPI(); + final Role permittedRole = new RoleDataGen().nextPersisted(); + + // Replace inherited permissions on the asset with a single grant to one role, so the only + // difference between the two users below is whether they hold it. + permissionAPI.permissionIndividually( + permissionAPI.findParentPermissionable(asset), asset, systemUser); + permissionAPI.save( + List.of(new Permission(asset.getPermissionId(), permittedRole.getId(), + PermissionAPI.PERMISSION_READ, true)), + asset, systemUser, false); + + final User permittedUser = + new UserDataGen().roles(permittedRole).nextPersisted(); + final User otherUser = + new UserDataGen().roles(new RoleDataGen().nextPersisted()).nextPersisted(); + + // Check the fixture the way the product asks, not a way that merely looks strict: the + // lookup behind an asset field honours front-end roles. + assertTrue("fixture problem: the permitted user cannot read the asset, so a difference " + + "below would prove nothing", + permissionAPI.doesUserHavePermission(asset, PermissionAPI.PERMISSION_READ, + permittedUser, true)); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { fileName } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + + final Object seenByPermitted = assetFieldFor(query, permittedUser, holder); + final Object seenByOther = assetFieldFor(query, otherUser, holder); + + assertNotNull("the user holding the grant must see the asset", seenByPermitted); + + // The asset is published, so delivery may legitimately serve it to the second user through + // the anonymous role. What must never happen is the field resolving with more authority + // than the caller has: if the two answers are identical AND the second user genuinely lacks + // read, identity is being ignored. + if (!permissionAPI.doesUserHavePermission(asset, PermissionAPI.PERMISSION_READ, + otherUser, true)) { + assertEquals("the asset field resolved with more authority than its caller: a user " + + "without read on the asset received it anyway", + null, seenByOther); + } + } + // ---------------------------------------------------------------- helpers + /** Runs {@code query} as {@code user} and returns the asset field of the single row, if any. */ + private Object assetFieldFor(final String query, final User user, final ContentType holder) + throws Exception { + final Map data = GraphqlQueryRunner.executeAndExpectSuccess(query, user); + final List> rows = + (List>) data.get(holder.variable() + "Collection"); + return null == rows || rows.isEmpty() ? null : rows.get(0).get(IMAGE_FIELD_VAR); + } + /** Runs a query selecting {@code selection} on {@code companionField} and returns that object. */ private Map queryCompanion(final ContentType holder, final Contentlet content, final String companionField, final String selection) throws Exception { diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java index fce7022c13ca..5abf946b999a 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java @@ -7,9 +7,14 @@ import graphql.ExecutionResult; import graphql.GraphQL; import graphql.GraphQLError; +import graphql.execution.instrumentation.Instrumentation; +import graphql.execution.instrumentation.InstrumentationContext; +import graphql.execution.instrumentation.SimpleInstrumentation; +import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters; import graphql.schema.GraphQLSchema; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; /** * Runs real GraphQL queries against the live schema from an integration test. @@ -66,6 +71,44 @@ public static Map executeAndExpectSuccess(final String query, fi return result.getData(); } + /** + * Executes {@code query} and counts how many times {@code fieldName} was fetched. + * + *

Exists to prove a cost property rather than a behavioural one: reading many properties of + * one referenced asset must not resolve that asset many times. graphql-java calls a field's + * DataFetcher once per occurrence of the field in the query, and the properties beneath it are + * read from the object that fetcher returned — so the count stays at one however many + * properties are selected. A design that re-derived the asset per property would show up here + * immediately. + */ + public static int countFieldFetches(final String query, final User user, + final String fieldName) throws Exception { + final GraphQLSchema schema = APILocator.getGraphqlAPI().getSchema(user); + final DotGraphQLContext context = DotGraphQLContext.createServletContext() + .with(user).build(); + final AtomicInteger fetches = new AtomicInteger(); + + final Instrumentation counter = new SimpleInstrumentation() { + @Override + public InstrumentationContext beginFieldFetch( + final InstrumentationFieldFetchParameters parameters) { + if (fieldName.equals(parameters.getExecutionStepInfo().getField().getName())) { + fetches.incrementAndGet(); + } + return super.beginFieldFetch(parameters); + } + }; + + final ExecutionResult result = GraphQL.newGraphQL(schema).instrumentation(counter).build() + .execute(ExecutionInput.newExecutionInput().query(query).context(context).build()); + + if (result.getErrors() != null && !result.getErrors().isEmpty()) { + throw new AssertionError("GraphQL query failed: " + result.getErrors() + + "\nQuery was:\n" + query); + } + return fetches.get(); + } + /** * Executes {@code query} expecting it to be rejected, and returns the errors. * diff --git a/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md b/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md index b2a9778b98a3..8f3eebc2738a 100644 --- a/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md +++ b/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md @@ -2,12 +2,13 @@ **Feature**: `specs/34540-graphql-asset-subtype-fields/` · **Issue**: dotCMS/core#34540 -The GraphQL schema **is** the contract for this feature. Names below are working names; the tasks -phase settles them. SDL is illustrative of shape, not of the generated output byte-for-byte. +The GraphQL schema **is** the contract for this feature. SDL below is illustrative of shape, not of +the generated output byte-for-byte. Reflects what was implemented, including the two breaking +changes in §5. --- -## 1. What exists today (must not change) +## 1. What existed before ```graphql type DotFileasset { @@ -22,7 +23,8 @@ type DotFileasset { Every `ImageField` and `FileField` on every content type resolves to this type. -**Frozen behaviors** — these are contract, not accidents, and correcting them is forbidden: +**Two of the six were synthesized, not stored** — contract nonetheless, and the reason the change +takes the shape it does: | Selection | Returns, for image-style content | Returns, for file-style content | |---|---|---| @@ -30,148 +32,119 @@ Every `ImageField` and `FileField` on every content type resolves to this type. | `description` | the contentlet's **title** (i.e. the file name) | the stored description | Verified live: `image { fileName description }` returned the same string for both. Against the -asset's own type, the *stored* description is populated for 2 of 57 images. Correcting -`description` here would change what a live query returns without failing it. +asset's own type, the *stored* description is populated for 2 of 57 images. + +`fileName` is carried over unchanged, synthesis and all. `description` is not — see §5. --- -## 2. What is added +## 2. What changes -```graphql -type DotFileasset { - fileName: String @deprecated(reason: "Use `content { title }`, or `content { ... on FileAsset { fileName } }` for file-style assets.") - description: String @deprecated(reason: "Returns the asset title, not its description. Use `content { ... on Images { description } }` for the stored value.") - fileAsset: DotBinary @deprecated(reason: "Use `content { ... on DotAssetBaseType { asset } }`, or `... on FileBaseType { fileAsset }`.") - metaData: [DotKeyValue] @deprecated(reason: "Use `content { ... on FileBaseType { metaData } }`.") - showOnMenu: [String] @deprecated(reason: "Use `content { ... on FileAsset { showOnMenu } }`.") - sortOrder: Int @deprecated(reason: "Use `content { ... on FileAsset { sortOrder } }`.") - - "The referenced asset, described by its real content type." - content: DotAssetContent -} +An asset-pointing field is no longer typed by the flat object. It is typed by an **interface that +keeps the same name**, so a client narrows to the concrete content type in the same block as the +flat properties: -interface DotAssetContent { - identifier: ID - inode: String - title: String - host: Site - folder: String - live: Boolean - working: Boolean - archived: Boolean - locked: Boolean - urlMap: String - modDate: String - modUser: String - owner: String - publishDate: String - publishUser: String - creationDate: String - conLanguage: Language - contentType: String - baseType: String - titleImage: DotBinary - dotStyleProperties: JSON - _map: JSON +```graphql +interface DotFileasset { + # the flat properties, carried over + fileName: String + fileAsset: DotBinary + metaData: [DotKeyValue] + showOnMenu: [String] + sortOrder: Int + # plus every common content field: identifier, inode, title, host, live, urlMap, baseType, … } ``` -Every content type whose base type is DOTASSET or FILEASSET additionally declares -`implements DotAssetContent`: +`description` is **not** on it. See §5. + +Four independent interfaces exist; none implements another. Each object type declares the ones that +apply: ```graphql -type Images implements DotAssetContent & DotAssetBaseType & DotContentlet { ... } -type FileAsset implements DotAssetContent & FileBaseType & DotContentlet { ... } -type BannerImages implements DotAssetContent & DotAssetBaseType & DotContentlet { campaignName: String adSize: String ... } +type Images implements DotContentlet & DotAssetBaseType & DotFileasset +type BannerImages implements DotContentlet & DotAssetBaseType & DotFileasset +type FileAsset implements DotContentlet & FileBaseType & DotFileasset +type PDFDocuments implements DotContentlet & FileBaseType & DotFileasset ``` +The five flat properties are declared on `DotFileasset`, on **both** base-type interfaces, and on +every concrete asset type — synthesized where absent, using the very same fetchers the flat view +used. A property present on some surfaces and not others would make a client's query change shape +depending on which clause it narrows through. + **The possible-type set is derived, never listed.** It is whatever content types exist when the -schema is built, and the schema is already rebuilt when a content type or field changes. A type a -customer creates after deployment is reachable with no administrative step (FR-001a, FR-005). +schema is built, and the schema is already rebuilt when a content type or field changes, so a type +a customer creates after deployment is reachable with no administrative step. + +**The old flat object type is gone from the schema**, not merely unreferenced: registering it would +leave an orphan visible in introspection and reachable by nobody. --- ## 3. Required client-visible behavior -### 3.1 Shared properties at both levels (FR-015) - ```graphql -image { - content { - identifier # directly on the interface - ... on Images { identifier # and again inside a clause - tags } +query MixedAssets { + AssetRefTestCollection(limit: 10) { + title + image { + fileName + ... on DotFileasset { fileName } + ... on DotAssetBaseType { asset { size mime } } + ... on FileBaseType { fileName fileAsset { size mime } } + ... on Images { tags } + ... on BannerImages { campaignName adSize } + } } } ``` -Both are valid. A client is never forced to choose one level. - -### 3.2 Narrowing (FR-016, FR-016a, FR-017, FR-018) +`... on DotFileasset` stays valid and always fires: a fragment on the position's own interface +always matches. That is why the interface kept the name — a new one would have invalidated every +such clause a customer has written. -| Client writes | Result | Status | -|---|---|---| -| a clause on a type the asset **is** | its properties are returned | 200, data | -| a clause on a possible type the asset **is not** | contributes nothing, rest of the response delivered, **warning** names the clause | 200, data + `extensions` | -| a clause on a type that **does not exist** | request fails | validation error, no data | -| clauses on the base kind **and** the concrete type | properties **merge** into one object | 200, data | - -Across a result set of mixed types, every matching asset is populated, every non-matching asset is -still returned, and one non-match never suppresses the matches (FR-016a). - -There is no cast to fail here: a clause is a condition, not a coercion. - -### 3.3 Warning shape - -```json -{ - "data": { "...": "delivered normally" }, - "extensions": { - "warnings": [ - { "path": "BannerCollection.image.content", - "typeCondition": "PDFDocuments", - "message": "No asset at this path was of type PDFDocuments." } - ] - } -} -``` +| Client writes | Result | +|---|---| +| a clause on a type the asset **is** | its properties are returned | +| a clause on a possible type the asset **is not** | contributes nothing, rest of the response delivered | +| a clause on a type that **does not exist** | request fails (validation error) | +| clauses on several applicable types | properties **merge** into one object | +| `baseType` | `DOTASSET` or `FILEASSET`, without any clause | -Warnings name only the type the client itself wrote and the path it chose — **never asset content** -(Constitution III). A client that ignores `extensions` is unaffected. +An asset field aimed at content that is not an asset resolves to `null`. Handing such a contentlet +on would raise `UnresolvedTypeException`, which fails the **whole request** rather than that field. --- -## 4. Worked example +## 4. Before and after -**Before** — works today, and must keep working unchanged: +**Before** — still works, except `description`: ```graphql -{ BannerCollection { title image { fileName description fileAsset { versionPath size mime } } } } +{ BannerCollection { title image { fileName fileAsset { versionPath size mime } } } } ``` -**After** — the same query still valid, plus what was unreachable: +**After** — the same, plus what was unreachable: ```graphql { BannerCollection { title image { - content { - identifier - title - __typename - ... on DotAssetBaseType { asset { versionPath size mime } } - ... on Images { tags description } - ... on BannerImages { campaignName adSize } - } + fileName + identifier + __typename + ... on Images { tags description } + ... on BannerImages { campaignName adSize } } } } ``` `... on Images { description }` returns the asset's **stored** description — a different value from -the top-level `image { description }`, which keeps returning the title. Two names, two meanings, -neither surprising the other. That separation is the point of FR-009a. +what `image { description }` used to return, which was the title. Two meanings, now two places, and +the old one fails rather than lying. --- @@ -179,8 +152,17 @@ neither surprising the other. That separation is the point of FR-009a. | Guarantee | Requirement | |---|---| -| Every selection valid today is still valid | FR-012, SC-008 | -| Every such selection returns the same value | FR-012, SC-008 | -| Superseded properties are marked in the schema itself, with replacements named | FR-012a, SC-009 | -| Removal happens in no release introduced by this feature | FR-012b | -| Rolling back removes only the added field; a client on the old selection set is unaffected | Legacy Impact | +| Five of the six flat properties stay selectable and return the same values | FR-012, SC-008 | +| Every removed selection fails **visibly**; none keeps working while returning different data | FR-009a, SC-009 | +| The break ships with announcement and migration guidance | FR-012c | + +**What breaks** + +| Selection | Why it could not be carried over | +|---|---| +| `image { description }` | The flat view answered with the contentlet *title*; the content answering it stores something else. Measured on a real instance: populated for 57 of 57 images before, 2 of 57 after. Carrying the name over would have returned different data without failing. Reachable through a clause on the concrete type, where it returns the stored value. | +| an asset field aimed at non-asset content | A contentlet outside the interface cannot be handed on; doing so fails the **whole request**. | + +**Not a rollback-safe change.** Unlike an additive field, this replaces a type and removes a +selection, so a client that has adopted the new shape breaks on rollback and one that has not +breaks on deploy. diff --git a/specs/34540-graphql-asset-subtype-fields/data-model.md b/specs/34540-graphql-asset-subtype-fields/data-model.md index 827135a987f2..1523aa845950 100644 --- a/specs/34540-graphql-asset-subtype-fields/data-model.md +++ b/specs/34540-graphql-asset-subtype-fields/data-model.md @@ -4,119 +4,81 @@ No persistent data model changes: no database table, no column, no index mapping, no serialized state. The entities below are **GraphQL schema elements** — the shape the delivery API presents. -Working names; the tasks phase settles the final ones. +Reflects what was implemented. --- ## Entities -### `DotFileasset` (existing — extended, never altered) +### `DotFileasset` — object type **replaced by an interface of the same name** -The type every `ImageField` and `FileField` resolves to today. Built in the static block of -`CustomFieldType`. +The name is kept deliberately: 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. -| Property | Type | Change | Notes | +| Property | Type | Fate | Notes | |---|---|---|---| -| `fileName` | String | **deprecated**, behavior frozen | Synthesized: falls back to `contentlet.getName()` when the base type is not FILEASSET. Must keep doing so (R4). | -| `description` | String | **deprecated**, behavior frozen | Synthesized: falls back to `contentlet.getTitle()` when the base type is DOTASSET. Returns the file name for image-style content. Must keep doing so (R4). | -| `fileAsset` | `DotBinary` | **deprecated**, behavior frozen | | -| `metaData` | `[DotKeyValue]` | **deprecated**, behavior frozen | | -| `showOnMenu` | `[String]` | **deprecated**, behavior frozen | | -| `sortOrder` | Int | **deprecated**, behavior frozen | | -| **`content`** | **`DotAssetContent`** | **NEW** | The referenced asset, described by its real content type. | +| `fileName` | String | **carried over** | Synthesized: falls back to the contentlet name when the base type is not FILEASSET. Kept behaving exactly so. | +| `fileAsset` | `DotBinary` | **carried over** | Resolved for both base types; the binary is named `asset` on DOTASSET content and the fetcher already maps it. | +| `metaData` | `[DotKeyValue]` | **carried over** | | +| `showOnMenu` | `[String]` | **carried over** | | +| `sortOrder` | Int | **carried over** | | +| `description` | String | **removed** | The one property whose meaning differs: the flat view answered with the contentlet *title*, while the content answering it stores something else. Carrying the name over would have returned different data without failing. | -**Invariants** - -- Every existing property keeps its name, its type, and the exact value it returns today - (FR-012, SC-008). "Frozen" includes the two synthesized values — correcting them is forbidden. -- Each deprecated property carries a reason naming its replacement path (FR-012a, SC-009). -- `@deprecated` marks **fields**; GraphQL cannot deprecate an object type, so the type itself - carries no marking. -- The customer's own `image` / `file1` field is **not** deprecated — it stays the way in. - ---- - -### `DotAssetContent` (new — interface) - -The referenced asset, described by what it actually is. +Plus every common content field — `identifier`, `inode`, `title`, `host`, `live`, `urlMap`, +`baseType`, `folder`, `modDate`, `_map` and the rest — none of which was reachable before. **Possible types**: every content type whose base type is DOTASSET or FILEASSET — the system ones and every customer-defined one, including types created after the schema was last built. Derived -from the content types present at schema-build time; **never enumerated** (FR-001a). Schema rebuild -is already triggered by `ContentTypeAndFieldsModsListeners` on content type and field changes -(FR-005). - -**Fields**: the common content fields — `identifier`, `inode`, `title`, `host`, `folder`, `live`, -`working`, `archived`, `locked`, `urlMap`, `modDate`, `modUser`, `owner`, `publishDate`, -`publishUser`, `creationDate`, `conLanguage`, `contentType`, `baseType`, `titleImage`, -`dotStyleProperties`, `_map`. - -Nothing base-type-specific: the DOTASSET binary is `asset` and the FILEASSET binary is `fileAsset`, -different names, so the binary is reached through a narrowing clause — either on the concrete type -or on the existing base-kind interface. +from the content types present at schema-build time; **never enumerated**. Schema rebuild is already +triggered by `ContentTypeAndFieldsModsListeners`. **Invariants** -- Spans **both** asset base types, so an Image field that points at file-style content, and a File - field that points at image-style content, both narrow correctly (FR-001b — verified: an Image - field resolved a `.vtl` FileAsset). -- Its fields are selectable directly **and** inside a narrowing clause (FR-015). -- Resolves to the concrete content type, so `__typename` distinguishes two asset types (FR-003). -- Interface, not union — a union has no fields of its own and would fail FR-015. +- Spans **both** asset base types, so an Image field pointing at file-style content, and a File + field pointing at image-style content, both narrow correctly. Verified: an Image field resolves a + `.vtl` FileAsset. +- Resolves to the concrete content type, so `__typename` distinguishes two asset types. +- Interface, not union — a union has no fields of its own, so the flat properties would be + unreachable without a clause. --- -### `DotAssetBaseType`, `FileBaseType` (existing — unchanged) +### `DotAssetBaseType`, `FileBaseType` (existing — extended) -The two base-kind interfaces. Already in the schema, already attached by -`ContentAPIGraphQLTypesProvider.createType()`. Untouched by this feature; they remain available as -narrowing targets inside `DotAssetContent` and their clauses merge with concrete-type clauses -(FR-018 — verified live). +The two base-kind interfaces, unchanged in purpose. Each now **also declares the five flat +properties**, because every one of their possible types carries them. Without that, `fileName` is +selectable on the asset interface and on the concrete types but not through these clauses, and a +client's query changes shape depending on how it narrows. ---- - -### Per-content-type object types (existing — one interface added) - -Every generated content type object already declares `DotContentlet` plus its base-kind interface. -Those whose base type is DOTASSET or FILEASSET additionally declare `DotAssetContent`. No field -changes; declaring the interface is what places them in its possible-type set. +They remain **independent** interfaces: none implements another, and none implements +`DotFileasset`. They share fields because the same fields are declared on each. --- -### Response warning (new — not a schema type) +### Per-content-type object types (existing — one interface added, five properties synthesized) -Non-fatal information about narrowing clauses that matched nothing, carried in the response's -`extensions` object. Not part of the type system, so it changes no selection set and cannot break a -client that ignores it. +Every asset content type additionally declares `DotFileasset`, and carries any of the five flat +properties it does not already define — synthesized with the very same fetchers the flat view used, +which is what makes them answer identically. FILEASSET-derived types usually define most already; +a customer-created one carries only the required fields, so `showOnMenu` and `sortOrder` can be +absent and are filled in. -| Attribute | Content | -|---|---| -| Which clause | The type name the client wrote, and the field path it appeared under | -| Why | It matched none of the assets returned at that path | +A property the customer already defined always wins. A duplicate definition fails the **whole** +schema build, taking every other content type down with it. -**Invariants** +--- + +### The flat object type (removed) -- Carries only type names the client wrote in its own query and paths it chose. **Never asset - content** — a warning must not become a channel for a value the caller could not otherwise read - (Constitution III). -- Emitted per unmatched clause, not as one opaque flag (FR-016a). -- Absent when every clause matched, and costs nothing when a query has no clauses. -- A warning never changes the status of the request: the data is still delivered (FR-016). +No longer registered as a schema type. Nothing references it once asset fields are interface-typed, +so registering it would leave an orphan visible in introspection and reachable by nobody. --- ## State transitions -Only one, and it spans releases rather than runtime — the ADR-0022 lifecycle of the superseded -surface: - -``` -present (today) - → marked superseded in the schema, still fully functional ← this feature (FR-012a) - → clients adopt `content`, old surface still functional ← after this feature - → bake: supported-version floor passed AND zero use observed ← gated, not scheduled here - → removed ← separate work (FR-012b) -``` - -This feature delivers the first arrow only. The retirement tracking item is opened when it ships, -naming the surface to be retired — not deferred to a later cleanup pass. +None. The flat view is replaced in one step rather than deprecated and retired over releases — a +product decision, recorded in the spec's "Decision: the flat view is replaced" section, and the +reason an exception to ADR-0022 is requested there. From 38606db6f2c1bfbf98039acab6fac896719e6309 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 18 Sep 2026 13:35:44 -0600 Subject: [PATCH 07/10] feat(graphql): report narrowing clauses that matched nothing (#34540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User Story 4, the last unbuilt part and the only genuinely new mechanism in the feature. Without it, `... on BannerImages { campaignName }` over content that happened to be another type is indistinguishable from `... on BannreImages { … }` — a typo. Both are legal, both quietly contribute nothing, and the response is identical. Only the client can tell them apart, and only if told which clause never applied. Delivered through the response's `extensions`, the standard place in the GraphQL response specification for non-fatal, out-of-band information. A client that ignores extensions is entirely unaffected; data and errors are untouched. Deliberately not a server log: the person who can fix a mistyped clause is the client developer, who never reads the server's log, and one entry per request on the delivery path would flood a shared instance with noise nobody can act on. Registered on the HTTP path through GraphQLQueryInvoker.withInstrumentation, which GraphQLConfiguration accepts. No dependency change — this was the last unverified item from the library investigation. Four things the implementation gets right only because a test insisted: - The value arrives wrapped in graphql.execution.FetchedValue, so the unwrap is not optional: without it nothing is ever recognised as content and EVERY clause is reported as unmatched. Three earlier attempts read the resolved type from the schema instead — the field's type gives `String` for `campaignName`, and the step's object type gives the declared parent, which for an interface-typed position is the interface. "Did this clause match" is a question about the data, not the schema. - Per-request state lives in createState(), never in a field of the instrumentation, which is shared across concurrent requests. The resolved-type set is concurrent because field resolution may run on several threads. - The chosen operation is walked, not the whole document, so a warning is never attributed to a query that did not run. - Named fragment definitions are not followed: a spread carries no type condition of its own, so following one would attribute a warning to a path the client never wrote. Tests: a clause that matched nothing is reported with its type and path; a clause that DID match produces no warning (a warning that always fires trains clients to ignore it); a query with no clauses produces none at all; and a warning carries neither property values nor identifiers, so it cannot become a channel for content the caller could not otherwise read. 74/74 (AssetSubtypeAccessTest 15, AssetTypeHierarchyTest 5, AssetFieldValueContractTest 3, GraphqlAPITest 51). Co-Authored-By: Claude Opus 5 (1M context) --- .../dotcms/graphql/DotGraphQLHttpServlet.java | 6 + ...UnmatchedTypeConditionInstrumentation.java | 221 ++++++++++++++++++ .../business/AssetSubtypeAccessTest.java | 120 ++++++++++ .../graphql/business/GraphqlQueryRunner.java | 29 +++ 4 files changed, 376 insertions(+) create mode 100644 dotCMS/src/main/java/com/dotcms/graphql/UnmatchedTypeConditionInstrumentation.java diff --git a/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java b/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java index baae5c772ed7..33634cb5a8f4 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java @@ -7,6 +7,7 @@ import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; import graphql.kickstart.servlet.AbstractGraphQLHttpServlet; +import graphql.kickstart.execution.GraphQLQueryInvoker; import graphql.kickstart.servlet.GraphQLConfiguration; import io.vavr.Lazy; import io.vavr.control.Try; @@ -43,6 +44,11 @@ protected GraphQLConfiguration getConfiguration() { .with(new DotGraphQLSchemaProvider()) .with(List.of(new DotGraphQLServletListener())) .with(new DotGraphQLContextBuilder()) + // Reports narrowing clauses that matched nothing, through the response's + // `extensions`. A client that ignores extensions is unaffected. See #34540. + .with(GraphQLQueryInvoker.newBuilder() + .withInstrumentation(new UnmatchedTypeConditionInstrumentation()) + .build()) .build(); } diff --git a/dotCMS/src/main/java/com/dotcms/graphql/UnmatchedTypeConditionInstrumentation.java b/dotCMS/src/main/java/com/dotcms/graphql/UnmatchedTypeConditionInstrumentation.java new file mode 100644 index 000000000000..3f5188988c95 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/graphql/UnmatchedTypeConditionInstrumentation.java @@ -0,0 +1,221 @@ +package com.dotcms.graphql; + +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import graphql.execution.FetchedValue; +import com.dotmarketing.util.Logger; +import graphql.ExecutionResult; +import graphql.ExecutionResultImpl; +import graphql.execution.instrumentation.InstrumentationContext; +import graphql.execution.instrumentation.InstrumentationState; +import graphql.execution.instrumentation.SimpleInstrumentation; +import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters; +import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters; +import graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters; +import graphql.language.Field; +import graphql.language.InlineFragment; +import graphql.language.Node; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Tells a client that a narrowing clause it wrote matched nothing. + * + *

Without this, {@code ... on BannerImages { campaignName }} is indistinguishable from + * {@code ... on BannreImages { campaignName }}: both are legal, both quietly contribute nothing, + * and the response looks the same. One is a correct query over content that happened to be another + * type; the other is a typo. Only the client can tell them apart, and only if told which clause + * never applied. See issue #34540. + * + *

Delivered through the response's {@code extensions} — the standard place in the GraphQL + * response specification for non-fatal, out-of-band information. A client that ignores + * {@code extensions} is entirely unaffected, and nothing about the data or the errors changes. + * Deliberately not a server log: the person who can fix a mistyped clause is the client developer, + * who never reads the server's log, and one entry per request on the delivery path would flood a + * shared instance's log with noise nobody can act on. + * + *

Costs nothing when a query contains no narrowing clauses: the document is walked once at the + * start of execution, and if it declares no type conditions the instrumentation does no further + * work. + * + *

Per-request state lives in {@link State}, obtained through {@link #createState()} — never in a + * field of this class, which is shared across concurrent requests. + */ +public class UnmatchedTypeConditionInstrumentation extends SimpleInstrumentation { + + static final String EXTENSIONS_KEY = "warnings"; + + /** + * Type conditions the query declared, and the concrete types actually resolved, for one + * request. A {@link ConcurrentHashMap}-backed set because field resolution may run on several + * threads within a single execution. + */ + static class State implements InstrumentationState { + + /** Type name -> the field paths it was written under. */ + private final Map> declared = new LinkedHashMap<>(); + private final Set resolved = Collections.newSetFromMap(new ConcurrentHashMap<>()); + + void declare(final String typeName, final String path) { + declared.computeIfAbsent(typeName, key -> new LinkedHashSet<>()).add(path); + } + + void resolvedAs(final String typeName) { + resolved.add(typeName); + } + + boolean isEmpty() { + return declared.isEmpty(); + } + + /** @return one warning per declared condition that no resolved type ever satisfied. */ + List> unmatched() { + final List> warnings = new ArrayList<>(); + declared.forEach((typeName, paths) -> { + if (resolved.contains(typeName)) { + return; + } + paths.forEach(path -> warnings.add(Map.of( + "path", path, + "typeCondition", typeName, + "message", "No content at this path was of type " + typeName + "."))); + }); + return warnings; + } + } + + @Override + public State createState() { + return new State(); + } + + @Override + public InstrumentationContext beginExecuteOperation( + final InstrumentationExecuteOperationParameters parameters) { + + // Walked here rather than at beginExecution: the document only becomes reachable once the + // operation to execute has been chosen, and walking the chosen operation — rather than the + // whole document — means a warning is never attributed to a query that did not run. + final State state = parameters.getInstrumentationState(); + if (null != state && null != parameters.getExecutionContext()) { + collectTypeConditions(parameters.getExecutionContext().getOperationDefinition(), + "", state); + } + return super.beginExecuteOperation(parameters); + } + + @Override + public InstrumentationContext beginFieldComplete( + final InstrumentationFieldCompleteParameters parameters) { + + final State state = parameters.getInstrumentationState(); + if (null != state && !state.isEmpty()) { + // Read from the fetched VALUE rather than from the schema's type bookkeeping. Asking + // graphql-java for the step's type gives the type OF the field (`String` for + // `campaignName`) and asking for its object type gives the declared parent, which for + // an interface-typed position is the interface -- neither is the concrete type the + // clause was testing for. The contentlet knows what it actually is, and "did this + // clause match" is a question about the data, not about the schema. + recordResolvedType(parameters.getFetchedValue(), state); + } + return super.beginFieldComplete(parameters); + } + + private void recordResolvedType(final Object value, final State state) { + // graphql-java hands the value wrapped, so the unwrap is not optional: without it nothing + // is ever recognised as content and every clause is reported as unmatched. + if (value instanceof FetchedValue) { + recordResolvedType(((FetchedValue) value).getFetchedValue(), state); + return; + } + if (value instanceof Contentlet) { + state.resolvedAs(((Contentlet) value).getContentType().variable()); + return; + } + if (value instanceof Iterable) { + ((Iterable) value).forEach(element -> recordResolvedType(element, state)); + } + } + + @Override + public CompletableFuture instrumentExecutionResult( + final ExecutionResult executionResult, + final InstrumentationExecutionParameters parameters) { + + final State state = parameters.getInstrumentationState(); + if (null == state || state.isEmpty()) { + return super.instrumentExecutionResult(executionResult, parameters); + } + + final List> warnings = state.unmatched(); + if (warnings.isEmpty()) { + return super.instrumentExecutionResult(executionResult, parameters); + } + + Logger.debug(this, () -> "Narrowing clauses that matched nothing: " + warnings); + + final Map extensions = new LinkedHashMap<>(); + if (null != executionResult.getExtensions()) { + extensions.putAll(executionResult.getExtensions()); + } + extensions.put(EXTENSIONS_KEY, warnings); + + return CompletableFuture.completedFuture( + ExecutionResultImpl.newExecutionResult().from(executionResult) + .extensions(extensions).build()); + } + + /** + * Walks the query for inline-fragment type conditions, recording the field path each was + * written under. + * + *

Named fragment definitions are not followed: a spread reaches them through + * {@code FragmentSpread}, which carries no type condition of its own, so following them would + * attribute a warning to a path the client did not write. The inline form is what the feature's + * documented usage recommends. + */ + private void collectTypeConditions(final Node node, final String path, final State state) { + if (null == node) { + return; + } + + for (final Node child : node.getChildren()) { + if (child instanceof InlineFragment) { + final InlineFragment fragment = (InlineFragment) child; + if (null != fragment.getTypeCondition()) { + state.declare(fragment.getTypeCondition().getName(), + path.isEmpty() ? "(root)" : path); + } + collectTypeConditions(child, path, state); + continue; + } + + if (child instanceof Field) { + final Field field = (Field) child; + final String childPath = + path.isEmpty() ? field.getName() : path + "." + field.getName(); + collectTypeConditions(child, childPath, state); + continue; + } + + collectTypeConditions(child, path, state); + } + } + + @Override + public boolean equals(final Object other) { + return other instanceof UnmatchedTypeConditionInstrumentation; + } + + @Override + public int hashCode() { + return Objects.hash(UnmatchedTypeConditionInstrumentation.class); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java index 8650ed2423eb..bbd074d488cd 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -2,6 +2,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -33,6 +34,7 @@ import com.dotmarketing.portlets.folders.business.FolderAPI; import com.dotmarketing.util.FileUtil; import com.liferay.portal.model.User; +import graphql.ExecutionResult; import java.io.File; import java.util.List; import java.util.Map; @@ -467,6 +469,124 @@ public void test_assetResolvesAsTheCallingUser() throws Exception { } } + /** + * Given: a query narrowing to a type that no returned asset is. + * When: it runs. + * Then: the data is delivered, and the response names the clause that matched nothing. + * + *

US4. Without this a client cannot tell {@code ... on BannerImages} over content that + * happened to be another type from {@code ... on BannreImages} — a typo. Both are legal, both + * contribute nothing, and the response looks identical. + */ + @Test + public void test_clauseThatMatchedNothing_isReportedInExtensions() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final ContentType unrelatedType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Warned"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { fileName " + + "... on %s { %s } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + unrelatedType.variable(), CUSTOM_PROPERTY_VAR); + + final ExecutionResult result = GraphqlQueryRunner.executeWithWarnings(query, systemUser); + + assertTrue("the request must still succeed", result.getErrors().isEmpty()); + assertNotNull("the data must still be delivered", result.getData()); + + final List> warnings = GraphqlQueryRunner.warningsOf(result); + assertEquals("exactly one clause matched nothing", 1, warnings.size()); + assertEquals("the warning must name the clause the client wrote", + unrelatedType.variable(), warnings.get(0).get("typeCondition")); + assertNotNull("and the path it was written under", warnings.get(0).get("path")); + } + + /** + * Given: a query whose every narrowing clause matches. + * When: it runs. + * Then: no warning is produced. + */ + @Test + public void test_clauseThatMatched_producesNoWarning() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Matched"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { " + + "... on %s { %s } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + assetType.variable(), CUSTOM_PROPERTY_VAR); + + final ExecutionResult result = GraphqlQueryRunner.executeWithWarnings(query, systemUser); + + assertTrue("a matching clause must not be reported — a warning that fires even when the " + + "clause matched is worse than none, since it trains clients to ignore it. " + + "Warnings were: " + GraphqlQueryRunner.warningsOf(result), + GraphqlQueryRunner.warningsOf(result).isEmpty()); + } + + /** + * Given: a query with no narrowing clauses at all. + * When: it runs. + * Then: the response carries no warnings — the mechanism costs nothing. + */ + @Test + public void test_queryWithoutClauses_carriesNoWarnings() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Plain"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { fileName } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + + final ExecutionResult result = GraphqlQueryRunner.executeWithWarnings(query, systemUser); + + assertTrue("a query with no clauses must produce no warnings", + GraphqlQueryRunner.warningsOf(result).isEmpty()); + } + + /** + * Given: a warning. + * When: its text is read. + * Then: it contains only the type name the client wrote and the path it chose. + * + *

A warning must never become a channel for content the caller could not otherwise read. + */ + @Test + public void test_warningCarriesNoAssetContent() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final ContentType unrelatedType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "SecretValue"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { " + + "... on %s { %s } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + unrelatedType.variable(), CUSTOM_PROPERTY_VAR); + + final String rendered = + GraphqlQueryRunner.warningsOf( + GraphqlQueryRunner.executeWithWarnings(query, systemUser)).toString(); + + assertFalse("a warning must not leak a property value", + rendered.contains("SecretValue")); + assertFalse("nor the asset's identifier", + rendered.contains(asset.getIdentifier())); + } + // ---------------------------------------------------------------- helpers /** Runs {@code query} as {@code user} and returns the asset field of the single row, if any. */ diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java index 5abf946b999a..a01f9d671d9b 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java @@ -1,6 +1,7 @@ package com.dotcms.graphql.business; import com.dotcms.graphql.DotGraphQLContext; +import com.dotcms.graphql.UnmatchedTypeConditionInstrumentation; import com.dotmarketing.business.APILocator; import com.liferay.portal.model.User; import graphql.ExecutionInput; @@ -71,6 +72,34 @@ public static Map executeAndExpectSuccess(final String query, fi return result.getData(); } + /** + * Executes {@code query} with the warning instrumentation attached, exactly as the HTTP path + * does, and returns the whole result so a caller can read {@code extensions}. + */ + public static ExecutionResult executeWithWarnings(final String query, final User user) + throws Exception { + final GraphQLSchema schema = APILocator.getGraphqlAPI().getSchema(user); + final DotGraphQLContext context = DotGraphQLContext.createServletContext() + .with(user).build(); + + return GraphQL.newGraphQL(schema) + .instrumentation(new UnmatchedTypeConditionInstrumentation()) + .build() + .execute(ExecutionInput.newExecutionInput().query(query).context(context).build()); + } + + /** + * @return the warnings a query produced, empty when it produced none + */ + @SuppressWarnings("unchecked") + public static List> warningsOf(final ExecutionResult result) { + if (null == result.getExtensions()) { + return List.of(); + } + final Object warnings = result.getExtensions().get("warnings"); + return null == warnings ? List.of() : (List>) warnings; + } + /** * Executes {@code query} and counts how many times {@code fieldName} was fetched. * From 7234d831cb517f6bd5288ef3bfffe8538a276c48 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 18 Sep 2026 17:31:33 -0600 Subject: [PATCH 08/10] feat(graphql): read an asset's binary properties directly on the asset (#34540) Absorbs the work from PR #35363, now closed as superseded, so all of #34540 lands in one place. That PR identified a real ergonomics gap: reaching an asset's size or mime type meant descending into the binary field. Ten of its twelve properties are now readable on the asset itself: image { name size mime versionPath idPath path sha256 isImage width height } Three differences from how that PR did it: - `title` and `modDate` are NOT flattened. On a contentlet those names already mean the contentlet's own title and modification date, and they are on the asset interface. Giving them the FILE's meaning here would make the same name answer differently depending on where it is read -- the silent divergence this whole feature exists to avoid. Both stay reachable through the binary field. A test asserts `title` still reports the contentlet's, so a later change that flattens it fails loudly. - The derivation is cached per contentlet, per request, on the DotGraphQLContext. BinaryToMapTransformer's constructor runs the full transformer pipeline, so deriving per property -- as #35363 did -- makes reading an asset cost ten full derivations for ten properties, per asset, per row of a result set. That was the main objection raised against that PR; repeating it here would have been incoherent. - computeIfAbsent over a concurrent map, since field resolution may run on several threads within one execution and deriving the same binary twice is precisely what the cache prevents. The flattened properties are asserted to MATCH the same values read the long way through the binary field: a shortcut to one source of truth, not a second one. Tests: 76/76 (AssetSubtypeAccessTest 17, AssetTypeHierarchyTest 5, AssetFieldValueContractTest 3, GraphqlAPITest 51). Co-Authored-By: Claude Opus 5 (1M context) --- .../com/dotcms/graphql/CustomFieldType.java | 20 ++++ .../AssetBinaryPropertyDataFetcher.java | 104 ++++++++++++++++++ .../business/AssetSubtypeAccessTest.java | 70 ++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetBinaryPropertyDataFetcher.java diff --git a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java index 909548d41960..8674d5628cd0 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java @@ -1,6 +1,7 @@ package com.dotcms.graphql; import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotcms.graphql.datafetcher.AssetBinaryPropertyDataFetcher; import com.dotcms.graphql.datafetcher.BinaryFieldDataFetcher; import com.dotcms.graphql.datafetcher.FieldDataFetcher; import com.dotcms.graphql.datafetcher.KeyValueFieldDataFetcher; @@ -196,6 +197,25 @@ public static Map getAssetFlatFields() { assetFlatFields = new HashMap<>(fileAssetTypeFields); assetFlatFields.remove(FILEASSET_DESCRIPTION_FIELD_VAR); + // The binary's own properties, flattened onto the asset so a client need not descend into + // the binary field to reach them. Ten of the twelve DotBinary carries: `title` and + // `modDate` are deliberately absent, because on a contentlet those names already mean the + // contentlet's title and modification date. Declaring them here would make the same name + // answer with the FILE's title on an asset and the CONTENT's title everywhere else — the + // class of silent divergence this work exists to avoid. Both remain reachable through the + // binary field itself. See issue #34540. + final AssetBinaryPropertyDataFetcher binaryProperty = new AssetBinaryPropertyDataFetcher(); + assetFlatFields.put("name", new TypeFetcher(GraphQLString, binaryProperty)); + assetFlatFields.put("size", new TypeFetcher(GraphQLLong, binaryProperty)); + assetFlatFields.put("mime", new TypeFetcher(GraphQLString, binaryProperty)); + assetFlatFields.put("versionPath", new TypeFetcher(GraphQLString, binaryProperty)); + assetFlatFields.put("idPath", new TypeFetcher(GraphQLString, binaryProperty)); + assetFlatFields.put("path", new TypeFetcher(GraphQLString, binaryProperty)); + assetFlatFields.put("sha256", new TypeFetcher(GraphQLString, binaryProperty)); + assetFlatFields.put("isImage", new TypeFetcher(GraphQLBoolean, binaryProperty)); + assetFlatFields.put("width", new TypeFetcher(GraphQLLong, binaryProperty)); + assetFlatFields.put("height", new TypeFetcher(GraphQLLong, binaryProperty)); + final Map siteTypeFields = new HashMap<>(ContentFields.getContentFields()); siteTypeFields.remove(HOST_KEY); // remove myself siteTypeFields.put("hostId", new TypeFetcher(GraphQLString)); diff --git a/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetBinaryPropertyDataFetcher.java b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetBinaryPropertyDataFetcher.java new file mode 100644 index 000000000000..6ab9e8abdb2f --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/graphql/datafetcher/AssetBinaryPropertyDataFetcher.java @@ -0,0 +1,104 @@ +package com.dotcms.graphql.datafetcher; + +import static com.dotcms.contenttype.model.type.BaseContentType.DOTASSET; + +import com.dotcms.contenttype.model.type.FileAssetContentType; +import com.dotcms.graphql.DotGraphQLContext; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.contentlet.transform.BinaryToMapTransformer; +import com.dotmarketing.util.Logger; +import graphql.schema.DataFetcher; +import graphql.schema.DataFetchingEnvironment; +import java.util.Collections; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Resolves one property of the binary an asset carries — {@code size}, {@code mime}, + * {@code versionPath} and the rest — directly on the asset, so a client need not descend into the + * binary field to reach them. + * + *

The binary is named differently by base type ({@code asset} for DOTASSET content, + * {@code fileAsset} for FILEASSET), which is the reason these properties could not simply be + * declared once and read by name. + * + *

The derivation is cached per contentlet, per request. {@link BinaryToMapTransformer}'s + * constructor runs the full transformer pipeline, so deriving it once per property selected would + * make the cost of reading an asset grow with the size of the selection — ten properties meaning + * ten full derivations, per asset, per row of a result set. That is the cost issue #34540 rules + * out, and the reason the earlier attempt at this (PR #35363) was not carried over as written. + * + *

The cache lives on the request's {@link DotGraphQLContext}, so it cannot leak between + * requests or between users. + */ +public class AssetBinaryPropertyDataFetcher implements DataFetcher { + + private static final String CACHE_PARAM = "assetBinaryMaps"; + + @Override + public Object get(final DataFetchingEnvironment environment) { + final Contentlet contentlet = environment.getSource(); + if (null == contentlet) { + return null; + } + + try { + final Map binaryMap = binaryMapOf(contentlet, environment); + return null == binaryMap ? null : binaryMap.get(environment.getField().getName()); + } catch (final IllegalArgumentException e) { + Logger.warn(this, "No binary on contentlet " + contentlet.getIdentifier() + + " for field: " + environment.getField().getName()); + return null; + } catch (final Exception e) { + Logger.error(this, e.getMessage(), e); + return null; + } + } + + @SuppressWarnings("unchecked") + private Map binaryMapOf(final Contentlet contentlet, + final DataFetchingEnvironment environment) { + + final Map> cache = cacheFor(environment); + if (null == cache) { + return derive(contentlet); + } + + // computeIfAbsent rather than get/put: field resolution may run on several threads within + // one execution, and deriving the same binary twice is exactly what this cache exists to + // prevent. + return cache.computeIfAbsent(contentlet.getInode(), inode -> { + final Map derived = derive(contentlet); + return null == derived ? Collections.emptyMap() : derived; + }); + } + + @SuppressWarnings("unchecked") + private Map derive(final Contentlet contentlet) { + final String binaryVar = DOTASSET == contentlet.getContentType().baseType() + ? "asset" + : FileAssetContentType.FILEASSET_FILEASSET_FIELD_VAR; + + Logger.debug(this, () -> "Deriving binary properties for contentlet: " + + contentlet.getIdentifier()); + + return (Map) new BinaryToMapTransformer(contentlet).asMap() + .get(binaryVar + "Map"); + } + + @SuppressWarnings("unchecked") + private Map> cacheFor(final DataFetchingEnvironment environment) { + if (!(environment.getContext() instanceof DotGraphQLContext)) { + return null; + } + final DotGraphQLContext context = environment.getContext(); + synchronized (context) { + Object cache = context.getParam(CACHE_PARAM); + if (null == cache) { + cache = new ConcurrentHashMap>(); + context.addParam(CACHE_PARAM, cache); + } + return (Map>) cache; + } + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java index bbd074d488cd..5ede6d9f7efe 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -587,6 +587,76 @@ public void test_warningCarriesNoAssetContent() throws Exception { rendered.contains(asset.getIdentifier())); } + /** + * Given: an asset behind an Image field. + * When: the binary's own properties are selected directly on the asset. + * Then: they come back, without descending into the binary field. + * + *

Carried over from PR #35363, which identified this ergonomics gap. `title` and `modDate` + * are deliberately NOT flattened: on a contentlet those names already mean the contentlet's + * own, and giving them the file's meaning here would make the same name answer differently + * depending on where it is read. + */ + @Test + public void test_binaryPropertiesAreReadableDirectlyOnTheAsset() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Flattened"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { " + + "name size mime versionPath idPath path sha256 isImage width height " + + "fileAsset { size mime } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + final Map field = (Map) row.get(IMAGE_FIELD_VAR); + + assertNotNull("the binary name must be readable on the asset", field.get("name")); + assertNotNull("as must its size", field.get("size")); + assertNotNull("and its mime type", field.get("mime")); + assertNotNull("and its version path", field.get("versionPath")); + + // The same values, reached the long way, must agree — the flattened properties are a + // shortcut to the same binary, not a second source of truth. + final Map binary = (Map) field.get("fileAsset"); + assertEquals("the flattened size must match the binary's own", + binary.get("size"), field.get("size")); + assertEquals("the flattened mime must match the binary's own", + binary.get("mime"), field.get("mime")); + } + + /** + * Given: the contentlet's own {@code title} and {@code modDate}. + * When: they are selected on an asset. + * Then: they report the CONTENTLET's values, not the file's. + * + *

The two properties the flattening above deliberately leaves out. If a later change + * flattens them too, this fails — which is the point. + */ + @Test + public void test_titleAndModDateStillMeanTheContentlet() throws Exception { + final ContentType assetType = newDotAssetSubtype(); + final Contentlet asset = newAssetOf(assetType, "Names"); + + final ContentType holder = newHolderType(); + final Contentlet content = newHolderContent(holder, IMAGE_FIELD_VAR, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { title } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + final Map field = (Map) row.get(IMAGE_FIELD_VAR); + + assertEquals("`title` on an asset must stay the contentlet's title, not the file's", + asset.getTitle(), field.get("title")); + } + // ---------------------------------------------------------------- helpers /** Runs {@code query} as {@code user} and returns the asset field of the single row, if any. */ From 80e01e84f4221a8287b59ce352ad81c6a513ced1 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 18 Sep 2026 20:41:00 -0600 Subject: [PATCH 09/10] fix(graphql): finish the ASSET_INTERFACE_NAME rename in GraphqlAPITest (#34540) The constant was renamed in InterfaceType and pushed, but this reference was left out of that commit, so the branch as published did not compile. Local runs passed only because the change sat uncommitted in the working tree. Co-Authored-By: Claude Opus 5 (1M context) --- .../test/java/com/dotcms/graphql/business/GraphqlAPITest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java index faae6b60c383..57e6710414c0 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlAPITest.java @@ -822,9 +822,9 @@ public void testAvailableGraphQLFieldsOnImageAndFileFields() // `... on DotFileasset` clauses, and still carries the long-standing flat // properties -- minus `description`, whose meaning differs between the flat view // (the contentlet title) and the content answering it. See #34540. - assertEquals(InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + assertEquals(InterfaceType.ASSET_INTERFACE_NAME, ((GraphQLNamedSchemaElement) fileFieldDefinition.getType()).getName()); - assertEquals(InterfaceType.ASSET_CONTENT_INTERFACE_NAME, + assertEquals(InterfaceType.ASSET_INTERFACE_NAME, ((GraphQLNamedSchemaElement) imageFieldDefinition.getType()).getName()); } finally { APILocator.getContentTypeAPI(APILocator.systemUser()).delete(contentType); From 37347ce20603526c626a7204a36bb0fb8531e540 Mon Sep 17 00:00:00 2001 From: fabrizzio-dotCMS Date: Fri, 18 Sep 2026 21:34:35 -0600 Subject: [PATCH 10/10] test(graphql): assert the binary properties are homogeneous across surfaces (#34540) The flattened binary properties were verified by eye, not by a test. Now AssetTypeHierarchyTest requires all ten -- name, size, mime, versionPath, idPath, path, sha256, isImage, width, height -- on the asset interface, on both base-type interfaces and on the concrete types, with the reason in the failure message: a client writing the same selection against a Binary field and against an asset field should not need two different queries. That homogeneity was the point of PR #35363's DotBinaryLike, and is the part of it worth keeping. It is not total, deliberately: `title` and `modDate` exist on DotBinary as the FILE's, and on an asset as the CONTENTLET's -- same name, different meaning -- and `focalPoint` has no meaning for a contentlet. Both exclusions are already covered by their own assertions. AssetTypeHierarchyTest 5/5. Co-Authored-By: Claude Opus 5 (1M context) --- .../business/AssetTypeHierarchyTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java index 5729c71d4278..3cba7cc607d3 100644 --- a/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java @@ -76,6 +76,16 @@ public class AssetTypeHierarchyTest extends IntegrationTestBase { private static final List FLAT_PROPERTIES = List.of("fileName", "fileAsset", "metaData", "showOnMenu", "sortOrder"); + /** + * The binary's own properties, flattened onto the asset so a client need not descend into the + * binary field. Ten of the thirteen {@code DotBinary} carries. {@code title} and {@code modDate} + * are excluded because on a contentlet those names already mean the contentlet's own, and + * {@code focalPoint} because it has no meaning for a contentlet. + */ + private static final List BINARY_PROPERTIES = + List.of("name", "size", "mime", "versionPath", "idPath", "path", "sha256", + "isImage", "width", "height"); + private static User systemUser; @BeforeClass @@ -160,6 +170,13 @@ public void test_flatProperties_reachableThroughEverySurface() throws Exception fileAssetType.variable())) { final Set fields = fieldNamesOf(schema, surface); + for (final String property : BINARY_PROPERTIES) { + assertTrue("binary property '" + property + "' must be reachable through '" + + surface + "' — a client writing the same selection against a " + + "Binary field and against an asset field should not need two " + + "different queries", + fields.contains(property)); + } for (final String property : FLAT_PROPERTIES) { assertTrue("'" + property + "' must be reachable through '" + surface + "' — otherwise the same query changes shape depending on which "