Refit: [BUG] Nullable [Query] parameters malformed in v4.7.9

[mikegoatly]

Describe the bug
Since updating from 4.6.107 to 4.7.9 requests that rely on nullable parameters marked with the [Query] attribute are sent through with the incorrect parameter name.

Steps To Reproduce
Given this interface:

public interface IMyClient
{
    [Get("/api/{id}")]
    Task<MyResult> GetData(
        int id, 
        [Query]string text= null,
        [Query]int? optionalId = null,
        [Query(CollectionFormat = CollectionFormat.Multi)]string[] filters = null)
}

When calling the client like this:

var result = await client.GetData(123, "title", 999, new[] { "A", "B" });

I'm seeing the request sent as:

https://myservice.com/api/123?Value=title&Value=999&HasValue=A&HasValue=B&filters=False

Expected behavior
The request should be sent as:

https://myservice.com/api/123?text=title&optionalId=999&filters=A&filters=B

Environment

• OS: Window 10
• Version 10.0.17763 Build 17763
• Device: DELL XPS15

Additional context

[bennor]

Definitely looks like a bug there, but you shouldn't need the [Query] attributes on any but the last parameter.

I don't expect removing them to help much and even if it does, you can't take it off the last parameter anyway.

[jamiehowarth0]

I'm going to pick this up.

Edit: @mikegoatly The Query attribute should only be applied to non-simple types (so, collections or POCOs). In a GET method, all parameters are explicitly serialized to querystring parameters, you only need to specify the attribute on the collection here.

However, nullable types should definitely be a thing.

[jamiehowarth0]

Basically the support for POCO objects as [Query] parameters was scoped to include support for arrays and IEnumerable's (which there are already test cases for, thus non-breaking), but nullable support has never been explicitly tested for, and thus automatically "worked" before we started explicitly checking the type of the [Query] param.
"string?" and corresponding nullables compile into a Nullable so I'm adding tests now.

[hhuseyinpay]

I have similar issue.

public interface IStudentService
{
    [Get("/date/?ordering=created_day")]
    Task<List<StudentModel>> GetStudentsWithDateAsync([Query(Format = "yyyy-MM-dd")] DateTime date, [Query] int student);
}

in 4.7.9 request sent as: date/?ordering=created_day&Date=1&Day=2019-06-13
in 4.6.107 request sent as: date/?ordering=created_day&date=2019-06-13&student=1

[jamiehowarth0]

@hhuseyinpay thanks for an additional test case, I'll pick this up over the weekend.

Edit: Thanks to supplying this test case, I've realised both Nullable and DateTime are structs as opposed to classes. This makes detecting them a lot easier!

[CK-Yong]

@onovotny Hi, awesome that this issue was picked up so quick! Any word on when this fix will be released? I don't see anything regarding a release schedule within the docs.

Thanks!

[jamiehowarth0]

This is available on the public MyGet feed if you need to use it ASAP – v4.7.20 and above: https://www.myget.org/F/refit/api/v3/index.json.

Edit: I'm not aware of the release schedule, one of the repo owners normally takes care of that.

Also, glad I could get it fixed quickly for you :)