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

Add @EnumNaming, EnumNamingStrategy to allow use of naming strategies for Enums #2667

Closed
cowtowncoder opened this issue Mar 26, 2020 · 19 comments
Labels
enum Related to handling of Enum values most-wanted Tag to indicate that there is heavy user +1'ing action
Milestone

Comments

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 26, 2020

(note: follow up for (unknown))

As things are, the only way to change representation of Enum values (which defaults to using Enum.name()) is to use either "use toString()" or explicit @JsonProperty overrides.
But it appears that there is usage where there is naming convention in use, similar to how @JsonNaming works mapping POJO fields to external JSON property names.

One possibility might be to try to directly support use of @JsonNaming on Enum classes.
This may or may not be practical, considering underlying PropertyNamingStrategy is tied to accessors (Field, Method) that POJOs expose -- but bigger issue may be that this would be two-dimensional mapping as unlike POJO property names that use Bean naming ([lower] camel case), Enum names themselves may use one convention, and external names another one.
As such it may be necessary to either come up with new abstraction(s), or maybe just alternate PropertyNamingStrategy implementations.

@cowtowncoder cowtowncoder added 2.13 most-wanted Tag to indicate that there is heavy user +1'ing action and removed 2.12 labels Oct 27, 2020
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@cowtowncoder cowtowncoder added the enum Related to handling of Enum values label Aug 3, 2022
@jomatt
Copy link

jomatt commented Jan 11, 2023

What's the status on this one?

I really like the proposal from @cowtowncoder. Usually, you want to decouple the internal enum values from their external JSON representation so that you can refactor and change enum values without directly impacting the clients of your application. In general, this doesn't only apply to enums but to all kinds of abstraction, so it would definitely make sense to have a possibility to configure this.

The MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS property already helps with this, but does not work in case you have multi-word enum values, e.g. MATCH_FILTER, which should be represented as MatchFilter or matchFilter depending on your naming convention

@cowtowncoder cowtowncoder added the pr-welcome Issue for which progress most likely if someone submits a Pull Request label Jan 12, 2023
@cowtowncoder
Copy link
Member Author

Hi @jomatt! I haven't had time to get this any further unfortunately. It is highly-voted for so I hope someone with time and interest might want to tackle it for 2.15. As usual I may not have time to implement this, but I make time to help others get their contributions in, if any.

@JooHyukKim
Copy link
Member

JooHyukKim commented Feb 8, 2023

Hi @jomatt! I haven't had time to get this any further unfortunately. It is highly-voted for so I hope someone with time and interest might want to tackle it for 2.15. As usual I may not have time to implement this, but I make time to help others get their contributions in, if any.

can we discuss a latest approach?

At first I thought of adding more ‘EnumFeture’ but I would say no because there will probably be per-enum-class naming strategy.

And also -1 from me on direct support from ‘@JsonNaming’ because as @cowtowncoder mentions above, there will be two dimensional mapping.

So my suggestion is, extending JsonNaming, something like ‘@JsonEnumFeature’ ?

@cowtowncoder
Copy link
Member Author

Cannot be "feature" as those are on/off things, so would be something like @JsonEnumNaming I think. But there's still the question of how to deal with PropertyNamingStrategys binding to POJO accessors.

@JooHyukKim
Copy link
Member

Cannot be "feature" as those are on/off things, so would be something like @JsonEnumNaming I think. But there's still the question of how to deal with PropertyNamingStrategys binding to POJO accessors.

I guess if target enum class is annotated with @JsonEnumNaming , override all other naming strategies?

@JooHyukKim
Copy link
Member

Plus we should also consider Enum used as json key or used as json value

@cowtowncoder
Copy link
Member Author

Cannot be "feature" as those are on/off things, so would be something like @JsonEnumNaming I think. But there's still the question of how to deal with PropertyNamingStrategys binding to POJO accessors.

I guess if target enum class is annotated with @JsonEnumNaming , override all other naming strategies?

Enum classes would not use @JsonNaming, so yes only this one would have effect.

Unless we tried to somehow apply rules of @JsonNaming (enum entries are expressed as.... fields?).

So I guess we could try adopting @JsonNaming if that works -- the main complication then would be whether global naming configuration should start applying to enums or not.

@JooHyukKim
Copy link
Member

So I guess we could try adopting @JsonNaming if that works

Yes, I agree on this one.
Otherwise, I can imagine users being confusing having totally separate naming strategy for only Enums.

@JooHyukKim
Copy link
Member

Seems like @JsonNaming be applied on enum is a viable option.
We could maybe write some code and see how it looks?
@cowtowncoder could you list down what you think should be implemented?

@cowtowncoder
Copy link
Member Author

Ok, I actually do not know quite how to approach this. There is one more thing to consider: enum names are typically different from POJO properties, as they tend to be all upper-case. So logic that existing PropertyNamingStrategy implementations use will likely not work -- they expect camel case as input, compared to "UPPER_SNAKE_CASE" that enums mostly use.

As to separate annotations, yes, that could be confusing. But then again, enum values are different from POJO properties and it is difficult to say how many users would be surprised when suddenly their enums start being translated.

Having said all of that, if:

  1. Enum values are only renamed with explicit @JsonNaming (but not with existing default PropertyNamingStrategy -- maybe we could introduce new one just for Enums?)
  2. We can figure out how to make existing naming strategies work

I think that would be workable. But I honestly do not know how much effort this is: seems like quite a but.
I do think it is useful thing to have, of course, just not a small undertaking.

I may well be missing something so happy to continue discussion and hear ideas others have!

@JooHyukKim
Copy link
Member

it is difficult to say how many users would be surprised when suddenly their enums start being translated.

Inspired by above being said, my suggestion is "our new feature" should work only IF when intentionally set. Because Enum serialization/deserialization should not work against how it used to work, creating confusion.

  1. Enum values are only renamed with explicit @JsonNaming (but not with existing default PropertyNamingStrategy -- maybe we could introduce new one just for Enums?)
  2. We can figure out how to make existing naming strategies work

What do you think using like below?

@JsonNaming(
     value = PropertyNamingStrategies.SnakeCaseStrategy.class,
     enumNamingStrategy = EnumNamingStrategiesa.SnakeCaseStrategy.class )
public class SomeCar {
     ...
}

@cowtowncoder
Copy link
Member Author

@JooHyukKim I don't think that'd work as annotation needs to be on enum class itself. But I think for this usage, having separate annotation would not be too bad, if we have different strategies anyway.

@JooHyukKim
Copy link
Member

. we have different strategies anyway.

Agreed. Enums are by convention treated differently.

@JooHyukKim
Copy link
Member

JooHyukKim commented Feb 20, 2023

After quite a discussion, I figured it won't be so bad to see the looks of our new feature. So I wrote a test like below image and made it pass ✌️. Before I start writing the actual implementation, let me know what you guys think!

I have one question, though. Would this feature be part of 2.15 or 3.0 @cowtowncoder ? If 3.0 version, I will implement this feature with 3.0 version of project structure in mind so less conflict will happen... I recall 3.0 version is being developed under master branch.

image

@cowtowncoder
Copy link
Member Author

I think at this point it'd make sense to implement in 2.x (for 2.15 if it gets there, or later).

And I agree, a new feature is probably the way to go based on discussions.

@cowtowncoder cowtowncoder changed the title Consider supporting some form of naming strategy for enums Add @EnumNaming, EnumNamingStrategy to allow use of naming strategies for Enums Mar 15, 2023
cowtowncoder added a commit that referenced this issue Mar 15, 2023
@cowtowncoder cowtowncoder added this to the 2.15.0 milestone Mar 15, 2023
@cowtowncoder cowtowncoder removed 2.14 pr-welcome Issue for which progress most likely if someone submits a Pull Request labels Mar 15, 2023
@cowtowncoder
Copy link
Member Author

Merged: humongous thank you to @JooHyukKim for implementing this new functionality!

@Azbesciak
Copy link

What with json schema after applying that :)? IMHO it is not managed, at least on my enum where I applied @EnumNaming it is still as the name of the enum

@cowtowncoder
Copy link
Member Author

@Azbesciak I am not sure I understand your question/comment here: are you saying that JsonSchema generation does not take into account renaming? If so, please file a bug against JSON Schema module (or if problem can be reproduce here, new issue on this tracker).

@Azbesciak
Copy link

Yes, I have 2.15.2 and it looks so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enum Related to handling of Enum values most-wanted Tag to indicate that there is heavy user +1'ing action
Projects
None yet
Development

No branches or pull requests

4 participants