Use static PropertyNamingStrategy instances in POJOPropertiesCollector._findNamingStrategy()

[takezoe]

Returns static PropertyNamingStrategy instances defined in PropertyNamingStrategies from POJOPropertiesCollector._findNamingStrategy() instead of creating new instance every time.

This also eliminates risk of dead-lock mentioned in #2715 when subclasses of PropertyNamingStrategy are used with @JsonNaiming though they have been already deprecated.

[JooHyukKim]

// Plz check Line 1448
PropertyNamingStrategy pns = hi.namingStrategyInstance(_config, _classDef, namingClass);

Possibly, at line 1448 (above) HandlerInstantiator is also doing what we are trying to prevent to happen here. namingStrategyInstance() JavaDoc says....

/**
  * Method called to construct a NamingStrategy instance used for specified
  * class.
  *
  * @since 2.1
  */

[takezoe]

HandlerInstantiator is given through configuration and it can return any instance of PropertyNamingStrategy. I think we can't rescue if HandlerInstantiator is used.

[pjfanning]

can this list of if stmts be made into a switch stmt with cases instead?

[JooHyukKim]

As far as I know the baseline version Java 8 doesn't support switch for Class 🧐

[cowtowncoder]

If we were to do this, I think switch logic should be moved into a static method of PropertyNamingStrategies, so here we would just call that method.

[takezoe]

Moved switching logic to PropertyNamingStrategies and added a test case.

[cowtowncoder]

@takezoe First of all, thank you for offering this contribution!

Now... I am not a big fan of this idea. I get that this could reduce memory usage slightly as well as tiny improvement in processing speed. But in most cases this occurs just once per class being (de)serialized.
If it occurs more often, usage may be wrong (not reusing ObjectMappers etc), symptom of something else.
But mostly I guess I don't like coupling between classes that are introduced here.

So let me ask you this: what is the motivation for suggesting the change?

[takezoe]

@cowtowncoder Sorry for late response. My motivation is eliminating risk of dead-lock caused by pre-defined strategies with @JsonNaming on the library side.

In fact, we faced the dead-lock issue by mixed use of

• @JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy.class) and
• @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)

Of course, we can fix our codes to only use @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) if all codes that use Jackson are under our management, but even third-party libraries could still use PropertyNamingStrategy. This will eliminate such risk without breaking backward compatibility.

[JooHyukKim]

Of course, we can fix our codes to only use @JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class) if all codes that use Jackson are under our management, but even third-party libraries could still use PropertyNamingStrategy.

✅ True, sounds like keeping consistency @JsonNaming can be challenging when third-party libraries are each doing their own thing. Reasonable thing to ask, from a user's perspective.

❌ Considering this fix pertains to a class deprecated since Jackson 2.7 or 2.12 2~3 years back, encouraging third-party authors to update their Jackson dependencies or the usage might be beneficial in the long run for everyone. As per #2715, numerous projects have applied the official fix without modifying Jackson directly.

[takezoe]

Then, I hope that deprecated static fields of PropertyNamingStrategy will be dropped shortly to prevent unlucky but critical accident 🙏

[cowtowncoder]

Then, I hope that deprecated static fields of PropertyNamingStrategy will be dropped shortly to prevent unlucky but critical accident 🙏

The challenge is that deprecation only occurred in 2.12, first released under 3 years ago (and latest patch bit over 1 year ago), so despite the risk, there's very likely code out there that would get broken when someone upgrades from 2.15 or earlier to 2.16, should we drop deprecated classes now. This compared risk of possible race condition due to someone using old implementations (I don't believe mixing of new and old should be issue but simply use of old ones).

What I can do is file an issue for dropping these classes from Jackson 2.17, if agreed to by discussions within Jackson developer community.

[takezoe]

What I can do is file an issue for dropping these classes from Jackson 2.17, if agreed to by discussions within Jackson developer community.

Thank you. That sounds good!

[takezoe]

If we were to do this, I think switch logic should be moved into a static method of PropertyNamingStrategies, so here we would just call that method.

Not sure if this pull request has a chance to be accepted but I anyway updated to cover this comment.

[cowtowncoder]

I think I'd rather go with the alternate approach on warning, deprecation, eventual removal.

[cowtowncoder]

Closing in favor of #4144