Skip to content

Commit f686212

Browse files
yarinvakyarinvak
committed
fix code review notes
1 parent c9f9e5f commit f686212

File tree

11 files changed

+173
-140
lines changed

11 files changed

+173
-140
lines changed

‎src/main/java/graphql/annotations/directives/AnnotationsDirectiveWiring.java‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ public interface AnnotationsDirectiveWiring {
2525
*
2626
* @return a non null element based on the original one
2727
*/
28-
default GraphQLObjectType onObject(AnnotationsWiringEnvironment<GraphQLObjectType> environment) {
29-
return environment.getElement();
28+
default GraphQLObjectType onObject(AnnotationsWiringEnvironment environment) {
29+
return (GraphQLObjectType) environment.getElement();
3030
}
3131

3232
/**
@@ -37,8 +37,8 @@ default GraphQLObjectType onObject(AnnotationsWiringEnvironment<GraphQLObjectTyp
3737
*
3838
* @return a non null element based on the original one
3939
*/
40-
default GraphQLFieldDefinition onField(AnnotationsWiringEnvironment<GraphQLFieldDefinition> environment) {
41-
return environment.getElement();
40+
default GraphQLFieldDefinition onField(AnnotationsWiringEnvironment environment) {
41+
return (GraphQLFieldDefinition) environment.getElement();
4242
}
4343

4444
/**
@@ -49,8 +49,8 @@ default GraphQLFieldDefinition onField(AnnotationsWiringEnvironment<GraphQLField
4949
*
5050
* @return a non null element based on the original one
5151
*/
52-
default GraphQLArgument onArgument(AnnotationsWiringEnvironment<GraphQLArgument> environment) {
53-
return environment.getElement();
52+
default GraphQLArgument onArgument(AnnotationsWiringEnvironment environment) {
53+
return (GraphQLArgument) environment.getElement();
5454
}
5555

5656
/**
@@ -61,8 +61,8 @@ default GraphQLArgument onArgument(AnnotationsWiringEnvironment<GraphQLArgument>
6161
*
6262
* @return a non null element based on the original one
6363
*/
64-
default GraphQLInterfaceType onInterface(AnnotationsWiringEnvironment<GraphQLInterfaceType> environment) {
65-
return environment.getElement();
64+
default GraphQLInterfaceType onInterface(AnnotationsWiringEnvironment environment) {
65+
return (GraphQLInterfaceType) environment.getElement();
6666
}
6767

6868
/**
@@ -73,8 +73,8 @@ default GraphQLInterfaceType onInterface(AnnotationsWiringEnvironment<GraphQLInt
7373
*
7474
* @return a non null element based on the original one
7575
*/
76-
default GraphQLUnionType onUnion(AnnotationsWiringEnvironment<GraphQLUnionType> environment) {
77-
return environment.getElement();
76+
default GraphQLUnionType onUnion(AnnotationsWiringEnvironment environment) {
77+
return (GraphQLUnionType) environment.getElement();
7878
}
7979

8080
/**
@@ -85,8 +85,8 @@ default GraphQLUnionType onUnion(AnnotationsWiringEnvironment<GraphQLUnionType>
8585
*
8686
* @return a non null element based on the original one
8787
*/
88-
default GraphQLEnumType onEnum(AnnotationsWiringEnvironment<GraphQLEnumType> environment) {
89-
return environment.getElement();
88+
default GraphQLEnumType onEnum(AnnotationsWiringEnvironment environment) {
89+
return (GraphQLEnumType) environment.getElement();
9090
}
9191

9292
/**
@@ -97,8 +97,8 @@ default GraphQLEnumType onEnum(AnnotationsWiringEnvironment<GraphQLEnumType> env
9797
*
9898
* @return a non null element based on the original one
9999
*/
100-
default GraphQLEnumValueDefinition onEnumValue(AnnotationsWiringEnvironment<GraphQLEnumValueDefinition> environment) {
101-
return environment.getElement();
100+
default GraphQLEnumValueDefinition onEnumValue(AnnotationsWiringEnvironment environment) {
101+
return (GraphQLEnumValueDefinition) environment.getElement();
102102
}
103103

104104
/**
@@ -109,8 +109,8 @@ default GraphQLEnumValueDefinition onEnumValue(AnnotationsWiringEnvironment<Grap
109109
*
110110
* @return a non null element based on the original one
111111
*/
112-
default GraphQLScalarType onScalar(AnnotationsWiringEnvironment<GraphQLScalarType> environment) {
113-
return environment.getElement();
112+
default GraphQLScalarType onScalar(AnnotationsWiringEnvironment environment) {
113+
return (GraphQLScalarType) environment.getElement();
114114
}
115115

116116
/**
@@ -121,8 +121,8 @@ default GraphQLScalarType onScalar(AnnotationsWiringEnvironment<GraphQLScalarTyp
121121
*
122122
* @return a non null element based on the original one
123123
*/
124-
default GraphQLInputObjectType onInputObjectType(AnnotationsWiringEnvironment<GraphQLInputObjectType> environment) {
125-
return environment.getElement();
124+
default GraphQLInputObjectType onInputObjectType(AnnotationsWiringEnvironment environment) {
125+
return (GraphQLInputObjectType) environment.getElement();
126126
}
127127

128128
/**
@@ -133,7 +133,7 @@ default GraphQLInputObjectType onInputObjectType(AnnotationsWiringEnvironment<Gr
133133
*
134134
* @return a non null element based on the original one
135135
*/
136-
default GraphQLInputObjectField onInputObjectField(AnnotationsWiringEnvironment<GraphQLInputObjectField> environment) {
137-
return environment.getElement();
136+
default GraphQLInputObjectField onInputObjectField(AnnotationsWiringEnvironment environment) {
137+
return (GraphQLInputObjectField) environment.getElement();
138138
}
139139
}

‎src/main/java/graphql/annotations/directives/AnnotationsWiringEnvironment.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,13 @@
1515
package graphql.annotations.directives;
1616

1717
import graphql.schema.GraphQLDirective;
18+
import graphql.schema.GraphQLDirectiveContainer;
1819

19-
public interface AnnotationsWiringEnvironment<T> {
20+
public interface AnnotationsWiringEnvironment {
2021
/**
2122
* @return the runtime element in play
2223
*/
23-
T getElement();
24+
GraphQLDirectiveContainer getElement();
2425

2526
/**
2627
* @return the directive that is being examined

‎src/main/java/graphql/annotations/directives/AnnotationsWiringEnvironmentImpl.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,17 @@
1717
import graphql.schema.GraphQLDirective;
1818
import graphql.schema.GraphQLDirectiveContainer;
1919

20-
public class AnnotationsWiringEnvironmentImpl<T extends GraphQLDirectiveContainer> implements AnnotationsWiringEnvironment<T> {
21-
private final T element;
20+
public class AnnotationsWiringEnvironmentImpl implements AnnotationsWiringEnvironment {
21+
private final GraphQLDirectiveContainer element;
2222
private final GraphQLDirective directive;
2323

24-
public AnnotationsWiringEnvironmentImpl(T element, GraphQLDirective directive) {
24+
public AnnotationsWiringEnvironmentImpl(GraphQLDirectiveContainer element, GraphQLDirective directive) {
2525
this.element = element;
2626
this.directive = directive;
2727
}
2828

2929
@Override
30-
public T getElement() {
30+
public GraphQLDirectiveContainer getElement() {
3131
return element;
3232
}
3333

@@ -41,7 +41,7 @@ public boolean equals(Object o) {
4141
if (this == o) return true;
4242
if (o == null || getClass() != o.getClass()) return false;
4343

44-
AnnotationsWiringEnvironmentImpl<?> that = (AnnotationsWiringEnvironmentImpl<?>) o;
44+
AnnotationsWiringEnvironmentImpl that = (AnnotationsWiringEnvironmentImpl) o;
4545

4646
if (element != null ? !element.equals(that.element) : that.element != null) return false;
4747
return directive != null ? directive.equals(that.directive) : that.directive == null;

‎src/main/java/graphql/annotations/directives/DirectiveWirer.java‎

Lines changed: 45 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -14,63 +14,67 @@
1414
*/
1515
package graphql.annotations.directives;
1616

17+
import graphql.annotations.processor.exceptions.GraphQLAnnotationsException;
1718
import graphql.introspection.Introspection;
1819
import graphql.schema.*;
1920

21+
import java.lang.reflect.InvocationTargetException;
2022
import java.util.Arrays;
2123
import java.util.HashMap;
2224
import java.util.Map;
2325

2426
public class DirectiveWirer {
27+
@FunctionalInterface
28+
interface WiringFunction {
29+
GraphQLDirectiveContainer apply(GraphQLDirective a, GraphQLDirectiveContainer b, AnnotationsDirectiveWiring wiring) throws NoSuchMethodException, InvocationTargetException, IllegalAccessException;
30+
}
31+
32+
private Map<Class, WiringFunction> functionMap;
33+
34+
public DirectiveWirer() {
35+
functionMap = createFunctionsMap();
36+
}
37+
38+
private void putInMap(Map<Class, WiringFunction> map, Class clazz, String functionName,
39+
Introspection.DirectiveLocation... locations) {
40+
map.put(clazz, (d, e, wiring) -> {
41+
assertLocation(d, e, locations);
42+
AnnotationsWiringEnvironmentImpl environment =
43+
new AnnotationsWiringEnvironmentImpl(e, e.getDirective(d.getName()));
44+
return (GraphQLDirectiveContainer) wiring.getClass().getMethod(functionName, AnnotationsWiringEnvironment.class).invoke(wiring, environment);
45+
});
46+
}
47+
48+
private Map<Class, WiringFunction> createFunctionsMap() {
49+
Map<Class, WiringFunction> functionMap = new HashMap<>();
50+
putInMap(functionMap, GraphQLFieldDefinition.class, "onField", Introspection.DirectiveLocation.FIELD, Introspection.DirectiveLocation.FIELD_DEFINITION);
51+
putInMap(functionMap, GraphQLObjectType.class, "onObject", Introspection.DirectiveLocation.OBJECT);
52+
putInMap(functionMap, GraphQLArgument.class, "onArgument", Introspection.DirectiveLocation.ARGUMENT_DEFINITION);
53+
putInMap(functionMap, GraphQLInterfaceType.class, "onInterface", Introspection.DirectiveLocation.INTERFACE);
54+
putInMap(functionMap, GraphQLUnionType.class, "onUnion", Introspection.DirectiveLocation.UNION);
55+
putInMap(functionMap, GraphQLEnumType.class, "onEnum", Introspection.DirectiveLocation.ENUM);
56+
putInMap(functionMap, GraphQLEnumValueDefinition.class, "onEnumValue", Introspection.DirectiveLocation.ENUM_VALUE);
57+
putInMap(functionMap, GraphQLScalarType.class, "onScalar", Introspection.DirectiveLocation.SCALAR);
58+
putInMap(functionMap, GraphQLInputObjectType.class, "onInputObjectType", Introspection.DirectiveLocation.INPUT_OBJECT);
59+
putInMap(functionMap, GraphQLInputObjectField.class, "onInputObjectField", Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION);
60+
61+
return functionMap;
62+
}
63+
2564
public GraphQLDirectiveContainer wire(GraphQLDirectiveContainer element, HashMap<GraphQLDirective, AnnotationsDirectiveWiring> directiveWiringMap) {
2665
for (Map.Entry<GraphQLDirective, AnnotationsDirectiveWiring> entry : directiveWiringMap.entrySet()) {
27-
System.out.println(entry.getKey());
2866
GraphQLDirective graphQLDirective = entry.getKey();
2967
AnnotationsDirectiveWiring wiring = entry.getValue();
3068

31-
if (element instanceof GraphQLFieldDefinition) {
32-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.FIELD, Introspection.DirectiveLocation.FIELD_DEFINITION);
33-
element = wiring.onField(new AnnotationsWiringEnvironmentImpl<>((GraphQLFieldDefinition) element, element.getDirective(graphQLDirective.getName())));
34-
} else if (element instanceof GraphQLObjectType) {
35-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.OBJECT);
36-
element = wiring
37-
.onObject(new AnnotationsWiringEnvironmentImpl<>((GraphQLObjectType) element, element.getDirective(graphQLDirective.getName())));
38-
} else if (element instanceof GraphQLArgument) {
39-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.ARGUMENT_DEFINITION);
40-
element = wiring
41-
.onArgument(new AnnotationsWiringEnvironmentImpl<>((GraphQLArgument) element, element.getDirective(graphQLDirective.getName())));
42-
} else if (element instanceof GraphQLInterfaceType) {
43-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.INTERFACE);
44-
element = wiring
45-
.onInterface(new AnnotationsWiringEnvironmentImpl<>((GraphQLInterfaceType) element, element.getDirective(graphQLDirective.getName())));
46-
} else if (element instanceof GraphQLUnionType) {
47-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.UNION);
48-
element = wiring
49-
.onUnion(new AnnotationsWiringEnvironmentImpl<>((GraphQLUnionType) element, element.getDirective(graphQLDirective.getName())));
50-
} else if (element instanceof GraphQLEnumType) { // todo support enum
51-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.ENUM);
52-
element = wiring
53-
.onEnum(new AnnotationsWiringEnvironmentImpl<>((GraphQLEnumType) element, element.getDirective(graphQLDirective.getName())));
54-
} else if (element instanceof GraphQLEnumValueDefinition) { // todo support enum value
55-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.ENUM_VALUE);
56-
element = wiring
57-
.onEnumValue(new AnnotationsWiringEnvironmentImpl<>((GraphQLEnumValueDefinition) element, element.getDirective(graphQLDirective.getName())));
58-
} else if (element instanceof GraphQLScalarType) { // todo: support scalars
59-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.SCALAR);
60-
element = wiring
61-
.onScalar(new AnnotationsWiringEnvironmentImpl<>((GraphQLScalarType) element, element.getDirective(graphQLDirective.getName())));
62-
} else if (element instanceof GraphQLInputObjectType) {
63-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.INPUT_OBJECT);
64-
element = wiring
65-
.onInputObjectType(new AnnotationsWiringEnvironmentImpl<>((GraphQLInputObjectType) element, element.getDirective(graphQLDirective.getName())));
66-
} else if (element instanceof GraphQLInputObjectField) {
67-
assertLocation(graphQLDirective, element, Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION);
68-
element = wiring
69-
.onInputObjectField(new AnnotationsWiringEnvironmentImpl<>((GraphQLInputObjectField) element, element.getDirective(graphQLDirective.getName())));
69+
Class<? extends GraphQLDirectiveContainer> aClass = element.getClass();
70+
try {
71+
element = functionMap.get(aClass).apply(graphQLDirective, element, wiring);
72+
} catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
73+
throw new GraphQLAnnotationsException(e.getMessage(), e);
7074
}
7175
}
72-
return element;
7376

77+
return element;
7478
}
7579

7680
private void assertLocation(GraphQLDirective graphQLDirective, GraphQLDirectiveContainer element, Introspection.DirectiveLocation... validLocations) {

‎src/main/java/graphql/annotations/directives/DirectiveWiringMapRetriever.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ public HashMap<GraphQLDirective, AnnotationsDirectiveWiring> getDirectiveWiringM
3939
throw new GraphQLAnnotationsException("Cannot create an instance of the wiring class " + x.wiringClass().getSimpleName(), e);
4040
}
4141
});
42-
System.out.println(map);
4342
return map;
4443
}
4544

‎src/main/java/graphql/annotations/processor/exceptions/GraphQLAnnotationsException.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,7 @@ public GraphQLAnnotationsException(String message, Throwable cause) {
2222
super(message, cause);
2323
}
2424

25+
public GraphQLAnnotationsException(Throwable cause) {
26+
super(cause);
27+
}
2528
}

‎src/main/java/graphql/annotations/processor/retrievers/fieldBuilders/DirectivesBuilder.java‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -48,23 +48,7 @@ private GraphQLDirective transformArgs(GraphQLDirective graphQLDirective, String
4848
}
4949

5050
for (int i = 0; i < argumentValues.length; i++) {
51-
int finalI = i;
52-
GraphQLArgument graphQLArgument = arguments.get(i);
53-
54-
directiveBuilder.argument(graphQLArgument.transform(builder -> {
55-
String argumentValue = argumentValues[finalI];
56-
if (graphQLArgument.getType() instanceof GraphQLScalarType) {
57-
58-
try {
59-
Object value = ((GraphQLScalarType) graphQLArgument.getType()).getCoercing().parseValue(argumentValue);
60-
builder.value(value);
61-
} catch (Exception e) {
62-
throw new GraphQLAnnotationsException("Could not parse argument value to argument type", e);
63-
}
64-
} else {
65-
throw new GraphQLAnnotationsException("Directive argument type must be a scalar!", null);
66-
}
67-
}));
51+
transformArgument(argumentValues, directiveBuilder, arguments, i);
6852
}
6953

7054
for (int i = argumentValues.length; i < arguments.size(); i++) {
@@ -74,6 +58,26 @@ private GraphQLDirective transformArgs(GraphQLDirective graphQLDirective, String
7458
return directiveBuilder.build();
7559
}
7660

61+
private void transformArgument(String[] argumentValues, GraphQLDirective.Builder directiveBuilder, List<GraphQLArgument> arguments, int i) {
62+
int finalI = i;
63+
GraphQLArgument graphQLArgument = arguments.get(i);
64+
65+
directiveBuilder.argument(graphQLArgument.transform(builder -> {
66+
String argumentValue = argumentValues[finalI];
67+
if (graphQLArgument.getType() instanceof GraphQLScalarType) {
68+
69+
try {
70+
Object value = ((GraphQLScalarType) graphQLArgument.getType()).getCoercing().parseValue(argumentValue);
71+
builder.value(value);
72+
} catch (Exception e) {
73+
throw new GraphQLAnnotationsException("Could not parse argument value to argument type", e);
74+
}
75+
} else {
76+
throw new GraphQLAnnotationsException("Directive argument type must be a scalar!", null);
77+
}
78+
}));
79+
}
80+
7781
@Override
7882
public GraphQLDirective[] build() {
7983
GraphQLDirectives directives = object.getAnnotation(GraphQLDirectives.class);

‎src/test/java/graphql/annotations/GraphQLDirectivesTest.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ public void init() {
6767

6868
public static class UpperWiring implements AnnotationsDirectiveWiring {
6969
@Override
70-
public GraphQLFieldDefinition onField(AnnotationsWiringEnvironment<GraphQLFieldDefinition> environment) {
71-
GraphQLFieldDefinition field = environment.getElement();
70+
public GraphQLFieldDefinition onField(AnnotationsWiringEnvironment environment) {
71+
GraphQLFieldDefinition field = (GraphQLFieldDefinition) environment.getElement();
7272
boolean isActive = (boolean) environment.getDirective().getArgument("isActive").getValue();
7373
DataFetcher dataFetcher = DataFetcherFactories.wrapDataFetcher(field.getDataFetcher(), (((dataFetchingEnvironment, value) -> {
7474
if (value instanceof String && isActive) {
@@ -82,8 +82,8 @@ public GraphQLFieldDefinition onField(AnnotationsWiringEnvironment<GraphQLFieldD
8282

8383
public static class SuffixWiring implements AnnotationsDirectiveWiring {
8484
@Override
85-
public GraphQLFieldDefinition onField(AnnotationsWiringEnvironment<GraphQLFieldDefinition> environment) {
86-
GraphQLFieldDefinition field = environment.getElement();
85+
public GraphQLFieldDefinition onField(AnnotationsWiringEnvironment environment) {
86+
GraphQLFieldDefinition field = (GraphQLFieldDefinition) environment.getElement();
8787
String suffix = (String) environment.getDirective().getArgument("suffix").getValue();
8888
DataFetcher dataFetcher = DataFetcherFactories.wrapDataFetcher(field.getDataFetcher(), (((dataFetchingEnvironment, value) -> {
8989
if (value instanceof String) {
@@ -171,7 +171,7 @@ public void queryNameWithNoArgs_directivesProvidedToRegistry_wiringIsActivated()
171171
assertEquals(((Map<String, String>) result.getData()).get("nameWithNoArgs").toString(), "YARIN");
172172
}
173173

174-
@Test(expectedExceptions = NullPointerException.class)
174+
@Test(expectedExceptions = GraphQLAnnotationsException.class)
175175
public void queryNameWithNoArgs_noDefaultValue_exceptionIsThrown() throws Exception {
176176
GraphQLDirective upperCase = newDirective().name("upperCase").argument(builder -> builder.name("isActive").type(GraphQLBoolean))
177177
.validLocations(Introspection.DirectiveLocation.FIELD_DEFINITION).build();

0 commit comments

Comments
 (0)