From c19f828461c4bc645513ae8104be1f4665a8b93c Mon Sep 17 00:00:00 2001 From: Aaron Steinfeld Date: Thu, 6 Oct 2022 18:31:50 -0400 Subject: [PATCH] refactor: update core gql libs --- .../DefaultGraphQlRequestContextBuilder.java | 36 +++--------- .../context/GraphQlRequestContext.java | 4 +- ...faultGraphQlRequestContextBuilderTest.java | 3 +- .../build.gradle.kts | 6 +- .../DefaultGraphQlSchemaRegistry.java | 2 +- .../schema/registry/DefaultSchema.java | 24 ++++++-- .../GraphQlAnnotatedSchemaMerger.java | 36 +++++++++++- .../GraphQlAnnotatedSchemaMergerTest.java | 55 +++++++++++-------- 8 files changed, 102 insertions(+), 64 deletions(-) diff --git a/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilder.java b/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilder.java index 8bdb718f..a5f452c9 100644 --- a/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilder.java +++ b/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilder.java @@ -1,9 +1,8 @@ package org.hypertrace.core.graphql.context; import com.google.common.collect.Streams; -import graphql.kickstart.servlet.context.DefaultGraphQLServletContext; +import graphql.kickstart.execution.context.DefaultGraphQLContext; import graphql.kickstart.servlet.context.DefaultGraphQLServletContextBuilder; -import graphql.kickstart.servlet.context.GraphQLServletContext; import graphql.schema.DataFetcher; import java.util.Arrays; import java.util.Map; @@ -14,10 +13,8 @@ import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.inject.Inject; -import javax.security.auth.Subject; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import org.dataloader.DataLoaderRegistry; import org.hypertrace.core.graphql.spi.config.GraphQlServiceConfig; class DefaultGraphQlRequestContextBuilder extends DefaultGraphQLServletContextBuilder @@ -40,32 +37,22 @@ class DefaultGraphQlRequestContextBuilder extends DefaultGraphQLServletContextBu @Override public GraphQlRequestContext build(HttpServletRequest request, HttpServletResponse response) { - return new DefaultGraphQlRequestContext(request, response); + return new DefaultGraphQlRequestContext(request); } - private final class DefaultGraphQlRequestContext implements GraphQlRequestContext { - private final GraphQLServletContext servletContext; + private final class DefaultGraphQlRequestContext extends DefaultGraphQLContext + implements GraphQlRequestContext { private final ContextualCachingKey cachingKey; private final String requestId = UUID.randomUUID().toString(); + private final HttpServletRequest request; - private DefaultGraphQlRequestContext(HttpServletRequest request, HttpServletResponse response) { - this.servletContext = - DefaultGraphQLServletContext.createServletContext().with(request).with(response).build(); + private DefaultGraphQlRequestContext(HttpServletRequest request) { + this.request = request; this.cachingKey = new DefaultContextualCacheKey(this, this.getTenantId().orElse(DEFAULT_CONTEXT_ID)); } - @Override - public Optional getSubject() { - return this.servletContext.getSubject(); - } - - @Override - public Optional getDataLoaderRegistry() { - return this.servletContext.getDataLoaderRegistry(); - } - @Override public DataFetcher> constructDataFetcher( Class>> dataFetcherClass) { @@ -75,29 +62,24 @@ public DataFetcher> constructDataFetcher( @Override public Optional getAuthorizationHeader() { - HttpServletRequest request = this.servletContext.getHttpServletRequest(); return Optional.ofNullable(request.getHeader(AUTHORIZATION_HEADER_KEY)) .or(() -> Optional.ofNullable(request.getHeader(AUTHORIZATION_HEADER_KEY.toLowerCase()))); } @Override public Optional getTenantId() { - HttpServletRequest request = this.servletContext.getHttpServletRequest(); return Optional.ofNullable(request.getHeader(TENANT_ID_HEADER_KEY)) .or(DefaultGraphQlRequestContextBuilder.this.serviceConfig::getDefaultTenantId); } @Override public Map getTracingContextHeaders() { - return Streams.stream( - this.servletContext.getHttpServletRequest().getHeaderNames().asIterator()) + return Streams.stream(request.getHeaderNames().asIterator()) .filter( header -> TRACING_CONTEXT_HEADER_KEY_PREFIXES.stream() .anyMatch(prefix -> header.toLowerCase().startsWith(prefix.toLowerCase()))) - .collect( - Collectors.toUnmodifiableMap( - String::toLowerCase, this.servletContext.getHttpServletRequest()::getHeader)); + .collect(Collectors.toUnmodifiableMap(String::toLowerCase, request::getHeader)); } @Nonnull diff --git a/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/GraphQlRequestContext.java b/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/GraphQlRequestContext.java index 98562f95..86a62cb7 100644 --- a/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/GraphQlRequestContext.java +++ b/hypertrace-core-graphql-context/src/main/java/org/hypertrace/core/graphql/context/GraphQlRequestContext.java @@ -1,13 +1,13 @@ package org.hypertrace.core.graphql.context; -import graphql.kickstart.execution.context.GraphQLContext; +import graphql.kickstart.execution.context.GraphQLKickstartContext; import graphql.schema.DataFetcher; import java.util.Map; import java.util.Optional; import java.util.concurrent.CompletableFuture; import javax.annotation.Nonnull; -public interface GraphQlRequestContext extends GraphQLContext { +public interface GraphQlRequestContext extends GraphQLKickstartContext { /** * A tool to create data fetchers via injection container due to limitations in the framework. For diff --git a/hypertrace-core-graphql-context/src/test/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilderTest.java b/hypertrace-core-graphql-context/src/test/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilderTest.java index 9ef475de..c3c82e5a 100644 --- a/hypertrace-core-graphql-context/src/test/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilderTest.java +++ b/hypertrace-core-graphql-context/src/test/java/org/hypertrace/core/graphql/context/DefaultGraphQlRequestContextBuilderTest.java @@ -7,7 +7,6 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.reset; @@ -67,7 +66,7 @@ void returnsEmptyOptionalIfNoAuthorizationHeaderPresent() { @Test void delegatesDataLoaderRegistry() { - assertTrue(this.requestContext.getDataLoaderRegistry().isPresent()); + assertNotNull(this.requestContext.getDataLoaderRegistry()); } @Test diff --git a/hypertrace-core-graphql-platform/build.gradle.kts b/hypertrace-core-graphql-platform/build.gradle.kts index ba6f8d99..6f1817eb 100644 --- a/hypertrace-core-graphql-platform/build.gradle.kts +++ b/hypertrace-core-graphql-platform/build.gradle.kts @@ -18,8 +18,8 @@ dependencies { api("org.hypertrace.core.attribute.service:caching-attribute-service-client:0.14.8") api("com.google.inject:guice:5.1.0") - api("com.graphql-java:graphql-java:15.0") - api("io.github.graphql-java:graphql-java-annotations:8.3") + api("com.graphql-java:graphql-java:19.2") + api("io.github.graphql-java:graphql-java-annotations:9.1") api("org.slf4j:slf4j-api:1.7.36") api("io.reactivex.rxjava3:rxjava:3.1.5") api("com.google.protobuf:protobuf-java-util:3.21.1") @@ -28,7 +28,7 @@ dependencies { api("com.google.code.findbugs:jsr305:3.0.2") api("com.typesafe:config:1.4.2") api("com.google.guava:guava:31.1-jre") - api("com.graphql-java-kickstart:graphql-java-servlet:10.1.0") + api("com.graphql-java-kickstart:graphql-java-servlet:14.0.0") api("com.fasterxml.jackson.core:jackson-databind:2.13.4") api("com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.13.4") diff --git a/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultGraphQlSchemaRegistry.java b/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultGraphQlSchemaRegistry.java index 860bc82d..0b31cb17 100644 --- a/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultGraphQlSchemaRegistry.java +++ b/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultGraphQlSchemaRegistry.java @@ -22,7 +22,7 @@ public Set getRegisteredFragments() { } @Override - public GraphQlSchemaFragment getRootFragment() { + public DefaultSchema getRootFragment() { return this.defaultSchemaFragment; } } diff --git a/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultSchema.java b/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultSchema.java index 89a6af95..4f6ecf4e 100644 --- a/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultSchema.java +++ b/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/DefaultSchema.java @@ -1,9 +1,17 @@ package org.hypertrace.core.graphql.schema.registry; +import graphql.annotations.annotationTypes.GraphQLDescription; +import graphql.annotations.annotationTypes.GraphQLField; import graphql.annotations.annotationTypes.GraphQLName; import org.hypertrace.core.graphql.spi.schema.GraphQlSchemaFragment; class DefaultSchema implements GraphQlSchemaFragment { + // Placeholder description is used to identify and remove placeholder fields before building the + // schema. Placeholders are used while the schema is under construction so it is always valid + // (i.e. has at least one value) + static final String PLACEHOLDER_DESCRIPTION = "::placeholder::"; + static final String ROOT_QUERY_NAME = "Query"; + static final String ROOT_MUTATION_NAME = "Mutation"; @Override public String fragmentName() { @@ -20,9 +28,17 @@ public Class annotatedMutationClass() { return MutationSchema.class; } - @GraphQLName("Query") - private interface QuerySchema {} + @GraphQLName(ROOT_QUERY_NAME) + private interface QuerySchema { + @GraphQLField + @GraphQLDescription(PLACEHOLDER_DESCRIPTION) + String placeholder(); + } - @GraphQLName("Mutation") - private interface MutationSchema {} + @GraphQLName(ROOT_MUTATION_NAME) + private interface MutationSchema { + @GraphQLField + @GraphQLDescription(PLACEHOLDER_DESCRIPTION) + String placeholder(); + } } diff --git a/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMerger.java b/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMerger.java index 65778874..13df8de9 100644 --- a/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMerger.java +++ b/hypertrace-core-graphql-schema-registry/src/main/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMerger.java @@ -12,6 +12,7 @@ import java.util.Collection; import java.util.Map; import java.util.Map.Entry; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -39,7 +40,8 @@ public GraphQLSchema get() { return fragments.stream() .map(fragment -> this.fragmentToSchema(fragment, annotationProcessor)) - .reduce(this.fragmentToSchema(rootFragment, annotationProcessor), this::merge); + .reduce(this.fragmentToSchema(rootFragment, annotationProcessor), this::merge) + .transform(this::removeRootPlaceholders); } private void registerAllTypeFunctions( @@ -134,4 +136,36 @@ private Map buildMapWithoutNulls( .filter(entry -> nonNull(entry) && nonNull(entry.getKey()) && nonNull(entry.getValue())) .collect(Collectors.toUnmodifiableMap(Entry::getKey, Entry::getValue)); } + + private void removeRootPlaceholders(GraphQLSchema.Builder mergedSchema) { + GraphQLSchema mergedWithPlaceholders = mergedSchema.build(); + // Queries must be defined, mutation is optional which is represented by a null + mergedSchema.query( + this.rebuildTypeWithoutPlaceholders(mergedWithPlaceholders.getQueryType()).orElseThrow()); + mergedSchema.mutation( + this.rebuildTypeWithoutPlaceholders(mergedWithPlaceholders.getMutationType()).orElse(null)); + } + + private Optional rebuildTypeWithoutPlaceholders(GraphQLObjectType objectType) { + // Schema validation now requires all objects to have at least one field. To merge partial + // fragments, we start with placeholders, identified by a constant, then remove them at the end. + GraphQLObjectType newType = + GraphQLObjectType.newObject(objectType) + .replaceFields( + objectType.getFields().stream() + .filter( + field -> + Optional.ofNullable(field.getDescription()) + .map( + description -> + !description.equals(DefaultSchema.PLACEHOLDER_DESCRIPTION)) + .orElse(true)) + .collect(Collectors.toList())) + .build(); + + if (newType.getFields().isEmpty()) { + return Optional.empty(); + } + return Optional.of(newType); + } } diff --git a/hypertrace-core-graphql-schema-registry/src/test/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMergerTest.java b/hypertrace-core-graphql-schema-registry/src/test/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMergerTest.java index e082ecf9..efc7f588 100644 --- a/hypertrace-core-graphql-schema-registry/src/test/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMergerTest.java +++ b/hypertrace-core-graphql-schema-registry/src/test/java/org/hypertrace/core/graphql/schema/registry/GraphQlAnnotatedSchemaMergerTest.java @@ -1,12 +1,14 @@ package org.hypertrace.core.graphql.schema.registry; +import static org.hypertrace.core.graphql.schema.registry.DefaultSchema.ROOT_MUTATION_NAME; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; +import graphql.Scalars; import graphql.annotations.annotationTypes.GraphQLDataFetcher; import graphql.annotations.annotationTypes.GraphQLField; -import graphql.annotations.annotationTypes.GraphQLName; import graphql.annotations.processor.ProcessingElementsContainer; import graphql.annotations.processor.typeFunctions.TypeFunction; import graphql.schema.DataFetcher; @@ -30,20 +32,11 @@ @ExtendWith(MockitoExtension.class) public class GraphQlAnnotatedSchemaMergerTest { - private static final String ROOT_QUERY_NAME = "root"; - private static final String ROOT_MUTATION_NAME = "rootMutation"; - @Mock private GraphQlSchemaRegistry mockRegistry; @Mock private DataFetchingEnvironment mockDataFetchingEnvironment; private GraphQlAnnotatedSchemaMerger merger; - @GraphQLName(ROOT_QUERY_NAME) - interface RootQuerySchema {} - - @GraphQLName(ROOT_MUTATION_NAME) - interface RootMutationSchema {} - interface FirstQuerySchema { @GraphQLField String first(); @@ -91,8 +84,7 @@ interface SharedType { public void beforeEach() { this.merger = new GraphQlAnnotatedSchemaMerger(mockRegistry); - when(mockRegistry.getRootFragment()) - .thenReturn(this.createSchemaFragment(RootQuerySchema.class, RootMutationSchema.class)); + when(mockRegistry.getRootFragment()).thenReturn(new DefaultSchema()); } @Test @@ -148,10 +140,13 @@ void mergesMutationSchemas() { @Test void supportsMutationOnlyFragment() { when(mockRegistry.getRegisteredFragments()) - .thenReturn(Set.of(this.createSchemaFragment(null, FirstMutationSchema.class))); + .thenReturn( + Set.of( + this.createSchemaFragment(FirstQuerySchema.class), + this.createSchemaFragment(null, FirstMutationSchema.class))); this.verifySchemaWithMutationFields(this.merger.get(), Set.of("mutateOne")); - this.verifySchemaWithQueryFields(this.merger.get(), Set.of()); + this.verifySchemaWithQueryFields(this.merger.get(), Set.of("first", "second")); } @Test @@ -169,7 +164,13 @@ public GraphQLType buildType( Class aClass, AnnotatedType annotatedType, ProcessingElementsContainer container) { - return GraphQLObjectType.newObject().name("typeFunctionType").build(); + return GraphQLObjectType.newObject() + .name("typeFunctionType") + .field( + GraphQLFieldDefinition.newFieldDefinition() + .name("typeFunctionTypeField") + .type(Scalars.GraphQLString)) + .build(); } }; @@ -220,7 +221,7 @@ public List typeFunctions() { } private void verifySchemaWithQueryFields(GraphQLSchema schema, Set fields) { - assertEquals(ROOT_QUERY_NAME, schema.getQueryType().getName()); + assertEquals(DefaultSchema.ROOT_QUERY_NAME, schema.getQueryType().getName()); List fieldDefinitions = schema.getQueryType().getFieldDefinitions(); assertEquals(fields.size(), fieldDefinitions.size()); assertTrue( @@ -231,13 +232,19 @@ private void verifySchemaWithQueryFields(GraphQLSchema schema, Set field } private void verifySchemaWithMutationFields(GraphQLSchema schema, Set fields) { - assertEquals(ROOT_MUTATION_NAME, schema.getMutationType().getName()); - List fieldDefinitions = schema.getMutationType().getFieldDefinitions(); - assertEquals(fields.size(), fieldDefinitions.size()); - assertTrue( - fields.containsAll( - fieldDefinitions.stream() - .map(GraphQLFieldDefinition::getName) - .collect(Collectors.toUnmodifiableSet()))); + if (fields.isEmpty()) { + // A type must have fields, so if no fields expected, no type expected + assertNull(schema.getMutationType()); + } else { + assertEquals(ROOT_MUTATION_NAME, schema.getMutationType().getName()); + List fieldDefinitions = + schema.getMutationType().getFieldDefinitions(); + assertEquals(fields.size(), fieldDefinitions.size()); + assertTrue( + fields.containsAll( + fieldDefinitions.stream() + .map(GraphQLFieldDefinition::getName) + .collect(Collectors.toUnmodifiableSet()))); + } } }