-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
#2990 Restores unintentional API changes #2991
Conversation
Thank you! One quick process thing: unless I have asked for and received one already, I would need a CLA now. 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 Looking forward to merging the fix for 2.12 branch (to be included in 2.12.1 patch). |
:-D I don't have a printer and have not had one in over a decade. I'll find some way. |
The CLA should be in your inbox |
@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). |
@cowtowncoder That's what I did. The CLA should be in your inbox. |
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 consider reverting PR in this form; I don't quite know |
Actually rewrote it slightly, leaving other parts there (I assume existing code was relying on 2 properties that were removed). If special handling is needed, I'd prefer an overridable access method (for example) and keeping workarounds on extending implementation. |
@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. |
@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... 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. |
Minor change so that signature of |
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. |
@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:
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. 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 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 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 |
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. |
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: 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). |
Resolves #2990