From c1730dc3dd1d25fdc75342d088a41165259c730d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20da=20Silva=20Br=C3=B6ker?= Date: Sat, 3 Apr 2021 01:51:10 +0200 Subject: [PATCH 1/4] Set method and property names correctly for public fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MethodName should be null (since no method is used) and fieldName should just be the fields name, without removing an get/is/set prefix. Could be used later to distinguish between direct field access an access by method. Signed-off-by: Yannick da Silva Bröker --- .../java/io/smallrye/graphql/schema/creator/FieldCreator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/schema-builder/src/main/java/io/smallrye/graphql/schema/creator/FieldCreator.java b/common/schema-builder/src/main/java/io/smallrye/graphql/schema/creator/FieldCreator.java index bae9fd856..acd69b738 100644 --- a/common/schema-builder/src/main/java/io/smallrye/graphql/schema/creator/FieldCreator.java +++ b/common/schema-builder/src/main/java/io/smallrye/graphql/schema/creator/FieldCreator.java @@ -166,8 +166,8 @@ public Optional createFieldForPojo(Direction direction, FieldInfo fieldIn Reference reference = referenceCreator.createReferenceForPojoField(direction, fieldType, fieldType, annotationsForPojo, parentObjectReference); - Field field = new Field(fieldInfo.name(), - MethodHelper.getPropertyName(direction, fieldInfo.name()), + Field field = new Field(null, + fieldInfo.name(), name, maybeDescription.orElse(null), reference); From fa27380387f833ee614dcc91243e8df63b390642 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20da=20Silva=20Br=C3=B6ker?= Date: Sun, 4 Apr 2021 00:35:58 +0200 Subject: [PATCH 2/4] Add new FieldDataFetcher to replace PropertyDataFetcher --- .../datafetcher/FieldDataFetcher.java | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/FieldDataFetcher.java diff --git a/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/FieldDataFetcher.java b/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/FieldDataFetcher.java new file mode 100644 index 000000000..06c7fe3e5 --- /dev/null +++ b/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/FieldDataFetcher.java @@ -0,0 +1,110 @@ +package io.smallrye.graphql.execution.datafetcher; + +import static io.smallrye.graphql.SmallRyeGraphQLServerLogging.log; + +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; + +import graphql.GraphQLContext; +import graphql.GraphQLException; +import graphql.TrivialDataFetcher; +import graphql.schema.DataFetcher; +import graphql.schema.DataFetchingEnvironment; +import io.smallrye.graphql.execution.context.SmallRyeContext; +import io.smallrye.graphql.execution.datafetcher.helper.FieldHelper; +import io.smallrye.graphql.schema.model.Field; +import io.smallrye.graphql.schema.model.Reference; +import io.smallrye.graphql.spi.ClassloadingService; +import io.smallrye.graphql.transformation.AbstractDataFetcherException; + +/** + * Custom property data fetcher to allow arbitrary method names (instead of getters/setters) and to + * intercept the result for some manipulation. + *

+ * + * @implNote method handles are used to get values,instead of plain reflection. + * This allows identical access to fields and methods. + * If the (graphql-) field has no methodName, a method handle for the (java-) field is is created. + * Otherwise, a method handle is created for the accessor method. + *

+ * The owner is used to create the method handle independently of the source object (which could be a different + * subtype of the owner class for each call). + */ +public class FieldDataFetcher implements DataFetcher, TrivialDataFetcher { + + private final FieldHelper fieldHelper; + + private final Field field; + + /** + * Owner of the field. + */ + private final Reference owner; + + /** + * MethodHandle to access the (java-) field or method. + */ + private MethodHandle methodHandle; + + public FieldDataFetcher(final Field field, final Reference owner) { + this.fieldHelper = new FieldHelper(field); + this.field = field; + this.owner = owner; + } + + @Override + public T get(DataFetchingEnvironment dfe) throws Exception { + GraphQLContext graphQLContext = dfe.getContext(); + graphQLContext.put("context", ((SmallRyeContext) graphQLContext.get("context")).withDataFromFetcher(dfe, field)); + + if (this.methodHandle == null) { + // lazy initialize method handle, does not have to be threadsafe + this.methodHandle = buildMethodHandle(); + } + + final Object resultFromMethodCall; + try { + Object source = dfe.getSource(); + resultFromMethodCall = methodHandle.invoke(source); + } catch (Throwable throwable) { + throw new GraphQLException(throwable); + } + try { + // See if we need to transform + @SuppressWarnings("unchecked") + T transformResponse = (T) fieldHelper.transformResponse(resultFromMethodCall); + return transformResponse; + } catch (AbstractDataFetcherException ex) { + log.transformError(ex); + @SuppressWarnings("unchecked") + T result = (T) resultFromMethodCall; + //TODO: if transformResponse was necessary but failed, + // resultFromMethodCall would most likely have the wrong type, + // so this would just produce another error. Better just throw GraphQLException? + return result; + } + } + + /** + * Creates an method handle for the (graphql-) field, using the (java-) field or method. + *

+ * Accessibility (e.g. {@link java.lang.reflect.Field#canAccess(Object)}}) and types (return tapes and parameters) aren't + * checked, + * since the schema builder should only allow public fields and methods. + * + * @return a method handle to access this property + */ + private MethodHandle buildMethodHandle() { + try { + final Class aClass = ClassloadingService.get().loadClass(owner.getClassName()); + final MethodHandles.Lookup lookup = MethodHandles.lookup(); + if (this.field.getMethodName() == null) { + return lookup.unreflectGetter(aClass.getField(this.field.getPropertyName())); + } + return lookup.unreflect(aClass.getMethod(this.field.getMethodName())); + } catch (IllegalAccessException | NoSuchFieldException | NoSuchMethodException e) { + throw new GraphQLException(e); + } + } + +} From 643c3384ccda7cb4a3ab7be81a959c9944997a40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20da=20Silva=20Br=C3=B6ker?= Date: Sun, 4 Apr 2021 00:37:12 +0200 Subject: [PATCH 3/4] Remove PropertyDataFetcher --- .../smallrye/graphql/bootstrap/Bootstrap.java | 18 ++++---- .../datafetcher/PropertyDataFetcher.java | 42 ------------------- 2 files changed, 8 insertions(+), 52 deletions(-) delete mode 100644 server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/PropertyDataFetcher.java diff --git a/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java b/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java index f17fb7c29..0526474d2 100644 --- a/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java +++ b/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java @@ -42,9 +42,7 @@ import graphql.schema.visibility.GraphqlFieldVisibility; import io.smallrye.graphql.execution.Classes; import io.smallrye.graphql.execution.context.SmallRyeContext; -import io.smallrye.graphql.execution.datafetcher.BatchDataFetcher; -import io.smallrye.graphql.execution.datafetcher.CollectionCreator; -import io.smallrye.graphql.execution.datafetcher.PropertyDataFetcher; +import io.smallrye.graphql.execution.datafetcher.*; import io.smallrye.graphql.execution.error.ErrorInfoMap; import io.smallrye.graphql.execution.event.EventEmitter; import io.smallrye.graphql.execution.resolver.InterfaceOutputRegistry; @@ -275,7 +273,7 @@ private void createGraphQLInterfaceType(InterfaceType interfaceType) { // Fields if (interfaceType.hasFields()) { interfaceTypeBuilder = interfaceTypeBuilder - .fields(createGraphQLFieldDefinitionsFromFields(interfaceType.getName(), + .fields(createGraphQLFieldDefinitionsFromFields(interfaceType, interfaceType.getFields().values())); } @@ -334,7 +332,7 @@ private void createGraphQLObjectType(Type type) { // Fields if (type.hasFields()) { objectTypeBuilder = objectTypeBuilder - .fields(createGraphQLFieldDefinitionsFromFields(type.getName(), type.getFields().values())); + .fields(createGraphQLFieldDefinitionsFromFields(type, type.getFields().values())); } // Operations @@ -431,15 +429,15 @@ private GraphQLFieldDefinition createGraphQLFieldDefinitionFromOperation(String return graphQLFieldDefinition; } - private List createGraphQLFieldDefinitionsFromFields(String ownerName, Collection fields) { + private List createGraphQLFieldDefinitionsFromFields(Reference owner, Collection fields) { List graphQLFieldDefinitions = new ArrayList<>(); for (Field field : fields) { - graphQLFieldDefinitions.add(createGraphQLFieldDefinitionFromField(ownerName, field)); + graphQLFieldDefinitions.add(createGraphQLFieldDefinitionFromField(owner, field)); } return graphQLFieldDefinitions; } - private GraphQLFieldDefinition createGraphQLFieldDefinitionFromField(String ownerName, Field field) { + private GraphQLFieldDefinition createGraphQLFieldDefinitionFromField(Reference owner, Field field) { GraphQLFieldDefinition.Builder fieldBuilder = GraphQLFieldDefinition.newFieldDefinition() .name(field.getName()) .description(field.getDescription()); @@ -450,8 +448,8 @@ private GraphQLFieldDefinition createGraphQLFieldDefinitionFromField(String owne GraphQLFieldDefinition graphQLFieldDefinition = fieldBuilder.build(); // DataFetcher - PropertyDataFetcher datafetcher = new PropertyDataFetcher(field); - this.codeRegistryBuilder.dataFetcher(FieldCoordinates.coordinates(ownerName, graphQLFieldDefinition.getName()), + FieldDataFetcher datafetcher = new FieldDataFetcher<>(field, owner); + this.codeRegistryBuilder.dataFetcher(FieldCoordinates.coordinates(owner.getName(), graphQLFieldDefinition.getName()), datafetcher); return graphQLFieldDefinition; diff --git a/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/PropertyDataFetcher.java b/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/PropertyDataFetcher.java deleted file mode 100644 index 6cb95ac86..000000000 --- a/server/implementation/src/main/java/io/smallrye/graphql/execution/datafetcher/PropertyDataFetcher.java +++ /dev/null @@ -1,42 +0,0 @@ -package io.smallrye.graphql.execution.datafetcher; - -import static io.smallrye.graphql.SmallRyeGraphQLServerLogging.log; - -import graphql.GraphQLContext; -import graphql.schema.DataFetchingEnvironment; -import io.smallrye.graphql.execution.context.SmallRyeContext; -import io.smallrye.graphql.execution.datafetcher.helper.FieldHelper; -import io.smallrye.graphql.schema.model.Field; -import io.smallrye.graphql.transformation.AbstractDataFetcherException; - -/** - * Extending the default property data fetcher to intercept the result for some manipulation - * - * @author Phillip Kruger (phillip.kruger@redhat.com) - */ -public class PropertyDataFetcher extends graphql.schema.PropertyDataFetcher { - - private final FieldHelper fieldHelper; - private final Field field; - - public PropertyDataFetcher(Field field) { - super(field.getPropertyName()); - this.field = field; - this.fieldHelper = new FieldHelper(field); - } - - @Override - public Object get(DataFetchingEnvironment dfe) { - GraphQLContext graphQLContext = dfe.getContext(); - graphQLContext.put("context", ((SmallRyeContext) graphQLContext.get("context")).withDataFromFetcher(dfe, field)); - - Object resultFromMethodCall = super.get(dfe); - try { - // See if we need to transform - return fieldHelper.transformResponse(resultFromMethodCall); - } catch (AbstractDataFetcherException ex) { - log.transformError(ex); - return resultFromMethodCall; - } - } -} From b25f7ded388e7cf09e98f24dce07e0919512ddf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yannick=20da=20Silva=20Br=C3=B6ker?= Date: Sun, 4 Apr 2021 01:56:29 +0200 Subject: [PATCH 4/4] Allow usage of `@Query` as an alternative to Getters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Yannick da Silva Bröker --- .../graphql/schema/helper/MethodHelper.java | 16 ++++++++++++---- .../fieldexistence/api/FieldExistencePojo.java | 7 +++++++ .../test/resources/tests/fieldExistenceTests.csv | 1 + .../test/resources/tests/fields/input.graphql | 5 +++++ .../src/test/resources/tests/fields/output.json | 7 +++++++ .../test/resources/tests/fields/test.properties | 2 ++ 6 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 server/tck/src/test/resources/tests/fields/input.graphql create mode 100644 server/tck/src/test/resources/tests/fields/output.json create mode 100644 server/tck/src/test/resources/tests/fields/test.properties diff --git a/common/schema-builder/src/main/java/io/smallrye/graphql/schema/helper/MethodHelper.java b/common/schema-builder/src/main/java/io/smallrye/graphql/schema/helper/MethodHelper.java index 27fa6d63a..3b0311b60 100644 --- a/common/schema-builder/src/main/java/io/smallrye/graphql/schema/helper/MethodHelper.java +++ b/common/schema-builder/src/main/java/io/smallrye/graphql/schema/helper/MethodHelper.java @@ -3,6 +3,8 @@ import org.jboss.jandex.MethodInfo; import org.jboss.jandex.Type; +import io.smallrye.graphql.schema.Annotations; + /** * Helping with method operations. * @@ -60,7 +62,7 @@ public static boolean isPropertyMethod(Direction direction, MethodInfo method) { if (direction.equals(Direction.IN)) { return isSetter(method); } else if (direction.equals(Direction.OUT)) { - return isGetter(method); + return isPropertyAccessor(method); } return false; } @@ -87,16 +89,22 @@ private static boolean isSetter(MethodInfo method) { *

    *
  • has an return type
    • *
  • has no parameter
    • - *
  • is appropriately named
    • + *
  • and: + *
      + *
    • is appropriately named (get/is)
      • + *
    • or is annotated with @Query
      • + *
    + *
    • *
* * @param method the method * @return true if it is */ - private static boolean isGetter(MethodInfo method) { + private static boolean isPropertyAccessor(MethodInfo method) { return method.returnType().kind() != Type.Kind.VOID && method.parameters().isEmpty() - && isGetterName(method.name()); + && (method.hasAnnotation(Annotations.QUERY) + || isGetterName(method.name())); } private static String toNameFromSetter(String methodName) { diff --git a/server/tck/src/test/java/io/smallrye/graphql/test/apps/fieldexistence/api/FieldExistencePojo.java b/server/tck/src/test/java/io/smallrye/graphql/test/apps/fieldexistence/api/FieldExistencePojo.java index 2ed6aa859..d0c25304f 100644 --- a/server/tck/src/test/java/io/smallrye/graphql/test/apps/fieldexistence/api/FieldExistencePojo.java +++ b/server/tck/src/test/java/io/smallrye/graphql/test/apps/fieldexistence/api/FieldExistencePojo.java @@ -1,5 +1,7 @@ package io.smallrye.graphql.test.apps.fieldexistence.api; +import org.eclipse.microprofile.graphql.Query; + public class FieldExistencePojo { public String publicField = "publicField"; @@ -8,4 +10,9 @@ public class FieldExistencePojo { public final String publicFinalField = "finalField"; + @Query + public String queryMethod() { + return "queryMethod"; + } + } diff --git a/server/tck/src/test/resources/tests/fieldExistenceTests.csv b/server/tck/src/test/resources/tests/fieldExistenceTests.csv index f3639eef8..02d12adb4 100644 --- a/server/tck/src/test/resources/tests/fieldExistenceTests.csv +++ b/server/tck/src/test/resources/tests/fieldExistenceTests.csv @@ -2,6 +2,7 @@ 1| type FieldExistencePojo | publicField: String | public field should exist on output types 2| type FieldExistencePojo | publicFinalField: String | public final field should exist on output types 3| type FieldExistencePojo | !PUBLIC_STATIC_FINAL_FIELD: String | static field should not exist on output types +3| type FieldExistencePojo | queryMethod: String | @Query-methods should exist on output types 4| input FieldExistencePojoInput | publicField: String | public field should exist on input types 5| input FieldExistencePojoInput | !publicFinalField: String | public final field should not exist on input types diff --git a/server/tck/src/test/resources/tests/fields/input.graphql b/server/tck/src/test/resources/tests/fields/input.graphql new file mode 100644 index 000000000..1495e66e4 --- /dev/null +++ b/server/tck/src/test/resources/tests/fields/input.graphql @@ -0,0 +1,5 @@ +{ + fieldExistencePojo { + queryMethod + } +} diff --git a/server/tck/src/test/resources/tests/fields/output.json b/server/tck/src/test/resources/tests/fields/output.json new file mode 100644 index 000000000..2b432ada3 --- /dev/null +++ b/server/tck/src/test/resources/tests/fields/output.json @@ -0,0 +1,7 @@ +{ + "data": { + "fieldExistencePojo": { + "queryMethod": "queryMethod" + } + } +} diff --git a/server/tck/src/test/resources/tests/fields/test.properties b/server/tck/src/test/resources/tests/fields/test.properties new file mode 100644 index 000000000..db74c7f9b --- /dev/null +++ b/server/tck/src/test/resources/tests/fields/test.properties @@ -0,0 +1,2 @@ +ignore=false +priority=100