Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
loop8ack opened this issue Dec 6, 2024 · 3 comments
Labels

Comments

@loop8ack
Copy link

loop8ack commented Dec 6, 2024

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 loop8ack added the bug label Dec 6, 2024
@loop8ack
Copy link
Author

loop8ack commented Dec 6, 2024

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.

@TimothyMakkison
Copy link
Contributor

TimothyMakkison commented Dec 10, 2024

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.

@loop8ack
Copy link
Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants