JOHNZON-282: add support for @JsonbTypeAdapter at class level #53
Conversation
There was a problem hiding this comment.
Hi Arne
Thanks a lot for the PR!
I would move the reflection in its owned location (DecoratedType) and harness a bit more with one more test, otherwise it looks good to me.
Btw, did you run jsonb tck? I can tomorrow if you need help on it to include that in the coming 1.2.1.
|
|
||
|
|
||
| import static org.hamcrest.CoreMatchers.*; |
| } else if (Class.class.isInstance(type)) { | ||
| return Class.class.cast(type); | ||
| } else /*if (ParameterizedType.class.isInstance(type))*/ { | ||
| return getRawType(ParameterizedType.class.cast(type).getRawType()); |
There was a problem hiding this comment.
Thinking about it, it neither needs recursion nor a loop since getRawType() always returns a class object
There was a problem hiding this comment.
Don't think so, Collection<Collection<Collection<....>>> ;)
There was a problem hiding this comment.
This would return Collection.class
| final JsonbTypeDeserializer deserializer = annotationHolder.getAnnotation(JsonbTypeDeserializer.class); | ||
| final JsonbTypeAdapter adapter = annotationHolder.getAnnotation(JsonbTypeAdapter.class); | ||
| final JsonbTypeAdapter typeAdapter = hasRawType ? getRawType(annotationHolder.getType()).getDeclaredAnnotation(JsonbTypeAdapter.class) : null; |
There was a problem hiding this comment.
Shouldnt it be done in the decorated type? There shouldnt be much type reflection there. Wdyt?
There was a problem hiding this comment.
You mean the hasRawType and getRawType stuff? I take a look into that
There was a problem hiding this comment.
Yep, today it is not super clean for the class (it is for fields and methods) but we support meta annotations and things like that, you can have a look to findFactory in the access mode and org.apache.johnzon.jsonb.JsonbAccessMode#getAnnotation. Guess you "just" want to reuse org.apache.johnzon.jsonb.JsonbAccessMode.ClassDecoratedType somehow here.
|
|
||
| final Foo read = jsonb.fromJson(toString, Foo.class); | ||
| assertEquals(foo.bar.value, read.bar.value); | ||
| assertEquals(foo.dummy.value, read.dummy.value); | ||
| assertEquals(foo.baz.value, read.baz.value); |
There was a problem hiding this comment.
I would also add a test where baz is the root object too, we had issues with that on other metadata by the past.
There was a problem hiding this comment.
Hi Arne,
Will try to rerun TCK over the weekend but looks almost perfect
Just put a comment on two minor things - i cna handle them if you prefer:
- (Trivial one) not sure why package line jumped on top of the header in the test
- Commented on the direct cast and i managed to be in a failling case (with libs creating custom impl of PT) so think it does not cost us much to harness our code even if it should have been more unlikely
That said, thanks a lot to have moved the code, really appreciated!
| @@ -1033,6 +1037,20 @@ private boolean isNumberType(final Type type) { | |||
| return Number.class.isAssignableFrom(clazz) || clazz.isPrimitive(); | |||
| } | |||
|
|
|||
| private boolean hasRawType(final Type type) { | |||
| return Class.class.isInstance(type) || ParameterizedType.class.isInstance(type); | |||
There was a problem hiding this comment.
Hmm, i get the point where java.lang.reflect will highly likely return a Class (oracle jdk guarantees it) but this comes from a reader - which is configurable in the jsonb builder - so potentially a custom PT impl, not java.reflect backed.
Any issue harnessing the check (Class.isInstance(pt.getRawType()) to avoid a ClassCastException which would be a regression for end users?
There was a problem hiding this comment.
No problem with that :-)
There was a problem hiding this comment.
Will not find the time to do the changes this weekend. So go ahead, if you like. Otherwise I'll do it next week.
No description provided.