reactiveui / refit

The automatic type-safe REST library for .NET Core, Xamarin and .NET. Heavily inspired by Square's Retrofit library, Refit turns your REST API into a live interface.
https://reactiveui.github.io/refit/
MIT License
8.47k stars 745 forks source link

Lists as url parameters #93

Closed carl-berg closed 6 years ago

carl-berg commented 9 years ago

I'm not sure if Refit supports list arguments in the url. As of now i have had to wrap lists in another type and serialize its values with comma separation. And then add some custom handling on the receiving side to turn it back into a list. I would however prefer it if a call like this:

public IMyApi
{
    [Get("api/resource")]
    Task<Response> GetResources(List<int> value);
}

var api = RestService.For<IMyApi>();
api.GetResource(new List<int>{ 1, 2, 3});

would generate a request like GET /api/resource?value=1&value=2&value=3

What do you guys think about that? Good idea or waste of time?

nekresh commented 9 years ago

Such a query can easily be generated using RFC 6570, discussed in #4.

In order to get /api/resource?value=1&value=2&value=3, you can specify an RFC 6570 route such as /api/resource{?list*}. For comma separated values (/api/resource?value=1,2,3), you can use the alternate form /api/resource{?list}.

carl-berg commented 9 years ago

@nekresh Is this actually implemented in refit? Seems like #4 is still open and has been for over a year...

anaisbetts commented 9 years ago

It is not implemented, and I'm not sure it's actually a good idea tbh

carl-berg commented 9 years ago

@paulcbetts, what do you think yourself of lists in querystrings? As far as i can tell there's not a standard on how to serialize them. I'm used to seeing some/url?param=1&param=2 but i have also seen some/url?param[]=1&param[]=2 neither of theese can be issued by refit out of the box as far as i can tell. Currently i'm commaseparating values and then deserializing that on the server as asp.net web api doesn't understand comma separation out of the box...

anaisbetts commented 9 years ago

@carl-berg How is this seen on the server-side in things like WebAPI or MVC? Most non-C# web frameworks stuff params into a dictionary, which means that this pattern is A Little Weird™ (even if it's legal from an RFC perspective).

If it's super common and I'm missing some case, it's definitely worth looking into but from my position it looks like kind of an antipattern

carl-berg commented 9 years ago

I'm trying to find some material to back up my argument, but there's really hard to find any consensus of what's supported or even promoted on different platforms. :disappointed:

omares commented 9 years ago

:+1: for the list feature.

Given the example of an endpoint that has three optional query parameters: filter.name=foo&filter.age=1&filter.gender=xx. To support that in the current version of refit, the interface definition at least needs 4 methods which is inconvenient and a codesmell.

[Get("/xx")]
Task<String> endpoint(string name);

[Get("/xx")]
Task<String> endpoint(string name, int age);

[Get("/xx")]
Task<String> endpoint(string name, int age, string gender);

[Get("/xx")]
Task<String> endpoint();

Also afterwards adding a required parameter would result in more messy code, because either you refactor all methods and implementations or add more methods.

With list parameters a single method with a filter parameter would cover all use cases.

[Get("/xx{?list*}")]
Task<String> endpoint(List<string> filters);

Actually this is an argument for us to not use refit :(

omares commented 9 years ago

Maybe an alternative would be to implement the retrofit @QueryMap solution

For complex query parameter combinations a Map can be used.

@GET("/group/{id}/users")
List<User> groupList(@Path("id") int groupId, @QueryMap Map<String, String> options);
carl-berg commented 9 years ago

@omares couldn't you solve your implementation with nullable parameters like

[Get("/xx")]
Task<String> endpoint(string name = null, int? age = null, string gender = null);
omares commented 9 years ago

@carl-berg Would work but thats a bad method implementation :( Also the method calls are extrem ugly if you omit parameters: endpoint(null, null, 'xx');

carl-berg commented 9 years ago

I agree that sending in null parameters is ugly but you could always call it like endpoint(age: 5, gender: "male");. No need to provide parameter values for the defaults.

omares commented 9 years ago

True that but if go further and add 10 or 20 filter parmeters, what a bad method that would be :)

SimonSimCity commented 9 years ago

:+1: for that from my side as well! What's actually hindering that one here? @carl-berg what's your reason for voting aginst this? I also use this in my application and just have to deal with how the API is build up, and they have this parameter-array stuff.

As of the argument of that the server does not know how to handle that: That's a problem of the implementation. The specs do not have any rules here and the systems implemented them quite differently (see http://stackoverflow.com/a/1746566/517914 and my comment on the first answer).

But that should not be a reason not implementing URI templates up to level 4!

martijn00 commented 9 years ago

As i also commented in #4 there are some library's available now that support URI templates to level 4. We just need to be able to use them, or build something into Refit to support it.

Yortw commented 9 years ago

Found this issue because I'm trying to use Refit to access an API that requires a query parameter as a command separated list of values (and I would like to expose it as IEnumerable in the interface), i.e;

/api/resource?value=1,2,3

Appears there is still no way to do this with Refit?

Perhaps an option would be an attribute that points to a "parameter serializer" type? Something like the JsonConverter attribute; specifies a type implementing a known interface that converts the value into a string to be embedded into the request query string? The parameter could be decorated with the attribute, Refit constructs and calls the serialiser type, and devs write serialisers for each custom format required? More work on the devs part, but extensible and allows for both value=1,2,3 and value=1&value=2&value=3 along with anything else that comes up in the future?

Something like;

public IMyApi
{
     [Get("api/resource")]
    Task<Response> GetResources([ApiParamConverter(typeof(CsvParamConverter))] List<int> value);
}
bennor commented 9 years ago

@Yortw You can wire this up yourself using a custom IUrlParameterFormatter that you set as RefitSettings.UrlParameterFormatter. Something like this would work:

public class CustomUrlParameterFormatter : DefaultUrlParameterFormatter
{
    public override string Format(object parameterValue, ParameterInfo parameterInfo)
    {
        if(parameterValue == null)
            return null;

        // Option 1: Just look for IEnumerable<int>
        if(typeof(IEnumerable<int>).IsAssignableFrom(parameterInfo.ParameterType))
            return string.Join(",", (IEnumerable<int>)parameterValue);

        // Option 2: Look for your custom attribute
        var converterAttribute = parameterInfo.GetCustomAttributes<ApiParameterConverterAttribute>().FirstOrDefault();
        if (converterAttribute != null)
        {
            // Assumes your converter is a TypeConverter, but you could use a custom converter type here too
            var converter = (TypeConverter)Activator.CreateInstance(converterAttribute.ConverterType);
            return converter.ConvertToString(parameterValue);
        }

        return base.Format(parameterValue, parameterInfo);
    }
}

// Then do this...
var settings = new RefitSettings 
{ 
    UrlParameterFormatter = new CustomUrlParameterFormatter() 
};
var api = RestService.For<IMyApi>("https://wherever.com", settings);

The options in the code above are alternatives i.e. you wouldn't need both.

bennor commented 9 years ago

There's an even simpler way to do it using a wrapper type and the magic (evil?) of implicit casting:

public class CsvList<T>
{
    IEnumerable<T> values;

    // Unfortunately, you have to use a concrete type rather than IEnumerable<T> here
    public static implicit operator CsvList<T>(List<T> values)
    {
        return new CsvList<T> { values = values };
    }

    public override string ToString()
    {
        if(values == null)
            return null;
        return string.Join(",", values);
    }
}

// Then your interface looks like this...
public interface IMyApi
{
    [Get("api/resource")]
    Task<Response> GetResources(CsvList<int> value);
}

// and you can still call it like this...
await api.GetResources(new List<int>{ 0, 42, 99 });

Easy.

carl-berg commented 9 years ago

As far as I'm concerned, I have had good use of Refit in the past but have moved on to a new library my co-worker and me have created (inspired by Refit) called Web-Anchor.

bennor commented 9 years ago

@carl-berg: Regarding your motivation for building your own - do you realise runtime generation using DynamicProxy is still possible with Refit using this simple shim?

I use it myself for using Refit in LINQPad.

Obviously, it's totally fine to build your own (that's the beauty of open source), but I'm curious as to why not just contribute what you want and make Refit better?

Yortw commented 9 years ago

:+1: Thanks... I somehow missed IUrlParameterFormatter when I was looking for a solution. Appreciate the quick and useful response.

carl-berg commented 9 years ago

@bennor No, i did not know about the possibility to use a shim. There was a few reasons for making a new framework. We were having some issues with querystring-formatting (foremost Lists and DateTime's that used the Thread's UICulture for formatting) that we had problems with. The code-generation approach that Refit took at v.2 seemed like a step in the wrong direction to us compared to DynamicProxy (but i understand that made the library able to run on more platforms, which wasn't something we needed for our use).

Having worked a little bit with the insides of Refit we felt like it was a bit hard to bend Refit into how we wanted it to work. Basically we thought we could make something better and more plugable by building something new from scratch (with focus on plug-ability) that suits a .net environment out of the box. I'm not saying Refit is bad. I've had a lot of use with Refit and it has simplified my work tons, but Web-Anchor just fit our needs better.

bennor commented 9 years ago

@carl-berg Fair enough. There's a lot of value in the experience of building something yourself anyway. Glad it's giving you what you need. :smile:

krishire commented 8 years ago

Somebody please tell me how to append Querystring to an URL in Refit.

[Get("/Test?query={value}] Task GetTest(string value); But the query is getting removed.

ahmedalejo commented 8 years ago

Try the following:

[Get("/Test")] Task GetTest(string query);

On Fri, 5 Feb 2016 15:24 krishire notifications@github.com wrote:

Somebody please tell me how to append Querystring to an URL in Refit.

[Get("/Test?query={value}] Task GetTest(string value); But the query is getting removed.

— Reply to this email directly or view it on GitHub https://github.com/paulcbetts/refit/issues/93#issuecomment-180452353.

krishire commented 8 years ago

Thanks Ahmed, I got it working. On 6 Feb 2016 07:44, "Ahmed Aderopo Alejo" notifications@github.com wrote:

Try the following:

[Get("/Test")] Task GetTest(string query);

On Fri, 5 Feb 2016 15:24 krishire notifications@github.com wrote:

Somebody please tell me how to append Querystring to an URL in Refit.

[Get("/Test?query={value}] Task GetTest(string value); But the query is getting removed.

— Reply to this email directly or view it on GitHub https://github.com/paulcbetts/refit/issues/93#issuecomment-180452353.

— Reply to this email directly or view it on GitHub https://github.com/paulcbetts/refit/issues/93#issuecomment-180714985.

Cheesebaron commented 8 years ago

You need to await it. Also super unrelated to this question...

clairernovotny commented 7 years ago

Closing as this issue appears stale and has a workaround of using IUrlParameterFormatter to specify the specific behavior. Please re-open if needed.

swimtver commented 6 years ago

Does somebody explain me, how can i get this result 'GET /api/resource?value=1&value=2&value=3' with refit?

mikegoatly commented 6 years ago

The list approach that @swimtver describes is the pattern that WebApi (2?) uses to pass array parameters in. For example you would call a controller method of:

public enum WidgetKind
{
    Foo,
    Bar
}

[RoutePrefix("widgets")]
public class MyApi : ApiController
{
    [Route("")]
    [HttpGet]
    public string Search(WidgetKind[] status)
    {
       // ...
    }
}

With this: http://localhost:1234/widgets?status=Foo&status=Bar

Given this is an "out of the box" behavior for WebApi, it feels like something we should get refit handling.

stoiveyp commented 6 years ago

Looking at the same pattern within Amazon Skill Management now. Examples such as:

https://developer.amazon.com/docs/smapi/smapi-migration.html#get-skill-status https://developer.amazon.com/docs/smapi/skill-operations.html#request-4

vitalyal commented 6 years ago

what is the status for this issue? it is anyhow possible to make refit pass array as multiple parameters like ?value=1&value=2&value=3

Having workaround for CSV list is definately not enough

clairernovotny commented 6 years ago

We would take a PR to add this. I would recommend using a parameter attribute to specify the behavior to distinguish it from the current behavior, which is default.

vitalyal commented 6 years ago

After some Refit code investigation i ended up with temporary workaround that looks like this:

   // interface
    [Get("/query")]
    Task QueryWithWrapper(object wrapper);

    // wrapper needed for workaround
    public class WrappedValue
    {
        private int _value;
        public WrappedValue(int value)
        {
            _value = value;
        }

        public override string ToString()
        {
            return _value.ToString();
        }
    }
   // client code
   var wrapped = new
   {
       numbers = new[] { new WrappedValue(1), new WrappedValue(2), new WrappedValue(42) }
   };
   api.QueryWithWrapper(wrapped);

    // this gives url like /query?numbers=1&numbers=2&numbers=42

This workaround is working only on newer Refit version. I had to update from 4.0.0 to 4.3.0

vitalyal commented 6 years ago

@onovotny will it be ok to extend QueryAttribute with collection format described here https://swagger.io/docs/specification/2-0/describing-parameters/ something like

public enum CollectionFormat { Csv, Ssv Tsv, Pipes, Multi }

and in QueryAttribute add property public CollectionFormat CollectionFormat {get; protected set;}

And i'm not sure about default value, because current default behaviour is to serialize .net type name. So i'm considering to set default value to Multi which is default behaviour for asp.net MVC. Any objections?

bennor commented 6 years ago

@vitalyal That sounds great to me.

vitalyal commented 6 years ago

PR is submitted. I've added RefitParameterFormatter option as a default one for backward compatibility with existing code using workaround with CustomUrlParameterFormatter. When major version is released default can be changed to Multi to support ASP.NET MVC integration out of the box

clairernovotny commented 6 years ago

Closing since there is now support for lists