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.

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

Record like property access support #2994

Merged
merged 8 commits into from Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/main/java/graphql/schema/PropertyDataFetcher.java
Expand Up @@ -8,20 +8,20 @@
import java.util.function.Function;

/**
* This is the default data fetcher used in graphql-java. It will examine
* maps and POJO java beans for values that match the desired name, typically the field name
* This is the default data fetcher used in graphql-java, and it will examine
* maps, records and POJO java beans for values that match the desired name, typically the field name,
* or it will use a provided function to obtain values.
* maps and POJO java beans for values that match the desired name.
* <p>
* It uses the following strategies
* <ul>
* <li>If the source is null, return null</li>
* <li>If the source is a Map, return map.get(propertyName)</li>
* <li>If a function is provided, it is used</li>
* <li>Find a public JavaBean getter method named `propertyName`</li>
* <li>Find any getter method named `propertyName` and call method.setAccessible(true)</li>
* <li>Find a public JavaBean getter method named `getPropertyName()` or `isPropertyName()`</li>
* <li>Find any getter method named `getPropertyName()` or `isPropertyName()` and call method.setAccessible(true)</li>
* <li>Find a public field named `propertyName`</li>
* <li>Find any field named `propertyName` and call field.setAccessible(true)</li>
* <li>Find a public Record like method named `propertyName()`</li>
* <li>If this cant find anything, then null is returned</li>
* </ul>
* <p>
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/graphql/schema/PropertyDataFetcherHelper.java
@@ -1,6 +1,7 @@
package graphql.schema;

import graphql.Internal;
import graphql.VisibleForTesting;

/**
* This class is the guts of a property data fetcher and also used in AST code to turn
Expand All @@ -27,6 +28,11 @@ public static boolean setUseSetAccessible(boolean flag) {
return impl.setUseSetAccessible(flag);
}

@VisibleForTesting
public static boolean setUseLambdaFactory(boolean flag) {
return impl.setUseLambdaFactory(flag);
}

public static boolean setUseNegativeCache(boolean flag) {
return impl.setUseNegativeCache(flag);
}
Expand Down
71 changes: 57 additions & 14 deletions src/main/java/graphql/schema/PropertyFetchingImpl.java
Expand Up @@ -31,6 +31,7 @@
public class PropertyFetchingImpl {

private final AtomicBoolean USE_SET_ACCESSIBLE = new AtomicBoolean(true);
private final AtomicBoolean USE_LAMBDA_FACTORY = new AtomicBoolean(true);
private final AtomicBoolean USE_NEGATIVE_CACHE = new AtomicBoolean(true);
private final ConcurrentMap<CacheKey, CachedLambdaFunction> LAMBDA_CACHE = new ConcurrentHashMap<>();
private final ConcurrentMap<CacheKey, CachedMethod> METHOD_CACHE = new ConcurrentHashMap<>();
Expand Down Expand Up @@ -104,7 +105,7 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g
// expensive operation here
//

Optional<Function<Object, Object>> getterOpt = LambdaFetchingSupport.createGetter(object.getClass(), propertyName);
Optional<Function<Object, Object>> getterOpt = lambdaGetter(propertyName, object);
if (getterOpt.isPresent()) {
Function<Object, Object> getter = getterOpt.get();
cachedFunction = new CachedLambdaFunction(getter);
Expand All @@ -113,23 +114,43 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g
}

boolean dfeInUse = singleArgumentValue != null;
//
// try by record like name - object.propertyName()
try {
MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName);
return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue);
} catch (NoSuchMethodException ignored) {
}
//
// try by public getters name - object.getPropertyName()
try {
MethodFinder methodFinder = (root, methodName) -> findPubliclyAccessibleMethod(cacheKey, root, methodName, dfeInUse);
MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse);
return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue);
} catch (NoSuchMethodException ignored) {
}
//
// try by accessible getters name - object.getPropertyName()
try {
MethodFinder methodFinder = (aClass, methodName) -> findViaSetAccessible(cacheKey, aClass, methodName, dfeInUse);
return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue);
} catch (NoSuchMethodException ignored) {
try {
MethodFinder methodFinder = (aClass, methodName) -> findViaSetAccessible(cacheKey, aClass, methodName, dfeInUse);
return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue);
} catch (NoSuchMethodException ignored2) {
try {
return getPropertyViaFieldAccess(cacheKey, object, propertyName);
} catch (FastNoSuchMethodException e) {
// we have nothing to ask for, and we have exhausted our lookup strategies
putInNegativeCache(cacheKey);
return null;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I unwound the nesting of the exceptions - there was no need for it and I think this is clearer code

}
//
// try by field name - object.propertyName;
try {
return getPropertyViaFieldAccess(cacheKey, object, propertyName);
} catch (NoSuchMethodException ignored) {
}
// we have nothing to ask for, and we have exhausted our lookup strategies
putInNegativeCache(cacheKey);
return null;
}

private Optional<Function<Object, Object>> lambdaGetter(String propertyName, Object object) {
if (USE_LAMBDA_FACTORY.get()) {
return LambdaFetchingSupport.createGetter(object.getClass(), propertyName);
}
return Optional.empty();
}

private boolean isNegativelyCached(CacheKey key) {
Expand All @@ -149,6 +170,11 @@ private interface MethodFinder {
Method apply(Class<?> aClass, String s) throws NoSuchMethodException;
}

private Object getPropertyViaRecordMethod(Object object, String propertyName, MethodFinder methodFinder, Object singleArgumentValue) throws NoSuchMethodException {
Method method = methodFinder.apply(object.getClass(), propertyName);
return invokeMethod(object, singleArgumentValue, method, takesSingleArgumentTypeAsOnlyArgument(method));
}

private Object getPropertyViaGetterMethod(Object object, String propertyName, GraphQLType graphQLType, MethodFinder methodFinder, Object singleArgumentValue) throws NoSuchMethodException {
if (isBooleanProperty(graphQLType)) {
try {
Expand Down Expand Up @@ -204,6 +230,20 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClas
return rootClass.getMethod(methodName);
}

/*
https://docs.oracle.com/en/java/javase/15/language/records.html

A record class declares a sequence of fields, and then the appropriate accessors, constructors, equals, hashCode, and toString methods are created automatically.

Records cannot extend any class - so we need only check the root class for a publicly declared method with the propertyName

However, we won't just restrict ourselves strictly to true records. We will find methods that are record like
and fetch them - e.g. `object.propertyName()`
*/
private Method findRecordMethod(CacheKey cacheKey, Class<?> rootClass, String methodName) throws NoSuchMethodException {
return findPubliclyAccessibleMethod(cacheKey,rootClass,methodName,false);
}

private Method findViaSetAccessible(CacheKey cacheKey, Class<?> aClass, String methodName, boolean dfeInUse) throws NoSuchMethodException {
if (!USE_SET_ACCESSIBLE.get()) {
throw new FastNoSuchMethodException(methodName);
Expand Down Expand Up @@ -306,6 +346,9 @@ public void clearReflectionCache() {
public boolean setUseSetAccessible(boolean flag) {
return USE_SET_ACCESSIBLE.getAndSet(flag);
}
public boolean setUseLambdaFactory(boolean flag) {
return USE_LAMBDA_FACTORY.getAndSet(flag);
}

public boolean setUseNegativeCache(boolean flag) {
return USE_NEGATIVE_CACHE.getAndSet(flag);
Expand Down
50 changes: 32 additions & 18 deletions src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java
Expand Up @@ -15,6 +15,7 @@
import java.util.List;
import java.util.Optional;
import java.util.function.Function;
import java.util.function.Predicate;

import static java.util.stream.Collectors.toList;

Expand Down Expand Up @@ -42,15 +43,32 @@ public static Optional<Function<Object, Object>> createGetter(Class<?> sourceCla
Function<Object, Object> getterFunction = mkCallFunction(sourceClass, candidateMethod.getName(), candidateMethod.getReturnType());
return Optional.of(getterFunction);
} catch (Throwable ignore) {
//
// if we cant make a dynamic lambda here, then we give up and let the old property fetching code do its thing
// this can happen on runtimes such as GraalVM native where LambdaMetafactory is not supported
// and will throw something like :
//
// com.oracle.svm.core.jdk.UnsupportedFeatureError: Defining hidden classes at runtime is not supported.
// at org.graalvm.nativeimage.builder/com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:89)
}
}
return Optional.empty();
}


private static Method getCandidateMethod(Class<?> sourceClass, String propertyName) {
List<Method> allGetterMethods = findGetterMethodsForProperty(sourceClass, propertyName);
// property() methods first
Predicate<Method> recordLikePredicate = method -> isRecordLike(method) && propertyName.equals(decapitalize(method.getName()));
List<Method> recordLikeMethods = findMethodsForProperty(sourceClass,
recordLikePredicate);
if (!recordLikeMethods.isEmpty()) {
return recordLikeMethods.get(0);
}

// getProperty() POJO methods next
Predicate<Method> getterPredicate = method -> isGetterNamed(method) && propertyName.equals(mkPropertyNameGetter(method));
List<Method> allGetterMethods = findMethodsForProperty(sourceClass,
getterPredicate);
List<Method> pojoGetterMethods = allGetterMethods.stream()
.filter(LambdaFetchingSupport::isPossiblePojoMethod)
.collect(toList());
Expand All @@ -60,10 +78,8 @@ private static Method getCandidateMethod(Class<?> sourceClass, String propertyNa
method = findBestBooleanGetter(pojoGetterMethods);
}
return checkForSingleParameterPeer(method, allGetterMethods);
} else {
return null;
}

return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Support in the lambda code to find recordLike() methods

}

private static Method checkForSingleParameterPeer(Method candidateMethod, List<Method> allMethods) {
Expand All @@ -88,21 +104,18 @@ private static Method findBestBooleanGetter(List<Method> methods) {
/**
* Finds all methods in a class hierarchy that match the property name - they might not be suitable but they
*
* @param sourceClass the class we are looking to work on
* @param propertyName the name of the property
* @param sourceClass the class we are looking to work on
*
* @return a list of getter methods for that property
*/
private static List<Method> findGetterMethodsForProperty(Class<?> sourceClass, String propertyName) {
private static List<Method> findMethodsForProperty(Class<?> sourceClass, Predicate<Method> predicate) {
List<Method> methods = new ArrayList<>();
Class<?> currentClass = sourceClass;
while (currentClass != null) {
Method[] declaredMethods = currentClass.getDeclaredMethods();
for (Method declaredMethod : declaredMethods) {
if (isGetterNamed(declaredMethod)) {
if (nameMatches(propertyName, declaredMethod)) {
methods.add(declaredMethod);
}
if (predicate.test(declaredMethod)) {
methods.add(declaredMethod);
}
}
currentClass = currentClass.getSuperclass();
Expand All @@ -113,12 +126,6 @@ private static List<Method> findGetterMethodsForProperty(Class<?> sourceClass, S
.collect(toList());
}


private static boolean nameMatches(String propertyName, Method declaredMethod) {
String methodPropName = mkPropertyName(declaredMethod);
return propertyName.equals(methodPropName);
}

private static boolean isPossiblePojoMethod(Method method) {
return !isObjectMethod(method) &&
returnsSomething(method) &&
Expand All @@ -127,6 +134,13 @@ private static boolean isPossiblePojoMethod(Method method) {
isPublic(method);
}

private static boolean isRecordLike(Method method) {
return !isObjectMethod(method) &&
returnsSomething(method) &&
hasNoParameters(method) &&
isPublic(method);
}

private static boolean isBooleanGetter(Method method) {
Class<?> returnType = method.getReturnType();
return isGetterNamed(method) && (returnType.equals(Boolean.class) || returnType.equals(Boolean.TYPE));
Expand All @@ -153,7 +167,7 @@ private static boolean isObjectMethod(Method method) {
return method.getDeclaringClass().equals(Object.class);
}

private static String mkPropertyName(Method method) {
private static String mkPropertyNameGetter(Method method) {
//
// getFooName becomes fooName
// isFoo becomes foo
Expand Down
89 changes: 89 additions & 0 deletions src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy
Expand Up @@ -2,9 +2,12 @@ package graphql.schema

import graphql.ExecutionInput
import graphql.TestUtil
import graphql.schema.fetching.ConfusedPojo
import graphql.schema.somepackage.ClassWithDFEMethods
import graphql.schema.somepackage.ClassWithInterfaces
import graphql.schema.somepackage.ClassWithInteritanceAndInterfaces
import graphql.schema.somepackage.RecordLikeClass
import graphql.schema.somepackage.RecordLikeTwoClassesDown
import graphql.schema.somepackage.TestClass
import graphql.schema.somepackage.TwoClassesDown
import spock.lang.Specification
Expand All @@ -20,6 +23,7 @@ class PropertyDataFetcherTest extends Specification {
PropertyDataFetcher.setUseSetAccessible(true)
PropertyDataFetcher.setUseNegativeCache(true)
PropertyDataFetcher.clearReflectionCache()
PropertyDataFetcherHelper.setUseLambdaFactory(true)
}

def env(obj) {
Expand Down Expand Up @@ -95,6 +99,91 @@ class PropertyDataFetcherTest extends Specification {
result == null
}

def "fetch via record method"() {
def environment = env(new RecordLikeClass())
when:
def fetcher = new PropertyDataFetcher("recordProperty")
def result = fetcher.get(environment)
then:
result == "recordProperty"

// caching works
when:
fetcher = new PropertyDataFetcher("recordProperty")
result = fetcher.get(environment)
then:
result == "recordProperty"

// recordArgumentMethod will not work because it takes a parameter
when:
fetcher = new PropertyDataFetcher("recordArgumentMethod")
result = fetcher.get(environment)
then:
result == null

// equals will not work because it takes a parameter
when:
fetcher = new PropertyDataFetcher("equals")
result = fetcher.get(environment)
then:
result == null

// we allow hashCode() and toString() because why not - they are valid property names
// they might not be that useful but they can be accessed

when:
fetcher = new PropertyDataFetcher("hashCode")
result = fetcher.get(environment)
then:
result == 666

when:
fetcher = new PropertyDataFetcher("toString")
result = fetcher.get(environment)
then:
result == "toString"
}

def "can fetch record like methods that are public and on super classes"() {
def environment = env(new RecordLikeTwoClassesDown())
when:
def fetcher = new PropertyDataFetcher("recordProperty")
def result = fetcher.get(environment)
then:
result == "recordProperty"
}

def "fetch via record method without lambda support"() {
PropertyDataFetcherHelper.setUseLambdaFactory(false)
PropertyDataFetcherHelper.clearReflectionCache()

when:
def environment = env(new RecordLikeClass())
def fetcher = new PropertyDataFetcher("recordProperty")
def result = fetcher.get(environment)
then:
result == "recordProperty"

when:
environment = env(new RecordLikeTwoClassesDown())
fetcher = new PropertyDataFetcher("recordProperty")
result = fetcher.get(environment)
then:
result == "recordProperty"
}

def "fetch via record method without lambda support in preference to getter methods"() {
PropertyDataFetcherHelper.setUseLambdaFactory(false)
PropertyDataFetcherHelper.clearReflectionCache()

when:
def environment = env(new ConfusedPojo())
def fetcher = new PropertyDataFetcher("recordLike")
def result = fetcher.get(environment)
then:
result == "recordLike"
}

def "fetch via public method"() {
def environment = env(new TestClass())
def fetcher = new PropertyDataFetcher("publicProperty")
Expand Down