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

Log WARN if deprecated subclasses of PropertyNamingStrategy is used #4144

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

takezoe
Copy link
Contributor

@takezoe takezoe commented Oct 6, 2023

Subclasses (and static fields) of PropertyNamingStrategy have been deprecated since Jackson 2.12 due to risk of deadlock described in #2715. Since Jackson respect backwards compatibility based on SemVer concept, those classes should be alive in Jackson 2.x.

However the impact of deadlock is significant depending on the use case but we can't notice use of those classes without recompile from the source code, and no chance if those classes are used inside third-party libraries.

This pull request is an idea to help users notice by producing warning logs at runtime when those classes are instantiated.

See the discussion in #4136 for more details.

@takezoe takezoe force-pushed the warn-PropertyNamingStrategy branch 2 times, most recently from 3030206 to 1afe9a5 Compare October 6, 2023 17:32
@JooHyukKim
Copy link
Member

JooHyukKim commented Oct 6, 2023

Can we consider comparing with #4109 what would affect users' runtime less but only benefit deadlock issue more?

PS: I am saying this because adding 'java.logging' package to give only warning about deadlock seems less effective than adding a couple more lines of code to actually address the issue.

@takezoe
Copy link
Contributor Author

takezoe commented Oct 7, 2023

A considerable difference is that #4109 only covers specific use cases (when deprecated classes are specified in @JsonNaming). Producing runtime warnings will give users a chance to notice any use of deprecated classes.

Of course, I personally prefer #4109 because it actually solves the issue even though covered use cases are limited so I proposed it first, but it was unlikely to be accepted?

@yawkat
Copy link
Member

yawkat commented Oct 9, 2023

java.util.logging is not amazing and in many cases is not compatible with frameworks.

I think an optional dependency on slf4j would be better. It will work seamlessly with most frameworks, and we can fall back to doing nothing if it's missing.

@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 10, 2023

While I much prefer slf4j all around, I do want to keep strict minimization of external dependencies. And given that this is ONLY to be used for warning about Deprecated cases and not for "real" logging, I think java.util.logging will (have to) do just fine.

And yes, @takezoe, I do prefer logging for this particular case.

@cowtowncoder cowtowncoder added the 2.16 Issues planned for 2.16 label Oct 10, 2023
@yawkat
Copy link
Member

yawkat commented Oct 10, 2023

the problem with jul is that it pulls in a bunch of code and does not work well with most logging setups people actually use.

But you're right, ideally this log call is never hit anyway, so it is low stakes.

@cowtowncoder cowtowncoder merged commit 6595974 into FasterXML:2.16 Oct 12, 2023
3 checks passed
@cowtowncoder cowtowncoder changed the title Warn if deprecated subclasses of PropertyNamingStrategy is used Log WARN if deprecated subclasses of PropertyNamingStrategy is used Oct 12, 2023
@JooHyukKim
Copy link
Member

@takezoe Greate job and patience! 🎉

cowtowncoder added a commit that referenced this pull request Oct 12, 2023
@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Oct 12, 2023
@cowtowncoder
Copy link
Member

@yawkat Exactly.

@cowtowncoder
Copy link
Member

Ugggggh. Just realized that this has a MAJOR problem -- due to way things are done, any reference PropertyNamingStrategy.class will trigger 4 warnings. And that is not good.

The issue is that while PropertyNamingStrategy has static instances of deprecated implementations, so when it gets loaded, warnings get logged. Regardless of whether any of 4 is actually used.
So this works exactly wrong. I should have caught this earlier, as this is obvious in hindsight.

The question, then, is whether we can change things to avoid this. One possibility would be to add new constructors that take bogus arg -- like single boolean -- and do NOT log; and make static instances use these constructors.
0-arg constructors need to be left in place for annotation use; these would still log warnings.

Alternatively I would have to revert this change, as the additionally incorrect warnings are a major nuisance and we really do not want that.

@JooHyukKim
Copy link
Member

related PR #4162 (Just linking things)

@cowtowncoder
Copy link
Member

I am proceeding with a work-around, PR to be created shortly. Crisis averted :)

@cowtowncoder
Copy link
Member

Fixing via #4163.

@takezoe
Copy link
Contributor Author

takezoe commented Oct 17, 2023

Ahhhh, I see... Thank you for fixing by #4163. 🙇‍♂️

@cowtowncoder
Copy link
Member

Np @takezoe . Thank you for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants