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

Support use of @JsonAlias for enum values #2352

Closed
RobertDiebels opened this issue Jun 11, 2019 · 19 comments
Closed

Support use of @JsonAlias for enum values #2352

RobertDiebels opened this issue Jun 11, 2019 · 19 comments
Milestone

Comments

@RobertDiebels
Copy link

RobertDiebels commented Jun 11, 2019

Feature request
Would like to suggest the following feature.
The enabling of @JsonAlias for enums.

Why?
Enabling @JsonAlias for enums can help users that are faced with legacy properties.

Assume that version 0.1.0 has:

{
  "key": "stringvalue"
}

and version 0.2.0 has:

{
  "key": "stringValue"
}

Assume both are valid values and we want to map them to one Enum value. Having @JsonAlias available for this purpose can help to avoid bloat-code or custom de-serializers that do this.

Current behaviour
Not implemented.

Expected behaviour
I'd assume the same behaviour as with class members applies. So using @JsonProperty and @JsonAlias combined would yield the same result except for values not keys.

Example

public enum MyEnum{
    @JsonProperty("stringvalue") //<- Value Jackson should use during serialization and de-serialization.
    @JsonAlias({"stringValue"}) //<- Additional value, valid only during de-serialization.
    STRING_VALUE //<- Would assume "string_value" is ignored and not added to the list of valid values
}

Extra information
Version: 2.9
Occurs in patch: Yes
Feature request was also mentioned in: this issue (Couldn't find the accompanying Feature request)

@GedMarc
Copy link

GedMarc commented Jun 11, 2019


/**
 * @author GedMarc
 * @since 22 Apr 2017
 */
public enum AjaxComponentInsertType
{
	Replace,
	Append,
	Prepend,
	Insert,
	Insert_Last("InsertLast");
	/**
	 * Any sub data
	 */
	private String data;

	/**
	 * A new AjaxComponentInsertType
	 */
	AjaxComponentInsertType()
	{

	}

	/**
	 * A new AjaxComponentInsertType with data
	 */
	AjaxComponentInsertType(String data)
	{

	}

	/**
	 * Returns the name or the data contained within
	 *
	 * @return
	 */
	@Override
	public String toString()
	{
		if (data != null && !data.isEmpty())
		{
			return data;
		}
		else
		{
			return name();
		}
	}
}

Going with the straight forward enum-value pattern is a lot better than the old jdk 6 @XmlElementValue route.

Pair it with the composite types <? extends Enum & SomethingElse> for proper power

public interface IClassificationValue<J extends Enum & IClassificationValue<J>>
{
	/**
	 * Overrides the enum and string
	 *
	 * @return The string for the enum
	 */
	String name();

Given since JDK7 it's all been catered for, I don't think putting in JDK 6 patterns at this point is conducive to moving forward.

-1 from me.

@cowtowncoder
Copy link
Member

@GedMarc perhaps this was meant for different issue?

@RobertDiebels interesting. I can see how this could be useful.

@GedMarc
Copy link

GedMarc commented Jun 12, 2019

Sigh, I wish it was as good as that -
Nothing more than a bad day rant on an unintended -

Sorry guys

@RobertDiebels
Copy link
Author

RobertDiebels commented Jun 12, 2019

@cowtowncoder Thanks for the reply. I added two sections on behaviour and an example just in case my intentions weren't sufficiently clear.

@GedMarc I'm going to assume you're referring to using the toString function combined with WRITE_ENUMS_USING_TO_STRING? That's great but not useful when you have multiple valid values that map to one Enum value. Right now you have to use @JsonCreator to create a stactic factory method which maps those multiple values onto a single value in the Enum. So you're annotating whilst also writing a lot of bloat-code to check if the value you receive matches one of the Enum values.

There's computation cost associated with looping through all of your Enum values and I feel that, since you're using annotations anyway during Deserialization (@JsonCreator), Jackson could do a much better job optimizing that code. Whilst also getting rid of all the bloat-code in the enum. EDIT: Just read your comment, scratch my reply.

@cowtowncoder
Copy link
Member

Ok. So, I guess all I am saying is that suggestion is consistent with "main" @JsonAlias use case: just additional value accepted on deserialization, not used on serialization. Whether such feature should be used instead of alternative is something I won't comment since there are so many possible use cases, some better than others.

So in that sense I think request makes sense, and I'd be especially happy to help if anyone wants to implement this. I'll probably eventually get to this myself too, but that may be a while.

And as to performance; if implemented by Jackson, annotations are only processed once, to build deserializer, and not every time deserialization occurs. Annotation access is pretty poorly optimized by JDK so anything that has to do it on the fly incurs pretty severe performance penalty.

@RobertDiebels
Copy link
Author

@cowtowncoder I'd be happy to take a crack at it. If you could help me get started I'd appreciate that.

@cowtowncoder
Copy link
Member

@RobertDiebels I think that [Basic]BeanDescription that is given when constructing JsonDeserializer has information including optional alias definitions. That would be where information would be available, I think, and then EnumDeserializer would need to use that. Serialization should not be affected.

@RobertDiebels
Copy link
Author

RobertDiebels commented Sep 20, 2019

@cowtowncoder Just letting you know I started working on this. Might take me a while to familiarize myself with the code-base. Hopefully I'll get it done by the end of next week.

@cowtowncoder
Copy link
Member

@RobertDiebels excellent! Let me know how it goes -- assuming no API changes needed, may go in a patch, so probably no hurry wrt 2.10.0 release (which needs to go out next week).

@RobertDiebels
Copy link
Author

RobertDiebels commented Sep 21, 2019

@cowtowncoder I looked at your suggestions for where I could find the alias definitions. Afaics the BeanDescription is available when:

  • The EnumDeserializer is constructed in BasicDeserializerFactory->createEnumDeserializer().
  • The EnumResolver is constructed in BasicDeserializerFactory->_createEnumKeyDeserializer().

The EnumDeserializer itself has no internal access to a BeanDescription.

I tried finding a different way to retrieve the aliases by doing the following:

  1. I tried to retrieve an AnnotationInspector via the DeserialiazationContext. This failed due to the unavailability of a JavaType that matches an allowed type. Resulting in unavailability of the Annotated members of an Enum class.
  2. I tried to add the aliases as valid enum values in the EnumResolver. This failed as I could not pass the AnnotationInspector any values. Due to unavailability of the Annotated members of the Enum class.

I think I might be able to get this in by adding a constructor to the EnumResolver and pass it the aliases. Letting the EnumDeserializer retrieve the aliases from the EnumResolver.
This would:

  • Prevent major changes to the EnumDeserializer itself.
  • Seems like a logic place to put the aliases since the defaultValue is also present in the EnumResolver.

Let me know what you think since I'm not sure that's the right way to go about it.

@cowtowncoder
Copy link
Member

@RobertDiebels I wish I had more time to look into this, but based on your description it seems legit that aliases would be accessed via EnumResolver, like you suggest. Your analysis seems sound to me.

I would want to get hold of aliases via BeanDescription if at all possible, and not directly via AnnotationIntrospector: this is likely to be easier to support just in case there would be -- for example -- addition to "config overrides" to let someone add aliases externally, without using annotations.

@RobertDiebels
Copy link
Author

@cowtowncoder In that case I'll have a crack at exposing the aliases through BeanDescription. Thanks for the reply :D !

@cowtowncoder
Copy link
Member

Sounds good! Also, one thing to note is that if new methods are needed, even in semi-public API, this is still possible for next couple of days: but if so, important thing is to @deprecate existing ones (not remove), and make them chain best as they can for possible old code.
But deadline is coming pretty soon as I hope to release 2.10.0 release within 4 days.
So if such a change is needed, let me know, and I expedite things.

@RobertDiebels
Copy link
Author

@cowtowncoder I'm not sure I can get this done within the next 4 days since I have work in between. I'd suggest to go ahead as planned with the release. I will keep you posted in the mean time.

@cowtowncoder
Copy link
Member

Makes sense, we'll see how things go.

@RobertDiebels
Copy link
Author

RobertDiebels commented Oct 7, 2019

@cowtowncoder This pull-request should get the job done [ /pull/2491 ] let me know what you think.

@cowtowncoder
Copy link
Member

Excellent! I'll add this to my work-in-progress list and hopefully have time to review tomorrow.

cowtowncoder added a commit that referenced this issue Nov 16, 2019
@cowtowncoder cowtowncoder added this to the 2.11.0 milestone Nov 24, 2019
@cowtowncoder cowtowncoder added 2.11 and removed 2.10 labels Nov 24, 2019
@cowtowncoder
Copy link
Member

Took quite a bit longer, but I am finally merging this in, will be in 2.11(.0).

@cowtowncoder cowtowncoder changed the title Add @JsonAlias for enum values Support use of @JsonAlias for enum values Nov 24, 2019
@cowtowncoder
Copy link
Member

Big thanks to @RobertDiebels for contributing this & apologies for merging taking time. Will be nice addition to 2.11.

@cowtowncoder cowtowncoder removed the 2.11 label Apr 12, 2020
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

3 participants