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

Ignore Records' immutable fields in BeanDeserializerFactory instead of ignoring it in POJOPropertiesCollector, to preserve any annotation info the fields may be carrying. #4627

Merged
merged 7 commits into from
Jul 21, 2024
5 changes: 3 additions & 2 deletions release-notes/CREDITS-2.x
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Co-Authors (with only partial listings below):

* Joo Hyuk Kim (JooHyukKim@github)
* PJ Fanning (pjfanning@github)
* Sim Yih Tsern (yihtsern@github)

----------------------------------------------------------------------------

Expand Down Expand Up @@ -1624,9 +1625,9 @@ Sim Yih Tsern (yihtsern@github)
* Contributed fix for #3897: 2.15.0 breaks deserialization when POJO/Record only has a
single field and is marked `Access.WRITE_ONLY`
(2.15.1)
* Contributed fux fix #3968: Records with additional constructors failed to deserialize
* Contributed fix fix #3968: Records with additional constructors failed to deserialize
(2.15.3)

... and many more

Ajay Siwach (Siwach16@github)
* Contributed #3637: Add enum features into `@JsonFormat.Feature`
Expand Down
6 changes: 6 additions & 0 deletions release-notes/VERSION-2.x
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ Project: jackson-databind
(reported by Eduard G)
#4617: Record property serialization order not preserved
(reported by @GeorgiPetkov)
#4626: `@JsonIgnore` on Record property ignored for deserialization, if there
is getter override
(contributed by @yihtserns)
#4630: `@JsonIncludeProperties`, `@JsonIgnoreProperties` ignored when serializing Records,
if there is getter override
(contributed by @yihtserns)

2.17.2 (05-Jul-2024)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -977,8 +977,14 @@ protected SettableBeanProperty constructSettableProperty(DeserializationContext
beanDesc.getClassAnnotations(), (AnnotatedMethod) mutator);
} else {
// 08-Sep-2016, tatu: wonder if we should verify it is `AnnotatedField` to be safe?
prop = new FieldProperty(propDef, type, typeDeser,
beanDesc.getClassAnnotations(), (AnnotatedField) mutator);
AnnotatedField field = (AnnotatedField) mutator;
// [databind#3736] Pointless to create a SettableBeanProperty for an immutable field
// Records' fields can't mutated via reflection (JDK-8247517)
// (also see [databind#4626]
if (beanDesc.isRecordType()) {
return null;
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
}
prop = new FieldProperty(propDef, type, typeDeser, beanDesc.getClassAnnotations(), field);
}
JsonDeserializer<?> deser = findDeserializerFromAnnotation(ctxt, mutator);
if (deser == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,13 +434,7 @@ protected void collectAll()
// First: gather basic accessors
LinkedHashMap<String, POJOPropertyBuilder> props = new LinkedHashMap<String, POJOPropertyBuilder>();

// 15-Jan-2023, tatu: [databind#3736] Let's avoid detecting fields of Records
// altogether (unless we find a good reason to detect them)
// 17-Apr-2023: Need Records' fields for serialization for cases
// like [databind#3628], [databind#3895] and [databind#3992]
if (!isRecordType() || _forSerialization) {
_addFields(props); // note: populates _fieldRenameMappings
}
_addFields(props); // note: populates _fieldRenameMappings
_addMethods(props);
// 25-Jan-2016, tatu: Avoid introspecting (constructor-)creators for non-static
// inner classes, see [databind#1502]
Expand Down Expand Up @@ -1301,10 +1295,7 @@ protected void _removeUnwantedProperties(Map<String, POJOPropertyBuilder> props)
*/
protected void _removeUnwantedAccessor(Map<String, POJOPropertyBuilder> props)
{
// 15-Jan-2023, tatu: Avoid pulling in mutators for Records; Fields mostly
// since there should not be setters.
final boolean inferMutators = !isRecordType()
&& _config.isEnabled(MapperFeature.INFER_PROPERTY_MUTATORS);
final boolean inferMutators = _config.isEnabled(MapperFeature.INFER_PROPERTY_MUTATORS);
Copy link
Contributor Author

@yihtserns yihtserns Jul 21, 2024

Choose a reason for hiding this comment

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

I didn't really understand this change, but I reverted it simply because it was part of #3737 even though it has no effect on #4626.

But I just found out that reverting this fixed #4630.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

Iterator<POJOPropertyBuilder> it = props.values().iterator();

while (it.hasNext()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,14 @@ public void testHelloRecord() throws Exception {
HelloRecord result = MAPPER.readValue(json, HelloRecord.class);
assertNotNull(result);
}

// [databind#4626]
@Test
public void testDeserialize() throws Exception {
HelloRecord expected = new HelloRecord("hello", null);

assertEquals(expected, MAPPER.readValue(a2q("{'text':'hello'}"), HelloRecord.class));
assertEquals(expected, MAPPER.readValue(a2q("{'text':'hello','hidden':null}"), HelloRecord.class));
assertEquals(expected, MAPPER.readValue(a2q("{'text':'hello','hidden':{'all': []}}"), HelloRecord.class));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,11 @@ public void testSerializeJsonIgnoreAccessorRecord() throws Exception {

@Test
public void testDeserializeJsonIgnoreAccessorRecord() throws Exception {
RecordWithIgnoreAccessor value = MAPPER.readValue("{\"id\":123,\"name\":\"Bob\"}",
RecordWithIgnoreAccessor.class);
assertEquals(new RecordWithIgnoreAccessor(123, null), value);
RecordWithIgnoreAccessor expected = new RecordWithIgnoreAccessor(123, null);

assertEquals(expected, MAPPER.readValue("{\"id\":123}", RecordWithIgnoreAccessor.class));
assertEquals(expected, MAPPER.readValue("{\"id\":123,\"name\":null}", RecordWithIgnoreAccessor.class));
assertEquals(expected, MAPPER.readValue("{\"id\":123,\"name\":\"Bob\"}", RecordWithIgnoreAccessor.class));
}

/*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package com.fasterxml.jackson.databind.records;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonIncludeProperties;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.testutil.DatabindTestUtil;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class RecordWithOverriddenAccessorTest extends DatabindTestUtil {

record Id2Name(int id, String name) {
}

record RecordWithJsonIncludeProperties(@JsonIncludeProperties("id") Id2Name child) {
@Override
public Id2Name child() {
return child;
}
}

record RecordWithJsonIgnoreProperties(@JsonIgnoreProperties("name") Id2Name child) {
@Override
public Id2Name child() {
return child;
}
}

private final ObjectMapper MAPPER = newJsonMapper();

// [databind#4630]
@Test
public void testSerializeJsonIncludeProperties() throws Exception {
String json = MAPPER.writeValueAsString(new RecordWithJsonIncludeProperties(new Id2Name(123, "Bob")));
assertEquals(a2q("{'child':{'id':123}}"), json);
}

// [databind#4630]
@Test
public void testSerializeJsonIgnoreProperties() throws Exception {
String json = MAPPER.writeValueAsString(new RecordWithJsonIgnoreProperties(new Id2Name(123, "Bob")));
assertEquals(a2q("{'child':{'id':123}}"), json);
}
}