Support Object Query with GET request

[DanielYWoo]

Let's say you have two mandatory parameters and an optional parameters, you will have to implement it this way:

foo(m1, m2, o1)

If you have more optional parameters you will have to implement a lot of overloaded methods, otherwise you can only implement a method with a lot of parameters and when it is called, you have to pass in a bunch of null values like this:

foo(m1, m2, null, null, null, null, o5, null, null, o8) 

This is very ugly, if we have object parameter we can do this: first, use a constructor to pass in all mandatory parameters, then use setters to set the optional parameters like this:

q = new FindUsersRequest(category, type);
q.setRowStart(10);
q.setGender(M);
feign.findUsers(q);

Relaated to #520

[Gui13]

I'm not affiliated to Feign but it would be nice to have some unit tests to test this feature?

[DanielYWoo]

yep, I will add some unit tests to the pull request. Thanks, Daniel

…

________________________________ From: Guillaume <notifications@github.com> Sent: Tuesday, January 2, 2018 10:47:01 PM To: OpenFeign/feign Cc: Daniel Wu; Author Subject: Re: [OpenFeign/feign] Support Object Query with GET request (#620) I'm not affiliated to Feign but it would be nice to have some unit tests to test this feature? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#620 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAfZcU1Ls7xac0KlXxqIWs1MIzlP093Bks5tGkFlgaJpZM4RO_f7>.

[DanielYWoo]

Too busy recently, will add unit tests later, sorry

[DanielYWoo]

unit test added, please review it, thanks!

[yangtao309]

good job ！ +1 @DanielYWoo

[benmanbs]

On the whole I think this looks like a pretty cool bit of functionality. I have one main concern - I don't think this is documented well at all. I would want to see some very clear documentation that indicates some of the assumptions that this code makes:

• the @QueryObject.Param annotation goes on the getter, not the field
• the valid entries for query params are only simple objects, things like lists or arrays are not supported
• this does not support boolean getters with the format isFoo instead of getFoo (your code directly checks for the presence of get at the beginning of the method)

I am unfamiliar with the format foo={bar}, just out of curiosity does this work on the other side? Like if you implement the feign interface with your actual controller, will the object also unpack from the request? (A common pattern at my company is to package a feign interface in the same repo as the controller itself, and have the controller implement the interface).

Would love to see some unit tests with edge cases for the ObjectEncoder in addition to the integration style tests you've got on the Feign class.

Finally, I would recommend that you read https://github.com/OpenFeign/feign/blob/master/CONTRIBUTING.md and format your PR accordingly. For example, I don't see any licensing information on new files.

[kdavisk6]

With regards to the isFoo vs getFoo, you may want to use the Java Beans api to determine the properties. It would allow for more robust handling and interoperability.

BeanInfo info = Introspector.getBeanInfo(Bean.class);
PropertyDescriptor[] pds = info.getPropertyDescriptors();

You can use the Introspector instead of reflection

[rage-shadowman]

Why not also allow the annotations on member variables? (kinda like the jackson library)

[rage-shadowman]

This is an excellent change. I would personally like to see it included, and even expanded upon (maybe allowing path param annotations as well, and definitely allowing annotations on member variables rather than just getter methods).

[kdavisk6]

If you want that, why not use Spring.

https://cloud.spring.io/spring-cloud-netflix/multi/multi_spring-cloud-feign.html

This adds support for the Spring MVC annotations include Path, Request, and other variables. It's a complete implementation of what you are asking for, but it does require Spring

If you would like to discuss this more, let's move that to the issue and not this PR. Let's keep this thread clear so @DanielYWoo can see the PR feedback.

[DanielYWoo]

@benmanbs I will change accordingly later, I am too busy recently, sorry about the late response.

[DanielYWoo]

@kdavisk6 Yes, you spotted a bug about the java bean convention. I will fix it and add more unit tests. And I probably will make the annotation only on member variables instead of getters, to be friendly to lombok. And I will do some benchmark to see how much performance can be improved with javassist by avoiding reflections on each call. If the performance improvements is worthy of it, I probably make javassist as an optional dependency.
The reason I don't use spring MVC annotations is dependency. We don't want to introduce Spring MVC dependency at the client side, the client could be a batch job or a mobile device, so, it's better not to add so many Spring dependencies to the client, and make the dependency tree as simple as possible, and I really like OpenFeign's dependency simplicity.

[kdavisk6]

@DanielYWoo with regards to Spring, I was making a reference to something that has already been implemented showing what you all wanted. If your desire is to add this to Feign proper, I think a more elegant solution is to add an additional Contract or modify the existing default Contract implementation. Have you looked at that Interface?

[rage-shadowman]

@kdavisk6 oohhh... That's nice. I hadn't noticed that before. The jax-rs contract seems like a good place to start: https://github.com/OpenFeign/feign/blob/master/jaxrs/src/main/java/feign/jaxrs/JAXRSContract.java

Thanks for the tip.

As regards Spring, I agree with @DanielYWoo that it isn't for libraries. Spring is way too much overkill for a simple client library that might be used in any context. We do have some spring services here, so it must be usable in such a context, but not everything we have needs or wants all of that complexity, so it shouldn't pull in spring as a dependency just to use (what should be) a simple client library.

[rage-shadowman]

@kdavisk6 looking at the Contract interface, it only tags the individual passed-in parameters, but does not have access to the actual objects themselves. It seems to expect all tagged arguments to be passed via the method parameters individually, handling the actual encoding/expanding of them elsewhere.

This doesn't look like it would work. Am I missing something? Do you have any examples that you could point us to where a single interface method argument can be used to support multiple query parameters?

[kdavisk6]

Again, I'll go back to Spring

https://github.com/spring-cloud/spring-cloud-openfeign/blob/master/spring-cloud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java

In their code, they created AnnotatedParameterProcessor that is used within the Contract, along with a ConversionService. The parameter processor is used to interpret the @RequestParam, @PathParam, and @RequestMapping annotations on the Feign interface and translate the arguments for use later by the Client.

The ConversionService is used convert items from Objects to the appropriate types. For example, Spring registers a MappingJacksonHttpMessageConverter, with the DefaultConversionService to handle objects that should be mapped to JSON output. The SpringEncoder recognizes this based on the information provided by the SpringMvcContract and will encode the objects in the Request accordingly.

Using Spring as an example, one way to make this work in core Feign would be expand the Contract to expand on what @DanielYWoo has already done in this PR. @benmanbs has already made some very good suggestions in that regards.

I'm learning too as I review this PR. The more I look into it, the more @DanielYWoo's approach is pretty close. I still think moving away from reflection is a better idea, and @DanielYWoo has already agreed. This is a great start.

[rage-shadowman]

@kdavisk6 that Spring class doesn't do what this pull request is trying to do, it can actually generate everything it needs with the method signature (since each thing that it is handling represents a single thing), so it can use a custom Contract.

I don't believe the Contract interface works where 1 thing being passed into the client (the query object) represents multiple things (optional query parameters) on the server. Also I believe @benmanbs was talking about using reflection and caching the results, instead of using reflection with each call (as opposed to moving away from reflection, or not using it at all).

[kdavisk6]

Understood. In that case, I still recommend that javaassist or the bean api be used instead of pure reflection. Otherwise I think we should be good to go.

[rage-shadowman]

I think that the cleaner solution would be to modify the default Contract to accept a new annotation and the default InvocationHandlerFactory to handle the new annotation.

[rage-shadowman]

I attempted the "cleaner solution" myself in #667. Mine is the 3rd attempt to solve this particular problem (#636 and this one are the first 2) that I've seen here. I hope one of these gets traction and actually gets into the feign code base.

[rage-shadowman]

@DanielYWoo Would #667 work for you?

It should allow you to create your QueryObject annotation and QueryObjectEncoder class locally and include it via:

@CustomParam(encoder = QueryObjectEncoder.class) MyQueryObject myQueryObject

That PR allows your custom encoder to modify query params (as your class is doing) or headers (we needed to use param context in generating our auth headers as well) or whatever you need (as with custom encoders and interceptors, use at your own risk since you can totally mangle everything in the template if you screw something up).

I'd like to find something that would work for you, me, #636 and anyone else who needs something similar (this might even solve #601 if it doesn't need to be run again on every retry).

[DanielYWoo]

@rage-shadowman I am fine with #667, it's more flexible, but I hope you can include QueryParams into your change, because this annotation will solve 95% of our real world problems so it should just work out of box.
And we probably need javassist for better performance.

[rage-shadowman]

@DanielYWoo what do you mean "QueryParams"? I updated the PR based on comments to just use the existing QueryMap annotation (@QueryMap MyPojo myPojo), and it now just looks up all fields on your POJO (rather than getter methods) and uses their names and values to internally generate a Map that should work the same as if you just passed in a Map as in: @QueryMap Map<String, Object> myOptionalQueryParams.

I would have like to have done it with annotations similar to the way you define JSON serialization in a Jackson annotated POJO, but the discussion went another way, and I think I can make that work for me.

[kdavisk6]

With support added for QueryMap in 9.7, should we consider that approach sufficient or is this issue still sufficiently different that we should continue this discussion?

[DanielYWoo]

@kdavisk6 Does QueryMap in 9.7 support POJO other than Map<Object, Object>?
What we want is POJO because
1). all mandatory parameters in its constructor, with only getters
2). all optional parameters with setters/getters

[rage-shadowman]

Yes, you can provide your own QueryMapEncoder or you can use the default one. It's documented in the README.md file (near the end, under the section "Dynamic Query Parameters").

[DanielYWoo]

@rage-shadowman It works but you will have to write a custom encoder for each query object class. This is not what I want. What I want is a single generic encoder to use the annotation info in your query object class.

[kdavisk6]

@DanielYWoo, I think you may have misunderstood the documentation. @QueryMap can now take a POJO in addition to a Map<>. The default encoder will transform all properties of the POJO into Map<> internally and expand them onto the URL. For example:

public class FormBean {
   private String name;
   private String favoriteIceCream;
}

public interface IceCreamService {
   @RequestLine("POST /favorite")
   public void favoriteIceCream(@QueryMap FormBean favoriteForm)
}

will expand to
/favorite?name={name}&favoriteIceCream={favoriteIceCream}

If you need to customize this processing, you can provide an optional QueryMapEncoder on the @QueryMap annotation. At this time, we decided to keep the parsing simple. If we find that more users want more control over the mapping, we can consider expanding the feature. I suggest giving 9.7 a try. If afterwards you still feel that it's not sufficient, please open an issue so we can gauge interest.

[kdavisk6]

@DanielYWoo what are your thoughts on this? Are there any additional use cases you have where a @QueryMap with a QueryMapEncoder will not suffice?

[DanielYWoo]

@kdavisk6 I think @rage-shadowman 's solution is more flexible but a little overkill, and you will have to write a whole lot of decoders, the QueryMap annotation is just fine for me. I think we can close this ticket now. Thank you!