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

@JsonInclude(value = JsonInclude.Include.CUSTOM,valueFilter = SomeFilter.class) does not work for fields of record (fixed in 2.18.0) #4754

Closed
1 task done
swpalmer-cl opened this issue Oct 17, 2024 · 12 comments
Labels
Milestone

Comments

@swpalmer-cl
Copy link

swpalmer-cl commented Oct 17, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

The valueFilter is not used at all when applied to a field of a record.
An object of the filter class is not even constructed.

Version Information

2.17.0

Reproduction

public record MyRecord(
    @JsonInclude(
        value = JsonInclude.Include.CUSTOM,
        valueFilter = AttributeTypeSerializationFilter.class)
    String attributeType,
    String attributeValue) {

  // Never return null for the attributeType, instead return the default
  public MyRecord {
    if (attributeType == null) attributeType = "String";
  }

  static class AttributeTypeSerializationFilter {

    public AttributeTypeSerializationFilter() {
      System.out.println("AttributeTypeFilter.init");
    }

    @Override
    // return true to exclude
    public boolean equals(Object obj) {
      System.err.println("AttributeTypeFilter: " + obj);
      return obj == null || "String".equals(obj);
    }
  }
}

Expected behavior

@JsonInclude(value=JsonInclude.Include.CUSTOM, ... ) should work on record fields as it does for class fields.

Additional context

The idea is to reduce the JSON payload by excluding default values, but those values are non-null.

@swpalmer-cl swpalmer-cl added the to-evaluate Issue that has been received but not yet evaluated label Oct 17, 2024
@JooHyukKim
Copy link
Member

Did it work in previous versions? If so, could you share with us which?

@yihtserns
Copy link
Contributor

yihtserns commented Oct 17, 2024

More importantly, where are the steps to reproduce the issue?

@cowtowncoder cowtowncoder added the need-test-case To work on issue, a reproduction (ideally unit test) needed label Oct 17, 2024
@cowtowncoder
Copy link
Member

Also: 2.17.0 is neither latest patch (2.17.2) (nor latest in general (2.18.0)).
So should:

  1. Verify it occurs on 2.17.2 (slight possibility of fix)
  2. Ideally verify against 2.18.0 (reasonable chance might be fixed)

so we won't waste time looking into something that might already be fixed.

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Oct 17, 2024
@yihtserns
Copy link
Contributor

BTW I'm asking for steps to reproduce the issue because I cannot reproduce it with a 2.17.0 + Record + custom filter + ObjectMapper.writeValueAsString(...), so I highly suspect that the report is missing some context.

@swpalmer-cl
Copy link
Author

swpalmer-cl commented Oct 18, 2024

I will try to create a more complete example from my code, but just for reference. I took the record I had, and re-wrote it as a class and only then did the filter get called. The ObjectMapper was configured exactly the same in both cases.

@yihtserns
Copy link
Contributor

All passed:

public class JsonIncludeCustomTest {

    private ObjectMapper objectMapper = new ObjectMapper();

    @Test
    public void testBean() throws Exception {
        MyBean beanNone = new MyBean();
        beanNone.setName("N/A");
        beanNone.setAddress("N/A");

        MyBean beanDoe = new MyBean();
        beanDoe.setName("Doe");
        beanDoe.setAddress("Doe");

        assertEquals("{\"address\":\"N/A\"}", objectMapper.writeValueAsString(beanNone));
        assertEquals("{\"name\":\"Doe\",\"address\":\"Doe\"}", objectMapper.writeValueAsString(beanDoe));
    }

    @Test
    public void testRecord() throws Exception {
        MyRecord recordNone = new MyRecord("N/A", "N/A");
        MyRecord recordDoe = new MyRecord("Doe", "Doe");

        assertEquals("{\"address\":\"N/A\"}", objectMapper.writeValueAsString(recordNone));
        assertEquals("{\"name\":\"Doe\",\"address\":\"Doe\"}", objectMapper.writeValueAsString(recordDoe));
    }

    public static class MyBean {

        @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = NameFilter.class)
        private String name;
        private String address;

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }

        public String getAddress() {
            return address;
        }

        public void setAddress(String address) {
            this.address = address;
        }
    }

    record MyRecord(
            @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = NameFilter.class) String name,
            String address) {
    }

    public static class NameFilter {

        @Override
        public boolean equals(Object obj) {
            return "N/A".equals(obj);
        }
    }
}

@swpalmer-cl
Copy link
Author

swpalmer-cl commented Oct 18, 2024

I think I found the issue...
Change the record to:

    record MyRecord(
            @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = NameFilter.class) String name,
            String address) {
        @Override
        public String name() {
            if (name == null) return "N/A";
            return name;
        }
    }

You don't even need to do anything "odd" in the name() accessor method. This also fails:

        @Override
        public String name() {
            return name;
        }

So even if you just wanted to add logging or something it will skip the filter simply by overriding the default accessor method.

@yihtserns
Copy link
Contributor

yihtserns commented Oct 18, 2024

Right, I missed the accessor method override, relying too much on the issue title (which should maybe be updated to "@JsonInclude(value = JsonInclude.Include.CUSTOM,valueFilter = SomeFilter.class) does not work for Record field when the accessor is overridden")

The cause is the same as #4626, and has been fixed in 2.18.0.

@cowtowncoder
Copy link
Member

@swpalmer-cl Did you try this with other versions like I requested? There is little value in working on non-latest patch (2.17.0) and there is strong indicates this might be fixed in 2.18.0 -- and if so, would be documented as such (fix unlikely to be backportable due to major rewrite in 2.18).

@swpalmer-cl
Copy link
Author

@cowtowncoder I did not, I apologise. It is indeed fixed in 2.18.0

@yihtserns
Copy link
Contributor

I forgot to mention an alternative/workaround if you can't/won't upgrade - move the annotation to the overridden accessor method:

record MyRecord(String name, String address) {

    @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = NameFilter.class) 
    @Override
    public String name() {
        if (name == null) return "N/A";
        return name;
    }
}

@cowtowncoder
Copy link
Member

Thank you @swpalmer-cl!

@cowtowncoder cowtowncoder added this to the 2.18.0 milestone Oct 18, 2024
@cowtowncoder cowtowncoder added 2.18 and removed need-test-case To work on issue, a reproduction (ideally unit test) needed labels Oct 18, 2024
@cowtowncoder cowtowncoder changed the title @JsonInclude(value = JsonInclude.Include.CUSTOM,valueFilter = SomeFilter.class) does not work for fields of record @JsonInclude(value = JsonInclude.Include.CUSTOM,valueFilter = SomeFilter.class) does not work for fields of record (fixed in 2.18.0) Oct 18, 2024
@cowtowncoder cowtowncoder changed the title @JsonInclude(value = JsonInclude.Include.CUSTOM,valueFilter = SomeFilter.class) does not work for fields of record (fixed in 2.18.0) @JsonInclude(value = JsonInclude.Include.CUSTOM,valueFilter = SomeFilter.class) does not work for fields of record (fixed in 2.18.0) Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants