Record like property access support #2994

bbakerman · 2022-10-25T04:56:41Z

This changes the PropertyDataFetcher so that support Java record like naming.

eg object.propertyName() method naming

While the code here is not specific to record classes only - it will work with them and with any class that uses propertyName() method naming

#2994 has been updated

I have decided to change the strategy - we will look for "direct property" methods first and then getters

So given a field issues - then we will look for issues() first and then if it fails to find that it will look for getissues or getIssues (since it decapitalizes)

I think this is better situation on reflection (pun intented)

Originally I wanted to avoid confusion


public behavior() {
   return "not wanted"'
}

public getBehavior()  {
   return "wanted";
}

where if some one previously had declared a record like method (behavior()) and didn't want it to be used. BUT this is crazy to have such confusion and I don't think anyone would do it on purpose AND graphql-java 20 can introduce new behaviors if on balance its worth it. And I think it's worth it.

So this is the new support.

This will also help Kotlin with its special isXXX naming (or lack there of)

given

class KotlinClass(val name : String, val age : Int, val cool: Boolean, var isOpen : Boolean, val issues : String) {
}

data class KotlinDataClass(val name: String, val age: Int, val cool: Boolean, var isOpen : Boolean, val issues : String)

public record JavaRecord(String name, int age, boolean cool, boolean isOpen, String issues) {
}

we get this generated

KotlinDataClass
public final java.lang.String KotlinDataClass.getName()
public boolean KotlinDataClass.equals(java.lang.Object)
public java.lang.String KotlinDataClass.toString()
public int KotlinDataClass.hashCode()
public final boolean KotlinDataClass.isOpen()
public final KotlinDataClass KotlinDataClass.copy(java.lang.String,int,boolean,boolean,java.lang.String)
public static KotlinDataClass KotlinDataClass.copy$default(KotlinDataClass,java.lang.String,int,boolean,boolean,java.lang.String,int,java.lang.Object)
public final int KotlinDataClass.getAge()
public final boolean KotlinDataClass.getCool()
public final void KotlinDataClass.setOpen(boolean)
public final java.lang.String KotlinDataClass.getIssues()
public final java.lang.String KotlinDataClass.component1()
public final int KotlinDataClass.component2()
public final boolean KotlinDataClass.component3()
public final boolean KotlinDataClass.component4()
public final java.lang.String KotlinDataClass.component5()
----
KotlinClass
public final java.lang.String KotlinClass.getName()
public final boolean KotlinClass.isOpen()
public final int KotlinClass.getAge()
public final boolean KotlinClass.getCool()
public final void KotlinClass.setOpen(boolean)
public final java.lang.String KotlinClass.getIssues()
----
JavaRecord
public java.lang.String JavaRecord.name()
public final boolean JavaRecord.equals(java.lang.Object)
public final java.lang.String JavaRecord.toString()
public final int JavaRecord.hashCode()
public boolean JavaRecord.isOpen()
public java.lang.String JavaRecord.issues()
public int JavaRecord.age()
public boolean JavaRecord.cool()
----

So this new code will find directly named methods first and then use the get/is style naming secondary

This will work for Kotlin and Java records a like

bbakerman · 2022-10-25T04:58:02Z

src/main/java/graphql/schema/PropertyFetchingImpl.java

-                    putInNegativeCache(cacheKey);
-                    return null;
-                }
-            }


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

dondonz · 2022-10-29T07:28:34Z

src/main/java/graphql/schema/PropertyFetchingImpl.java

+       Records cannot extend any class - so we need only check the root class for a publicly declared method with the propertyName
+     */
+    private Method findRecordMethod(CacheKey cacheKey, Class<?> rootClass, String methodName) throws NoSuchMethodException {
+        if (Modifier.isPublic(rootClass.getModifiers())) {


Records are also final. findRecordMethod should be restricted to records or other final classes.

if (Modifier.isPublic(rootClass.getModifiers()) && Modifier.isFinal(rootClass.getModifiers())) {

dondonz · 2022-10-29T07:29:15Z

src/test/groovy/graphql/schema/somepackage/RecordLikeClass.java

+ * smells like one and that's enough really.  Its public, not derived from another
+ * class and has a public method named after a property
+ */
+public class RecordLikeClass {


Make this public final class

I have decided we will look up record like methods - not truly final ones like records are.

# Conflicts: # src/main/java/graphql/schema/PropertyFetchingImpl.java

bbakerman · 2022-11-01T09:29:06Z

src/main/java/graphql/schema/PropertyFetchingImpl.java

+            MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName);
+            return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue);
+        } catch (NoSuchMethodException ignored) {
+        }


I unwound the nesting because I think this reads better than try inside try inside try

bbakerman · 2022-11-01T09:30:10Z

src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java

+        if (!recordLikeMethods.isEmpty()) {
+            return recordLikeMethods.get(0);
+        }
+        return null;


Support in the lambda code to find recordLike() methods

bbakerman · 2022-11-01T09:31:30Z

src/test/groovy/graphql/schema/fetching/ConfusedPojo.java

+
+    public String recordLike() {
+        return "recordLike";
+    }


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

bbakerman · 2022-11-01T09:32:41Z

src/test/groovy/graphql/schema/somepackage/RecordLikeTwoClassesDown.java

+package graphql.schema.somepackage;
+
+public class RecordLikeTwoClassesDown extends RecordLikeClass {
+}


Hence derivation works with record like methods

Moved record like to the front of the search pattern

Record like naming

7d7d072

bbakerman added this to the 20.0 milestone Oct 25, 2022

bbakerman requested review from andimarek and dondonz October 25, 2022 04:56

bbakerman commented Oct 25, 2022

View reviewed changes

bbakerman added 2 commits October 25, 2022 16:18

Record like naming - tweaks

91f703e

Record like naming - javadoc tweaks

0cafaae

dondonz reviewed Oct 29, 2022

View reviewed changes

bbakerman added 4 commits November 1, 2022 16:46

Merge remote-tracking branch 'origin/master' into record_support

5c64285

# Conflicts: # src/main/java/graphql/schema/PropertyFetchingImpl.java

Updated record like code including the Lambda mechanism

ebc445e

Added more tests and a flag to turn Lambda off

759b1fa

JavaDoc update

2921e63

bbakerman commented Nov 1, 2022

View reviewed changes

turboezh mentioned this pull request Nov 9, 2022

Kotlin isX properties access #3010

Closed

More work on record like access

c3a69e2

Moved record like to the front of the search pattern

bbakerman mentioned this pull request Nov 21, 2022

getPropertyViaFieldAccess ignoring superclass fields #3019

Closed

bbakerman merged commit 477d9a0 into master Nov 22, 2022

AlexanderPraegla mentioned this pull request Mar 29, 2023

feature: Update to graphql-java:20.1 Netflix/dgs-framework#1467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record like property access support #2994

Record like property access support #2994

bbakerman commented Oct 25, 2022 •

edited

bbakerman Oct 25, 2022

dondonz Oct 29, 2022 •

edited

dondonz Oct 29, 2022

bbakerman Nov 1, 2022

bbakerman Nov 1, 2022

bbakerman Nov 1, 2022

bbakerman Nov 1, 2022

bbakerman Nov 1, 2022

Record like property access support #2994

Record like property access support #2994

Conversation

bbakerman commented Oct 25, 2022 • edited

bbakerman Oct 25, 2022

Choose a reason for hiding this comment

dondonz Oct 29, 2022 • edited

Choose a reason for hiding this comment

dondonz Oct 29, 2022

Choose a reason for hiding this comment

bbakerman Nov 1, 2022

Choose a reason for hiding this comment

bbakerman Nov 1, 2022

Choose a reason for hiding this comment

bbakerman Nov 1, 2022

Choose a reason for hiding this comment

bbakerman Nov 1, 2022

Choose a reason for hiding this comment

bbakerman Nov 1, 2022

Choose a reason for hiding this comment

bbakerman commented Oct 25, 2022 •

edited

dondonz Oct 29, 2022 •

edited