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

Adds support for @JsonKey annotation #2905

Merged
merged 4 commits into from
Nov 2, 2020

Conversation

Anusien
Copy link
Contributor

@Anusien Anusien commented Oct 26, 2020

When serializing the key of a Map, look for a @JsonKey annotation.
When present (taking priority over @JsonValue), skip the
StdKey:Serializer and attempt to find a serializer for the inner type.

Depends on FasterXML/jackson-annotations#179

Fixes #2871

When serializing the key of a Map, look for a `@JsonKey` annotation.
When present (taking priority over `@JsonValue`), skip the
StdKey:Serializer and attempt to find a serializer for the inner type.

Fixes FasterXML#2871
@Anusien Anusien changed the base branch from master to 2.12 October 26, 2020 20:48
ObjectMapper mapper = new ObjectMapper();
Map<String, NoKeyOuter> mapA = Collections.singletonMap("key", new NoKeyOuter(new Inner("innerKey", "innerValue")));
String actual = mapper.writeValueAsString(mapA);
Assert.assertEquals("{\"key\":\"innerValue\"}", actual);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what the code does right now; this I'm not sure if this is appropriate.

These were added accidentally and need to be removed.
*
* @since TODO
*/
public Boolean hasAsKey(Annotated a) {
Copy link
Member

Choose a reason for hiding this comment

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

One suggestion: could you take MapperConfig<?> as the first parameter? While it is not currently needed, it is something I have had to retrofit for a few methods (see f.ex findCreatorAnnotation()), and in 3.0 every method will be changed to take it. So adding it now reduces conversion work slightly.

*
* @since TODO
*/
public abstract AnnotatedMember findJsonKeyAccessor();
Copy link
Member

Choose a reason for hiding this comment

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

I know that some additions have been left as abstract, but for maximum backwards-compatibility, let's add "return null;" as implementation just in case.
(this is not meant as an extension point for others but I am guessing it is possible someone might add alt. implementation -- will make pure abstract in 3.0)

@@ -106,6 +106,11 @@

protected LinkedList<AnnotatedMember> _anySetterField;

/**
* Method(s) annotated with 'JsonKey' annotation
Copy link
Member

Choose a reason for hiding this comment

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

Method(s) and/or Field(s)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the comment here from jsonValueAccessors. I'll update them both to match the better language in JsonValue.

// If @JsonKey defined, must have a single one
if (_jsonKeyAccessors != null) {
if (_jsonKeyAccessors.size() > 1) {
reportProblem("Multiple 'as-value' properties defined (%s vs %s)",
Copy link
Member

Choose a reason for hiding this comment

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

as-key accessors

@cowtowncoder
Copy link
Member

Looks pretty good, added couple of small change notes but those are minor.
I'll have to think through logic wrt tests but I assume those probably make sense.

Just none design idea/question: whether it might make sense to unify lookup of "if there is key-annotated accessor, return that; if not, if there is value-annotated return that" at some point (POJOPropertiesCollector maybe?), instead of making BasicSerializerFactory handle that.

Another thing is CLA; I added a not on jackson-annotations PR which you probably saw.

Miscellaneous cleanups, mostly tweaks on things that were copied from
how `@JsonValue` was handled.
@Anusien
Copy link
Contributor Author

Anusien commented Oct 28, 2020

Just none design idea/question: whether it might make sense to unify lookup of "if there is key-annotated accessor, return that; if not, if there is value-annotated return that" at some point (POJOPropertiesCollector maybe?), instead of making BasicSerializerFactory handle that.
Imagine we have this basic code structure:

class Inner {
    @JsonKey
    @JsonValue
    String value;
}

class Outer {
    @JsonKey
    @JsonValue
   Inner inner;
}

when we are serializing Outer.class as the value in a Map (or before this PR when we are serializing it as the key in a Map) we build a Serializer for Outer, but it doesn't yet know how to do anything. It knows that it will serialize Outer by serializing inner, but it hasn't figured out how to do that. It won't until inside its serialize() method.
This PR works slightly different than that. It goes ahead and builds a serializer for inner right away, rather than letting that get deferred. It does this largely because everything else is orders of magnitude more disruptive.

I don't think I'd unify everything into POJOPropertiesCollector. I think keeping it in the serializer factory makes sense. What I'd probably do is have some context variable somewhere that could tell the serializer factory, "We are trying to serializer something as the key in a map." Exactly how it's done I'm not sure. It would depend on some design decisions; for example, what if Outer.inner is only annotated with @JsonValue, should we use the thing in Inner.classthat's annotated with@jsonkey, @JsonValue, or simply fall back to the toString()` method?

As to whether we should defer looking up how to serialize Inner until serialize() is called or not, I don't understand the current approach well enough to have an informed decision.

@Anusien
Copy link
Contributor Author

Anusien commented Oct 28, 2020

Compilation is failing because it depends on FasterXML/jackson-annotations#179

@cowtowncoder
Copy link
Member

By unifying I was only suggesting combining lookups for accessors, nothing else. My understanding is that earlier code would already look for @JsonValue for key serializer, but addition will add higher-precedence alternative. Not to unify nesting or recursion aspects.

@Anusien
Copy link
Contributor Author

Anusien commented Oct 30, 2020

Maybe I'm not understanding what you're asking. From my investigation into the codebase, I couldn't find a way to make this tidy. Because we use @JsonKey and @JsonValue so differently and in different situations, something still needs to look through a bunch of AnnotatedMember objects and pick the one that's right and do the right thing with it. BasicSerializerFactory#createKeySerializer can't just ask the POJOPropertiesCollector, "Find me the thing annotated with @JsonKey, and if not, the thing annotated with @JsonValue." Because it needs to handle those two cases very differently, it would still need to know which annotation was used. The one simplification we could do would be to have POJOPropertiesCollector return some sort of structure with all the applicable AnnotatedMember objects (maybe even structured in some way?), but it seems like the callers have to become more complex to adapt to that.

@cowtowncoder cowtowncoder added the hacktoberfest-accepted Tag for issues, PRs that are explicitly marked as accepted for Hacktoberfest label Oct 30, 2020
@cowtowncoder
Copy link
Member

@Anusien I understand that both cannot be combined in general case: I was more thinking along conceptual "give me annotation for value" (only considers @JsonValue) and "give me annotation for key" (in which @JsonKey, if there is one, if not, @JsonValue).
I agree that collection part should remain separate, like you say.

But I'm fine with this and can play to see if I can figure something out as a later step.
At this point only waiting for CLA.

@cowtowncoder cowtowncoder merged commit ecc9bfe into FasterXML:2.12 Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Tag for issues, PRs that are explicitly marked as accepted for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add @JsonKey annotation (similar to @JsonValue) for customizable serialization of Map keys
2 participants