Support `@JsonIgnoreProperties` to work with `@JsonValue` #4070

JooHyukKim · 2023-08-05T16:04:02Z

relates #3647 .

JooHyukKim · 2023-08-05T16:08:46Z

src/main/java/com/fasterxml/jackson/databind/ser/std/JsonValueSerializer.java

@@ -254,6 +258,20 @@ public void serialize(Object bean, JsonGenerator gen, SerializerProvider ctxt) t
        }
    }

+    private JsonSerializer<Object> _updateserWithIgnorals(JsonSerializer<Object> ser, SerializerProvider ctxt) {


This method "hypothetically" lazy initializes a serializer with @JsonIgnoreProperties applied. Only because at line 248

ser = _findDynamicSerializer(ctxt, value.getClass());

... is called, at least for our failing-test. We need to figure out how to make the serializer final 🤔

JooHyukKim · 2023-08-12T14:29:08Z

Closed for now. I am not sure if current version takes right approach.

cowtowncoder · 2023-08-19T00:16:00Z

I think this is a good approach in principle. There may be some edge cases -- like, should the new set of ignored properties replace what might already exist or combine it with union (I suspect union is the right thing to do).
This would occur if value class itself has @JsonIgnoreProperties and then @JsonValue annotated method adds more.

Also: not sure if you already saw it, but BeanDeserializerBase does have withIgnorableProperties to sort of implement this for embedded properties. Maybe that could help implementation here.

JooHyukKim · 2023-08-19T15:36:04Z

I think this is a good approach in principle.

I see. My concern was with timing, because with current revision we are detecting ignorals lazily in _findDynamicSerializer(). It might be reasonable after all for it being "Dynamic" 🤔.

There may be some edge cases -- like, should the new set of ignored properties replace what might already exist or combine it with union (I suspect union is the right thing to do).

Will try to include tests for such edge cases.

Thank you for explanation! 👍🏻

JooHyukKim · 2023-09-05T23:53:27Z

Accidentally closed 😅

cowtowncoder · 2023-10-01T01:05:35Z

src/main/java/com/fasterxml/jackson/databind/ser/BeanSerializer.java

@@ -126,6 +126,11 @@ protected BeanSerializerBase withProperties(BeanPropertyWriter[] properties,
        return new BeanSerializer(this, properties, filteredProperties);
    }

+    @Override // @since 2.16
+    public JsonSerializer<?> withIgnoredProperties(Set<String> toIgnore) {
+        return new BeanSerializer(this, toIgnore, null);


My only concern here is that passing null could remove effects of possible @JsonIncludeProperties. But looking at BeanSerializerBase, this information is not really retained explicitly -- but hopefully src._filteredProps has information.

So this is why having those tests is good verification.

So this is why having those tests is good verification.

True true, Makes sense 👌🏽

Or maybe pass empty Set<> instead? Whichever keeps codebase consistent, that's why I somehow thought passing 'null' was the way to go

Sorry, no, null is fine (wrt empty). I was thinking out aloud that originally provided inclusion cannot be passed again (which would come from @JsonIncludeProperties).

But let's worry about this if someone can figure out case where this is problematic.

cowtowncoder · 2023-10-01T01:44:04Z

Ok, merging to master is challenging here... but I'll get it done.

One thing I realized after merging, however, is that I think that possible Set<String> of ignored properties should probably obtained from within createContextual() and then used when actually constructing JsonSerializer. This avoids having to call AnnotationIntrospector multiple times.

Another thing: I wonder if making value class final would make things fail since different call path is used (serializer can be fetched eagerly).

Anyway I'll work on merge now, almost done.

cowtowncoder · 2023-10-01T02:24:40Z

src/test/java/com/fasterxml/jackson/databind/ser/filter/JsonValueIgnore3647Test.java

+// [databind#3647] : Support @JsonIgnoreProperties to work with @JsonValue
+public class JsonValueIgnore3647Test extends BaseMapTest
+{
+    static class Foo3647 {


@JooHyukKim Making this final will make test fail. I think this is because _valueSerializer has then been pre-fetched.... so fix here won't work for all cases.

Hmmm, I chose where fix is currently is (JsonValueSerializer.dynamicSerializer()) because of dynamically fetched types such as List in mind🤔

I think we can fix this either with reverting current change or just on top of this one. (Later in the evening, its 12nn atm)

JooHyukKim · 2023-10-01T03:06:08Z

should probably obtained from within createContextual() and then used when actually constructing JsonSerializer. This avoids having to call AnnotationIntrospector multiple times.

I recall trying to apply fix at 'creatContexual' and failed because not enough information or something. You think it's possible?

cowtowncoder · 2023-10-01T03:26:13Z

@JooHyukKim it's not either-or but both -- in static (final) case has to be done in createContextual(), in dynamic case in the place it's done now. I can try to see if I can do that change.

JooHyukKim · 2023-10-01T03:29:38Z

it's not either-or but both

I see, I thought there may be a common and reasonable call path for both case. Thank you for helping out🙏🙏

cowtowncoder · 2023-10-01T03:39:31Z

Hmmh. Looking at code, I think that Set<String> can and should actually be fetched right when constructing JsonValueSerializer: maybe adding a factory method for constructing instance, passing necessary config or context.
And then apply if non-empty Set, wherever JsonSerializer being delegated to is fetched.

Let me see if I can get anything further done...

cowtowncoder · 2023-10-02T02:43:30Z

@JooHyukKim I got it all working, I think. Including 3.0 where code had been refactored (and JsonValueSerializer implementation has diverged).

JooHyukKim · 2023-10-02T05:37:10Z

@cowtowncoder Phew, great! I saw commit messages in master branch and also did some research via #4134.

Quick question: Are refactoring works welcome in master branch now? Considering 2.16-rc is coming soon, I thought it would be effective point in time to prepare for 3.0 and such. My scope will cover non-intrusive refactors or increase test coverage.

JooHyukKim · 2023-10-02T16:45:07Z

My scope will cover non-intrusive refactors or increase test coverage.

Thinking now, maybe some other that I don't know of 😆

cowtowncoder · 2023-10-02T16:49:37Z

Yes. On test coverage, I hope we could reduce amount of test code without reducing test coverage: since test code is code to maintain just like regular code, over time there's more baggage. So new test code needs to be meaningful and not just to increase metrics -- I am not a big fan of coverage metrics just because it sometimes leads to "silly" tests that have more code than existing in non-test code being covered.

On refactoring, reducing amount of code (without adding overhead) would be welcome as well.

just sample code that just does not break things

3f56725

JooHyukKim commented Aug 5, 2023

View reviewed changes

JooHyukKim mentioned this pull request Aug 5, 2023

@JsonIgnoreProperties not working with @JsonValue #3647

Closed

Move ignoreCheckTo init part

df49aae

JooHyukKim closed this Aug 12, 2023

Merge branch '2.16' into 3647-

01097c0

JooHyukKim reopened this Aug 19, 2023

JooHyukKim added 5 commits August 20, 2023 00:49

Add test cases

c6ef792

Minimize new API in BeanSerializer

982fef9

Add Doc

d0b9253

Update JsonValueSerializer.java

4a7cbcf

Update JsonValueIgnore3647Test.java

ea952cf

JooHyukKim marked this pull request as ready for review August 19, 2023 15:57

JooHyukKim added 2 commits August 22, 2023 20:28

Merge branch '2.16' into 3647-

4975e23

Merge branch '2.16' into 3647-

c9d318b

JooHyukKim closed this Sep 5, 2023

JooHyukKim deleted the 3647- branch September 5, 2023 23:34

JooHyukKim restored the 3647- branch September 5, 2023 23:36

JooHyukKim deleted the 3647- branch September 5, 2023 23:38

JooHyukKim restored the 3647- branch September 5, 2023 23:53

JooHyukKim reopened this Sep 5, 2023

JooHyukKim added 3 commits September 6, 2023 08:53

Merge branch '2.16' into 3647-

8475deb

Merge branch '2.16' into 3647-

6b12519

Improve JavaDoc

0414fad

cowtowncoder reviewed Oct 1, 2023

View reviewed changes

cowtowncoder approved these changes Oct 1, 2023

View reviewed changes

cowtowncoder merged commit ae77a0c into FasterXML:2.16 Oct 1, 2023
3 checks passed

cowtowncoder reviewed Oct 1, 2023

View reviewed changes

JooHyukKim deleted the 3647- branch October 1, 2023 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `@JsonIgnoreProperties` to work with `@JsonValue` #4070

Support `@JsonIgnoreProperties` to work with `@JsonValue` #4070

JooHyukKim commented Aug 5, 2023

JooHyukKim Aug 5, 2023

JooHyukKim commented Aug 12, 2023

cowtowncoder commented Aug 19, 2023

JooHyukKim commented Aug 19, 2023 •

edited

Loading

JooHyukKim commented Sep 5, 2023

cowtowncoder Oct 1, 2023

JooHyukKim Oct 1, 2023

JooHyukKim Oct 1, 2023

cowtowncoder Oct 1, 2023

cowtowncoder commented Oct 1, 2023

cowtowncoder Oct 1, 2023

JooHyukKim Oct 1, 2023

JooHyukKim commented Oct 1, 2023

cowtowncoder commented Oct 1, 2023

JooHyukKim commented Oct 1, 2023

cowtowncoder commented Oct 1, 2023

cowtowncoder commented Oct 2, 2023

JooHyukKim commented Oct 2, 2023

JooHyukKim commented Oct 2, 2023

cowtowncoder commented Oct 2, 2023

Support @JsonIgnoreProperties to work with @JsonValue #4070

Support @JsonIgnoreProperties to work with @JsonValue #4070

Conversation

JooHyukKim commented Aug 5, 2023

JooHyukKim Aug 5, 2023

Choose a reason for hiding this comment

JooHyukKim commented Aug 12, 2023

cowtowncoder commented Aug 19, 2023

JooHyukKim commented Aug 19, 2023 • edited Loading

JooHyukKim commented Sep 5, 2023

cowtowncoder Oct 1, 2023

Choose a reason for hiding this comment

JooHyukKim Oct 1, 2023

Choose a reason for hiding this comment

JooHyukKim Oct 1, 2023

Choose a reason for hiding this comment

cowtowncoder Oct 1, 2023

Choose a reason for hiding this comment

cowtowncoder commented Oct 1, 2023

cowtowncoder Oct 1, 2023

Choose a reason for hiding this comment

JooHyukKim Oct 1, 2023

Choose a reason for hiding this comment

JooHyukKim commented Oct 1, 2023

cowtowncoder commented Oct 1, 2023

JooHyukKim commented Oct 1, 2023

cowtowncoder commented Oct 1, 2023

cowtowncoder commented Oct 2, 2023

JooHyukKim commented Oct 2, 2023

JooHyukKim commented Oct 2, 2023

cowtowncoder commented Oct 2, 2023

Support `@JsonIgnoreProperties` to work with `@JsonValue` #4070

Support `@JsonIgnoreProperties` to work with `@JsonValue` #4070

JooHyukKim commented Aug 19, 2023 •

edited

Loading