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

Record constructor with single write-only parameter should result in properties-based creator, to fix #3897 #3910

Merged
merged 1 commit into from
May 4, 2023
Merged

Record constructor with single write-only parameter should result in properties-based creator, to fix #3897 #3910

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
4 changes: 4 additions & 0 deletions src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,10 @@ private boolean _checkIfCreatorPropertyBased(BeanDescription beanDesc,
}
}
}
// [databind#3897]: Record canonical constructor will have implicitly named propDef
if (propDef != null && !propDef.isExplicitlyNamed() && beanDesc.isRecordType()) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor thing but can remove first check and move inside previous if-block (which checks for propDef != null).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I agonized over these 2 options:

Option 1 Option 2 
private boolean _checkIfCreatorPropertyBased(...) {
    ...
    if (propDef != null) {
        ...
        if (implName != null && !implName.isEmpty()) {
            if (propDef.couldSerialize()) {
                return true;
            } else if (beanDesc.isRecordType()) {
                // [databind#3897]: Record canonical constructor will have implicitly named propDef
                return true;
            }
        }
    }
    // in absence of everything else, default to delegating
    return false;
}

private boolean _checkIfCreatorPropertyBased(...) {
    ...
    if (propDef != null) {
        ...
    }
    // [databind#3897]: Record canonical constructor will have implicitly named propDef
    if (propDef != null && !propDef.isExplicitlyNamed() && beanDesc.isRecordType()) {
        return true;
    }
    // in absence of everything else, default to delegating
    return false;
}

I chose Option 2 because it looks easier to understand with just a quick glance. But I'll change it if you feel strongly about Option 1.

return true;
}
// in absence of everything else, default to delegating
return false;
}
Expand Down
61 changes: 61 additions & 0 deletions src/test-jdk14/java/com/fasterxml/jackson/databind/records/RecordBasicsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ record SnakeRecord(String myId, String myValue){}

record RecordWithJsonDeserialize(int id, @JsonDeserialize(converter = StringTrimmer.class) String name) { }

record RecordSingleWriteOnly(@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) int id) { }

record RecordSomeWriteOnly(
@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) int id,
@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) String name,
String email) {
}

record RecordAllWriteOnly(
@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) int id,
@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) String name,
@JsonProperty(access = JsonProperty.Access.WRITE_ONLY) String email) {
}

private final ObjectMapper MAPPER = newJsonMapper();

/*
Expand Down Expand Up @@ -204,6 +218,53 @@ public void testDeserializeJsonDeserializeRecord() throws Exception {
assertEquals(new RecordWithJsonDeserialize(123, "Bob"), value);
}

/*
/**********************************************************************
/* Test methods, JsonProperty(access=WRITE_ONLY)
/**********************************************************************
*/

public void testSerialize_SingleWriteOnlyParameter() throws Exception {
String json = MAPPER.writeValueAsString(new RecordSingleWriteOnly(123));

assertEquals("{}", json);
}

// [databind#3897]
public void testDeserialize_SingleWriteOnlyParameter() throws Exception {
RecordSingleWriteOnly value = MAPPER.readValue("{\"id\":123}", RecordSingleWriteOnly.class);

assertEquals(new RecordSingleWriteOnly(123), value);
}

public void testSerialize_SomeWriteOnlyParameter() throws Exception {
String json = MAPPER.writeValueAsString(new RecordSomeWriteOnly(123, "Bob", "[email protected]"));

assertEquals("{\"email\":\"[email protected]\"}", json);
}

public void testDeserialize_SomeWriteOnlyParameter() throws Exception {
RecordSomeWriteOnly value = MAPPER.readValue(
"{\"id\":123,\"name\":\"Bob\",\"email\":\"[email protected]\"}",
RecordSomeWriteOnly.class);

assertEquals(new RecordSomeWriteOnly(123, "Bob", "[email protected]"), value);
}

public void testSerialize_AllWriteOnlyParameter() throws Exception {
String json = MAPPER.writeValueAsString(new RecordAllWriteOnly(123, "Bob", "[email protected]"));

assertEquals("{}", json);
}

public void testDeserialize_AllWriteOnlyParameter() throws Exception {
RecordAllWriteOnly value = MAPPER.readValue(
"{\"id\":123,\"name\":\"Bob\",\"email\":\"[email protected]\"}",
RecordAllWriteOnly.class);

assertEquals(new RecordAllWriteOnly(123, "Bob", "[email protected]"), value);
}

/*
/**********************************************************************
/* Internal helper methods
Expand Down