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
Changes from 7 commits
7d7d072
91f703e
0cafaae
5c64285
ebc445e
759b1fa
2921e63
c3a69e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<>(); | ||
|
@@ -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); | ||
|
@@ -113,23 +114,45 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g | |
} | ||
|
||
boolean dfeInUse = singleArgumentValue != null; | ||
// | ||
// 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 { | ||
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; | ||
} | ||
} | ||
} | ||
// | ||
// 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 by field name - object.propertyName; | ||
try { | ||
return getPropertyViaFieldAccess(cacheKey, object, propertyName); | ||
} catch (NoSuchMethodException ignored) { | ||
} | ||
// | ||
// try by record name - object.propertyName() | ||
try { | ||
// we do records last because if there was ever a previous situation where there was a `getProp()` and a `prop()` in place | ||
// then previously it would use the `getProp()` method so we want that same behavior | ||
MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName); | ||
return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue); | ||
} catch (NoSuchMethodException ignored) { | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I unwound the nesting because I think this reads better than |
||
// 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) { | ||
|
@@ -149,6 +172,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 { | ||
|
@@ -204,6 +232,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); | ||
|
@@ -306,6 +348,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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -42,15 +43,21 @@ 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); | ||
List<Method> allGetterMethods = findMethodsForProperty(sourceClass, propertyName, LambdaFetchingSupport::isGetterNamed); | ||
List<Method> pojoGetterMethods = allGetterMethods.stream() | ||
.filter(LambdaFetchingSupport::isPossiblePojoMethod) | ||
.collect(toList()); | ||
|
@@ -60,10 +67,12 @@ private static Method getCandidateMethod(Class<?> sourceClass, String propertyNa | |
method = findBestBooleanGetter(pojoGetterMethods); | ||
} | ||
return checkForSingleParameterPeer(method, allGetterMethods); | ||
} else { | ||
return null; | ||
} | ||
|
||
List<Method> recordLikeMethods = findMethodsForProperty(sourceClass, propertyName, LambdaFetchingSupport::isRecordLike); | ||
if (!recordLikeMethods.isEmpty()) { | ||
return recordLikeMethods.get(0); | ||
} | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Support in the lambda code to find |
||
} | ||
|
||
private static Method checkForSingleParameterPeer(Method candidateMethod, List<Method> allMethods) { | ||
|
@@ -93,13 +102,13 @@ private static Method findBestBooleanGetter(List<Method> methods) { | |
* | ||
* @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, String propertyName, 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 (predicate.test(declaredMethod)) { | ||
if (nameMatches(propertyName, declaredMethod)) { | ||
methods.add(declaredMethod); | ||
} | ||
|
@@ -127,6 +136,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)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package graphql.schema.fetching; | ||
|
||
public class ConfusedPojo { | ||
|
||
public String getRecordLike() { | ||
return "getRecordLike"; | ||
} | ||
|
||
public String recordLike() { | ||
return "recordLike"; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IN theory this is possible - stupid but possible - in the old days the getter would be found so this shows that this still happens. eg record getters are looked up after pojo getters |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,4 +65,8 @@ public String is() { | |
return "is"; | ||
} | ||
|
||
public String recordLike() { | ||
return "recordLike"; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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