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 @@ -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://github.com/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://github.com/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,21 @@
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 = "{\"aDouble\":0.0,\"aStr\":\"stringData\",\"anInt\":0,\"metaClass\":{}}";
Shounaks marked this conversation as resolved.
Show resolved Hide resolved
assertEquals(data, expected)
}

private class MyClass {
public int anInt; //testing groovy primitive
public String aStr = "stringData"; //testing allocated object

public double aDouble; //
public Double aDoublesd; //testing boxing object
}
}