Support enum constants annotated with JsonAlias in WireSafeEnum

[egoodhall]

Adds aliases defined with @JsonAlias on enum constants to the WireSafeEnum JSON lookup cache during initialization. This will allow mapping of multiple JSON values to a single enum value.

@axiak @kmclarnon

[axiak]

could you add an assertion for the JSON representation of the wrapper? I'd expect it to be "ABC" instead of the asString but I'd like to see that tested

[egoodhall]

The serialized JSON value will actually be "abc" (the alias) here, since asString is used as the @JsonValue. I can use the non-alias name in the jsonValue when writing into the cache, but doing that feels a little odd because it won't match the JSON string that we deserialized from anymore.

Either way, I can add serialization/deserialization tests to make sure the expected behavior is matched

[axiak]

hmm I'm not sure how I feel about this lol

[axiak]

how does a bare enum work with @JsonAlias? If it's serialized using the actual constant name, I think we should prefer that behavior

[egoodhall]

Yep, it'll use the constant name - I'll update the behavior to reflect that.

Tested with the following

  enum TestEnum {
    @JsonAlias("abc")
    ABC,
  }

  @Test
  void testJsonAliasRoundtrip() throws JsonProcessingException {
    ObjectMapper objectMapper = new ObjectMapper();
    TestEnum val = objectMapper.readValue("\"abc\"", TestEnum.class);
    System.out.println(objectMapper.writeValueAsString(val));
  }

Output:

"ABC"

[kmclarnon]

The "most correct / consistent with jackson" behavior seems (to me) to be that if we deserialize and resolve a real enum constant, then the json value emitted during subsequent re-serialization should be the non-aliased (primary) json representation. But that also is somewhat counter to the way wiresafe enum works today, and it runs counter to how existing implementations behave with WireSafeEnum when utilizing a @JsonCreator that maps multiple json values to a single enum instance. For example

public enum TestEnum {
  ONE("one");

  private static final Map<String, TestEnum> LOOKUP = Maps.uniqueIndex(
    Arrays.asList(values()),
    TestEnum::getValue
  );
  private final String value;

  TestEnum(String value) {
    this.value = value;
  }

  @JsonValue
  public String getValue() {
    return value;
  }

  @JsonCreator
  public static TestEnum fromValue(String value) {
    return LOOKUP.get(value.toLowerCase());
  }
}

public static class TestObject {

  @JsonProperty("value")
  final WireSafeEnum<TestEnum> value;

  public TestObject(@JsonProperty("value") WireSafeEnum<TestEnum> value) {
    this.value = value;
  }
}

@Test
public void testJson() {
  TestObject object = JsonUtils.readValue(
    "{\"value\": \"oNe\"}",
    new TypeReference<>() {}
  );
}

All that is to say while storing the alias value as a json value feels less correct to me, it's also the most consistent with what people are already doing.

[egoodhall]

Interesting, thanks for the info about existing behavior. Another benefit of keeping the existing behavior is preservation of the actual JSON value that the enum was deserialized from - that's what I'd expect if I was looking at a deserialized WSE in a debugger. How important do we think matching the existing behavior is?

I do agree that it feels more "correct" to use the matched enum though, and it would likely make it more straightforward to find in logs (eg if the asString value gets logged for debugging, it will provide a consistent value rather than any of the possible aliases).

[kmclarnon]

Thinking about this more, I think i'm coming down more strongly on the side of "preserve the received string as is", since that seems (to me) to be more likely to result in the expected behavior in instances where you're communicating through a chain of services/deployables where some of them may not know about the new aliasing setup yet 🤔

[egoodhall]

Hey @axiak @kmclarnon - been a little while since there was any activity on this PR. I updated it to re-serialize with the original JSON value, since that matches the existing behavior of WireSafeEnum. Wanted to see if y'all have any more feedback/thoughts?