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

Dont track unknown props in buffer if ignoreAllUnknown is true #3082

Merged
merged 2 commits into from Mar 30, 2021

Conversation

dhofftgt
Copy link
Contributor

In performance testing I was seeing that keeping track of unknown properties had an impact. If ignoreUnknownProperties is true, this buffer is ignored anyway.

@cowtowncoder
Copy link
Member

Sounds good, in general. Performance optimization would be nice.

My only/main concern is that beside regular well-defined properties, there are at least 2 kinds of "unknown" property types:

  1. "Any" properties (via @JsonAnySetter)
  2. Unwrapped properties (properties of flattened child POJOs, via @JsonUnwrapped)
  3. (maybe?) Properties for polymorphic cases

and specifically I am not 100% sure if there is test coverage; for cases where "ignore all unknown" is enabled and one of above is used.
Looking at method itself, any properties seem to be handled ok, and I suspect that unwrapped case would go to different method.
And not sure if (3) is valid.

Anyway: it would be great if you could add a new test under src/test/java/com/fasterxml/jackson/databind/ser/filter/ -- even if just covering "any setter" and single @JsonUnwrapped case?
I can add polymorphic one.

@dhofftgt
Copy link
Contributor Author

ok, I added a few test cases.

@cowtowncoder
Copy link
Member

@dhofftgt Excellent, thank you & apologies for slow follow up here.

The only thing then, before I merge this in, would be CLA (unless I have gotten one before -- just let me know if so, I didn't notice one). If needed, it's a 1-page doc here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and the common way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
Once I get it I'll merge this PR and it'll be in 2.13.0!

Thank you once again for contributing this.

@dhofftgt
Copy link
Contributor Author

@cowtowncoder I work at Target and I had them sign a CCLA. Do I still need to sign an individual CLA?

@cowtowncoder
Copy link
Member

Nope: CCLA is fine.

@cowtowncoder
Copy link
Member

@dhofftgt I assume CCLA will be sent in near future, and if so, that's fine and I'll keep checking my fasterxml.com account.

But just in case it was sent earlier, I don't think I have seen it: I mention this because occasionally clas have ended up on gmail spam folder for some reason only known to gods of gmail. :)
So when it will be send feel free to ping me and I make sure to locate the document.

@dhofftgt
Copy link
Contributor Author

@cowtowncoder should have been sometime around 3/16 when it was sent. I will follow up today to see how/when it was sent or resend it.

@dhofftgt
Copy link
Contributor Author

@cowtowncoder Just heard back, it was sent to clas@fasterxml.com. It's being forwarded to info@fasterxml.com now.

@cowtowncoder
Copy link
Member

Ah. Yes, received it. Thanks!

@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Mar 30, 2021
@cowtowncoder cowtowncoder merged commit 23f9c61 into FasterXML:2.13 Mar 30, 2021
@cowtowncoder cowtowncoder changed the title dont track unknown props in buffer if ignoreAllUnknown is true Dont track unknown props in buffer if ignoreAllUnknown is true Mar 30, 2021
cowtowncoder added a commit that referenced this pull request Mar 30, 2021
dhofftgt added a commit to dhofftgt/jackson-databind that referenced this pull request Apr 5, 2021
…rXML#3082)

Dont track unknown props in buffer if ignoreAllUnknown is true.

(cherry picked from commit 23f9c61)
dhofftgt pushed a commit to dhofftgt/jackson-databind that referenced this pull request Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants