#2990 Restores unintentional API changes

[fdutton]

Resolves #2990

[cowtowncoder]

Thank you!

One quick process thing: unless I have asked for and received one already, I would need a CLA now.
It can be found from here:

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

and the usual way is to print the document, fill & sign, scan/photo, email to info at fasterxml dot com.
This only needs to be done once and is then valid for any and all contributions to Jackson repositories.

Looking forward to merging the fix for 2.12 branch (to be included in 2.12.1 patch).

[fdutton]

:-D I don't have a printer and have not had one in over a decade. I'll find some way.

[fdutton]

The CLA should be in your inbox

[cowtowncoder]

@fdutton a few contributors have found digital means to do that so physical printing definitely not a requirement: copy-pasting scanned signature on doc would be fine (for example).

[fdutton]

@cowtowncoder That's what I did. The CLA should be in your inbox.

[cowtowncoder]

Hmm.

I should have read this more carefully... I am not really happy to have to add all the complexity for backwards complexity here -- regarding POJOPropertiesCollector, especially introduction of another internal class.
Will need to see if there is no way to rip out some of the pieces.

[cowtowncoder]

Will consider reverting PR in this form; I don't quite know MonitoredList is needed here and would do not want to add more complexity like that without understanding purpose.

[cowtowncoder]

Actually rewrote it slightly, leaving other parts there (I assume existing code was relying on 2 properties that were removed).
Did not leave MonitoredList as its intent was unclear: a unit test showing the issue without additional handling would make sense here I think (reproduction of the problem this PR was to resolve).

If special handling is needed, I'd prefer an overridable access method (for example) and keeping workarounds on extending implementation.

[fdutton]

@cowtowncoder The method, _updateCreatorProperty, has a breaking change. Since it is protected and not final, it is possible that someone has overridden it in a subclass. The new behavior returns true whenever it updates the supplied list. MonitoredList was my attempt to detect changes in the list without affecting the method's signature.

The API changes caused my build to fail (i.e., compilation error) so I'm not sure how I would go about creating a unit test for that.

[cowtowncoder]

@fdutton I think the test case would be something that worked in 2.11 (can add it to that branch), and then merged into 2.12 and work (needs appropriate changes to databind non-test code too).

But I can see there is a signature change (now that I am looking) so it gets bit trickier...
This was not really a method that was expected to be ever overridden (it's in "internal helper methods" section), although since it was (unfortunately) not defined as private it was possible to do so.

What can be done is that instead of changing the signature of existing method, could add a new method (different name) which at least would not collide with possible override. It could break the override handling, still, just not at point of loading the class.

I am not quite sure how to proceed here: if changing name of override would help I can definitely do that.

[cowtowncoder]

Minor change so that signature of _updateCreatorProperty() is now same as in 2.11.x, new version is _replaceCreatorProperty(). While this may not resolve the problem it should be possible to have a reproduction of overridden handling that works with 2.11.x but probably not yet with 2.12.1-SNAPSHOT.

[fdutton]

FWIW, my project and the one open-source project I use that extends BasicClassIntrospector only does so to instantiate a custom POJOPropertiesCollector. It is quite possible that no other project has overridden any of the other protected methods or used the protected fields directly. My pull-request was an effort to eliminate any breaking changes that could cause a project to forego adopting 2.12 and get stuck on 2.11.

I am particularly interested in 2.12 because of its support for Java 14 records. My Jackson module supports Java 14 records but in a way that is not backwards compatible with Java 7. I was really hoping to drop this code now that records are supported. My module also contains a custom AnnotationIntrospector that does not infer a delegating creator when using records. I can work around the breaking API change in 2.12. However, I also use many open-source projects that use Jackson and many of these projects also inject custom introspectors. I cannot wait for all of them to develop their own strategy for dealing with a breaking API change.

One open-source project I use that is affected by this is spring-auto-restdocs. It introduces a custom POJOPropertiesCollector to to stop Jackson from removing accessors in the call to _removeUnwantedAccessor. I have a pull-request open with their project to work around the breaking change in 2.12 but have received no feedback yet. I suspect this is due to having to fork jackson-databind four times to accommodate all the variations in Jackson's version and JDK version.

Another is Apache Tinkerpop. Tinkerpop's reference implementation of Gremlin uses a shaded Jackson library, which will also have issues with Java 14's records.

Microsoft publishes spring-data-gremlin to simplify incorporating graph databases in Spring, which I wanted to use. Unfortunately, it uses a version of Tinkerpop that is stuck on Jackson 2.9. As you know, this version of Jackson receives a lot of attention from the security community due to deserializing untrusted data into various gadgets.

I don't want to sound like I am complaining about Jackson. I really admire what you have done and think that you should be proud of how much you have affected the JVM ecosystem. My only motivation is to make adoption of 2.12 as easy as possible by eliminating any issues I find.

[cowtowncoder]

@fdutton No problem at all wrt pointing out compatibility problems: I appreciate this since without feedback I could not rectify problems introduced. So I'd like to work through all issues you observe -- ideally it'd be great to solve the general problem, but sometimes piece-wise fixing (enough changes to cover observed problems, even if not all that may exist) will have to do.

I also would really like to make 2.12.x as much pop-in replacement as possible: I think I somewhat underestimated existing use of Jackson wrt Records -- I knew it is possible, with the caveat that it may be based on unsupported modifiability of fields -- and in a way did not consider possible complications regarding 1-argument case (which has been a problematic case every since allowing auto-detection of constructors, from Jackson 2.0).

So let's see if I understand specific challenges:

1. Record instropection, wrt delegating-creator
2. String-auto-restdocs, changing pruning of accessors
3. Tinkerpop using Jackson 2.9 still

So, on (1), there is a change from 2.11.x -> 2.12 in that Record auto-detection will never auto-detect delegating constructor, whereas POJOs may well do. And due to existence of field with same name as constructor parameter, 2.11 would likely have considered delegating-case to be the default.
But it sounds like the module you have would also default to Mode.PROPERTIES? Or did I misunderstand this?
(2.12 also allows changing default handling wrt POJOs, using ConstructorDetector, fwtw)

On (2) this is bit trickier since my use of underscore in method names is meant to indicate that method is NOT part of API for developers to use. Then again, in case of POJOPropertiesCollector, these are exact methods to override to make significant behavioral changes without tons of work to replicate existing logic. This class was not designed to really be extensible, despite being a significant rewrite of introspection logic (I don't remember which version this was; possibly 2.0)... which is a pity as it actually is one place where big changes are possible by sub-classing (compared to designing proper full extension points that allow replacing bits and pieces).

Anyway: once I know of existing cases of external usage and agree that it is feasible to retain compatibility, I'd really like to get a regression (unit) test to cover simplified case -- while that is not fool-proof against accidental refactoring (IDEs make it easy to automatically change test method too, alas), there is better change of realizing retrictions (Javadoc or comment for method also helps). Actually maybe even better would be test for jackson-integration-tests project which I typically do NOT keep open and would very likely catch the change.

On (3) I guess the point is that not only is it important to try to minimize 2.11 -> 2.12 challenges but also consider bigger jumps -- 2.9.x is possibly the most widely used Jackson version.

So. At this point, are there remaining known problems for 2.12.1-SNAPSHOT? (I understand that POJOPropertiesCollector can be a potential problems still, for example).
I'd like to address those, if any; hopefully releasing 2.12.1 in about a week (maybe December 29 or 30) due to couple of important fixes. But without leaving out things that could be addressed.

[fdutton]

Re 1: You are correct. My annotation introspector defaults to Mode.PROPERTIES for records. I have found that this is less confusing for developers new to Jackson. However, changing Jackson's default strategy may cause some confusion when refactoring a class into a record. At least you will get some feedback on how often delegating creators are used.

[cowtowncoder]

Right, #2980 was for the change wrt 1-arg Record constructors.

I have always been bit torn on whether delegating/properties choice for 1-argument case. The reason for delegation as sort of default (... although with heuristics if parameter names found to further confuse it) is historical: @JsonCreator did not exist in Jackson 1.0 (was added in 1.5, I think), but there was auto-detection of a subset of 1-argument constructors, mostly to support coercion from JSON String and JSON Number (int/long).
If I was to do it again, I'd probably rather not do that and assume properties-style as default always, and maybe even require different annotation to indicate delegation-based constructor/factory method.

There is also the question of whether default should change for Jackson 3.0 (just the default, requiring different annotation is probably a non-starter).
To me the biggest challenge tends to be that of estimating how widely specific feature is used: I don't really have any good way to gauge that. Asking on mailing list does not get wide enough participation and even feedback via bug reports is probably skewed (squeaky wheel problem).