Skip to content

Commit

Permalink
Merge pull request #735 from ybroeker/backport/feat/propertyFetching
Browse files Browse the repository at this point in the history
Backport of #732
  • Loading branch information
phillip-kruger authored Apr 6, 2021
2 parents 0126854 + b25f7de commit d66355c
Show file tree
Hide file tree
Showing 10 changed files with 154 additions and 58 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ public Optional<Field> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import org.jboss.jandex.MethodInfo;
import org.jboss.jandex.Type;

import io.smallrye.graphql.schema.Annotations;

/**
* Helping with method operations.
*
Expand Down Expand Up @@ -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;
}
Expand All @@ -87,16 +89,22 @@ private static boolean isSetter(MethodInfo method) {
* <ul>
* <li>has an return type</li>
* <li>has no parameter</li>
* <li>is appropriately named</li>
* <li>and:
* <ul>
* <li>is appropriately named (get/is)</li>
* <li>or is annotated with @Query</li>
* </ul>
* </li>
* </ul>
*
* @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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()));
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -431,15 +429,15 @@ private GraphQLFieldDefinition createGraphQLFieldDefinitionFromOperation(String
return graphQLFieldDefinition;
}

private List<GraphQLFieldDefinition> createGraphQLFieldDefinitionsFromFields(String ownerName, Collection<Field> fields) {
private List<GraphQLFieldDefinition> createGraphQLFieldDefinitionsFromFields(Reference owner, Collection<Field> fields) {
List<GraphQLFieldDefinition> 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());
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
* <p>
*
* @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.
* <p>
* 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<T> implements DataFetcher<T>, TrivialDataFetcher<T> {

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.
* <p>
* 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);
}
}

}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -8,4 +10,9 @@ public class FieldExistencePojo {

public final String publicFinalField = "finalField";

@Query
public String queryMethod() {
return "queryMethod";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions server/tck/src/test/resources/tests/fields/input.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
fieldExistencePojo {
queryMethod
}
}
7 changes: 7 additions & 0 deletions server/tck/src/test/resources/tests/fields/output.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"data": {
"fieldExistencePojo": {
"queryMethod": "queryMethod"
}
}
}
2 changes: 2 additions & 0 deletions server/tck/src/test/resources/tests/fields/test.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ignore=false
priority=100

0 comments on commit d66355c

Please sign in to comment.