diff --git a/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java b/dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java index b3270d541190..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; @@ -19,6 +20,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; @@ -46,7 +48,7 @@ public enum CustomFieldType { KEY_VALUE("DotKeyValue"), LANGUAGE("DotLanguage"), USER("DotUser"), - FILEASSET("DotFileasset"), + FILEASSET("DotFileassetFlat"), STORY_BLOCK("DotStoryBlock"); CustomFieldType(String typeName) { @@ -61,6 +63,25 @@ public String getTypeName() { private static Map customFieldTypes = new HashMap<>(); + private static Map assetFlatFields; + + /** + * @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 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); + } + static { final Map binaryTypeFields = new HashMap<>(); binaryTypeFields.put("versionPath", GraphQLString); @@ -158,7 +179,42 @@ 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())); - 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 + // 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_INTERFACE_NAME. + 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 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/InterfaceType.java b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java index e17fa0a33d31..498c74e2fad0 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java @@ -69,6 +69,22 @@ 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. + * + *

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_INTERFACE_NAME = "DotFileasset"; + public static final String DOT_CONTENTLET = "DotContentlet"; static { @@ -87,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); @@ -124,7 +145,47 @@ 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 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_INTERFACE_NAME, + 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. + */ + 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/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/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java b/dotCMS/src/main/java/com/dotcms/graphql/business/ContentAPIGraphQLTypesProvider.java index c8d01befb8d6..06d7310d7675 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_INTERFACE_NAME)); + this.fieldClassGraphqlTypeMap.put(FileField.class, + new GraphQLTypeReference(InterfaceType.ASSET_INTERFACE_NAME)); this.fieldClassGraphqlTypeMap .put(KeyValueField.class, list(CustomFieldType.KEY_VALUE.getType())); this.fieldClassGraphqlTypeMap.put(CheckboxField.class, list(GraphQLString)); @@ -152,6 +171,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 +215,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()); @@ -235,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/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/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 f9332c46ce24..81ec633defdf 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite1b.java @@ -21,6 +21,9 @@ 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.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 new file mode 100644 index 000000000000..b91175b298bd --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetFieldValueContractTest.java @@ -0,0 +1,222 @@ +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")); + + 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", "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", "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 " + + "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..5ede6d9f7efe --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetSubtypeAccessTest.java @@ -0,0 +1,786 @@ +package com.dotcms.graphql.business; + +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; + +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.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; +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; +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. + * + *

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 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. + */ +@SuppressWarnings("unchecked") +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 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; + + @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 companion 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 Map assetContent = queryCompanion(holder, content, IMAGE_COMPANION, + String.format("... on %s { %s }", assetType.variable(), CUSTOM_PROPERTY_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 companion 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 Map assetContent = queryCompanion(holder, content, FILE_COMPANION, + String.format("... on %s { %s }", assetType.variable(), CUSTOM_PROPERTY_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 companion field. + * Then: they are returned. + * + *

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 + 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 Map assetContent = queryCompanion(holder, content, IMAGE_COMPANION, + String.format("... on %s { tags }", assetType.variable())); + + 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 view, 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 Map assetContent = + queryCompanion(holder, content, IMAGE_COMPANION, "identifier inode live title"); + + 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. + * 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 Map assetContent = queryCompanion(holder, content, IMAGE_COMPANION, + String.format("__typename ... on %s { %s }", + lateType.variable(), CUSTOM_PROPERTY_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")); + } + + /** + * 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. + * + *

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_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, asset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { " + + "fileName __typename ... on %s { %s } } } }", + holder.variable(), content.getIdentifier(), IMAGE_FIELD_VAR, + assetType.variable(), CUSTOM_PROPERTY_VAR); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + final Map field = (Map) row.get(IMAGE_FIELD_VAR); + + 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. + */ + @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 = newHolderContentWithBoth(holder, imageAsset, fileAsset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { %s { __typename } %s { __typename } } }", + holder.variable(), content.getIdentifier(), IMAGE_COMPANION, FILE_COMPANION); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + + 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); + 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 = newHolderContentWithBoth(holder, fileAsset, imageAsset); + + final String query = String.format( + "{ %sCollection(query: \"+identifier:%s\") { " + + "%s { __typename ... on %s { %s } } " + + "%s { __typename ... on %s { %s } } } }", + holder.variable(), content.getIdentifier(), + IMAGE_COMPANION, fileStyle.variable(), CUSTOM_PROPERTY_VAR, + FILE_COMPANION, imageStyle.variable(), CUSTOM_PROPERTY_VAR); + + final Map row = firstRow( + GraphqlQueryRunner.executeAndExpectSuccess(query, systemUser), holder); + + 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")); + 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)); + } + + /** + * 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)); + } + + /** + * 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); + } + } + + /** + * 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())); + } + + /** + * 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. */ + 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 { + 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()); + return rows.get(0); + } + + /** 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; + } + + 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..3cba7cc607d3 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/AssetTypeHierarchyTest.java @@ -0,0 +1,284 @@ +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. + * + *

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: + * + * 

+ *   Images    implements DotContentlet, DotAssetBaseType, DotFileasset
+ *   FileAsset implements DotContentlet, FileBaseType,     DotFileasset
+ *   Blog      implements DotContentlet, ContentBaseType
+ *
+ * + *

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 + * 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"); + + /** + * 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 + 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_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_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_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_INTERFACE_NAME, + InterfaceType.DOTASSET_INTERFACE_NAME, + InterfaceType.FILE_INTERFACE_NAME, + dotAssetType.variable(), + 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 " + + "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_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 5d8f5d8fbf1f..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 @@ -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_INTERFACE_NAME, + ((GraphQLNamedSchemaElement) fileFieldDefinition.getType()).getName()); + assertEquals(InterfaceType.ASSET_INTERFACE_NAME, + ((GraphQLNamedSchemaElement) imageFieldDefinition.getType()).getName()); } finally { APILocator.getContentTypeAPI(APILocator.systemUser()).delete(contentType); } @@ -961,13 +972,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..a01f9d671d9b --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/graphql/business/GraphqlQueryRunner.java @@ -0,0 +1,155 @@ +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; +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. + * + *

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} 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. + * + *

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. + * + * @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/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/contracts/graphql-schema.md b/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md new file mode 100644 index 000000000000..8f3eebc2738a --- /dev/null +++ b/specs/34540-graphql-asset-subtype-fields/contracts/graphql-schema.md @@ -0,0 +1,168 @@ +# 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. 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 existed before + +```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. + +**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 | +|---|---|---| +| `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. + +`fileName` is carried over unchanged, synthesis and all. `description` is not — see §5. + +--- + +## 2. What changes + +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: + +```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, … +} +``` + +`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 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, 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 + +```graphql +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 } + } + } +} +``` + +`... 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 | +|---|---| +| 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 | + +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. Before and after + +**Before** — still works, except `description`: + +```graphql +{ BannerCollection { title image { fileName fileAsset { versionPath size mime } } } } +``` + +**After** — the same, plus what was unreachable: + +```graphql +{ + BannerCollection { + title + image { + fileName + identifier + __typename + ... on Images { tags description } + ... on BannerImages { campaignName adSize } + } + } +} +``` + +`... on Images { description }` returns the asset's **stored** description — a different value from +what `image { description }` used to return, which was the title. Two meanings, now two places, and +the old one fails rather than lying. + +--- + +## 5. Compatibility guarantees + +| Guarantee | Requirement | +|---|---| +| 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 new file mode 100644 index 000000000000..1523aa845950 --- /dev/null +++ b/specs/34540-graphql-asset-subtype-fields/data-model.md @@ -0,0 +1,84 @@ +# 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. +Reflects what was implemented. + +--- + +## Entities + +### `DotFileasset` — object type **replaced by an interface of the same name** + +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 | Fate | Notes | +|---|---|---|---| +| `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. | + +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**. Schema rebuild is already +triggered by `ContentTypeAndFieldsModsListeners`. + +**Invariants** + +- 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 — extended) + +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. + +They remain **independent** interfaces: none implements another, and none implements +`DotFileasset`. They share fields because the same fields are declared on each. + +--- + +### Per-content-type object types (existing — one interface added, five properties synthesized) + +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. + +A property the customer already defined always wins. A duplicate definition fails the **whole** +schema build, taking every other content type down with it. + +--- + +### The flat object type (removed) + +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 + +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. 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.