-
Notifications
You must be signed in to change notification settings - Fork 175
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
Preserve read after write #406
Open
dinomite
wants to merge
3
commits into
2.12
Choose a base branch
from
preserve-read-after-write
base: 2.12
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem right to me -- more like an arbitrary change that makes specific case not fail the way it did.
And it is not a good place to make the change either, as name mangling aspects are not expected to be handled by
AnnotationIntrospector
.I realize that there might not be a better way to fix it at the moment (2.12 or earlier), but, FWTW, I plan on revamping property introspection for 2.13. And this time try to keep module owners better in the loop regarding changes too.
Anyway: If change is to be made, I would recommend adding a comment with link to this PR and/or issue (yes, I know, git history can show it if one knows where to look) to explain the logic of making a change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If name mangling isn't expected in the introspector, why is it mangling (decapitalizing, etc) the names that start with 'get' and 'is'? Also, mangling is explicitly managed by the introspector in the next method (findRenameByField), so if this isn't the right place for mangling, a lot more changes are needed to move it to the right place...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible that some other (relatively recent) changes are similarly sub-optimal (especially code right before your change in same method) but adding more of the same does not help.
But specifically, I think name-mangling in "findImplicitPropertyName()" seems wrong just due to its place in call sequences; it is a low-level accessor, whereas "findRenameByField()" has slightly different function.
I agree that "findRenameByField()" is not the best design choice either (and it is something I added).
I think my main concern here is just that I am not sure I understand intended logic, and strongly suspect that this is working around a real problem, but one that is not due to missing name-mangling but due to missing linkage between constructors and fields -- something that I know is an existing problem wrt auto-detected (non-annotated) constructors.
Still, if you could explain the intended logic in a comment, that would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shall add commentary in the morning, after finding references to where the standard getXxx and setXxx names get mangled, too (which is where I learned of the mangling routines in the first place).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Jackson 2.12 changed this handling a bit, trying to make it easier to replace pieces, moving away (ideally) from static helper methods.
I will also try to keep everyone updated on my plans for
jackson-databind
2.13, via "jackson-dev" mailing list (and related Github issue for jackson-databind, once I get little bit further).So even if there is a work-around here for 2.12, perhaps it could be further refactored into nicer packaging, once there is a better way (there might not be in 2.12).