Skip to content

Handle repeated query directives #3270

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

Merged
dondonz merged 1 commit into from
Jul 18, 2023
Merged

Handle repeated query directives #3270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 5 additions & 4 deletions src/main/java/graphql/execution/directives/DirectivesResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import graphql.schema.GraphQLDirective;
import graphql.schema.GraphQLSchema;

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
Expand All @@ -25,14 +26,14 @@ public class DirectivesResolver {
public DirectivesResolver() {
}

public Map<String, GraphQLDirective> resolveDirectives(List<Directive> directives, GraphQLSchema schema, Map<String, Object> variables, GraphQLContext graphQLContext, Locale locale) {
Copy link
Contributor Author

@gnawf gnawf Jul 17, 2023

Choose a reason for hiding this comment

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

I wonder if we should just return a List<GraphQLDirecitve>since we don't even use the map result. It's immediately discarded as we call Map.values()

public Map<String, List<GraphQLDirective>> resolveDirectives(List<Directive> directives, GraphQLSchema schema, Map<String, Object> variables, GraphQLContext graphQLContext, Locale locale) {
GraphQLCodeRegistry codeRegistry = schema.getCodeRegistry();
Map<String, GraphQLDirective> directiveMap = new LinkedHashMap<>();
Map<String, List<GraphQLDirective>> directiveMap = new LinkedHashMap<>();
directives.forEach(directive -> {
GraphQLDirective protoType = schema.getDirective(directive.getName());
if (protoType != null) {
GraphQLDirective newDirective = protoType.transform(builder -> buildArguments(builder, codeRegistry, protoType, directive, variables, graphQLContext, locale));
directiveMap.put(newDirective.getName(), newDirective);
Copy link
Contributor Author

@gnawf gnawf Jul 17, 2023

Choose a reason for hiding this comment

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

Previously this would just replace the last directive instance. Now it accumulates the values for repeated directives.

directiveMap.computeIfAbsent(newDirective.getName(), k -> new ArrayList<>()).add(newDirective);
}
});
return ImmutableMap.copyOf(directiveMap);
Expand Down Expand Up @@ -60,4 +61,4 @@ private void buildArguments(GraphQLDirective.Builder directiveBuilder,
}
});
}
}
}
7 changes: 4 additions & 3 deletions src/main/java/graphql/execution/directives/QueryDirectivesImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import graphql.schema.GraphQLArgument;
import graphql.schema.GraphQLDirective;
import graphql.schema.GraphQLSchema;
import graphql.util.FpKit;

import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand All @@ -22,7 +23,7 @@

/**
* These objects are ALWAYS in the context of a single MergedField
*
* <p>
* Also note we compute these values lazily
*/
@Internal
Expand Down Expand Up @@ -57,11 +58,11 @@ private void computeValuesLazily() {
final Map<Field, List<QueryAppliedDirective>> byFieldApplied = new LinkedHashMap<>();
mergedField.getFields().forEach(field -> {
List<Directive> directives = field.getDirectives();
ImmutableList<GraphQLDirective> resolvedDirectives = ImmutableList.copyOf(
ImmutableList<GraphQLDirective> resolvedDirectives = ImmutableList.copyOf(FpKit.flatList(
directivesResolver
.resolveDirectives(directives, schema, variables, graphQLContext, locale)
.values()
);
));
byField.put(field, resolvedDirectives);
// at some point we will only use applied
byFieldApplied.put(field, ImmutableKit.map(resolvedDirectives, this::toAppliedDirective));
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/util/FpKit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ public static <T> CompletableFuture<List<T>> flatList(CompletableFuture<List<Lis
return cf.thenApply(FpKit::flatList);
}

public static <T> List<T> flatList(List<List<T>> listLists) {
public static <T> List<T> flatList(Collection<List<T>> listLists) {
Copy link
Contributor Author

@gnawf gnawf Jul 17, 2023

Choose a reason for hiding this comment

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

Needed as Map.values() returns a Collection (Maps aren't ordered).

return listLists.stream()
.flatMap(List::stream)
.collect(ImmutableList.toImmutableList());
Expand Down
27 changes: 24 additions & 3 deletions src/test/groovy/graphql/execution/directives/QueryDirectivesImplTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ class QueryDirectivesImplTest extends Specification {
directive @cached(forMillis : Int = 99) on FIELD | QUERY

directive @upper(place : String) on FIELD

directive @rep(place : String) repeatable on FIELD

type Query {
f : String
Expand All @@ -24,9 +26,7 @@ class QueryDirectivesImplTest extends Specification {

def schema = TestUtil.schema(sdl)


def "can get immediate directives"() {

def f1 = TestUtil.parseField("f1 @cached @upper")
def f2 = TestUtil.parseField("f2 @cached(forMillis : \$var) @timeout")

Expand Down Expand Up @@ -68,7 +68,6 @@ class QueryDirectivesImplTest extends Specification {
}

def "builder works as expected"() {

def f1 = TestUtil.parseField("f1 @cached @upper")
def f2 = TestUtil.parseField("f2 @cached(forMillis : \$var) @timeout")

Expand All @@ -87,6 +86,28 @@ class QueryDirectivesImplTest extends Specification {

then:
appliedDirectivesByName.keySet().sort() == ["cached", "timeout", "upper"]
}

def "gets repeated definitions"() {
def f1 = TestUtil.parseField("f1 @rep(place: \$var) @rep(place: \"HELLO\")")

def mergedField = MergedField.newMergedField([f1]).build()

def queryDirectives = QueryDirectives.newQueryDirectives()
.mergedField(mergedField)
.schema(schema)
.coercedVariables(CoercedVariables.of([var: "ABC"]))
.graphQLContext(GraphQLContext.getDefault())
.locale(Locale.getDefault())
.build()

when:
def appliedDirectivesByName = queryDirectives.getImmediateAppliedDirectivesByName()

then:
appliedDirectivesByName.keySet() == ["rep"] as Set
appliedDirectivesByName["rep"].size() == 2
// Groovy is a pathway to many abilities some consider to be unnatural
Copy link
Member

@bbakerman bbakerman Jul 17, 2023

Choose a reason for hiding this comment

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

Groovy is a pathway to many abilities some consider to be unnatural
Lol

Copy link
Contributor

@russellyou russellyou Jul 17, 2023

Choose a reason for hiding this comment

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

My favourite is groovy will ignore all 'private' modifier. I am not selfish. I can even share other's private with you.

Copy link
Member

@dondonz dondonz Jul 17, 2023

Choose a reason for hiding this comment

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

Lol you put this in to verify that I read all the way to the end, right?

appliedDirectivesByName["rep"].arguments.value.flatten().sort() == ["ABC", "HELLO"]
}
}