Skip to content

Commit

Permalink
fix(GraphQL) Refs #29289 (#29351)
Browse files Browse the repository at this point in the history 
### 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
  • Loading branch information
@fabrizzio-dotCMS
fabrizzio-dotCMS authored Jul 31, 2024
1 parent 3d4f6fc commit fbe625f
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 69 deletions.
3 changes: 2 additions & 1 deletion dotCMS/src/main/java/com/dotcms/contenttype/model/field/Field.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion dotCMS/src/main/java/com/dotcms/contenttype/model/field/FieldBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
77 changes: 39 additions & 38 deletions dotCMS/src/main/java/com/dotcms/graphql/DotGraphQLHttpServlet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<HashMap<String, String>> corsHeaders = Function0.of(() -> {

Expand All @@ -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
Expand All @@ -91,10 +92,10 @@ protected void doOptions(final HttpServletRequest request, final HttpServletResp


}).memoized();
}






}
13 changes: 7 additions & 6 deletions dotCMS/src/main/java/com/dotcms/graphql/InterfaceType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -39,6 +37,7 @@
import java.util.Set;

public enum InterfaceType {

CONTENTLET(SimpleContentType.class),
CONTENT(SimpleContentType.class),
FILEASSET(FileAssetContentType.class),
Expand Down Expand Up @@ -141,13 +140,15 @@ public enum InterfaceType {
*/
private static void addBaseTypeFields(final Map<String, TypeFetcher> baseTypeFields,
final List<Field> 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)));
}
}

Expand Down Expand Up @@ -181,4 +182,4 @@ public static GraphQLInterfaceType getInterfaceForBaseType(final BaseContentType
return type;
}

}
}
36 changes: 13 additions & 23 deletions dotcms-postman/src/main/resources/postman/GraphQLTests.json
View file
Original file line number Diff line number Diff line change
@@ -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": [
{
Expand Down Expand Up @@ -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 = [",
Expand All @@ -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\");",
"",
Expand Down Expand Up @@ -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": ""
}
},
Expand Down Expand Up @@ -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": {}
}
}
],
Expand All @@ -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": ""
}
},
Expand Down Expand Up @@ -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": {}
}
}
],
Expand All @@ -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": ""
}
},
Expand Down

0 comments on commit fbe625f

Please sign in to comment.