Refit: [BUG] Dictionary contained inside ComplexQueryParam not properly transformed in QueryString

Created on 14 Sep 2020 · 2Comments · Source: reactiveui/refit

Describe the bug
Dictionaries contained inside object for GET methods aren't managed properly. While they are managed properly when directly provided as parameters

 public class MyComplexQueryParams
 {
       //Other properties omitted for example purpose

        public Dictionary<string, object> MetaData { get; set; } = new Dictionary<string, object>();

       //Other properties omitted for example purpose
 }

Instead of transforming properly the dictionary as Query parameter with the following format
&{prefix}{delimiter}{keyvalue.Key}={keyvalue.Value}

it consider it as an IEnumerable and use the following format
&{prefix} = [keyvalue.Key, keyvalue.Value]

Expected behavior

Use the format : &{prefix}{delimiter}{keyvalue.Key}={keyvalue.Value}

Additional Information

I've checked, the code to see what's happening.
The problem is located in the RequestBuilderImplementation.cs

Some treatments are done to handle the IEnumerable before the specific test to the IDictionary => But the IDictionary Implement IEnumerable so it means the IDictionary is not handled anymore , but instead is always handled as IEnumerable.

                if (!(obj is string) && obj is IEnumerable ienu)
                {
                    foreach (var value in ParseEnumerableQueryParameterValue(ienu, propertyInfo, propertyInfo.PropertyType, queryAttribute))
                    {
                        kvps.Add(new KeyValuePair<string, object>(key, value));
                    }

                    continue;
                }

 `              if (DoNotConvertToQueryMap(obj))
                {
                    kvps.Add(new KeyValuePair<string, object>(key, obj));
                    continue;
                }

                switch (obj)
                {
                    case IDictionary idict:
                        foreach (var keyValuePair in BuildQueryMap(idict, delimiter))
                        {
                            kvps.Add(new KeyValuePair<string, object>($"{key}{delimiter}{keyValuePair.Key}", keyValuePair.Value));
                        }

                        break;

Suggested Fix
A possible fix would be to simple change the condition
if (!(obj is string) && obj is IEnumerable ienu)
by
if (!(obj is string) && obj is IEnumerable ienu && !(obj is IDictionary) )

If you are fine with that solution, I can make a PR

Problem with Unit tests

When checking the code to try to understand why it doesn't work, I've found some strange behaviors in the unit tests.
But except if I am wrong, or if I miss something, some of them are simply testing the Mock instead of testing the transformation of object in querystring.

        [Fact]
        public async Task ComplexDynamicQueryparametersTestWithIncludeParameterName()
        {
            var mockHttp = new MockHttpMessageHandler();

            var settings = new RefitSettings
            {
                HttpMessageHandlerFactory = () => mockHttp
            };

            mockHttp.Expect(HttpMethod.Get, "https://httpbin.org/get")
                .Respond("application/json", "{'url': 'https://httpbin.org/get?search.FirstName=John&search.LastName=Rambo&search.Addr.Zip=9999&search.Addr.Street=HomeStreet 99', 'args': {'search.Addr.Street': 'HomeStreet 99','search.Addr.Zip': '9999','search.FirstName': 'John','search.LastName': 'Rambo'}}");

            var myParams = new MyComplexQueryParams
            {
                FirstName = "John",
                LastName = "Rambo"
            };
            myParams.Address.Postcode = 9999;
            myParams.Address.Street = "HomeStreet 99";

            var fixture = RestService.For<IHttpBinApi<HttpBinGet, MyComplexQueryParams, int>>("https://httpbin.org/get", settings);

            var resp = await fixture.GetQueryWithIncludeParameterName(myParams);

            Assert.Equal("John", resp.Args["search.FirstName"]);
            Assert.Equal("Rambo", resp.Args["search.LastName"]);
            Assert.Equal("9999", resp.Args["search.Addr.Zip"]);
        }

In this example, the mock usage is not correct

     mockHttp.Expect(HttpMethod.Get, "https://httpbin.org/get")
                .Respond(

Here the mock expect a call to be done on the GET but without checking the generated parameters. The method WithQueryString should have been used to ensure it receives the correct parameters.

And then the Asserts simply check what has been hardcoded in the Respond which doesn't properly test what we expect.
Furthermore, for that case, it doesn't include the Dictionary Metadata, which could have been useful to test.

bug

Source