Skip to content

Add a generic enum-as-number serdes #59

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: jackson29-proto3
Choose a base branch
from
Open

Add a generic enum-as-number serdes #59

wants to merge 5 commits into from

Conversation

mjball
Copy link

@mjball mjball commented Mar 6, 2019

This is particularly useful when you want to serialize a specific field on a
specific type in this manner, without modifying the root object mapper config.

@jhaber

Add a generic enum-as-number serdes
f584f64 
This is particularly useful when you want to serialize a specific field on a
specific type in this manner, without modifying the root object mapper config.
@mjball mjball force-pushed the enum-as-number-serdes branch from 3c779f6 to f584f64 Compare March 6, 2019 21:06
mjball
mjball commented Mar 6, 2019
View reviewed changes
src/main/java/com/hubspot/jackson/datatype/protobuf/ProtobufEnumAsNumber.java
import com.google.common.base.Preconditions;
import com.google.protobuf.ProtocolMessageEnum;

public final class ProtobufEnumAsNumber {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to reorganize this into two separate classes if you prefer that style.

src/main/java/com/hubspot/jackson/datatype/protobuf/ProtobufEnumAsNumber.java Outdated
int intValue = jsonParser.getIntValue();
Class<?> enumClass = Preconditions.checkNotNull(javaType, "javaType").getRawClass();
try {
return (ProtocolMessageEnum) enumClass.getMethod("forNumber", int.class).invoke(null, intValue);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's worth memoizing the class → method mapping, but I'm not super concerned about the performance implications of this implementation in the context I'd like to use this (deserializing from SQL via Rosetta).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be moved into the Deserializer constructor but I don't remember off the top of my head whether contextual deserializers get cached automatically

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it into createContextual(). 9d513ec

src/main/java/com/hubspot/jackson/datatype/protobuf/ProtobufEnumAsNumber.java
try {
return (ProtocolMessageEnum) enumClass.getMethod("forNumber", int.class).invoke(null, intValue);
} catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
context.reportWrongTokenException(ProtocolMessageEnum.class, JsonToken.VALUE_NUMBER_INT, wrongTokenMessage(context));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I borrowed this pattern from the other deserializer implementations, but I'm not sure if this is really a wrong-token exception.

src/test/java/com/hubspot/jackson/datatype/protobuf/ProtobufEnumAsNumberTest.java
import com.hubspot.jackson.datatype.protobuf.util.TestProtobuf;
import com.hubspot.jackson.datatype.protobuf.util.TestProtobuf.Enum;

public class ProtobufEnumAsNumberTest {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to add more test cases if you think there are major blind spots.

@jhaber
Copy link
Member

jhaber commented Mar 11, 2019

lgtm, might be worth moving the reflective method lookup to the Deserializer constructor (assuming that the deserializers get reused by Jackson)

@mjball
Copy link
Author

mjball commented Mar 11, 2019

Hm, not sure if there's a way to move the reflective method lookup out of deserialize() and memoize its result as a field in the serializer, because StdDeserializer is Serializable but java.lang.Method is not:

[INFO] --- findbugs-maven-plugin:3.0.3:check (default) @ jackson-datatype-protobuf ---
[INFO] BugInstance size is 1
[INFO] Error size is 0
[INFO] Total bugs: 1
[INFO] Class com.hubspot.jackson.datatype.protobuf.ProtobufEnumAsNumber$Deserializer defines non-transient non-serializable instance field forNumberMethod [com.hubspot.jackson.datatype.protobuf.ProtobufEnumAsNumber$Deserializer] In ProtobufEnumAsNumber.java SE_BAD_FIELD
[INFO]

@mjball mjball force-pushed the enum-as-number-serdes branch from fb43105 to f7322f1 Compare March 11, 2019 22:18
@mjball
Copy link
Author

mjball commented Mar 11, 2019

I got this building again. Feel free to merge and cut the necessary releases whenever, I'm not in a massive hurry to get this feature out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jhaber jhaber jhaber left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mjball @jhaber