-
-
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
Add support for new JsonTypeInfo.Id.SIMPLE_NAME
polymorphic type id option
#4065
Add support for new JsonTypeInfo.Id.SIMPLE_NAME
polymorphic type id option
#4065
Conversation
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/SimpleClassNameIdResolver.java
Outdated
Show resolved
Hide resolved
// no name? Need to figure out default; for now, let's just | ||
// use non-qualified class name | ||
Class<?> cls = t.getType(); | ||
String id = t.hasName() ? t.getName() : cls.getSimpleName(); // not {@code _defaultTypeId(cls);} |
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.
Here we use cls.getSimpleName();
not _defaultTypeId(cls);
like in TypeNameIdResolver
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 think it still makes sense to extract _defaultTypeId()
method here, too, same as in TypeNameIdResolver.java
.
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.
True, will put back _defaultTypeId()
and instead modify change its implementation a little bit. Here is the link 👉🏼 https://github.com/FasterXML/jackson-databind/pull/4065/files#r1283295309
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/SimpleClassNameIdResolver.java
Outdated
Show resolved
Hide resolved
* Since lazily constructed will require synchronization (either internal | ||
* by type, or external) | ||
*/ | ||
protected final ConcurrentHashMap<String, String> _typeToId; |
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 don't think we need to use concurrent data structure when it instances are immutable.
EDIT: I missed the part where we do actually modify the Map
. So this is actually needed, just like TypeNameIdResolver
.
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 not immutable. It has a mutable Map.
I think what is meant is whether a single SimpleClassNameIdResolver instance can be used by 2 threads at the same time. If it can be used by multiple threads, then this should be a concurrent map.
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.
@pjfanning Yes you are correct, I first only looked at construction but then noticed mutation later on. So my comment was based on only reading part of the code.
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.
👍🏻👍🏻
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/SimpleClassNameIdResolver.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/SimpleClassNameIdResolver.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/SimpleClassNameIdResolver.java
Outdated
Show resolved
Hide resolved
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/SimpleNameIdResolver.java
Show resolved
Hide resolved
* | ||
* @since 2.16 | ||
*/ | ||
public class JsonTypeInfoSimpleClassName4061Test extends BaseMapTest |
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.
Tried to get some coverage, at least as much as other TypeIdResolver
classes
JsonTypeInfo.Id.SIMPLE_NAME
that uses Class::getSimpleName
JsonTypeInfo.Id.SIMPLE_NAME
I merged the annotation change so this can proceed I think. Not sure why JDK 11 tests fail... |
...va/com/fasterxml/jackson/databind/jsontype/JsonTypeInfoSimpleClassName4061TestContainer.java
Outdated
Show resolved
Hide resolved
After applying your suggestion in #4065 (comment), now JDK 17 tests fail. It seems that the ordering mechanism does not work as I thought it would.🤔 Will look into this. |
Seems like type resolution is done in unpredictable order, specifically in First I filed PR #4086 to resolve this issue wrt unpredictable ordering in type resolution. Then created reproducible "resolved" demonstration in my personal fork which shows that PR #4086 can actually fix this issue. /cc @cowtowncoder |
Merged #4086, tests now seem to pass. |
JsonTypeInfo.Id.SIMPLE_NAME
JsonTypeInfo.Id.SIMPLE_NAME
polymorphic type id option
(resolves #4061)
Description
TypeNameIdResolver
fits right in, so implementation is heavily referenced.Class.getSimpleName()
, goal is handling type value equivalent to whatClass::getSimpleName
returns.