Issue 17515 graphql reserved names for certain types (#17989)

* #17515 determine if field name and type will clash with graphql inherited fields

* #17515 better comment

* #17515 add integration tests

* #17515 more test cases

* #17515 codacy and code-review

dotCMS/src/integration-test/java/com/dotcms/graphql/business/GraphqlAPITest.java

@@ -748,7 +748,7 @@ private static ContentType createType(final String typeName, final BaseContentTy
         }
     }
 
-    private Field createField(final ContentType contentType, final String fieldVarName, final Class<? extends Field> fieldType,
+    public static Field createField(final ContentType contentType, final String fieldVarName, final Class<? extends Field> fieldType,
             final boolean fieldRequired) {
         try {
             final FieldAPI fieldAPI = APILocator.getContentTypeFieldAPI();

dotCMS/src/main/java/com/dotcms/contenttype/business/FieldFactoryImpl.java

@@ -238,6 +238,11 @@ private Field dbSaveUpdate(final Field throwAwayField) throws DotDataException {
       final List<String> takenFieldVars = fieldsAlreadyAdded.stream().map(Field::variable).collect(
               Collectors.toList());
 
+      // check if the field variable is compatible with GraphQL
+      if(!GraphQLUtil.isVariableGraphQLCompatible(throwAwayField)) {
+          takenFieldVars.add(throwAwayField.name());
+      }
+
       String tryVar = (throwAwayField.variable() == null)
           ? suggestVelocityVar(throwAwayField.name(), takenFieldVars) : throwAwayField.variable();
       builder.variable(tryVar);
@@ -588,7 +593,6 @@ public String suggestVelocityVar( String tryVar, List<String> takenFieldsVariabl
 
     // adds the GraphQL Reserved field names to the "taken fields variables" list
     final List<String> forbiddenFieldVariables = new ArrayList<>(takenFieldsVariables);
-    forbiddenFieldVariables.addAll(GraphQLUtil.getFieldReservedWords());
 
     String var = StringUtils.camelCaseLower(tryVar);
     // if we don't get a var back, we are looking at UTF-8 or worse

dotCMS/src/main/java/com/dotcms/graphql/CustomFieldType.java

@@ -1,20 +1,19 @@
 package com.dotcms.graphql;
 
-import com.dotcms.graphql.datafetcher.MapFieldPropertiesDataFetcher;
-import com.dotcms.graphql.util.TypeUtil;
-
-import java.util.HashMap;
-import java.util.Map;
-
-import graphql.schema.GraphQLObjectType;
-import graphql.schema.GraphQLOutputType;
-
 import static graphql.Scalars.GraphQLBoolean;
 import static graphql.Scalars.GraphQLID;
 import static graphql.Scalars.GraphQLInt;
 import static graphql.Scalars.GraphQLLong;
 import static graphql.Scalars.GraphQLString;
 
+import com.dotcms.graphql.datafetcher.MapFieldPropertiesDataFetcher;
+import com.dotcms.graphql.util.TypeUtil;
+import graphql.schema.GraphQLObjectType;
+import graphql.schema.GraphQLOutputType;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
 public enum CustomFieldType {
     BINARY,
     CATEGORY,
@@ -109,4 +108,8 @@ public enum CustomFieldType {
     public GraphQLObjectType getType() {
         return customFieldTypes.get(this.name());
     }
+
+    public static Collection<GraphQLObjectType> getCustomFieldTypes() {
+        return customFieldTypes.values();
+    }
 }

dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java

@@ -65,7 +65,7 @@ public enum InterfaceType {
         this.baseContentType = baseContentType;
     }
 
-    public static Set<String> RESERVED_GRAPHQL_FIELD_NAMES = new HashSet<>();
+    public static Set<String> CONTENT_INTERFACE_FIELDS = new HashSet<>();
 
     private static Map<String, GraphQLInterfaceType> interfaceTypes = new HashMap<>();
 
@@ -78,9 +78,9 @@ public enum InterfaceType {
     public static final String KEY_VALUE_INTERFACE_NAME = "KeyValueBaseType";
     public static final String FORM_INTERFACE_NAME = "FormBaseType";
 
-    static {
+    private static final Map<String, TypeFetcher> contentFields = new HashMap<>();
 
-        final Map<String, TypeFetcher> contentFields = new HashMap<>();
+    static {
         contentFields.put(MOD_DATE, new TypeFetcher(GraphQLString));
         contentFields.put(TITLE, new TypeFetcher(GraphQLString));
         contentFields.put(TITLE_IMAGE_KEY, new TypeFetcher(CustomFieldType.BINARY.getType(), new TitleImageFieldDataFetcher()));
@@ -99,7 +99,7 @@ public enum InterfaceType {
         contentFields.put(OWNER_KEY, new TypeFetcher(CustomFieldType.USER.getType(), new UserDataFetcher()));
         contentFields.put(MOD_USER_KEY, new TypeFetcher(CustomFieldType.USER.getType(), new UserDataFetcher()));
 
-        RESERVED_GRAPHQL_FIELD_NAMES.addAll(contentFields.keySet());
+        CONTENT_INTERFACE_FIELDS.addAll(contentFields.keySet());
 
         interfaceTypes.put("CONTENTLET", createInterfaceType("Contentlet", contentFields, new ContentResolver()));
 
@@ -156,4 +156,8 @@ public static GraphQLInterfaceType getInterfaceForBaseType(final BaseContentType
 
         return type;
     }
+
+    public static Map<String, TypeFetcher> getContentletInheritedFields() {
+        return contentFields;
+    }
 }

dotCMS/src/main/java/com/dotcms/graphql/business/GraphqlAPI.java

@@ -3,14 +3,21 @@
 import com.dotcms.contenttype.model.field.Field;
 import com.dotmarketing.exception.DotDataException;
 
+import graphql.schema.GraphQLObjectType;
+import java.util.Collection;
 import java.util.Map;
 
 import graphql.schema.GraphQLOutputType;
 import graphql.schema.GraphQLSchema;
 
 public interface GraphqlAPI {
     GraphQLSchema getSchema() throws DotDataException;
+
     GraphQLOutputType getGraphqlTypeForFieldClass(final Class<? extends Field> fieldClass, final Field field);
+
     void invalidateSchema();
 
+    Map<Class<? extends Field>, GraphQLOutputType> getFieldTypesWithCustomGraphQLTypes();
+
+    Collection<GraphQLObjectType> getCustomFieldTypes();
 }

dotCMS/src/main/java/com/dotcms/graphql/business/GraphqlAPIImpl.java

@@ -69,6 +69,7 @@
 import java.io.IOException;
 import java.nio.file.Files;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -347,4 +348,14 @@ private ContentType getRelatedContentTypeForField(final Field field, final User
 
         return APILocator.getContentTypeAPI(user).find(relatedContentTypeId);
     }
+
+    @Override
+    public Map<Class<? extends Field>, GraphQLOutputType> getFieldTypesWithCustomGraphQLTypes() {
+        return fieldClassGraphqlTypeMap;
+    }
+
+    @Override
+    public Collection<GraphQLObjectType> getCustomFieldTypes() {
+        return CustomFieldType.getCustomFieldTypes();
+    }
 }

dotCMS/src/main/java/com/dotcms/graphql/util/GraphQLUtil.java

@@ -1,11 +1,39 @@
 package com.dotcms.graphql.util;
 
+import com.dotcms.contenttype.model.field.Field;
 import com.dotcms.graphql.InterfaceType;
+import com.dotmarketing.business.APILocator;
+import graphql.schema.GraphQLList;
+import graphql.schema.GraphQLType;
 import java.util.Set;
 
 public class GraphQLUtil {
 
-    public static Set<String> getFieldReservedWords() {
-        return InterfaceType.RESERVED_GRAPHQL_FIELD_NAMES;
+    public static boolean isVariableGraphQLCompatible(final Field field) {
+        // first let's check if there's an inherited field with the same variable
+        if(InterfaceType.getContentletInheritedFields().containsKey(field.name())) {
+            // now let's check if the graphql types are compatible
+
+            // get inherited field's graphql type
+            final GraphQLType inheritedFieldGraphQLType = InterfaceType.getContentletInheritedFields()
+                    .get(field.name()).getType();
+
+            // get new field's type
+            final GraphQLType fieldGraphQLType = APILocator.getGraphqlAPI()
+                    .getGraphqlTypeForFieldClass(field.type(), field);
+
+            // if at least one of them is a custom type, they need to be equal to be compatible
+            return (!isCustomFieldType(inheritedFieldGraphQLType)
+                    && !isCustomFieldType(fieldGraphQLType))
+                    || inheritedFieldGraphQLType.equals(fieldGraphQLType);
+        }
+
+        return true;
+    }
+
+    private static boolean isCustomFieldType(final GraphQLType type) {
+           return  type instanceof GraphQLList ? APILocator.getGraphqlAPI().getCustomFieldTypes()
+                   .contains(((GraphQLList) type).getWrappedType())
+                   : APILocator.getGraphqlAPI().getCustomFieldTypes().contains(type);
     }
 }