Extend ResourceVisitor to hold parameters type information

[tedynaidenov]

Using autorest with external serializer/deserializer framework might be painful since there is no type information associated with REST methods parameters and return result. Thus, selecting appropriate serializer/deserializer require additional efforts. The problems become even bigger when using generic parameters/return result since we have to select serializer/deserializer not only for the generic type but for the type parameters as well.

Proposed changes affect the annotation processor and ResourceVisitor builder.

• Newly created Type class is a recursive structure, that allows to model generic types and their corresponding classes;

• ResourceVisitor interface is extended to receive type information associated with each REST method parameter and method return result;

• Annotation processor inspects each REST method parameter and return result type and generate corresponding code to build Type object for the parameter or methods return result;

[ivmarkov]

@ibaca Here some general info what we are trying to accomplish:

• While we do like the simplicity of the current autorest philosophy (i.e. "just use @jstype(isNative = true, namespace = JsPackage.GLOBAL, name = "Object") classes for return type / method parameters"), we cannot really do that as we are migrating a large legacy GWT-RPC codebase, which uses a lot of custom classes as well as Java collections;
• These do not lend themselves to being annotated with @JsType without too many changes. Needless to say, that's not even possible for Java collections. Therefore, we would like to use autorest together with https://github.com/DominoKit/gwt-jackson-apt instead.

But how to integrate autorest with gwt-jackson-apt without breaking the simplicity of autorest and while still keeping these two libraries unaware of each other?
Turns out, there is an easy way (thanks to autorest being easily configurable/extensible):

• All we need is a chunk of extra type information (modeled with the Type class provided in the pull request) provided for all parameters and the return type in the ResourceVisitor interface. (This type information should account for Java generic type parameters, but that's no big deal as you can see in the pull request.)
• Once ResourceVisitor is extended the way we suggest, the user implementing ResourceVisitor really has two options:
  -- Option 1: Continue to use @jstype(isNative = true) classes, together with JSON.parse/JSON.stringify, and not pay attention at all to the extra type information provided;
  -- Option 2: Take advantage of the extra type information so as to serialize/deserialize the method payload in a way she finds appropriate for the task at hand. In our case, we just instantiate gwt-jackson-apt serializers/deserializers and use them, that's all there is to it!

One more change we got kind of as a "free bonus" as part of this pull request:

• While using Rx is very attractive, we don't feel ready to embrace it in all of our REST APIs as of yet (particularly RxJava. We are actively considering using RxJS via jsinterop annotations, but this is also not fully decided);
• At the same time, the autorest ability to reuse the server-side REST interfaces as a client-side proxy interface is just too great to pass on. But how is it possible to still rely on autorest's "single interface to rule the server and the client" approach while not embracing RxJava (yet)?
• Turns out, that's not that hard either. All is needed is to allow the user to specify additional/custom response wrapper classes/interfaces besides Observable, SingleValue, etc.;
• All that was necessary was to just generify the signature of method RequestBuilder::as from
  -- <T> T RequestBuilder::as(Class<? super T> container, Class<?> type);;
  -- To <T> T RequestBuilder::as(Type type);.

Turns out, once you do this, there is enough type information so that you can use your own response wrapper class, which might of course further return a collection of something etc. etc. Fully generified, but also completely backwards compatible with the original code that uses RxJava.

[ibaca]

Thanks for the PR and for this detailed explanation! The long-term solution I have in mind is a bit different. But as it might take some time to implement it I might merge this PR as a temporary solution. This will also ensure that when the final solution is implemented, it will support this use case.

Question about the container. I strongly recommend using the concept of a container to reduce casuistics. I also discourage callback, so the container solves 2 things, 1. asynchronicity and 2. high-level transport type (empty response or command, single element, and many elements or stream). I know that you can return a scalar value directly but this is a pretty bad idea. Or you can generalize as you suggest. But, forcing you to differentiate the container might be more clear. Why just replace the second argument Class<?> type as Type is not enough? What container class are you using Collection and its subtypes?

[ivmarkov]

Thanks for the PR and for this detailed explanation! The long-term solution I have in mind is a bit different. But as it might take some time to implement it I might merge this PR as a temporary solution. This will also ensure that when the final solution is implemented, it will support this use case.

We would still be interested in an informal explanation of the long-term solution.

Question about the container. I strongly recommend using the concept of a container to reduce casuistics. I also discourage callback, so the container solves 2 things, 1. asynchronicity and 2. high-level transport type (empty response or command, single element, and many elements or stream). I know that you can return a scalar value directly but this is a pretty bad idea. Or you can generalize as you suggest.

I don't think we are suggesting not to use a container. We just don't want to use the RxJava containers, that's all. However, the change of the signature of the 'as' method is not because we want to use a different container, but because of other reasons, see below.

But, forcing you to differentiate the container might be more clear. Why just replace the second argument Class<?> type as Type is not enough? What container class are you using Collection and its subtypes?

No, we don't want to use "collection" and subtypes as the container. How would that work is actually beyond me. The main reason for using a container is because without it you can't really do an async call, can you? How would you be notified when the async result has arrived if you use a collection as a "container"? We'll just use our own, simple, custom container. User code wrapping it in an Observable<> is always possible btw.

As for whether replacing just the second argument as Type would work - yes, that would work. But you introduce an asymmetry this way - the container is represented with a Class<?> object, and the Type - which really is just the instantiated generic parameter of the container is a Type. I.e.:

• If I use e.g. Observable<List<Map<MyKey, MyBean>>, I need to artificially split the Type representing this into a Class<?> instance for Observable<T>, and into a Type instance for List<Map<MyKey, MyBean>> which is the instantiation of the generic variable T of your Observable.

What gives?

[ibaca]

We are going to call those arguments the containerType and itemType.

The goal of splitting is to encourage independent processing. I think this makes more sense in my mind that in the code 😉. But, for example maybe if we change the Class<?> container with a ContainerDescriptor<?> containerType makes more sense. In my mind the Class<?> containerType is like this suggested descriptor, and it is associated with the transport and a bit with the content, but just the minimal to differentiate between the single or a streaming response because this streaming response type will require special handling by the transport. Obviously, I know there are a lot of wholes, and this is just my experience and I'm not 100% sure at all.

The second argument is associated with the final parsing of each item (the item might be a list). You might choose between a Single<List<T>> or an Observable<T>. Probably both are finally encoded in JSON as an array, but if you define it as an Observable<T> you might use a streaming parser that starts emitting items before the response completes, and this streaming parser ends up calling the per-item decoder Class<T> itemType. OTOH, if you define the response as a Single<List<T>>, you will use the Class<List<T>> itemType as a parser, but it will require the whole response.

This is how I differentiate both containerType and itemType. Not sure that the explanation is clear enough 😬. But I really don't see both types of the same thing, and I think that the main problem is that both are just Class<?> but if you see it as a ContainerDescriptor and as ItemDescriptor with some extra tools inside it, it looks more clear that splitting it makes it easier to reuse each container and item descriptors independently.

[ivmarkov]

We are going to call those arguments the containerType and itemType.

The goal of splitting is to encourage independent processing. I think this makes more sense in my mind that in the code 😉. But, for example maybe if we change the Class<?> container with a ContainerDescriptor<?> containerType makes more sense. In my mind the Class<?> containerType is like this suggested descriptor, and it is associated with the transport and a bit with the content, but just the minimal to differentiate between the single or a streaming response because this streaming response type will require special handling by the transport. Obviously, I know there are a lot of wholes, and this is just my experience and I'm not 100% sure at all.

As already communicated, we can split them and it would still work, but I still fail to see the point. Two counter-arguments:

Argument 1:
You said it yourself: the type of the container - streaming or not (containerType) - affects the type of the item (itemType):

• If I go for a streaming container (i.e. Observable), then the item type should be just MyBean
• If I go for a non-streaming container (like, our Request container from below), then the item type should be List<MyBean>

I'm trying to point out that the notions of transport (streaming or not), containerType and itemType are so closely interrelated that trying to differentiate them in the signature of method RequestBuilder::as is probably not worth it.

Argument 2 (actually a much stronger one):
By introducing containerType and itemType as two separate notions in the ResourceVisitor ::as method you actually create an API leak. Why should the autorest generator and the ResourceVisitor interface even be aware that we are using "containers" in a concrete implementation of the ResourceVisitor interface? That's an implementation detail of your particular ResourceVisitor implementation (and - granted - of how you model your REST interface), and should not leak into the ResourceVisitor interface at all.

Here's a small use case that gets ugly with having the notion of "container" in the ResourceVisitor interface: imagine that I don't care a single bit about GWT / JavaScript. All my "client" code is Java/Android. There, I don't need asynchronicity at all. I want old, synchronous REST interfaces on the server, and on the client. I.e. MyBean getMyBean(). If autorest's ResourceVisitor does not have the "container" notion in the RequestBuilder::as method, I can use the synchronous method signature from above, and my Android-specific ResourceVisitor implementation can build a completely synchronous proxy based on Java HttpClient for the above method signature. Now imagine I have a containerType and an itemType. What would each one be for the above signature? containerType possibly null, and itemType would be MyBean.class? Or the other way around? See where I'm going?

The second argument is associated with the final parsing of each item (the item might be a list). You might choose between a Single<List<T>> or an Observable<T>. Probably both are finally encoded in JSON as an array, but if you define it as an Observable<T> you might use a streaming parser that starts emitting items before the response completes, and this streaming parser ends up calling the per-item decoder Class<T> itemType. OTOH, if you define the response as a Single<List<T>>, you will use the Class<List<T>> itemType as a parser, but it will require the whole response.

OK but how is this at all an argument for or against having containerType and itemType vs a single "type"?
In any case what you say is completely possible, regardless of the approach we are taking? Or am I missing something?

This is how I differentiate both containerType and itemType. Not sure that the explanation is clear enough 😬. But I really don't see both types of the same thing, and I think that the main problem is that both are just Class<?> but if you see it as a ContainerDescriptor and as ItemDescriptor with some extra tools inside it, it looks more clear that splitting it makes it easier to reuse each container and item descriptors independently.

My Argument 2 from above applies here too. Why not keeping autorest a tad more generic so that it can be used without containers as well?

Also important and I thought it is obvious: while you can get-by with a Class<?> for containerType, you cannot for itemType, as we want stuff like List<MyBean> to be a valid itemType, and you can't really model this with a Class<?> instance as it does not grok generics, particularly in the context of GWT. That's the no 1 reason why we introduced the Type class in the first place... not so much to unite your containerType and itemType in a single notion. This just came for free...

[ivmarkov]

A small note:
I had to edit my last comment, as I had named the ResourceVisitor interface wrongly originally.

[ivmarkov]

Hold on with this patch for a moment. We are working on the following changes:

• Our Type renamed to TypeToken<T> and generified. Now represents a real "supertype" token; explanation what is this to follow
• ResourceVisitor's methods slightly generified
• Last but not least, a java.util.Proxy-based alternative to AutorestGwtProcessor (only usable with JavaSE & Android, of course).

[ibaca]

Yep, I'm reading the gwt-jackson-apt issue. And actually, the TypeToken is kind of coupled with this external lib. This makes even more sense in autorest, but it forces me to implement the extension mechanism. But, yep, I'll wait a bit.

I know it is possible to use proxies, note that autorest is like a subset of https://github.com/intendia-oss/qualifier/ (in this lib, the extension mechanism used at APT time and at runtime, is the one that I need to integrate into autorest and should make possible to solve this PR externally). And both qualifiers or autorest can be solved with proxies, as I say in the qualifier readme, if you can do that you should do that. BUT, AutoREST is focused in APT. If you want to do it using proxies, then just use Retrofit which is a pretty awesome library! Anyway, at a higher level, you can create a "proxy autorest" and we can share a "spec", but introducing the code and maintenance of a proxied based implementation is not really the goal of this lib. But a shared TCK might be a pretty nice solution if you end up implementing it.

[ivmarkov]

• Reg Retrofit... you know that it does NOT support, and has no intention to support JAX-RS annotations , right? That defeats the whole idea of sharing an interface between the client and the server which is one of, if not the reason we are looking into AutoRest. Moreover, our JAXRSProxy is 121 lines of code. Everything else (ResourceVisitor & friends), even the JAX-RS annotations' scanning machinery is reused. I don't think it is going to add much weight to your lib, or make it in any way impure, especially if we put it in the JRE project, where you already do have JRE-specific ResourceVisitor instances

• Reg Qualifier... I'm not sure where you are going with that. Care to elaborate a bit more? Do you plan on generating - with qualifier - the meta-data for the JAX-RS annotated interface? That's certainly possible but I'm not sure what's the big gain here. In any case - with or without Qualifier, I don't see how you can get by without something like TypeToken<T> (that I'll reapply to the pull request soon) and still model complex Java types. If you don't do something like TypeToken, you can't link to other libraries, like jackson-gwt-apt, or in a JRE/Android environment - to Gson & Jackson. Or do you plan to somehow take over - inside autorest - the serialization/deserialization of the JSON payload somehow? Basically taking over the work of the JSON mappers? That would be quite ambitious and perhaps unnecessary, given that these things already do exist.

As for your TypeToken<T> and jackson-gwt-apt's TypeToken<T> being the same - not quite. They will be very similar, but not the same (yours might have some extra superpowers making it more interoperable with Gson & Jackson in a JRE environment). jackson-gwt-apt does not need this superpower as it really is just a (semi-complete) jackson fork specifically for GWT/J2CL. Conversion from yours to jackson-gwt-apt's one however would be a trivial two-liner, so having them identical is not really necessary, I think.

[tedynaidenov]

Thanks to @ivmarkov, we have the patch completed. As he already highlighted, it includes

• Using TypeToken instead of initially introduced Type;

• Some of the ResourceVisitor's methods are generified;

• Newly introduced AutorestProxy, which can be used in Java SE/Android application as alternative to JAX-RS/AutorestGwtProcessor.

[ibaca]

The AutorestProxy adds a bit of noise in this PR. Also, this should really be managed as an independent project. I see no advantage, for now, to deploy it merged in this project, it will just limit your ability to improve and deploy. Create a project called autorest-proxy in your own github and start from there. Initially will be easy if you repeat a bit of code, specifically the AnnotationProcessor code (this cross-reference, jre to processor, is not a good idea). If you want, I can guide you in how to deploy to maven central. And, also I promise that eventually if both autorest and your module are used more frequently, we can move it to an autorest organization an moving your autorest-proxy to that organization too, so it be in the same organization but you will be the maintainer of this specific module (😉 win-win). What do you think?

I really think that you should create an issue or PR or at least move this discussion to gitter about the creation and usefulness of the proxy based approach to not pollute this PR. 👍

[ivmarkov]

How exactly is AutorestProxy limiting your ability to manage and deploy, given that it is in your "jre" project - which - by definition, is only useable in a proper JRE environment?

Also, aren't we overdoing it by having another project (in addition to your "core", "jre", "processor" and "examples"), just to keep these ~ 200 lines of code separate?

Also, why is the cross reference from "jre" to "processor" so bad? Annotation processors, and your "processor" too do run in a JRE environment anyway by definition.

Also, where would you put the annotation-processing code which is shared by AutorestProxy and AutorestGwtPocessor? In another project of its own? Like, two additional projects now for < 1000 lines of code?

Last but not least, what about the elephant in the room - the code in TypeToken then, which has JRE aspects (marked as @GwtIncompatible) as well as pure GWT aspects? The JRE aspects are necessary so that AutorestProxy (or even AutorestGwtProcessor - in a JRE environment) can interoperate with stuff like GSON or Jackson-proper, which do expect Java Reflection. Shall we somehow split TypeToken well, and how?

(Please no offense!)... but this all seems to me a bit like a storm in a glass of water. AutorestGwt is a botique, miniature project. That's the beauty of it. It is so small, that everybody can immediately grasp how it works. Even after our contribution, I still believe this spirit is preserved. Yet, you treat it as if it is a huge codebase, used by thousands of users, where huge changes (our poor AutorestProxy!) are being introduced that require time to mature as a separate project, in a separate "organization" etc. etc. That's a bit too much work... not really sure it is worth the hassle, honestly...

With that said, in the spirit of cooperation, and in the light of a constantly shrinking GWT community with a handful of projects used by just a few individuals who can't agree on basic stuff, e.g. look at GWT-React (12 stars) vs React4j (15 stars) here (a very unhealthy situation which is not giving me a warm fuzzy feeling for the future of GWT/J2CL - how about to you?), would the following modifications to the patch work for you:

• Remove AutorestProxy altogether from the patch
• Restore your annotation-processing code back into AutorestGwtProcessor, rather than isolate it in a special class
• You tell me what to do with TypeToken and its JRE aspects? Shall we remove the JRE aspects as well?

We somehow maintain AutorestProxy privately, in our proprietary code base. We might or might not bother publishing these a few hundred lines of code to the open source.

What do you say?

[ibaca]

Yep, I completely understand your point, but it is quite difficult to maintain and satisfy the requirement of every use case. What you describe as a warm-fuzzy-feeling might be also to accept those requirements and don't stick to the basics of the lib which make it solid. You can see this as just the opposite; libs are useful until features just destroy it (complexity), and at that point, someone creates a new lib from the start until features destroy the lib again. So, not an easy decision at all.

Again, I really don't see any advantage in the proxy tool, even the name is not very appropriate, I name it AUTO bc of guava auto libs, which are related to annotation processor and how this outputs the boilerplate using APT that was previously created using proxies. Your project should be named ProxyRest or something like that. Also you think that using a common ResourceVisitor might be a good idea, but again this is against the whole idea of this library, this library makes it trivial to map services to request, but creating a nice request builder is NOT the purpose of this library, there are a lot of them and this library should delegate to those libraries (eg. RequestBuilder in GWT or jersey REST client in JRE, etc). I think that you are mixing "easy to do" with "should be doing". So the more you think the proxy is a good idea, the more I think you don't see the goal of this library. Proxy is an alternative to APT, and supporting both means common-denominator, and common-denominator means less flexibility. And, why you want to use the proxy implementation instead of the APT one? Even if this is true, what advantage it has to share the ResourceVisitor? Because if the advantage is too high, then I have done it badly, bc the ResourceVisitor should not be too clever the implementation, the goal is to make it easy to create your own clients with whatever library you choose.

About the dependencies, or even the implementations itself. I personally, I would have omitted to publish the JRE and GWT modules, are just there to makes demos easier. And if you read the README, I encourage to create your own clients (so JRE and GWT clients are just reference implementations). This is the greatest discovery with this library, forcing you to make the implementation end up being much more simple and clear code. So, the processor module depending on a reference implementation module is a really bad idea. Any shared code should be moved to core, and the less code is in core the better.

Anyway, at least you MUST create an independent PR for the ProxyVisitor, move it to a proxy folder and module name autorest-proxy. And DO NOT depend on the processor, only on the core (like the processor module). MUST be an isolated module. And please, think that maybe these rules are what make AutoREST looks so simple and easy to read and extend 😉.

[ivmarkov]

I don't think it is productive to continue arguing anymore on general topics. Instead, a summary of what we did:

• AutoRestproxy completely removed. we might contribute it later to this or to a forked repo as a separate patch
• Dependency between "processor" and "jre" removed

If that's good enough for you in terms of code arrangement, would you please now spend some time reviewing the patch in details, including TypeToken & friends (the elephant in the room, as per my previous post)? Thanks.

[tedynaidenov]

Hi @ibaca, I was wondering if it is possible to review the latest patch? Taking into account your considerations, I removed the AutorestProxy completely.
Please let me know if you like to discuss TypeToken and other related changes. Thanks.

[ibaca]

Hi, sorry for the late response, I have been on holidays. Now it is easier to review, but I need some time. Also, I need to try it out combined with jackson-apt which I believe it is the practical goal of this PR. I need to find also some examples that justify the parameter type tokens. Thanks for your work and patience.