Skip to content
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

Expose translate() method of standard PropertyNamingStrategy implementations #3633

Closed
toolforger opened this issue Oct 18, 2022 · 8 comments
Closed
Milestone

Comments

@toolforger
Copy link

Is your feature request related to a problem? Please describe.

I work on a REST server that uses PropertyNamingStrategies.SNAKE_CASE to translate Java property names to JSON field names.
The application code needs to report errors to the REST client, using the translated property names, but it has only the Java names.
PropertyNamingStrategies.SNAKE_CASE does have a translate function that does exactly what I need, but it is declared in NamingBase while SNAKE_CASE is declaredas PropertyNamingStrategy.

Describe the solution you'd like

  • Declare public translate(String) in PropertyNamingStrategy, or
  • Declare PropertyNamingStrategy SNAKE_CASE as NamingBase or SnakeCaseStrategy.

Usage example

Current application code:

import com.fasterxml.jackson.databind.PropertyNamingStrategies.SnakeCaseStrategy;
...
private static SnakeCaseStrategy toSnakeCaseConverter = new SnakeCaseStrategy();
...
propertyName = toSnakeCaseConverter.translate(propertyName);

Application code after change:

import static com.fasterxml.jackson.databind.PropertyNamingStrategies.SNAKE_CASE;
...
propertyName = toSnakeCaseConverter.translate(propertyName);
@toolforger toolforger added the to-evaluate Issue that has been received but not yet evaluated label Oct 18, 2022
@cowtowncoder
Copy link
Member

While I see how this might be convenient, from design perspective translate() method is an internal implementation detail for "simple" strategies. It is specifically not part of PropertyNamingStrategy because it may not work for all strategies: for example in some cases Field and Method names might require different handling. Or more likely translation is configurable with MapperConfig; that is not passed to translate().

So I will not make such a change, although I am not completely against improvements that would allow usage.
Another possibility would be to change type of SNAKE_CASE etc, but this would unfortunately be bytecode-incompatible change and as such cannot be done in a 2.x version.

@cowtowncoder cowtowncoder added will-not-fix Closed as either non-issue or something not planned to be worked on and removed to-evaluate Issue that has been received but not yet evaluated labels Oct 22, 2022
@toolforger
Copy link
Author

Oh. If translate being public makes the reader think it's official API.

Would changing public static final PropertyNamingStrategy SNAKE_CASE to a subclass indeed be binary-incompatible?
If that's the case, then I have a third suggestion:

  • Have SnakeCaseStrategy declare public static final SnakeCaseStrategy INSTANCE = new SnakeCaseStrategy(), as there's no harm in that. (PropertyNamingStrategy.SNAKE_CASE could use that, without changing the declared type.)

@cowtowncoder
Copy link
Member

Hmmh. The one downside is just that of sort of promoting use of translate(). You are right in that the method really should have been marked as protected, and yes, making it public makes it tempting thing to use.
My main concern would just be that going forward some of implementations may essentially drop use of translate().

However, given all of above, yes, I think I'll do what you suggest and create INSTANCE for all of types.

Thank you for suggestion!

@cowtowncoder cowtowncoder reopened this Oct 23, 2022
@cowtowncoder cowtowncoder changed the title Make NamingBase.translate(String) to application code Expose translate() method of standard PropertyNamingStrategy implementations Oct 23, 2022
@cowtowncoder cowtowncoder added 2.14 and removed will-not-fix Closed as either non-issue or something not planned to be worked on labels Oct 23, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Oct 23, 2022
@cowtowncoder
Copy link
Member

Implemented; @toolforger if you can easily build 2.14.0-SNAPSHOT from 2.14 it'd be great if you could verify this is what works. It'll go in 2.14.0-rc3 (if I release one more RC), and in 2.14.0 regardless.

@toolforger
Copy link
Author

public static final Xxx INSTANCE or public static Xxx getInstance()?
I'm fine with either style (I picked INSTANCE more or less at random), but if the getInstance() style is what Jackson prefers, by all means stick with that.

@toolforger
Copy link
Author

I should be able to check on Monday.
Code review already happened, nothing I could see.

@toolforger
Copy link
Author

Oh. Wait. Sometimes it's static final and sometimes final static, not sure if you have a canonic modifier order in the project.
(I tend to go with what IntelliJ wants me to use, and there is a recommended order somewhere, but too small to lose any sweat about it.)

@cowtowncoder
Copy link
Member

Yup, no auto-formatter. I tend to end up fighting more wrt indentation and never get it quite right. As to static final vs final static I forget which one is the canonical one which ends up being one or the other; compilers accept either so to me it does not greatly matter. Just should be same within single class file.

Anyway, please LMK if somehow something was missing; otherwise I assume this works like it should :)

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

No branches or pull requests

2 participants