refit icon indicating copy to clipboard operation
refit copied to clipboard

[Bug]: Query parameters not added when using both [Property] and [Query] attributes

Open loop8ack opened this issue 1 year ago • 3 comments

Describe the bug 🐞

When using both [Property] and [Query] attributes on method parameters, the query parameters are not added to the request URI, while path parameters work as expected. This behavior blocks our testing setup where we need both:

  • [Property] to access parameter values in test handlers
  • [Query] to add parameters to the request URI

Environment:

  • Refit 8.0.0
  • Refit.Newtonsoft.Json

Step to reproduce

ITestApi:

public interface ITestApi
{
    [Get("/{value1}")]
    Task DoWork(
        [Property("value1")] string value1,
        [Property("value2")][Query] string value2,
        [Query] string value3,
        CancellationToken cancellationToken);
}

DebugHandler:

public class DebugHandler : DelegatingHandler
{
    public DebugHandler(HttpMessageHandler innerHandler)
        : base(innerHandler)
    {
    }
        
    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var uri = request.RequestUri;
        var options = request.Options;
        Debugger.Break();
        return await base.SendAsync(request, cancellationToken);
    }

    protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        var uri = request.RequestUri;
        var options = request.Options;
        Debugger.Break();
        return base.Send(request, cancellationToken);
    }
}

Program:

using var handler = new DebugHandler(new HttpClientHandler());
using var httpClient = new HttpClient(handler);
httpClient.BaseAddress = new Uri("https://www.google.de/");
var api = RestService.For<ITestApi>(httpClient);
await api.DoWork("1", "2", "3", CancellationToken.None);

Uri:

https://www.google.de/1?value3=3

Note: The path parameter is correctly replaced in the route, only the query parameters are affected.

I would expect:

https://www.google.de/1?value2=2&value3=3

Reproduction repository

No response

Expected behavior

Parameters should be accessible via Properties AND added to request URI when both attributes are present.

Screenshots 🖼️

No response

IDE

No response

Operating system

No response

Version

No response

Device

No response

Refit Version

8.0.0

Additional information ℹ️

This issue is blocking our test phase starting next week. We need either:

  • A quick fix or simple workaround
  • An estimated timeline for a framework fix to help plan our approach

I just discovered this during testing today, apologies for the urgent notice - you know how these things go!

loop8ack avatar Dec 06 '24 15:12 loop8ack

I think I may have found the relevant line in RestMethodInfo.cs line 141 Is there any reason for this?

If you don't have time for that: Would you be open to me submitting a PR with the fix and corresponding tests? That way you'd only need to do the code review and release.

loop8ack avatar Dec 06 '24 21:12 loop8ack

Would this work?

public interface ITestApi
{
    [Get("/{value1}")]
    Task DoWork(
        [Property("value1")] string value1,
        [Query] string value2,
        [Property("value2")] string propValue2,
        [Query] string value3,
        CancellationToken cancellationToken);
}

Is there any reason for this?

Not sure, you could try asking in slack or ping one of the main contributors. IMO it should be documented or have an error / diagnostic associated with it 🤔

Would you be open to me submitting a PR with the fix and corresponding tests?

I suspect you'd have to expand this to all attribute combinations not just your current case. This would be a breaking change and I don't know if it would be approved.

TimothyMakkison avatar Dec 10 '24 22:12 TimothyMakkison

Would this work?

Hm - good idea, why didn't I think of that :D Yes, this works great, it's a bit messy because the parameters are duplicated, but I don't need a complex DelegatingHandler that searches for the parameters and then appends them to the query. This is a much better workaround, thanks for the idea :)

Is there any reason for this?

@anaisbetts Can you say anything about this? It looks like a bug to me, or am I missing something?


And as a small update: My deadline before the test phase this week was postponed, luckily not by us, so I gained some more time :)

loop8ack avatar Dec 11 '24 00:12 loop8ack