-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
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
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as-key accessors
Looks pretty good, added couple of small change notes but those are minor. 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 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.
when we are serializing I don't think I'd unify everything into As to whether we should defer looking up how to serialize |
Compilation is failing because it depends on FasterXML/jackson-annotations#179 |
By unifying I was only suggesting combining lookups for accessors, nothing else. My understanding is that earlier code would already look for |
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 |
@Anusien I understand that both cannot be combined in general case: I was more thinking along conceptual "give me annotation for value" (only considers But I'm fine with this and can play to see if I can figure something out as a later step. |
When serializing the key of a Map, look for a
@JsonKey
annotation.When present (taking priority over
@JsonValue
), skip theStdKey:Serializer and attempt to find a serializer for the inner type.
Depends on FasterXML/jackson-annotations#179
Fixes #2871