Skip to content

Conversation

MikeDombo
Copy link
Contributor

@MikeDombo MikeDombo commented May 11, 2021

Adds options for case insensitive bean properties and enum values.
This change will close #80.

Added new tests to verify this functionality which are passing. All existing tests are passing.

As this is my first PR on this code base please forgive any issues and let me know what needs to be done to address them. I'm more than happy to work with you to get this in. Our team is trying to migrate to jackson-jr from databind to reduce our memory requirements and we've seen a large impact already.

@cowtowncoder
Copy link
Member

Ah, forgot to say first: thank you! These sound like useful improvements.

Couple of general notes:

  • Being new features, these must go in 2.13 branch (cannot add in a patch release)
  • It would make sense to split this PR in 2, to add features separately
  • Ideally patches should minimize formatting changes (removal of trailing white space is fine, but ordering of imports & expanding of wildcards should be avoided -- I know that's probably IDE doing it)

Changes are done by cleverly reusing existing mechanisms. I will need to go over this a few times but so far looks good.

@cowtowncoder
Copy link
Member

One other thing: one thing I would need, before merging, is CLA (unless I have asked for one earlier). It's here:

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

and the simplest way is usually to print, fill & sign,scan/photo, email to info at fasterxml dot com.
This is only needed once before the first contribution to any Jackson projects; one CLA is fine for all future contributions.

@MikeDombo
Copy link
Contributor Author

Thank you for the initial round of feedback. I'll work on it again tomorrow to split it up, rebase on 2.13, and undo my IDE formatting.

@MikeDombo MikeDombo changed the base branch from 2.12 to 2.13 May 11, 2021 04:47
@MikeDombo
Copy link
Contributor Author

Updated and split into #82 and #83.
CLA should be covered by existing CCLA from Amazon/Amazon Web Services, I'm following up with legal to make sure everything is good with that.

@MikeDombo MikeDombo requested a review from cowtowncoder May 12, 2021 21:03
@cowtowncoder
Copy link
Member

Very cool, thank you for covering everything I brough up. Yes, C(C)LA is fine.
I hope to go over the changes very soon now: this is on top of my todo list. This week has been bit hectic (and I will be traveling next week), but I should be able to get these merged during this week.

I really appreciate help with jackson-jr: I haven't had quite as much time to look into improving it due to other Jackson related things. But I think there is a lot of potential, and due to smaller size it is still quite a bit easier to make targeted improvements compared to "full" Jackson.
So I want to avoid being a big bottleneck wrt PRs; but at the same time obviously want to understand changes instead of just merging everything (both to help sanity check some global aspects and keep my understanding of the whole up to date).

@MikeDombo
Copy link
Contributor Author

Rebasing and ensuring every test is passing.

@MikeDombo
Copy link
Contributor Author

Rebased and all tests are passing

Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, will merge!

propMap = Collections.emptyMap();
} else {
propMap = new HashMap<String, BeanPropertyReader>();
propMap = JSON.Feature.ACCEPT_CASE_INSENSITIVE_PROPERTIES.isEnabled(_features) ? new TreeMap<String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually make TreeMap use case-insensitive matching?
Did not realize that... pretty neat if so. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and yes, after googling and reading about it found out that's exactly what happens)

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

Successfully merging this pull request may close these issues.

2 participants