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

EnumMap serialization ignores SerializationFeature.WRITE_ENUMS_USING_TO_STRING #749

Closed
scubasau opened this issue Apr 3, 2015 · 13 comments
Milestone

Comments

@scubasau
Copy link

scubasau commented Apr 3, 2015

Discovered this when upgrading from Jackson 2.4.1 to 2.5.2. Starting in 2.4.4, EnumMapSerializer is deprecated in favor of MapSerializer. However MapSerializer modifies the expected behavior of Enum serialization by always using toString().

Failing unit tests added here:
scubasau@8e2c4cd

@cowtowncoder
Copy link
Member

Ok. Thank you for reporting this.

@cowtowncoder
Copy link
Member

Hmmh. This is a nasty one to fix actually, due to added dynamicity for root-level values.
I was able to fix one easier aspect, but serializer root-level EnumMaps gets trickier thanks to type erasure.

cowtowncoder added a commit that referenced this issue Apr 8, 2015
@cowtowncoder
Copy link
Member

Ok. So, the problem here is due to the fact that at root-level, an EnumMap value does not have generic type information, and key type can not be detected before actual serialization. As a result, a StdKeySerializer is used, and while it knows how to handle some basic types, for most (including all Enums), it simply calls toString().

There are couple of ways to solve the issue. One would require MapSerializer to detect this, and try to find a better key serializer as needed. Since implementation is already complicated, this is not ideal.
On the other hand, maybe StdKeySerializer could be improved to be able to lookup proper serializer for Enum key. Alas, while there is EnumSerializer that works fine, there is no matching key serializer; and problem is that low-level write call to make is different.
So maybe there needs to be EnumKeySerializer to use.

Anyway, there are ways to solve this, but fix is somewhat more involved than I was hoping for, so it'll take bit longer.

In the mean time, the usual work-arounds for type erasure would allow working around the problem.
So, for example, using something like:

class EnumXMap<V> extends EnumMap<EnumX, V> { }

should let enum of type EnumX be detected and work as expected.
Similarly ObjectWriter allows definition of root value type with

ObjectWriter w = mapper.writerFor(new TypeReference<EnumMap<MyEnum, ValueType>>()  {});

While not ideal, one of these approaches should work to get things going until fix is available.

@bimargulies
Copy link

My variation on this is not using an EnumMap. Just using a HashMap that happens to have enum keys, the code in MapSerializer is not looking for custom key serializers based on the key type, at all.

I think I see how to do this in MapSerializer, I'm going to make a commit and a PR.

@bimargulies
Copy link

I've added an @ignored test case on the PR for #749. I thing this is too hard for me without more coaching.

@cowtowncoder
Copy link
Member

Test case is welcome, I can try to see if I can find a solution.

@cowtowncoder
Copy link
Member

On the new test: I'll have to see how it should go, but this may also be a limitation due to type erasure.
Problem being that no type information is passed on serialization, so values are considered just Objects, and not Map<TestEnum, Map<String, String>>. While for values serializer is still accessed dynamically, key serializer is fetched using static type I think.
Although it is possible there was some kludge for nominal type of java.lang.Object to do more dynamic lookup.
The reason for static lookup is, if I recall correctly, performance: there is no caching for key (de)serializers, and having to fetch (de)serializer for every entry is a major performance drain.

Work-around here would be to specify TypeReference when writing, something like:

mapper.writerFor(new TypeReference<Map<KeyEnum, Map<TestEnum,Object>>>() {})

which appears to fix observed behavior here.

But it has been a while since I had a look at the implementation so I may be underestimating how implementation works...

@bimargulies
Copy link

So, adding a cache for key deserializers, at least to the MapSerializer,
might make it plausible to respect individual key values?

On Thu, Sep 24, 2015 at 8:49 PM, Tatu Saloranta [email protected]
wrote:

On the new test: I'll have to see how it should go, but this may also be a
limitation due to type erasure.
Problem being that no type information is passed on serialization, so
values are considered just Objects, and not Map<TestEnum, Map<String,
String>>. While for values serializer is still accessed dynamically, key
serializer is fetched using static type I think.
Although it is possible there was some kludge for nominal type of
java.lang.Object to do more dynamic lookup.
The reason for static lookup is, if I recall correctly, performance: there
is no caching for key (de)serializers, and having to fetch (de)serializer
for every entry is a major performance drain.

Work-around here would be to specify TypeReference when writing,
something like:

mapper.writerFor(new TypeReference<Map<KeyEnum, Map<TestEnum,Object>>>() {})

But it has been a while since I had a look at the implementation so I may
be underestimating how implementation works...


Reply to this email directly or view it on GitHub
#749 (comment)
.

@bimargulies
Copy link

bimargulies commented Sep 25, 2015 via email

@cowtowncoder
Copy link
Member

Right: so original reason for requiring Map key type to be (more) static was because it would have been hassle to replicate much of scaffolding. It's not a good reason, but it did not seem like a major limitation for most uses, esp. at the time, and since Map-valued properties are usually (fully) typed.

Now: in specific case of Enums, it may be easier to get root-level "untyped" Maps to still work as expected, wrt values. For serialization I think default handling would be to call toString() anyway, if that helps.

But like I said, I haven't re-read code to see how much of Enum magic might be supported even via "std" key deserializer.

One other possible work-around idea is that if you can use Map sub-classes that preserve generic declarations -- like, say, public class MyEnumStringMap extends HashMap<MyEnum,String>, it will not lose type information, even for root-values. Such approach is only needed for root values (or, in case of Collections, Maps, content type of root values as well).

@cowtowncoder
Copy link
Member

@bimargulies Looking at default key serializer, StdKeySerializer, used if no useful static information is available, it is rather ghetto implementation, and I would like to improve it. It seems reasonable to, for example, add logic to at least handle Enum keys dynamically, locating whatever key serializer is needed.
And as to caching. that should not be too problematic either. This is solved dynamically for value serializers for many structured types, using local caching, and not requiring centralized caching.

So I think this is actually quite solvable now I am thinking about it some more.

@cowtowncoder
Copy link
Member

... even worse, looks like existing key serializer does not follow default Enum serialization, wrt name() vs toString(). And fixing this exposes test failures, in areas that I thought had been fixed.
So part of the problem is that a few unit tests have been passing more or less accidentally...
I am bit concerned about fixing name() vs toString() in a patch, but it probably does make sense as current behavior is incorrect compared to other default Enum serialization handlint.

@cowtowncoder cowtowncoder added this to the 2.6.3 milestone Sep 25, 2015
@cowtowncoder
Copy link
Member

Ok. So fixing this particular problem, as reported, was not difficult, as it only affects choice of Enum.name() vs Enum.toString(). I will fix this for 2.6.3.

@bimargulies There are other problems for root-level handling (or, more generally, "untyped"), regarding custom serialization via @JsonValue and custom key serializers. But I'll mark this issue fixed as-is, and will pursue fixes to other problems under separate issue(s).

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