Skip to content

[Bug]: derived types erroneously add additional query parameters. #1882

Open
@TimothyMakkison

Description

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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions