Use locale passed to ObjectMapper configuration everywhere

[Dimezis]

Allows to fix Turkish i parsing #953 (comment).

So the idea behind this fix is to propagate a proper Locale that user set during ObjectMapper configuration. This way, the user can decide if he wants to parse in a particular Locale.

There's another consequence as well, previously, because the Json readers are cached, we could end up in a state when Locale.getDefault() is different than it was during reader creation. So the same reader would fail during parsing of the exact same Json it tried to parse before.

Example:

1. Set Turkish locale
2. Parse to Bean.class having uppercase I in the property name
3. Set UK locale
4. Parse Bean.class again
5. Parsing fails

I deliberately didn't create unit tests for classes that use toLowerCase(locale), and decided to test the whole ObjectMapper integrated with real implementations of those.

Apologies if I didn't follow some other conventions of the project, or if I'm targetting the wrong branch.

[Dimezis]

What to do here? can I change the constructor even though it's deprecated?

[cowtowncoder]

Odd, 3.0 should not have any deprecated ones, must have leaked via merge from 2.x (where addition of new constructor does lead to marking of old ones as deprecated). Signatures of deprecated methods should not be changed in most cases as they are strictly left for backwards compatibility (for old code that is compiled to access them).
But at the same time, deprecated methods need not support new features so using bogus (or default) Locale would be fine.

[Dimezis]

I'm not sure how to approach this one. We can't really pass the locale here.
But at the same time it's not causing (known) problems yet

[cowtowncoder]

Ok, one quick comment: one immediate problem is that we can not just change PropertyNamingStrategy API in a minor version without backwards compatibility. Could definitely be done with 3.0 (major change). Not sure if that'd be easy enough to work around by keeping old methods, deprecated, delegate to new methods.

[Dimezis]

Hm, and when is 3.0 planned to be released? Another option would be to not change the PropertyNamingStrategy methods, but to pass the Locale to the constructor of LowerCaseStrategy. That's actually what I wanted to do initially, but this stuff is exposed as a static field, so I wasn't sure how to pass a locale there without a lot of refactoring

[cowtowncoder]

@Dimezis 3.0 is still quite long from being released, at least 6 months out, so I think it would be good to address the issue in 2.x too.

But I'll continue reading the code to see what other mechanisms there could be and what challenges are, and possible alternative approaches.

[cowtowncoder]

Ok. So the general approach is to pass configured Locale to every place in code that requires case change or unification. That makes sense.
The main challenge is just that of how to pass it through efficiently and with limited amount of API changes if at all possible. Secondary challenge is to see how to do this in 2.x AND 3.0 since some parts of internal API have changed; typically 3.0 is easier to deal with since it passes more contextual information (fixes issues that have been found in cases where some info was missing).

[cowtowncoder]

One more significant problem: currently Locale is actually NOT presumed (or guaranteed) to be constant for ObjectMapper: it may actually be changed for every read/write call, through ObjectReader and ObjectWriter. So it is not consistent with behavior and in fact I am not sure it is possible to reconcile this aspect -- for various reasons it should be constant, for property naming aspects to work. Yet it may be changed for different calls, resulting in potentially inconsistent handling as serializers/deserializers constructed and cached with certain Locale setting are reused with ostensibly different settings.

[cowtowncoder]

Another problem: use of Character.toLowerCase() (which does not have Locale-taking counterpart) in BeanUtil and PropertyNamingStrategy. Those would probably need to be changed as well.

[Dimezis]

Another problem: use of Character.toLowerCase() (which does not have Locale-taking counterpart) in BeanUtil and PropertyNamingStrategy. Those would probably need to be changed as well.

AFAIR, Character.toLowerCase() doesn't actually produce that Turkish i. Not sure if it has other consequences though

[cowtowncoder]

@Dimezis I am not sure I fully understand the semantics, but it seems as if Character.toLowerCase() would basically suffer same problem as String.toLowerCase().

At this I wonder if reducing the scope of fix would make sense: instead of fixing the name mangling rules (to correctly detect and handle lower casing, detecting boundary), perhaps it should first focus on handling "case-insensitive" use case and leaving other parts as they are.
This would require making sure lower-casing when creating BeanPropertyMap as well as for lookup. But not in other places such as PropertyNamingStrategy.

I will file a new issue for 3.0 to move Locale from per-call settings to be per-mapper setting: this is necessary for things that affect construction of deserializers as they are cached and can not vary on per-call basis.

[cowtowncoder]

Ok, with some tweaks I was able to reproduce the failing case for case-insensitive case. Will check that in under failing, targeting fix for 2.11.

[cowtowncoder]

@Dimezis Thank you for the path: it helped me with partial solution for #953, especially unit test. I will close the patch itself, but add a reference from follow up issue #2563.