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]: derived types erroneously add additional query parameters. #1882

Open
TimothyMakkison opened this issue Oct 15, 2024 · 0 comments
Open
Labels

Comments

@TimothyMakkison
Copy link
Contributor

TimothyMakkison commented Oct 15, 2024

Describe the bug 🐞

Passing a more derived type to a Refit method will cause properties to be used in both the path and query.

Step to reproduce

var client = RestService.For<IApi>("http://bar");

// http://bar/foo/road?MyQuery=quiz
var _ = await client.Send(new MyObject { MyPath="road", MyQuery = "quiz"});

// http://bar/foo/road?MyPath=road&MyQuery=quiz&MyQuery2=quibble
// adds MyPath as a query parameter
// expected:
// http://bar/foo/road?MyQuery=quiz&MyQuery2=quibble
var _ = await client.Send(new MyDerived { MyPath="road", MyQuery = "quiz", MyQuery2 = "quibble"});

public interface IApi
{
    [get("/foo/{request.MyPath}"]
    Task<string> Send(MyObject request);
}

public class MyObject
{
    public string MyPath { get; set; }
    public string MyQuery { get; set; }
}

public class MyDerived : MyObject { public string MyQuery2 {get; set; } }

Reproduction repository

https://github.com/reactiveui/refit

Expected behavior

Refit should not add a property as a query parameter if it is already mapped to the path.

Cause:

  • Refit constructs RestMethodInfoInternal using MethodInfo
  • BuildParameterMap identifies that the url is parameterised and that the corresponding property belongs to a parameters property.
  • request.MyPath, is added to the ParameterProperties list, adding the ParameterInfo for MyBase.MyPath.
  • RequestBuilderImplementation uses this list in BuildQueryMap to ensure that it doesn't add properties already used in the path to the query.
    • Note that BuildQueryMap uses reflection for each object passed to it. In our case it is building a query map for the type MyDerived.
  • This comparison compares the current property MyDerived.MyPath to any values in ParameterProperties.
    • The relevant value has a ParameterInfo for MyBase.MyPath, the comparison fails because they are different properties.
    • Refit adds MyDerived.MyPath to the query.
  • Note that AddObjectParametersToUrl doesn't have this issues because it uses the ParameterInfo for MyBase.MyPath to get the value from parameter, so will succesfully add the value to the path. (This might causes bugs as its using the base types property) var propertyObject = propertyInfo.PropertyInfo.GetValue(param)

I don't think this is intended behaviour, but I imagine its been in the Refit for so long that fixing it would be a breaking change.

Screenshots 🖼️

No response

IDE

No response

Operating system

No response

Version

No response

Device

No response

Refit Version

No response

Additional information ℹ️

No response

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

3 participants
@TimothyMakkison and others