(note: inspired by #4109)
As per #2715 there is a nasty race condition possibility for anyone using constants like PropertyNamingStrategy.LOWER_CAMEL_CASE or classes themselves like PropertyNamingStrategy.SnakeCaseStrategy.class; and because of this, constants and implementations were marked as deprecated.
But while these are deprecated, problematic usage is still possible. Question then is, for existing legacy usage, which is worse:
- Continuing potentially risky usage, leading to potential breakage (likely hard to diagnose)
- Explicit breakage if these classes and constants are removed and Jackson dependency is upgraded
Arguably (1) is worse, as even new code could start using problematic classes, constants (when copy-pasting).
At the same time, SemVer would suggest we only remove these from Jackson 3.0 (from which they are removed, see master.
I propose these classes, constants, would, however, be removed earlier than this, from Jackson 2.17. Please add comments on WDYT.
Comment From: JooHyukKim
~~+1 on "Not" dropping deprecated class~~ updated my vote
🔍 Reasons
- To follow
SemVer: We want people to know that Jackson followsSemVer, not the opposite. - Minimize compatibility issues : Building(compiling) projects will give deprecation warnings which users are responsible to at least read them.
- Avoid influx of inquiries : there will be. Even with best-effort warning announcements, people will come inquiring, filing issues, and making PRs, handling them and explaning over again will take up too much resources that could've been spent on improvements --compare this with recent CVE happening.
đź“‘ Todo's (More like what I think we can do)
- Amplify Warnings : If there is something more stonger than
@Deprecated - Documentation: Enhance visibility of deprecation and related risks in documentation.
- Encourage users to encourage each other : to use non-deprecated one.
In essence
Pragmatic perspective, not dropping the deprecated will improve things.
Comment From: takezoe
Thank you for raising this topic.
I personally prefer to drop these fields even in 2.x series because impact of dead-lock issue caused by these deprecated classes is significant depending on the use case even though it could happen very rarely. However, I also understand importance of backwards compatibility in SemVer concept. That's reason why I raised #4109 which (slightly) mitigates the issue without breaking backwards compatibility.
One idea is that mentioning a potential dead-lock in the deprecated message. It may make risk of using deprecated classes easier to notice.
However, we have no ways to realize that deprecated classes are used unless recompile from the source code, in particular, if those classes are used inside third-party libraries. I initially thought that producing warning log in a constructor of deprecated classes might be helpful but Jackson doesn't have a logger, unfortunately.
Anyway, it would be great if we can do something to reduce risk of dead-lock issue even if deprecated classes will be maintained in 2.x series.
Comment From: cowtowncoder
@takezoe Hmmmh. I'd be ok adding use of JDK-bundled Logger for constructors of deprecated instances. If you or anyone else wanted to do a PR.
Jackson tries to avoid use of loggers in general, but this would be reasonable case to use them.
We could even combine approach to get logging into 2.16, warning about imminent removal, then consider removal for a later 2.x version (whether 2.17 or later).
Comment From: takezoe
I see. I have seen https://github.com/FasterXML/jackson-databind/pull/2913 that dropped a dependency on java.logging module. Is it okay to re-add?
Comment From: cowtowncoder
Yes, if we add logging, would need that to be re-added as part of changes.
Comment From: JooHyukKim
@takezoe does have a point also.
Comment From: takezoe
Created a pull request for warning logs: https://github.com/FasterXML/jackson-databind/pull/4144
Comment From: JooHyukKim
Created a pull request for warning logs: https://github.com/FasterXML/jackson-databind/pull/4144
+1 on work on more and merge #4109, if logging were to be considered.
Because of potential side effects around 'java.logging' deps. Thanks
Comment From: cowtowncoder
Merged #4144 so Jackson 2.16 will start warning about usage.
We can then consider dropping from 2.17 or perhaps 2.18 (depending on how things go).
Comment From: genslein
What is the workaround or replacement for using databind annotation `@JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy.class) then? I've not seen any documentation allowing equal changes
Comment From: takezoe
@genslein @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) should be used.
Comment From: cowtowncoder
Quick note: I don't think this should be yet done for 2.17, will update title.
Comment From: cowtowncoder
Due to timing (2.19.0-rc2 already out), probably has to wait for 2.20...
Comment From: yeikel
I am late to the party, and it is no longer reversible but I personally think that it was not the right call to remove this in a minor version as it is effectively a breaking change
We encountered this unexpected issue when upgrading from what seemed a small upgrade (2.19.4 to 2.20.x), where the class was missing at runtime when a transitive dependency needed it
As a side note, this class was annotated with @Deprecated but not @Deprecated(forRemoval = true) which may explain why people were using it despite the deprecation warning
Comment From: cowtowncoder
@yeikel While I can understand friction, the reason these were dropped were due to extremely nasty possible side-effect of usage: actual JVM deadlock (which was reported). Otherwise these would have been kept until 3.0. Deprecation was done as far back as 2.12.
Good point on @Deprecated(forRemoval = true), I actually did not realize that was a thing (although TBH almost all deprecations in Jackson are for eventual removal).
Comment From: JooHyukKim
Did some research and looks like 'forRemoval=true' is used to indicate that...
its scheduled for removal in the next major release.
All say major release, But i guess next minor release can also be applied, given the benefit.
Some tools actually respect that attribute to give stronger warning.
Comment From: yeikel
@yeikel While I can understand friction, the reason these were dropped were due to extremely nasty possible side-effect of usage: actual JVM deadlock (which was reported). Otherwise these would have been kept until 3.0. Deprecation was done as far back as 2.12.
Thanks for the explanation. I completely understand your reasoning and why you chose this approach. I just wanted to comment in the hope that we can avoid this pattern for future removals. I’m not sure whether the goal of the project is full semantic compatibility, but my understanding is that, under semantic release practices, this approach wouldn’t be considered acceptable. It definitely caught me off-guard to see a broken build with a smaller bump like this but I'll consider checking the release notes more closely in the future
Good point on
@Deprecated(forRemoval = true), I actually did not realize that was a thing (although TBH almost all deprecations in Jackson are for eventual removal).
I just found out that forRemoval was introduced in Java 9, so it is possible that this may be why you missed it given that 2.x targets Java 8. I am sorry for the confusion! One alternative is to use Javadocs
For Jackson 3.x this is definitely a pattern worth considering if possible. In IDEA, for example, deprecated elements for removal are highlighted similarly to errors, which naturally creates a higher sense of urgency to move away from them.
Another piece of metadata we might consider is using the since field in the deprecated annotation
Both of these practices are recognized by many static analysis tools and could be a useful pattern to adopt if possible. I’m not imposing, just suggesting with full respect for the team’s work. Thank you as always for your efforts :)
For reference ,in case you are curious, here is how IDEA highlights them:
@Deprecated
@Deprecated(forRemoval = true)
The red underlying is the same the IDE uses for errors, which of course, draws more attention from the users
Comment From: cowtowncoder
We can definitely start using forRemoval with 3.x, so first deprecating with that 'false', then changing to 'true'.
Removal is intended only for next major version for public API, but for internal methods, types (non-public, by intent if not by visibility), things are different. SemVer has the problem it is one-size-fits-all can be impractical to follow for projects like Jackson where major version bump is similar to fork, happening once a decade and where internal API needs to evolve within a major version.
But I am sure we can improve things within those constraints.
@since is interesting as so far I assumed it was only for use wrt version introduced in ("available since ...") but makes sense it could/would mean "deprecated since" if used with @deprecated annotation.