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.

Sign up for GitHub

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

Some review comments on records support #1

Merged
merged 1 commit into from
Jul 7, 2020
+49 −28
Merged

Some review comments on records support #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,9 @@
</profile>
<profile>
<!-- Build Record tests using Java 14 if JDK is available -->
<id>java14</id>
<id>java14+</id>
<activation>
<jdk>14</jdk>
<jdk>[14,</jdk>
Copy link
Owner

Choose a reason for hiding this comment

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

👍

</activation>
<build>
<plugins>
Expand Down Expand Up @@ -234,9 +234,8 @@
<inherited>true</inherited>
<configuration>
<optimize>true</optimize>
<!-- Enable Java 14 for all sources so that Intellij picks the right language level -->
<source>14</source>
<target>14</target>
<!-- Enable Java 14+ for all sources so that Intellij picks the right language level -->
<release>${java.vm.specification.version}</release>
Copy link
Owner

Choose a reason for hiding this comment

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

Using this property, in my setup Intellij does not pickup the correct version for language level support
Would putting 14 be acceptable?

Copy link
Author

Choose a reason for hiding this comment

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

It works fine for me with maven, but i have not tried with Intellij.

The reason I proposed this change is so that the release version will work with Java versions greater than 14, say e.g. 15 and beyond. java.vm.specification.version is a standard Java SE system property, and Maven exposes all properties from java.lang.System, so it is surprising that it is not working in Intellij. It's ok to drop this, for now. We can figure out if/how we want the version to roll later, if needed.

<compilerArgs>
<arg>-parameters</arg>
<arg>--enable-preview</arg>
Expand Down
54 changes: 38 additions & 16 deletions src/main/java/com/fasterxml/jackson/databind/util/RecordUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.fasterxml.jackson.databind.introspect.AnnotatedClass;
import com.fasterxml.jackson.databind.introspect.AnnotatedConstructor;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Arrays;

Expand All @@ -13,17 +14,41 @@
*/
public final class RecordUtil {

private static final String RECORD_CLASS_NAME = "java.lang.Record";
private static final String RECORD_GET_RECORD_COMPONENTS = "getRecordComponents";
private static final Method IS_RECORD;
private static final Method GET_RECORD_COMPONENTS;
private static final Method GET_NAME;
private static final Method GET_TYPE;

private static final String RECORD_COMPONENT_CLASS_NAME = "java.lang.reflect.RecordComponent";
private static final String RECORD_COMPONENT_GET_NAME = "getName";
private static final String RECORD_COMPONENT_GET_TYPE = "getType";
static {
Copy link
Owner

Choose a reason for hiding this comment

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

👍 and it has been suggested by @cowtowncoder as well

Method isRecord;
Method getRecordComponents;
Method getName;
Method getType;
try {
isRecord = Class.class.getDeclaredMethod("isRecord");
getRecordComponents = Class.class.getMethod("getRecordComponents");
Class c = Class.forName("java.lang.reflect.RecordComponent");
getName = c.getMethod("getName");
getType = c.getMethod("getType");
} catch (ClassNotFoundException| NoSuchMethodException e) {
// pre-Java-14
isRecord = null;
getRecordComponents = null;
getName = null;
getType = null;
}
IS_RECORD = isRecord;
GET_RECORD_COMPONENTS = getRecordComponents;
GET_NAME = getName;
GET_TYPE = getType;
}

public static boolean isRecord(Class<?> aClass) {
return aClass != null
&& aClass.getSuperclass() != null
&& aClass.getSuperclass().getName().equals(RECORD_CLASS_NAME);
try {
return IS_RECORD == null ? false : (boolean) IS_RECORD.invoke(aClass);
} catch (IllegalAccessException | InvocationTargetException e) {
throw new AssertionError();
}
}

/**
Expand All @@ -35,13 +60,11 @@ public static String[] getRecordComponents(Class<?> aRecord) {
}

try {
Method method = Class.class.getMethod(RECORD_GET_RECORD_COMPONENTS);
Object[] components = (Object[]) method.invoke(aRecord);
Object[] components = (Object[]) GET_RECORD_COMPONENTS.invoke(aRecord);
String[] names = new String[components.length];
Method recordComponentGetName = Class.forName(RECORD_COMPONENT_CLASS_NAME).getMethod(RECORD_COMPONENT_GET_NAME);
for (int i = 0; i < components.length; i++) {
Object component = components[i];
names[i] = (String) recordComponentGetName.invoke(component);
names[i] = (String) GET_NAME.invoke(component);
}
return names;
} catch (Throwable e) {
Expand All @@ -65,17 +88,16 @@ public static AnnotatedConstructor getCanonicalConstructor(AnnotatedClass aRecor

private static Class<?>[] getRecordComponentTypes(Class<?> aRecord) {
try {
Method method = Class.class.getMethod(RECORD_GET_RECORD_COMPONENTS);
Object[] components = (Object[]) method.invoke(aRecord);
Object[] components = (Object[]) GET_RECORD_COMPONENTS.invoke(aRecord);
Class<?>[] types = new Class[components.length];
Method recordComponentGetName = Class.forName(RECORD_COMPONENT_CLASS_NAME).getMethod(RECORD_COMPONENT_GET_TYPE);
for (int i = 0; i < components.length; i++) {
Object component = components[i];
types[i] = (Class<?>) recordComponentGetName.invoke(component);
types[i] = (Class<?>) GET_TYPE.invoke(component);
}
return types;
} catch (Throwable e) {
return new Class[0];
}
}
}

10 changes: 5 additions & 5 deletions src/test-jdk14/java/com/fasterxml/jackson/databind/RecordTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public void setUp() {
jsonMapper = new JsonMapper();
}

record SimpleRecord(int id, String name) {
public record SimpleRecord(int id, String name) {
}

public void testSerializeSimpleRecord() throws JsonProcessingException {
Expand Down Expand Up @@ -52,7 +52,7 @@ public void testDeserializeSimpleRecord_DisableAnnotationIntrospector() throws I
assertEquals(new SimpleRecord(123, "Bob"), value);
}

record RecordOfRecord(SimpleRecord record) {
public record RecordOfRecord(SimpleRecord record) {
Copy link
Owner

Choose a reason for hiding this comment

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

All test records should be public

Why? (just for me to understand :))

Copy link
Author

Choose a reason for hiding this comment

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

In Java 14 a record's canonical constructor is always public ( it has the public modifier ).
In Java 15 (and beyond) a record's canonical constructor inherits the access control modifier from that of the record declaration.

So without the change to make the record declarations public, then in Java 15 the canonical constructor is package-private, and by default Jackson cannot locale it. Now, this could be an issue or change with the default deserialisation policy in the version of the latest branch that I tested against.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the explanation

Copy link

Choose a reason for hiding this comment

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

We might want to try access of non-public Records too, for parity with POJOs. Although I don't know if it is possible to configure access for tests in a way that this is actually allowed (I assume module-info declarations are needed?).

Copy link
Author

@ChrisHegarty ChrisHegarty Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Adding tests to check for non-public Records would be good ( we have such in the JDK serialization unit tests ).

When running the existing RecordTest in my local environment with a JDK 15 early-access build I did see failures - since the record's canonical constructor was not public (it was package-private inherited from the record declaration). These failures may be because I was using the tip of jackson-databind where the default visibility of creator differs? Or maybe because of not explicitly setting the visibility checker in the test? I'm not sure. Either way, it should be possible to write tests where the record (and it's canonical constructor) are not accessible to Jackson ( package-private ), and ensure that whatever deserialization/visibility configuration work as expected.

}

public void testSerializeRecordOfRecord() throws JsonProcessingException {
Expand All @@ -63,7 +63,7 @@ public void testSerializeRecordOfRecord() throws JsonProcessingException {
assertEquals("{\"record\":{\"id\":123,\"name\":\"Bob\"}}", json);
}

record JsonIgnoreRecord(int id, @JsonIgnore String name) {
public record JsonIgnoreRecord(int id, @JsonIgnore String name) {
}

public void testSerializeJsonIgnoreRecord() throws JsonProcessingException {
Expand All @@ -74,7 +74,7 @@ public void testSerializeJsonIgnoreRecord() throws JsonProcessingException {
assertEquals("{\"id\":123}", json);
}

record RecordWithConstructor(int id, String name) {
public record RecordWithConstructor(int id, String name) {
public RecordWithConstructor(int id) {
this(id, "name");
}
Expand All @@ -86,7 +86,7 @@ public void testDeserializeRecordWithConstructor() throws IOException {
assertEquals(new RecordWithConstructor(123, "Bob"), value);
}

record JsonPropertyRenameRecord(int id, @JsonProperty("rename")String name) {
public record JsonPropertyRenameRecord(int id, @JsonProperty("rename")String name) {
}

public void testSerializeJsonRenameRecord() throws JsonProcessingException {
Expand Down
4 changes: 2 additions & 2 deletions src/test-jdk14/java/com/fasterxml/jackson/databind/util/RecordUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public void getRecordComponents() {
assertArrayEquals(new String[]{}, RecordUtil.getRecordComponents(String.class));
}

record SimpleRecord(String name, int id) {
public record SimpleRecord(String name, int id) {
public SimpleRecord(int id) {
this("", id);
}
Expand All @@ -47,4 +47,4 @@ public void getCanonicalConstructor() {
)));
}

}
}