From fbe625f6fc3fad81cd5c0babe12d4f538c846e9b Mon Sep 17 00:00:00 2001 From: Fabrizzio Araya <37148755+fabrizzio-dotCMS@users.noreply.github.com> Date: Wed, 31 Jul 2024 13:34:17 -0600 Subject: [PATCH] fix(GraphQL) Refs #29289 (#29351) ### Proposed Changes * This solves a problem that breaks the GraphQL API when removing non-required fields from the CT the Schema becomes invalid and throws an error. * The error looks more or less like this: ``` object type 'Page2024' does not implement interface 'PageBaseType' because field 'seokeywords' is missing object type 'Page2024' does not implement interface 'PageBaseType' because field 'pagemetadata' is missing ``` Also, fix an issue that prevents the user from seeing the real error thrown by the GraphQL Servlet There is an issue with our GraphQL impl where the library code uses a `NOPLogger` probably due to a conflict in the slf4j dependency. with this fix, we use our code to dispatch and process the request and handle and report the errors The postman was adjusted not to fail. Because this new restriction creating the schema will leave out some the non-required fields from the BaseType collections --- .../dotcms/contenttype/model/field/Field.java | 3 +- .../contenttype/model/field/FieldBuilder.java | 3 +- .../dotcms/graphql/DotGraphQLHttpServlet.java | 77 ++++++++++--------- .../com/dotcms/graphql/InterfaceType.java | 13 ++-- .../main/resources/postman/GraphQLTests.json | 36 ++++----- 5 files changed, 63 insertions(+), 69 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/contenttype/model/field/Field.java b/dotCMS/src/main/java/com/dotcms/contenttype/model/field/Field.java index affbb8d29106..8ad7f2c9bc56 100644 --- a/dotCMS/src/main/java/com/dotcms/contenttype/model/field/Field.java +++ b/dotCMS/src/main/java/com/dotcms/contenttype/model/field/Field.java @@ -116,9 +116,10 @@ public boolean readOnly() { /** * Determines whether the field must be returned by the API (for instance, the GraphQL API) or not, even if the field * is removable. - * + * @deprecated Since 24.07, for removal in a future version. * @return If the field must be returned by the API, set it to {@code true}. */ + @Deprecated(since = "24.07", forRemoval = true) @Value.Default public boolean forceIncludeInApi() { return false; diff --git a/dotCMS/src/main/java/com/dotcms/contenttype/model/field/FieldBuilder.java b/dotCMS/src/main/java/com/dotcms/contenttype/model/field/FieldBuilder.java index 1371285351dd..661d1a1e9772 100644 --- a/dotCMS/src/main/java/com/dotcms/contenttype/model/field/FieldBuilder.java +++ b/dotCMS/src/main/java/com/dotcms/contenttype/model/field/FieldBuilder.java @@ -37,11 +37,12 @@ public interface FieldBuilder { /** * Determines whether the field must be returned by the API (for instance, the GraphQL API) or not, even if the * field is removable. - * + * @deprecated Since 24.07, for removal in a future version. * @param include If the field must be returned by the API, set it to {@code true}. * * @return The current {@link FieldBuilder} instance. */ + @Deprecated(since = "24.07", forRemoval = true) FieldBuilder forceIncludeInApi(boolean include); public static FieldBuilder builder(Field field) throws DotStateException{ diff --git a/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java b/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java index b6230ed13457..90f731b551e2 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java @@ -2,66 +2,67 @@ import com.dotcms.rest.api.CorsFilter; import com.dotmarketing.util.Config; +import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; -import graphql.kickstart.execution.GraphQLObjectMapper; -import graphql.kickstart.execution.GraphQLQueryInvoker; import graphql.kickstart.servlet.AbstractGraphQLHttpServlet; import graphql.kickstart.servlet.GraphQLConfiguration; -import graphql.kickstart.servlet.input.GraphQLInvocationInputFactory; import io.vavr.Function0; -import java.io.IOException; import java.util.HashMap; import java.util.List; - -import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -/** - * Main point access for graphql. - * For cache see: {@link GraphqlCacheWebInterceptor} - */ public class DotGraphQLHttpServlet extends AbstractGraphQLHttpServlet { - - - final private static String CORS_DEFAULT=CorsFilter.CORS_PREFIX + "." + CorsFilter.CORS_DEFAULT; - - final private static String CORS_GRAPHQL = CorsFilter.CORS_PREFIX + ".graphql"; + + private static final String CORS_DEFAULT=CorsFilter.CORS_PREFIX + "." + CorsFilter.CORS_DEFAULT; + + private static final String CORS_GRAPHQL = CorsFilter.CORS_PREFIX + ".graphql"; @Override protected GraphQLConfiguration getConfiguration() { return GraphQLConfiguration - .with(new DotGraphQLSchemaProvider()) - .with(List.of(new DotGraphQLServletListener())) - .with(new DotGraphQLContextBuilder()) - .build(); + .with(new DotGraphQLSchemaProvider()) + .with(List.of(new DotGraphQLServletListener())) + .with(new DotGraphQLContextBuilder()) + .build(); } @Override protected void doGet(final HttpServletRequest request, final HttpServletResponse response) { - corsHeaders.apply().entrySet().stream().forEach(e -> response.setHeader(e.getKey(), e.getValue())); - super.doGet(request, response); + handleRequest(request, response); } @Override protected void doPost(final HttpServletRequest request, final HttpServletResponse response) { - corsHeaders.apply().entrySet().stream().forEach(e -> response.setHeader(e.getKey(), e.getValue())); - super.doPost(request, response); + handleRequest(request, response); } @Override - protected void doOptions(final HttpServletRequest request, final HttpServletResponse response) - throws ServletException, IOException { - corsHeaders.apply().entrySet().stream().forEach(e -> response.setHeader(e.getKey(), e.getValue())); - super.doOptions(request, response); + protected void doOptions(final HttpServletRequest request, final HttpServletResponse response) { + handleRequest(request, response); + } + + /** + * We're moving the request processing from the superclass down here to be able to apply the CORS headers and use our own logger + * @param request + * @param response + */ + protected void handleRequest(HttpServletRequest request, HttpServletResponse response) { + corsHeaders.apply().forEach(response::setHeader); + try { + getConfiguration().getHttpRequestHandler().handle(request, response); + } catch (Exception t) { + // There's a problem with our slf4j logger in our GraphQL Impl + // so we're using the dotCMS logger for now + Logger.error("Error executing GraphQL request!", t); + } } - - + /** * header list is computed but once. It first reads all values set as default, e.g. * api.cors.default.access-control-allow-headers that start with api.cors.default. It then overrides * those with the specific ones for graphql, api.cors.graphql.access-control-allow-headers - * + * */ protected final Function0> corsHeaders = Function0.of(() -> { @@ -71,7 +72,7 @@ protected void doOptions(final HttpServletRequest request, final HttpServletResp final String prop = Config.getStringProperty(CORS_DEFAULT + "." + k, null); if (UtilMethods.isSet(prop)) { headers.put(k.toLowerCase(), prop); - } + } }); // then override with graph @@ -91,10 +92,10 @@ protected void doOptions(final HttpServletRequest request, final HttpServletResp }).memoized(); - - - - - - -} + + + + + + +} \ No newline at end of file diff --git a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java index f72082206ee1..e17fa0a33d31 100644 --- a/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java +++ b/dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java @@ -27,10 +27,8 @@ import com.dotcms.enterprise.LicenseUtil; import com.dotcms.enterprise.license.LicenseLevel; import com.dotcms.graphql.business.ContentAPIGraphQLTypesProvider; -import com.dotcms.graphql.datafetcher.DotJSONDataFetcher; import com.dotcms.graphql.resolver.ContentResolver; import com.dotmarketing.util.Logger; -import graphql.scalars.ExtendedScalars; import graphql.schema.GraphQLInterfaceType; import java.util.HashMap; import java.util.HashSet; @@ -39,6 +37,7 @@ import java.util.Set; public enum InterfaceType { + CONTENTLET(SimpleContentType.class), CONTENT(SimpleContentType.class), FILEASSET(FileAssetContentType.class), @@ -141,13 +140,15 @@ public enum InterfaceType { */ private static void addBaseTypeFields(final Map baseTypeFields, final List requiredFormFields) { + final ContentAPIGraphQLTypesProvider instance = ContentAPIGraphQLTypesProvider.INSTANCE; + //Only Add the fixed fields to the base type fields as others can be removed breaking the GraphQLSchema for (final Field formField : requiredFormFields) { - if (!formField.fixed() && !formField.forceIncludeInApi()) { + if (!formField.fixed()) { + Logger.warn(InterfaceType.class, "Field " + formField.variable() + " is not fixed, skipping."); continue; } baseTypeFields.put(formField.variable(), new TypeFetcher( - ContentAPIGraphQLTypesProvider.INSTANCE - .getGraphqlTypeForFieldClass(formField.type(), formField))); + instance.getGraphqlTypeForFieldClass(formField.type(), formField))); } } @@ -181,4 +182,4 @@ public static GraphQLInterfaceType getInterfaceForBaseType(final BaseContentType return type; } -} +} \ No newline at end of file diff --git a/dotcms-postman/src/main/resources/postman/GraphQLTests.json b/dotcms-postman/src/main/resources/postman/GraphQLTests.json index 3517b8cee77a..0310d519d758 100644 --- a/dotcms-postman/src/main/resources/postman/GraphQLTests.json +++ b/dotcms-postman/src/main/resources/postman/GraphQLTests.json @@ -1,11 +1,11 @@ { "info": { - "_postman_id": "69f6ea4e-5b29-4a77-b599-47f07dc3eb80", + "_postman_id": "4a5eb0ca-8dc7-4659-b5fe-fb89cbc0f08f", "name": "GraphQL", "description": "This suite verifies that the GraphQL REST Endpoint is working as expected.\n\n[GraphQL](https://graphql.org/) is an open query language which allows you to perform real-type dynamic queries which specify exactly what data you want, and in what order. The dotCMS GraphQL content delivery API has a number of advantages over querying content via REST:\n\n- A single endpoint to query all content: `api/v1/graphql`.\n \n- Self documenting via schema introspection.\n \n- No over-fetching of data (e.g. unneeded fields).\n \n- No need for multiple requests to get combined data.\n \n- Client control over both the query and the data received.", "schema": "https://schema.getpostman.com/json/collection/v2.1.0/collection.json", - "_exporter_id": "5403727", - "_collection_link": "https://cloudy-robot-285072.postman.co/workspace/JCastro-Workspace~5bfa586e-54db-429b-b7d5-c4ff997e3a0d/collection/5403727-69f6ea4e-5b29-4a77-b599-47f07dc3eb80?action=share&source=collection_link&creator=5403727" + "_exporter_id": "10041132", + "_collection_link": "https://speeding-firefly-555540.postman.co/workspace/dot3~4b54255a-c61a-4ce4-b605-fe26ebdfdd91/collection/10041132-4a5eb0ca-8dc7-4659-b5fe-fb89cbc0f08f?action=share&source=collection_link&creator=10041132" }, "item": [ { @@ -1545,7 +1545,6 @@ "", " // persona", " pm.expect(persona[\"name\"], 'FAILED:[persona.name]').equal(\"Winter Enthusiast\");", - " pm.expect(persona[\"description\"], 'FAILED:[persona.description]').equal(\"People who are passionate about winter sports.\");", " pm.expect(persona[\"photo\"].idPath, 'FAILED:[persona.photo.idPath]').equal(\"/dA/792c7c9f6b/photo/mountain-persona.jpg?language_id=1\");", " pm.expect(persona[\"keyTag\"], 'FAILED:[persona.keyTag]').equal(\"WinterEnthusiast\");", " let expectedPersonTags = [", @@ -1562,8 +1561,7 @@ "", " var visitorPersona = visitor.persona;", " // visitor's persona", - " pm.expect(visitorPersona[\"name\"], 'FAILED:[visitorPersona.name]').equal(\"Winter Enthusiast\");", - " pm.expect(visitorPersona[\"description\"], 'FAILED:[visitorPersona.description]').equal(\"People who are passionate about winter sports.\");", + " pm.expect(visitorPersona[\"name\"], 'FAILED:[visitorPersona.name]').equal(\"Winter Enthusiast\"); ", " pm.expect(visitorPersona[\"photo\"].idPath, 'FAILED:[visitorPersona.photo.idPath]').equal(\"/dA/792c7c9f6b/photo/mountain-persona.jpg?language_id=1\");", " pm.expect(visitorPersona[\"keyTag\"], 'FAILED:[visitorPersona.keyTag]').equal(\"WinterEnthusiast\");", "", @@ -1641,7 +1639,7 @@ "body": { "mode": "graphql", "graphql": { - "query": "{\n page(url:\"/destinations/costa-rica\", pageMode: \"live\", languageId: \"1\", persona:\"WinterEnthusiast\") {\n viewAs {\n persona {\n name\n description\n photo {\n idPath\n }\n keyTag\n tags\n }\n visitor {\n persona {\n name\n description\n photo {\n idPath\n }\n keyTag\n tags\n }\n device\n isNew\n tags {\n tag\n count\n }\n userAgent {\n operatingSystem\n browser\n id\n browserVersion {\n version\n majorVersion\n minorVersion\n }\n }\n personas {\n persona \n count\n }\n }\n mode\n language {\n id\n country\n countryCode\n language\n languageCode\n }\n }\n }\n}", + "query": "{\n page(url:\"/destinations/costa-rica\", pageMode: \"live\", languageId: \"1\", persona:\"WinterEnthusiast\") {\n viewAs {\n persona {\n name\n photo {\n idPath\n }\n keyTag\n tags\n }\n visitor {\n persona {\n name \n photo {\n idPath\n }\n keyTag\n tags\n }\n device\n isNew\n tags {\n tag\n count\n }\n userAgent {\n operatingSystem\n browser\n id\n browserVersion {\n version\n majorVersion\n minorVersion\n }\n }\n personas {\n persona \n count\n }\n }\n mode\n language {\n id\n country\n countryCode\n language\n languageCode\n }\n }\n }\n}", "variables": "" } }, @@ -3763,20 +3761,12 @@ " pm.expect(page[\"url\"], 'FAILED:[url]').equal(\"/blog/index\");", " pm.expect(page.hostFolder.hostName, 'FAILED:[hostFolder.hostName]').equal(\"demo.dotcms.com\");", " pm.expect(page.hostFolder.folderName, 'FAILED:[hostFolder.folderName]').equal(\"blog\");", - " pm.expect(page.template, 'FAILED:[template]').equal(\"64269d16-2710-4919-88ec-3b09c89ea004\");", - " pm.expect(page.showOnMenu.length, 'FAILED:[showOnMenu]').to.eql(0);", - " pm.expect(page[\"sortOrder\"], 'FAILED:[sortOrder]').equal(0);", - " pm.expect(page[\"cachettl\"], 'FAILED:[cachettl]').equal(\"0\");", - " pm.expect(page[\"friendlyName\"], 'FAILED:[friendlyName]').equal(\"Blog\");", - " pm.expect(page[\"redirecturl\"], 'FAILED:[redirecturl]').equal(null);", - " pm.expect(page.httpsreq.length, 'FAILED:[httpsreq]').to.eql(0);", - " pm.expect(page[\"seodescription\"], 'FAILED:[seodescription]').equal(\"Help understanding your financial future and hoe Quest Financial and our advisors and help you plan for tomorrow with confidence\");", - " pm.expect(page[\"seokeywords\"], 'FAILED:[seokeywords]').equal(\"Financial Blog\");", - " pm.expect(page[\"pagemetadata\"], 'FAILED:[pagemetadata]').equal(null); ", + " pm.expect(page.template, 'FAILED:[template]').equal(\"64269d16-2710-4919-88ec-3b09c89ea004\"); ", "", "});" ], - "type": "text/javascript" + "type": "text/javascript", + "packages": {} } } ], @@ -3786,7 +3776,7 @@ "body": { "mode": "graphql", "graphql": { - "query": "{\n PageBaseTypeCollection(query:\"+identifier:9c5f42da-31b1-4935-9df6-153f5de1bdf2\") {\n title\n url\n hostFolder {\n hostName\n folderName\n }\n template\n showOnMenu\n sortOrder\n cachettl\n friendlyName\n redirecturl\n httpsreq\n seodescription\n seokeywords\n pagemetadata\n }\n}", + "query": "{\n PageBaseTypeCollection(query:\"+identifier:9c5f42da-31b1-4935-9df6-153f5de1bdf2\") {\n title\n url\n hostFolder {\n hostName\n folderName\n }\n template \n\n }\n}", "variables": "" } }, @@ -3824,12 +3814,12 @@ " pm.expect(page[\"keyTag\"], 'FAILED:[keyTag]').equal(\"OceanEnthusiast\");", " pm.expect(page.hostFolder.hostName, 'FAILED:[hostFolder.hostName]').equal(\"demo.dotcms.com\");", " pm.expect(page.hostFolder.folderName, 'FAILED:[hostFolder.folderName]').equal(\"system folder\");", - " pm.expect(page.tags.length, 'FAILED:[tags]').to.eql(4);", - " pm.expect(page[\"description\"], 'FAILED:[description]').equal(\"People who typically vacation around water sports such as surfing, wind surfing, etc.\");", + " pm.expect(page.tags.length, 'FAILED:[tags]').to.eql(4); ", "", "});" ], - "type": "text/javascript" + "type": "text/javascript", + "packages": {} } } ], @@ -3839,7 +3829,7 @@ "body": { "mode": "graphql", "graphql": { - "query": "{\n PersonaBaseTypeCollection(query:\"+identifier:d948d85c-3bc8-4d85-b0aa-0e989b9ae235\") {\n name\n hostFolder {\n hostName\n folderName\n }\n keyTag\n photo {\n name\n }\n tags\n description\n }\n}", + "query": "{\n PersonaBaseTypeCollection(query:\"+identifier:d948d85c-3bc8-4d85-b0aa-0e989b9ae235\") {\n name\n hostFolder {\n hostName\n folderName\n }\n keyTag\n photo {\n name\n }\n tags \n }\n}", "variables": "" } },