Adding Supports for Records (Groovy) + Passing Record Test. #122

Shounaks · 2024-02-22T16:55:34Z

Issue : support for records #94

This will enable the introspector to catch Groovy generated getters, and serialize the records.

Shounaks · 2024-02-22T16:57:31Z

jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanPropertyIntrospector.java

@@ -142,6 +143,13 @@ private static void _introspect(Class<?> currType, Map<String, PropBuilder> prop
                        name = decap(name.substring(2));
                        _propFrom(props, name).withIsGetter(m);
                    }
+                } else {
+                    // This will allow getters with field name as their getters, like the ones generated by Groovy
+                    boolean isDirectNameGetter = Arrays.stream(currType.getDeclaredFields()).map(Field::getName).anyMatch(f -> f.equals(m.getName()));


i believe if we get a list of fields & check if there is a field with the same name as current method, then we need to add it as a prop

, this can be optimized, but as of now, its working.

I think this capability needs to be behind Json.Feature flag since while it makes sense wrt Records (and probably other value types), it can also be unexpected for other users. That is, it changes compatibility.

Other than that it'd make sense to build a map from name to Field in loop above, to avoid linear scanning for every potential "non-get-getter".

cowtowncoder · 2024-02-23T04:47:58Z

Quick note: if this is meant for Groovy records specifically, should probably file a new separate issue -- #94 is, I think, for Java 17 Record type and require immutability including construction by multi-argument constructor.
This seems to be just for automatically recognizing "non-get getters" (so, name() for property "name" if (and only if) there is matching Field), which would also work for Java Records, but wouldn't be full Record support in my mind.

I also think this should be behind a new JSON.Feature (DETECT_FIELD_MATCHING_GETTERS? or some better name, ideally :) ) just because it is behavioral change that might not be ideal for some users.
(I think I'd be ok but it defaulting to true, just as long as it is possible to disable it for compatibility reasons).

cowtowncoder · 2024-02-23T04:50:28Z

jr-objects/src/test/groovy/com/fasterxml/jackson/jr/passing/GroovyTest.groovy

@@ -0,0 +1,68 @@
+package com.fasterxml.jackson.jr.passing


Is the new passing needed to avoid split packages or... ?

If necessary, maybe use com.fasterxml.jackson.jr.ob.groovy.

my mistake, removed in latest commits.

Shounaks · 2024-02-23T19:07:03Z

Yes, this will fix Java14's Records as well as Groovy Records.

This seems to be just for automatically recognizing "non-get getters" (so, name() for property "name" if (and only if) there is matching Field), which would also work for Java Records, but wouldn't be full Record support in my mind.

Okay so, how should we go on to check which object is a record and which is not, without having access to native internal methods?

I tried to do some digging, and found out this:

but we dont really support JDK 16..

and i couldnt get to the end of how this native method is implemented. (no experience in this field sadly..)

One immediate way that i can think of is to check constructor parameters matching the field names .

but i am not sure how it will interact with current code. Might need your help here @cowtowncoder .

cowtowncoder · 2024-02-24T00:43:33Z

It is possible to detect "Record-ness" of a type, even from JDK 8: jackson-databind does that. Actually detecting something is Record is relatively easy: src/main/java/com/fasterxml/jackson/databind/util/ClassUtil.java: has this:

    public static boolean isRecordType(Class<?> cls) {
        Class<?> parent = cls.getSuperclass();
        return (parent != null) && "java.lang.Record".equals(parent.getName());
    }

which we could re-purpose. Same might work for Groovy types.

Anything fancier (finding canonical constructor, declared properties) gets trickier, but just knowing something is a Record type is quite doable.

cowtowncoder · 2024-02-24T00:46:34Z

I think that we have a choice, so either:

Use a JSON.Feature to enable detection, regardless of type (whether it's Record or not)
Auto-detect Records and apply different rules

I guess one could combine things too, to basically default to auto-detecting via (2), but allowing disabling by (1). Not sure how necessary that is.

cowtowncoder · 2024-02-24T00:53:19Z

Ok, couple of additional notes:

Groovy test seems to fail for some reason in CI
Not sure how to test Java Records: jackson-databind uses different profiles to optionally included Java 17+ tests, that is a possibility -- but baseline for Jackson 2.x jackson-jr is JDK 8 so they can't be added in main src/test/java
... although, since jackson-jr is multi-module Maven project, it potentially could have separate test-only package that runs after building jr-objects. Either way, gets bit complicated

We could, however, also test case of "not Record but has field-name matching getter methods" as surrogates for Records. This assuming we do not want to limit this feature to just Java/Groovy Record types.

Shounaks · 2024-02-24T03:01:57Z

I think that we have a choice, so either:

Use a JSON.Feature to enable detection, regardless of type (whether it's Record or not)
Auto-detect Records and apply different rules
I guess one could combine things too, to basically default to auto-detecting via (2), but allowing disabling by (1). Not sure how necessary that is.

I believe 1st choice is better, since we have the knob to turn this feature off, when working with other libraries,

…a.nio.file.Path`

… going to throw this time)

…erXML#123)

Shounaks · 2024-02-27T08:39:48Z

This failing test, it seems, that the since, the getter is public, we are getting the values, and hence we are able to serialize these items. looks like correct behavior. can you confirm once again? so that I will remove this test. or this is the expected behavior?

I didnt really think about JsonAutoDetect.Visibility.PROTECTED_AND_PUBLIC when implementation of this feature. how should these features interact with that?

cowtowncoder · 2024-02-29T23:25:35Z

jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanPropertyIntrospector.java

@@ -145,6 +149,15 @@ private static void _introspect(Class<?> currType, Map<String, PropBuilder> prop
                        name = decap(name.substring(2));
                        _propFrom(props, name).withIsGetter(m);
                    }
+                } else if(USE_FIELD_NAME_GETTERS.enabledByDefault()){


As with other comment, enabledByDefault() is not to be used here -- we need actual configuration settings.

There's an example earlier on:

final boolean noStatics = JSON.Feature.INCLUDE_STATIC_FIELDS.isDisabled(features);

that shows expected usage.

cowtowncoder · 2024-02-29T23:29:02Z

Quick request: would it be possible to extract change to add new test module for Groovy classes into separate PR? We already added jr-objects/src/test/groovy/com/fasterxml/jackson/jr/GroovyTest.groovy and that could now be moved to the new module; and that PR could be merged before figuring out details of actual feature.

Shounaks · 2024-03-01T13:24:10Z

sounds good, let me do that.

Shounaks · 2024-03-01T16:52:08Z

Quick request: would it be possible to extract change to add new test module for Groovy classes into separate PR? We already added jr-objects/src/test/groovy/com/fasterxml/jackson/jr/GroovyTest.groovy and that could now be moved to the new module; and that PR could be merged before figuring out details of actual feature.

Created a new PR: #128

Shounaks · 2024-03-05T18:37:50Z

Closing this PR, will start with clean slate, with minimal changes, and formatting.

Shounaks commented Feb 22, 2024

View reviewed changes

Shounaks force-pushed the 2.17-add-failing-test-for-records branch from eb9e69e to a039df4 Compare February 22, 2024 17:32

cowtowncoder reviewed Feb 23, 2024

View reviewed changes

cowtowncoder and others added 20 commits February 27, 2024 13:51

Minor test refactoring

5ecd36a

Minor refactoring wrt java.nio.file.Path handling

0816a2d

Change post FasterXML#116 to potentially change serialization of `jav…

2324570

…a.nio.file.Path`

Simplify handling as per @jhonnen's suggestions

ac7838d

Last minor tweaks

0c74ef1

comment cleanup

27a82c0

Streamline child pom's (extract common stuff to parent) (FasterXML#120)

eeb1fcb

Skipping over groovy metadata class + groovy test. (FasterXML#118)

adfb1db

Adding Supports for Records (Groovy) + Passing Record Test.

1dff458

Trying to fix Record Test#2

b086e45

Trying to fix Record Test#3

d694999

Trying to fix Record Test#4 (I dont even know what groovy compiler is…

8d4f96f

… going to throw this time)

Cherrypick couple of cleanup items from FasterXML#121

64ce4cf

Adding Feature Flag for configurability.

28ae21b

Implement FasterXML#25: add support for single-int Constructors (Fast…

380913d

…erXML#123)

Updated release notes wrt FasterXML#25, authorship

d7419f4

Adding check for Field Type + Surrogate Object Test

59f581a

Minor addition in testcase

a6ba6f4

Minor clean up from FasterXML#125

0c6eee9

Adding module for tests

53b9bde

Shounaks force-pushed the 2.17-add-failing-test-for-records branch from 9cc4bfc to 53b9bde Compare February 27, 2024 08:22

Shounaks added 2 commits February 27, 2024 13:55

Merge branch '2.17' into 2.17-add-failing-test-for-records

69f9a04

Fixing Parent Pom

caff38c

Shounaks and others added 2 commits February 29, 2024 09:44

Merge branch '2.17' into 2.17-add-failing-test-for-records

4081396

Merge branch '2.17' into 2.17-add-failing-test-for-records

9688804

cowtowncoder reviewed Feb 29, 2024

View reviewed changes

Merge branch '2.17' into 2.17-add-failing-test-for-records

44fc62e

Shounaks closed this Mar 5, 2024

Shounaks mentioned this pull request Mar 7, 2024

Adding support for Java 17 & Groovy Records #130

Merged

Shounaks deleted the 2.17-add-failing-test-for-records branch March 10, 2024 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Supports for Records (Groovy) + Passing Record Test. #122

Adding Supports for Records (Groovy) + Passing Record Test. #122

Shounaks commented Feb 22, 2024

Shounaks Feb 22, 2024

cowtowncoder Feb 23, 2024

cowtowncoder commented Feb 23, 2024

cowtowncoder Feb 23, 2024

Shounaks Feb 23, 2024

Shounaks commented Feb 23, 2024

cowtowncoder commented Feb 24, 2024 •

edited

Loading

cowtowncoder commented Feb 24, 2024

cowtowncoder commented Feb 24, 2024

Shounaks commented Feb 24, 2024

Shounaks commented Feb 27, 2024

cowtowncoder Feb 29, 2024 •

edited

Loading

cowtowncoder commented Feb 29, 2024

Shounaks commented Mar 1, 2024

Shounaks commented Mar 1, 2024

Shounaks commented Mar 5, 2024

Adding Supports for Records (Groovy) + Passing Record Test. #122

Adding Supports for Records (Groovy) + Passing Record Test. #122

Conversation

Shounaks commented Feb 22, 2024

Shounaks Feb 22, 2024

Choose a reason for hiding this comment

cowtowncoder Feb 23, 2024

Choose a reason for hiding this comment

cowtowncoder commented Feb 23, 2024

cowtowncoder Feb 23, 2024

Choose a reason for hiding this comment

Shounaks Feb 23, 2024

Choose a reason for hiding this comment

Shounaks commented Feb 23, 2024

cowtowncoder commented Feb 24, 2024 • edited Loading

cowtowncoder commented Feb 24, 2024

cowtowncoder commented Feb 24, 2024

Shounaks commented Feb 24, 2024

Shounaks commented Feb 27, 2024

cowtowncoder Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

cowtowncoder commented Feb 29, 2024

Shounaks commented Mar 1, 2024

Shounaks commented Mar 1, 2024

Shounaks commented Mar 5, 2024

cowtowncoder commented Feb 24, 2024 •

edited

Loading

cowtowncoder Feb 29, 2024 •

edited

Loading