Add support for Mapper/Reader/Writer of a Collection #10
Conversation
There was a problem hiding this comment.
Looks interesting, thanks for the patch! The tests are a big light, and I'd personally like to see a little more detail on what might end up in the registry at runtime, make sure that the kinds of pitfalls that happen to gwt-rpc don't happen here.
Can you provide some docs or more tests that show what is supposed to happen here with some complex cases?
| public static void tearDownAfterClass() throws Exception { | ||
| } | ||
|
|
||
| @Before |
There was a problem hiding this comment.
if empty/unused, these setup/teardown methods should be removed.
There was a problem hiding this comment.
Thanks, @niloc132! I will remove them.
| .build(); | ||
| } | ||
|
|
||
| private void processType(TypeMirror type, StringBuilder result, List<ClassName> classNames) { |
There was a problem hiding this comment.
consider making this return a CodeBlock or the like instead, so that you can assemble the code as objects isntead of just concating strings.
There was a problem hiding this comment.
Agree with that. I will refactor the code to CodeBlock & CodeBlock.Builder instead of string concatenation.
|
|
||
| for (TypeMirror type: declaredType.getTypeArguments()) { | ||
| result.append(".typeParam("); | ||
| processType(type, result, classNames); |
There was a problem hiding this comment.
As this is a visitor, wouldnt it make more sense to do type.accept(this, v);?
There was a problem hiding this comment.
Good suggestion, I will take into account for the code refactoring!
| @Override | ||
| protected Iterable<MethodSpec> getMapperMethods(Element element, Name beanName) { | ||
| return Stream.of(makeNewDeserializerMethod(element, beanName), makeNewSerializerMethod(element, beanName)) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
Does this (and the other toSet calls) have to use a Set, or might a List make more sense? Using this requires that the underlying MethodSpec be made into a string (possibly more than once) in the process of building the set to check that none of the MethodSpecs are duplicates of each other. Since we just return an Iterable, it looks like the calling code isn't concerned with this, and from a quick look at makeNewDeserializerMethod it doesn't look like it should generate duplicates, so why spend the extra work here?
There was a problem hiding this comment.
Good point here... It should be Collectors.toList(). The original code was (and for BeanMapperGenerator still is) using Collectors.toSet().
| .addCode( | ||
| CodeBlock.builder() | ||
| .add("return ") | ||
| .add(new FieldDeserializersChainBuilder(getElementType(element)).getInstance(getElementType(element))) |
There was a problem hiding this comment.
as this is a CodeBlock it might not be safe to preface it with a return like this, it would be legal for getInstance to provide a multi-line statement.
(note that this is wrong in other places too in the existing codebase)
There was a problem hiding this comment.
@niloc132 I got your point but is not an easy fix. What we need here is a notion for an expression i.e. "return [expression]", but it seems the javapoet does not have such metaphor.
Probably it is better to include such restriction into the contract for:
FieldDeserializersChainBuilder.getInstance(..)
and
FieldSerializersChainBuilder.getInstance(..)
i.e. the CodeBlock returned from getInstance() must be valid java expression.
There was a problem hiding this comment.
Agreed, and as you noted elsewhere, the original code was also making this mistake.
Valid Java expression is one option, along the same lines you could just make it a plain String (since this is what add(CodeBlock) will do anyway, this way without ambiguity of it being a code block), a third could be to actually make it return a valid Java codeblock - let it generate the return and ; too?
I'm leaning toward the last, but perhaps in another patch, it would be a bigger change.
| @@ -106,11 +115,10 @@ private MethodSpec createGetMethod(String name, String mapName, Class<?> returnT | |||
|
|
|||
| private FieldSpec createConstantMap(String name, Class<?> jsonType) { | |||
| ClassName mapType = ClassName.get(Map.class); | |||
| ParameterizedTypeName classOfWildcard = ParameterizedTypeName.get(ClassName.get(Class.class), | |||
| WildcardTypeName.subtypeOf(Object.class)); | |||
| ClassName typeClassName = ClassName.get(Type.class); | |||
There was a problem hiding this comment.
given the ClassName.get("org.dominokit..." reference above, pick a convention on how you reference other types? generally prefer the string version to avoid classpath issues when the user runs the codegen, but in this case the jars might only work tightly coupled together, i'm not sure.
There was a problem hiding this comment.
Thanks, I will change it to string version to avoid possible conflicts.
|
@niloc132 - thanks for looking into it! We update the pull request with your recommendations and will provide a bit more extensive tests by tomorrow. The pull request is indeed a bit more extensive than what is advertised. Besides "just" allowing the user to request an
As for "avoiding the common GWT-RPC pitfalls", I'm not sure what you mean...
One thing we should probably look into is how to address Java types with wildcards (like |
|
See also intendia-oss/autorest#6 |
|
@ivmarkov and thank you for working on this! I'm very pleased to see someone like you take an interest, and got permission from the authors here before assisting in the review, since it isn't an easy problem to tackle. I didnt run through all the code in my head, or start to build examples with it, hoping to get a little more clarity from you in tests or docs first. My concern was mostly the second point - I had understood your initial statement to suggest that a hierarchy of types would be supported. In "normal" jackson, this works easily when serializing (reflection...) but you have to provide @JsonSubTypes or your own mapper to deserialize, and I wasn't sure if this patch was covering any or all of those cases. Can you also talk a bit about the new Type class, and if it is part of the public api? if not, could it potentially just be replaced by string literals or int ids for brevity in compiled code? |
|
OK, I see where you are going. We are not as ambitious as to support (Also, the state of gwt-jackson-apt as of now is such that it basically does not support neither We just wanted to start with a support for a simpler case - assuming your collections are homogeneous, you "ask" gwt-jackson-apt to generate an object mapper for a collection with the precise bean type you expect. In other words, no support for polymorphism so far. Needless to say, the precise bean type (and collection) needs to be reflected in the method return type as specified in the JAX-RS interface that we reuse on the client with autorest, and more over, you need some description of it at runtime passed to you by autorest, so that you can retrieve the proper object mapper from gwt-jackson-apt's registry ( Hopefully this (and more) can be clarified with a few examples coming soon. As for the |
| import org.dominokit.jacksonapt.processor.serialization.FieldSerializerChainBuilder; | ||
|
|
||
| import com.squareup.javapoet.ClassName; | ||
| import com.squareup.javapoet.CodeBlock; |
There was a problem hiding this comment.
The unused import needs to be removed
There was a problem hiding this comment.
Thanks, I will fix unused imports.
|
|
||
| import javax.annotation.Generated; | ||
| import javax.annotation.processing.Processor; | ||
| import javax.annotation.processing.RoundEnvironment; | ||
| import javax.lang.model.element.Element; | ||
| import javax.lang.model.element.Modifier; | ||
| import javax.lang.model.element.TypeElement; | ||
| import javax.lang.model.type.ArrayType; | ||
| import javax.lang.model.type.DeclaredType; | ||
| import javax.lang.model.type.ErrorType; |
There was a problem hiding this comment.
Thanks, I will fix unused imports.
| import org.dominokit.jacksonapt.ObjectMapper; | ||
| import org.dominokit.jacksonapt.ObjectReader; | ||
| import org.dominokit.jacksonapt.ObjectWriter; | ||
| import org.dominokit.jacksonapt.annotation.JSONRegistration; | ||
| import org.dominokit.jacksonapt.processor.AbstractMapperProcessor; | ||
| import org.dominokit.jacksonapt.registration.JsonRegistry; | ||
| import org.dominokit.jacksonapt.registration.Type; |
There was a problem hiding this comment.
Thanks, I will fix unused imports.
|
@tedynaidenov @tedynaidenov First i would like to thank you so much for this work, i do appreciate it. of course documenting any other parts is also very welcome .. Am i acting greedy here?! 😅 |
|
@vegegoku , I will try to document this extension. Of course, I could help with documenting already existing functionalities. Latter won't happen immediately, probably gradually along with the actual integration and usage of gwt-jackson-apt. One question here - do you look for documentation in README.md file or more extensive Javadoc or both? |
|
@tedynaidenov i would say i am looking for bot, in the readme we can go with a sample usage, and since we are into this ..i think it is the time for me to start working in a proper wiki pages for the lib. |
|
Just a heads up that we'll be likely doing a few small changes to the code as well:
|
|
@tedynaidenov Should we elaborate more on this PR, do you think we are ready to review and start merging? |
|
@vegegoku You can review the changes for this PR and eventually merge it. Currently we are working on an internal PoC including gwt-jackson-apt and autorest (https://github.com/intendia-oss/autorest). Once we are done, you can expect some examples and some updates for gwt-jackson-apt wiki. |
|
Meanwhile i will find some time to prepare the wiki, thanks a lot |
|
@vegegoku I am about to create another PR to extend gwt-jackson-apt to deal with generic classes. It depends on changes introduced in this one. Could you please tell me how merging is going? |
|
Sorry, i didnt find enough time to go through it all ..and there was some conflict with changes introduced to fix an issue with the registry. i might find some time this weekend |
|
@vegegoku Thanks a lot! In respect to the registry, probably you are talking about your recent "PECS" changes. I saw this conflict and I am not sure how TypeToken and PECS can live together in the registry. Furthermore, in the next PR, we are going to have mappers for generic objects i.e.:
which will make things even more complicated. We have to elaborate on this... |
|
@tedynaidenov yes exactly .. i think that we need to push the registry out of the lib itself in its own module, and accordingly we might also think of different registry implementations but we need to split the concern, generating mapper is different from collecting them, what do you think? |
|
@vegegoku Well, I find the beauty of JSONRegistrationProcessor in its simplicity and small size, hence I have no concerns to see it living along with the ObjectMapperProcessor. But generally speaking you are right - object mapper generation is something very different from collecting them. Your statement holds true in the case we consider some serious development there. I am struggling with another question. Can you please explain me the rational behind having PECS in the registry? Probably you were considering a use case, which I am missing. |
|
@vegegoku This weekend I spent some time thinking about the JsonRegistry and PECS and I think this might be somehow wrong. This reader is also referenced in the JsonRegistry. If you want to access the reader, you would request it from the JsonRegistry by Class key. I don't think relaxing of signature of getReader() and getWriter() methods won't do any good. For an example when you relax getReader() signature to
you allow clients to request a reader for subclass (as an example"GreenApple") and receive a reader for subclass (i.e. "Apple"). But that is wrong. I think it would best if we setup a short session (via Hangouts, Skype, etc.) to elaborate on this and probably on TypeToken(s) in general. How do you think? |
|
This is a link to the discussion where the PECS was introduced and TBH i am not a fan of the registry, yet i want this to implement this in the lib but as an separate module in the lib project. i would confirm with the user f the registry if he still uses the PECS implementation and if he is not we can remove it, then we move the registry outside the main module. |
|
@tedynaidenov the PECS changes have been reverted .. no we only need to go again through the PR for the last time before we merge it. the next step after this is to move the registry to its own module. |
|
@vegegoku I have some good news - the patch for handling generic types is ready! I guess the best thing to submit it, is to open another PR. |
|
@vegegoku - sorry to bother but... we have invested a lot of time and resources in this and it is a bit demotivating when we can't land a pull request for multiple months... How can we speed up this process? |
|
@ivmarkov i am sorry for this, and really appreciate your work, i have a very limited time, but i will try to finish this today |
|
@tedynaidenov @ivmarkov |
Current gwt-jackson-apt implementation lacks support for Mapper (or Reader/Writer) over Collection class such as Set or List.
Nearly all needed code is already there, so the changes are minimal. There are 2 sets of changes:
addressing ObjectMapperProcessor and MapperGenerator hierarchy. In the case when Mapper is declared over a collection i.e.
@JSONMapper interface CollectionMapper extends ObjectMapper<List<BeanObject>> { }generated MapperImp code creates corresponding code (in newSerializer()/newDeserializer()) to create needed collection serializers/deserializers
addressing JsonRegistry and JSONRegistrationProcessor. JsonRegistry is extended to have a key of newly introduced class Type. The latter is used to model relations between generic class and its parameters. The JSONRegistrationProcessor is modified to create corresponding code to register appropriate keys.