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

Skipping over groovy metadata class + groovy test. #118

Merged
merged 9 commits into from
Feb 21, 2024
9 changes: 6 additions & 3 deletions jr-objects/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ has no other dependencies, and provides additional builder-style content generat
<artifactId>jackson-core</artifactId>
<version>${jackson.version.core}</version>
</dependency>
<dependency>
<groupId>org.codehaus.groovy</groupId>
<artifactId>groovy</artifactId>
<version>3.0.18</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand All @@ -64,9 +70,6 @@ has no other dependencies, and provides additional builder-style content generat
<goal>replace</goal>
</goals>
<phase>generate-sources</phase>
<!--
<phase>process-sources</phase>
-->
</execution>
</executions>
<configuration>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private POJODefinition _construct(Class<?> beanType, int features)
private static void _introspect(Class<?> currType, Map<String, PropBuilder> props,
int features)
{
if (currType == null || currType == Object.class) {
if (currType == null || currType == Object.class || isGroovyMetaClass(currType)) {
return;
}
// First, check base type
Expand Down Expand Up @@ -117,7 +117,7 @@ private static void _introspect(Class<?> currType, Map<String, PropBuilder> prop
// 13-Jun-2015, tatu: Skip synthetic, bridge methods altogether, for now
// at least (add more complex handling only if absolutely necessary)
if (Modifier.isStatic(flags)
|| m.isSynthetic() || m.isBridge()) {
|| m.isSynthetic() || m.isBridge() || isGroovyMetaClass(m.getReturnType())) {
continue;
}
Class<?> argTypes[] = m.getParameterTypes();
Expand Down Expand Up @@ -158,12 +158,7 @@ private static void _introspect(Class<?> currType, Map<String, PropBuilder> prop
}

private static PropBuilder _propFrom(Map<String,PropBuilder> props, String name) {
PropBuilder prop = props.get(name);
if (prop == null) {
prop = Prop.builder(name);
props.put(name, prop);
}
return prop;
return props.computeIfAbsent(name, Prop::builder);
}

private static String decap(String name) {
Expand All @@ -182,4 +177,13 @@ private static String decap(String name) {
return name;
}

/**
* Another helper method to deal with Groovy's problematic metadata accessors
*
* @implNote Groovy MetaClass have cyclic reference, and hence the class containing it should not be serialised without
* either removing that reference, or skipping over such references.
*/
protected static boolean isGroovyMetaClass(Class<?> clazz) {
return clazz.getName().startsWith("groovy.lang");
Copy link
Member

Choose a reason for hiding this comment

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

This needs to check for class name, not just package, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially i implemented it to check for just MetaClassImpl, but after checking jackson-databind, i just replicated its exclusion.

https:/FasterXML/jackson-databind/blob/db95863d6f48cb5695bf8e14af1e7af2ece8e52e/src/main/java/com/fasterxml/jackson/databind/util/BeanUtil.java#L205

is this correct? or should we go for targeted exclusions?

Copy link
Member

Choose a reason for hiding this comment

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

Note calling line:

https:/FasterXML/jackson-databind/blob/db95863d6f48cb5695bf8e14af1e7af2ece8e52e/src/main/java/com/fasterxml/jackson/databind/util/BeanUtil.java#L59

which actually checks class name: helper method name is bit misleading I agree. But yes, I think we should target to just that class, unless proven something else is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood, I missed this code, will implement in a similar fashion.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package com.fasterxml.jackson.jr

import com.fasterxml.jackson.jr.ob.JSON
import com.fasterxml.jackson.jr.ob.TestBase

class GroovyTest extends TestBase {

void testSimpleObject() throws Exception {
var data = JSON.std.asString(new MyClass())
var expected = """{"AAAAA_A_Field_Starting_With_Two_Capital_Letters":"XYZ","aDouble":0.0,"aPublicInitializedInteger":56,"aPublicInitializedIntegerObject":1516,"aPublicUninitializedInteger":0,"anInitializedIntegerObject":1112,"anInitializedPublicString":"stringData","anInitializedString":"ABC","anInteger":0,"anIntegerWithValue":12}"""
Copy link
Member

Choose a reason for hiding this comment

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

Was about to ask about var and triple quotes wrt JDK 8 before realizing this is Groovy :)

Copy link
Member

@cowtowncoder cowtowncoder Feb 21, 2024

Choose a reason for hiding this comment

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

One question tho: how does this test get run? It doesn't look like it runs with ./mvnw clean test from main level...
And commenting out handling of Groovy metadata makes nothing fail (ditto for explicit fail() in test).

Maybe test would need to be under src/test/groovy instead of src/test/java?

Copy link
Member

Choose a reason for hiding this comment

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

assertEquals(data, expected)
}

private class MyClass {
int anInteger
int anIntegerWithValue = 12

static int anStaticInteger = 34
static int anStaticIntegerWithValue = 34

public int aPublicUninitializedInteger
public int aPublicInitializedInteger = 56

private int aPrivateUninitializedInteger
private int aPrivateInitializedInteger = 78

public static int aPublicStaticUninitializedInteger
public static int aPublicStaticInitializedInteger = 910

Integer anIntegerObject
Integer anInitializedIntegerObject = 1112

static Integer aStaticIntegerObject
static Integer aStaticInitializedIntegerObject = 1314

public Integer aPublicUninitializedIntegerObject
public Integer aPublicInitializedIntegerObject = 1516

public static Integer aPublicStaticUninitializedIntegerObject
public static Integer aPublicStaticInitializedIntegerObject = 1718

String aString
String anInitializedString = "ABC"

static String aStaticString = "jacksonJR"

public String aPublicString
public String anInitializedPublicString = "stringData"

public String AAAAA_A_Field_Starting_With_Two_Capital_Letters = "XYZ"
//Other Items
public static String staticStr = "jacksonJR" // Public Static Object
static int anStaticInt // Uninitialized Static Object
public double aDouble // uninitialized primitive
public Double aDoubleObject // testing boxing object
private int hiddenvalue = 123 // private value
}
}