Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GraphQL setter to GraphqlServiceBuilder #5269

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Add GraphQL setter to GraphqlServiceBuilder #5269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62372a1
Add GraphQL setter to GraphqlServiceBuilder
minwoox Oct 26, 2023
f1008ee
Deprecate setter for build a GraphQl
minwoox May 23, 2024
10db64f
Merge branch 'main' into graphql_builder
minwoox May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 133 additions & 68 deletions graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.linecorp.armeria.server.graphql;

import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -45,6 +46,7 @@
import com.linecorp.armeria.server.ServiceRequestContext;

import graphql.GraphQL;
import graphql.execution.ExecutionIdProvider;
import graphql.execution.instrumentation.ChainedInstrumentation;
import graphql.execution.instrumentation.Instrumentation;
import graphql.schema.GraphQLSchema;
Expand All @@ -65,32 +67,57 @@

private static final List<String> DEFAULT_SCHEMA_FILE_NAMES = ImmutableList.of("schema.graphqls",
"schema.graphql");
private final ImmutableList.Builder<URL> schemaUrls = ImmutableList.builder();

private final ImmutableList.Builder<RuntimeWiringConfigurator> runtimeWiringConfigurators =
ImmutableList.builder();
private final ImmutableList.Builder<GraphQLTypeVisitor> typeVisitors = ImmutableList.builder();
private final ImmutableList.Builder<Instrumentation> instrumentations = ImmutableList.builder();
private final ImmutableList.Builder<GraphqlConfigurator> graphqlBuilderConsumers =
ImmutableList.builder();

@Nullable
private ImmutableList.Builder<Consumer<? super DataLoaderRegistry>> dataLoaderRegistryConsumers;
private boolean useBlockingTaskExecutor;
private GraphQL graphql;

// Fields for building a graphql
@Nullable
private Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> dataLoaderRegistryFactory;
private ExecutionIdGenerator executionIdGenerator;
@Nullable
private ImmutableList.Builder<Instrumentation> instrumentations;
@Nullable
private ImmutableList.Builder<GraphqlConfigurator> graphqlBuilderConsumers;

// Fields for building a schema in a graphql
@Nullable
private GraphQLSchema schema;
@Nullable
private ImmutableList.Builder<URL> schemaUrls;
@Nullable
private ImmutableList.Builder<RuntimeWiringConfigurator> runtimeWiringConfigurators;
@Nullable
private ImmutableList.Builder<GraphQLTypeVisitor> typeVisitors;

// Fields for building a DataLoaderRegistry
@Nullable
private GraphqlErrorHandler errorHandler;
private ImmutableList.Builder<Consumer<? super DataLoaderRegistry>> dataLoaderRegistryConsumers;
@Nullable
private Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> dataLoaderRegistryFactory;

private ExecutionIdGenerator executionIdGenerator = ExecutionIdGenerator.of();
// Others
private boolean useBlockingTaskExecutor;
@Nullable
private GraphqlErrorHandler errorHandler;
Comment on lines +100 to +103
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two properties are only provided by Armeria and the majority are necessary to build GraphQL. Too many checkState() looks messy. How about moving properties used to build GraphQL to GraphqlBuilder?

GraphqlService
  .builder()
  .graphql() // Return GraphqlBuilder
    .schema(...)
    .dataLoaderRegistry(...)
    .and() // Return GraphqlServiceBuilder
  .useBlockingTaskExecutor(true);
  .bulid();


GraphqlService
  .builder()
  .graphql(graphql) // Directly inject GraphQL
  .useBlockingTaskExecutor(true);
  .bulid()

Copy link
Contributor Author

@minwoox minwoox Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking about it but GraphQL provides its own builder via new GraphQL.Builder().
Also, I found out that our builder, which is GraphqlServiceBuilder, does not provide all setters that the GraphQL provides. I think there's always a mismatch unless we keep up with the upstream change.
So I was rather thinking deprecating the setters:
https://github.com/line/armeria/pull/5269/files#diff-6b64ad742ab0aa687e003b83510d32495d933e5b3fd2a2236184d6f01bbfcc50R109-R110

Too many checkState() looks messy.

Yes, but we need it anyway because we can't just remove the setters after deprecating them. 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was rather deprecating the setters:

I agree with this idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've deprecated all the setters. PTAL. 🙇


GraphqlServiceBuilder() {}

/**
* Sets the {@link GraphQL}.
*/
public GraphqlServiceBuilder graphql(GraphQL graphql) {
// TODO(minwoox): Consider deprecating setters for GraphQLSchema and DataLoaderRegistry so that users
// can just build GraphQL using its own builder.
checkState(executionIdGenerator == null && instrumentations == null &&
graphqlBuilderConsumers == null && schema == null &&
schemaUrls == null && runtimeWiringConfigurators == null &&
typeVisitors == null &&
dataLoaderRegistryConsumers == null && dataLoaderRegistryFactory == null,
"graphql() and setting properties for a GraphQL are mutually exclusive.");
this.graphql = requireNonNull(graphql, "graphql");
return this;
}

/**
* Adds the schema {@link File}s.
* If not set, the {@code schema.graphql} or {@code schema.graphqls} will be imported from the resource.
Expand Down Expand Up @@ -140,6 +167,10 @@
}

private GraphqlServiceBuilder schemaUrls0(Iterable<URL> schemaUrls) {
checkState(graphql == null, "graphql() and schemaUrls() are mutually exclusive.");
if (this.schemaUrls == null) {
this.schemaUrls = ImmutableList.builder();
}
this.schemaUrls.addAll(requireNonNull(schemaUrls, "schemaUrls"));
return this;
}
Expand All @@ -148,6 +179,7 @@
* Sets the {@link GraphQLSchema}.
*/
public GraphqlServiceBuilder schema(GraphQLSchema schema) {
checkState(graphql == null, "graphql() and schema() are mutually exclusive.");
this.schema = requireNonNull(schema, "schema");
return this;
}
Expand All @@ -157,6 +189,7 @@
*/
public GraphqlServiceBuilder dataLoaderRegistry(
Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> dataLoaderRegistryFactory) {
checkState(graphql == null, "graphql() and dataLoaderRegistry() are mutually exclusive.");
checkState(dataLoaderRegistryConsumers == null,
"configureDataLoaderRegistry() and dataLoaderRegistry() are mutually exclusive.");
this.dataLoaderRegistryFactory =
Expand All @@ -183,6 +216,7 @@
@Deprecated
public GraphqlServiceBuilder configureDataLoaderRegistry(
Iterable<? extends Consumer<? super DataLoaderRegistry>> configurers) {
checkState(graphql == null, "graphql() and configureDataLoaderRegistry() are mutually exclusive.");
checkState(dataLoaderRegistryFactory == null,
"configureDataLoaderRegistry() and dataLoaderRegistry() are mutually exclusive.");
if (dataLoaderRegistryConsumers == null) {
Expand All @@ -204,6 +238,10 @@
* Adds the {@link RuntimeWiringConfigurator}s.
*/
public GraphqlServiceBuilder runtimeWiring(Iterable<? extends RuntimeWiringConfigurator> configurators) {
checkState(graphql == null, "graphql() and runtimeWiring() are mutually exclusive.");
if (runtimeWiringConfigurators == null) {
runtimeWiringConfigurators = ImmutableList.builder();
}
runtimeWiringConfigurators.addAll(requireNonNull(configurators, "configurators"));
return this;
}
Expand All @@ -219,6 +257,10 @@
* Adds the {@link GraphQLTypeVisitor}s.
*/
public GraphqlServiceBuilder typeVisitors(Iterable<? extends GraphQLTypeVisitor> typeVisitors) {
checkState(graphql == null, "graphql() and typeVisitors() are mutually exclusive.");
if (this.typeVisitors == null) {
this.typeVisitors = ImmutableList.builder();
}
this.typeVisitors.addAll(requireNonNull(typeVisitors, "typeVisitors"));
return this;
}
Expand All @@ -235,7 +277,12 @@
* Adds the {@link Instrumentation}s.
*/
public GraphqlServiceBuilder instrumentation(Iterable<? extends Instrumentation> instrumentations) {
this.instrumentations.addAll(requireNonNull(instrumentations, "instrumentations"));
checkState(graphql == null, "graphql() and instrumentation() are mutually exclusive.");
requireNonNull(instrumentations, "instrumentations");
if (this.instrumentations == null) {
this.instrumentations = ImmutableList.builder();
}
this.instrumentations.addAll(instrumentations);
return this;
}

Expand All @@ -251,7 +298,12 @@
*/
public GraphqlServiceBuilder configureGraphql(
Iterable<? extends GraphqlConfigurator> configurers) {
graphqlBuilderConsumers.addAll(requireNonNull(configurers, "configurers"));
checkState(graphql == null, "graphql() and configureGraphql() are mutually exclusive.");
requireNonNull(configurers, "configurers");
if (graphqlBuilderConsumers == null) {
graphqlBuilderConsumers = ImmutableList.builder();
}
graphqlBuilderConsumers.addAll(configurers);
return this;
}

Expand Down Expand Up @@ -284,6 +336,7 @@
* If not specified, {@link ExecutionIdGenerator#of()} is used by default.
*/
public GraphqlServiceBuilder executionIdGenerator(ExecutionIdGenerator executionIdGenerator) {
checkState(graphql == null, "graphql() and executionIdGenerator() are mutually exclusive.");
this.executionIdGenerator = requireNonNull(executionIdGenerator, "executionIdGenerator");
return this;
}
Expand All @@ -292,80 +345,69 @@
* Creates a {@link GraphqlService}.
*/
public GraphqlService build() {
final GraphQLSchema schema = buildSchema();
GraphQL.Builder builder = GraphQL.newGraphQL(schema)
.executionIdProvider(executionIdGenerator.asExecutionProvider());
final List<Instrumentation> instrumentations = this.instrumentations.build();
if (!instrumentations.isEmpty()) {
builder = builder.instrumentation(new ChainedInstrumentation(instrumentations));
}

final List<GraphqlConfigurator> graphqlBuilders = graphqlBuilderConsumers.build();
for (GraphqlConfigurator configurer : graphqlBuilders) {
configurer.configure(builder);
}

Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> dataLoaderRegistryFactory = null;
if (this.dataLoaderRegistryFactory != null) {
dataLoaderRegistryFactory = this.dataLoaderRegistryFactory;
} else if (dataLoaderRegistryConsumers != null) {
final DataLoaderRegistry dataLoaderRegistry = new DataLoaderRegistry();
for (Consumer<? super DataLoaderRegistry> configurer : dataLoaderRegistryConsumers.build()) {
configurer.accept(dataLoaderRegistry);
}
dataLoaderRegistryFactory = ctx -> dataLoaderRegistry;
} else {
assert dataLoaderRegistryFactory == null && dataLoaderRegistryConsumers == null;
dataLoaderRegistryFactory = ctx -> new DataLoaderRegistry();
}

final GraphQL graphql = buildGraphql();
final Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> dataLoaderRegistryFactory =
buildDataLoaderRegistry();
final GraphqlErrorHandler errorHandler;
if (this.errorHandler == null) {
errorHandler = GraphqlErrorHandler.of();
} else {
errorHandler = this.errorHandler.orElse(GraphqlErrorHandler.of());
}
return new DefaultGraphqlService(builder.build(),
return new DefaultGraphqlService(graphql,
dataLoaderRegistryFactory,
useBlockingTaskExecutor,
errorHandler);
}

private GraphQLSchema buildSchema() {
final List<URL> schemaUrls = this.schemaUrls.build();
final List<RuntimeWiringConfigurator> runtimeWiringConfigurators =
this.runtimeWiringConfigurators.build();
final List<GraphQLTypeVisitor> typeVisitors = this.typeVisitors.build();
private GraphQL buildGraphql() {
if (graphql != null) {
return graphql;

Check warning on line 365 in graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java

View check run for this annotation

Codecov / codecov/patch

graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java#L365 

Added line #L365 was not covered by tests
}
final GraphQLSchema schema = buildSchema();
final ExecutionIdProvider executionProvider =
firstNonNull(executionIdGenerator, ExecutionIdGenerator.of()).asExecutionProvider();

final GraphQL.Builder builder = GraphQL.newGraphQL(schema)
.executionIdProvider(executionProvider);
if (instrumentations != null) {
final List<Instrumentation> instrumentations = this.instrumentations.build();
builder.instrumentation(new ChainedInstrumentation(instrumentations));
}
if (graphqlBuilderConsumers != null) {
final List<GraphqlConfigurator> graphqlBuilders = graphqlBuilderConsumers.build();
for (GraphqlConfigurator configurer : graphqlBuilders) {
configurer.configure(builder);
}
}
return builder.build();
}

private GraphQLSchema buildSchema() {
if (schema != null) {
checkState(schemaUrls.isEmpty() && runtimeWiringConfigurators.isEmpty() &&
typeVisitors.isEmpty(),
checkState(schemaUrls == null && runtimeWiringConfigurators == null &&
typeVisitors == null,
"Cannot add schemaUrl(or File), runtimeWiringConfigurator and " +
"typeVisitor when GraphqlSchema is specified.");
return schema;
}

final TypeDefinitionRegistry registry = typeDefinitionRegistry(schemaUrls);
final RuntimeWiring runtimeWiring = buildRuntimeWiring(runtimeWiringConfigurators);
final TypeDefinitionRegistry registry = typeDefinitionRegistry();
final RuntimeWiring runtimeWiring = buildRuntimeWiring();
GraphQLSchema schema = new SchemaGenerator().makeExecutableSchema(registry, runtimeWiring);
for (GraphQLTypeVisitor typeVisitor : typeVisitors) {
schema = SchemaTransformer.transformSchema(schema, typeVisitor);
if (typeVisitors != null) {
for (GraphQLTypeVisitor typeVisitor : typeVisitors.build()) {
schema = SchemaTransformer.transformSchema(schema, typeVisitor);
}
}
return schema;
}

private static TypeDefinitionRegistry typeDefinitionRegistry(List<URL> schemaUrls) {
private TypeDefinitionRegistry typeDefinitionRegistry() {
final TypeDefinitionRegistry registry = new TypeDefinitionRegistry();
final SchemaParser parser = new SchemaParser();
if (schemaUrls.isEmpty()) {
schemaUrls = defaultSchemaUrls();
}
if (schemaUrls.isEmpty()) {
throw new IllegalStateException("Not found schema file(s)");
}

logger.info("Found schema files: {}", schemaUrls);
schemaUrls.forEach(url -> {
final List<URL> schemaUrlList = schemaUrls != null ? schemaUrls.build() : defaultSchemaUrls();
schemaUrlList.forEach(url -> {
try (InputStream inputStream = url.openStream()) {
registry.merge(parser.parse(inputStream));
} catch (IOException e) {
Expand All @@ -375,19 +417,42 @@
return registry;
}

private static RuntimeWiring buildRuntimeWiring(
List<RuntimeWiringConfigurator> runtimeWiringConfigurators) {
private RuntimeWiring buildRuntimeWiring() {
final RuntimeWiring.Builder runtimeWiringBuilder = RuntimeWiring.newRuntimeWiring();
runtimeWiringConfigurators.forEach(it -> it.configure(runtimeWiringBuilder));
if (runtimeWiringConfigurators != null) {
runtimeWiringConfigurators.build().forEach(it -> it.configure(runtimeWiringBuilder));
}
return runtimeWiringBuilder.build();
}

private static List<URL> defaultSchemaUrls() {
final ClassLoader classLoader = GraphqlServiceBuilder.class.getClassLoader();
return DEFAULT_SCHEMA_FILE_NAMES
final List<URL> schemaFiles = DEFAULT_SCHEMA_FILE_NAMES
.stream()
.map(classLoader::getResource)
.filter(Objects::nonNull)
.collect(toImmutableList());

if (schemaFiles.isEmpty()) {
throw new IllegalStateException("Not found schema file(s)");
}
logger.info("Found schema files: {}", schemaFiles);
return schemaFiles;

Check warning on line 440 in graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java

View check run for this annotation

Codecov / codecov/patch

graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java#L439-L440 

Added lines #L439 - L440 were not covered by tests
}

private Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> buildDataLoaderRegistry() {
final Function<? super ServiceRequestContext, ? extends DataLoaderRegistry> dataLoaderRegistryFactory;
if (this.dataLoaderRegistryFactory != null) {
dataLoaderRegistryFactory = this.dataLoaderRegistryFactory;
} else if (dataLoaderRegistryConsumers != null) {
final DataLoaderRegistry dataLoaderRegistry = new DataLoaderRegistry();

Check warning on line 448 in graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java

View check run for this annotation

Codecov / codecov/patch

graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java#L448 

Added line #L448 was not covered by tests
for (Consumer<? super DataLoaderRegistry> configurer : dataLoaderRegistryConsumers.build()) {
configurer.accept(dataLoaderRegistry);
}
dataLoaderRegistryFactory = ctx -> dataLoaderRegistry;
} else {

Check warning on line 453 in graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java

View check run for this annotation

Codecov / codecov/patch

graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilder.java#L450-L453 

Added lines #L450 - L453 were not covered by tests
dataLoaderRegistryFactory = ctx -> new DataLoaderRegistry();
}
return dataLoaderRegistryFactory;
}
}
11 changes: 11 additions & 0 deletions graphql/src/test/java/com/linecorp/armeria/server/graphql/GraphqlServiceBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@

import com.google.common.collect.ImmutableList;

import graphql.GraphQL;
import graphql.GraphQL.Builder;
import graphql.execution.instrumentation.SimplePerformantInstrumentation;
import graphql.schema.GraphQLSchema;
import graphql.schema.GraphQLTypeVisitor;
Expand Down Expand Up @@ -166,4 +168,13 @@ void bothDataLoaderConfig() {
.isInstanceOf(IllegalStateException.class)
.hasMessage("configureDataLoaderRegistry() and dataLoaderRegistry() are mutually exclusive.");
}

@Test
void graphqlAndSchemaCannotSetTogether() throws URISyntaxException {
final GraphQLSchema schema = makeGraphQLSchema();
final GraphQL graphQL = new Builder(schema).build();
assertThatThrownBy(() -> GraphqlService.builder().graphql(graphQL).schema(schema))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("are mutually exclusive.");
}
}