[BUG] Index out of bounds Error when passing array as Query if its not first parameter

[moliver-bb]

Describe the bug [Get("/v1/project/{projectId}/markups")] Task<List<Users>> GetMarkups(string projectId, [Query(CollectionFormat.Multi)] string[] status); fails with: System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. Parameter name: index at System.Linq.Enumerable.ElementAt[TSource](IEnumerable1 source, Int32 index) at Refit.RequestBuilderImplementation.<>cDisplayClass17_0.<b0>d.MoveNext() in d:\a\1\s\Refit\RequestBuilderImplementation.cs:line 569 --- End of stack trace from previous location where exception was thrown --- at Refit.RequestBuilderImplementation.<>cDisplayClass14_0`2.<b0>d.MoveNext() in d:\a\1\s\Refit\RequestBuilderImplementation.cs:line 244 --- End of stack trace from previous location where exception was thrown --- `

Steps To Reproduce Pass any array as Query where it is not the first parameter and it will throw. When passed as the first argument everything works fine. This works fine: [Get("/v1/project/{projectId}/markups")] Task<List<Users>> GetMarkups([Query(CollectionFormat.Multi)] string[] status, string projectId);

Expected behavior Shouldn't throw an error should work as argument in any position

Environment

• OS: dotnet core 2.2 on Mac
• Version 4.7.40
• Device: NA

[jamiehowarth0]

I'll take this one.

[DOMZE]

Hello,

Having the same problem when using the multi parameter in the Query attribute

Here's the signature of the problematic method [Get("/api/v1/user/info")] Task<IEnumerable<UserInformation>> GetUserInformationAsync([Header("Authorization")] string authorization, [Query(CollectionFormat.Multi)][AliasAs("userId")]IEnumerable<string> userIds);

However this works [Get("/api/v1/user/info")] Task<IEnumerable<UserInformation>> GetUserInformationAsync([Query(CollectionFormat.Multi)][AliasAs("userId")]string[] userIds, [Header("Authorization")] string authorization);

[jamiehowarth0]

@DOMZE @moliver-bb can you both try and reproduce in the latest Refit that just got published to Nuget, 4.7.51?

[john-manktelow]

I'm getting the same exception, but only since 4.7.51. Changing the order of the parameters can work around it, though. For me, it's having a [Header] parameter first that triggers it.

Repro

using System;
using System.Net.Http;
using System.Threading.Tasks;
using Refit;
using RichardSzalay.MockHttp;

namespace refitRepro
{
    public interface ITheService
    {
        // Works
        [Get("/v1/HeaderSecond")]
        Task<string> HeaderSecond(string term, [Header("Some-Header")]string someHeader);

        // Breaks
        [Get("/v1/HeaderFirst")]
        Task<string> HeaderFirst([Header("Some-Header")]string someHeader, string term);
    }

    class Program
    {
        static void Main(string[] args)
        {
            const string rootUrl = "http://localhost";

            var mockHttp = new MockHttpMessageHandler();

            mockHttp.Expect(HttpMethod.Get, rootUrl + "/v1/HeaderSecond")
                .WithHeaders("Some-Header", "someHeader")
                .Respond("application/json", "result");

            mockHttp.Expect(HttpMethod.Get, rootUrl + "/v1/HeaderFirst")
                .WithHeaders("Some-Header", "someHeader")
                .Respond("application/json", "result");

            var sut = RestService.For<ITheService>(rootUrl,
                new RefitSettings() {HttpMessageHandlerFactory = () => mockHttp});

            // If the header is not the first param, everything is fine.
            var taskWorks = sut.HeaderSecond("headerSecond", "someHeader");
            Task.WaitAll(taskWorks);
            Console.WriteLine("Got a result when the header is the second param.");

            // If the header is the first param, we get an ArgumentOutOfRangeException from the RequestBuilderImplementation
            var taskBreaks = sut.HeaderFirst("someHeader", "headerFirst");
            Task.WaitAll(taskBreaks);
            Console.WriteLine("Exception thrown if the header is the first param.");
        }
    }
}

Exception

Unhandled exception. System.AggregateException: One or more errors occurred. (Specified argument was out of the range of valid values. (Parameter 'index'))
 ---> System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'index')
   at System.Linq.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument)
   at System.Linq.Enumerable.ElementAt[TSource](IEnumerable`1 source, Int32 index)
   at Refit.RequestBuilderImplementation.<>c__DisplayClass17_0.<<BuildRequestFactoryForMethod>b__0>d.MoveNext() in d:\a\1\s\Refit\RequestBuilderImplementation.cs:line 594
--- End of stack trace from previous location where exception was thrown ---
   at Refit.RequestBuilderImplementation.<>c__DisplayClass14_0`2.<<BuildCancellableTaskFuncForMethod>b__0>d.MoveNext() in d:\a\1\s\Refit\RequestBuilderImplementation.cs:line 244
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.WaitAllCore(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
   at refitRepro.Program.Main(String[] args) in Program.cs:line 47

Workaround Change the order of the parameters, or revert to Refit 4.7.9

[martincostello]

Same problem here with 4.7.51. This interface worked in 4.7.9 but throws ArgumentOutOfRangeException in 4.7.51:

internal interface IFoo
{
    [Put("/foo/{id1}/bar")]
    Task<ApiResponse<string>> DoThingAsync(
        [Header("x-custom-header")] string myHeader,
        string id1,
        string id2,
        JObject body,
        CancellationToken cancellationToken);
}

   at System.Linq.Enumerable.ElementAt[TSource](IEnumerable`1 source, Int32 index)
   at Refit.RequestBuilderImplementation.<>c__DisplayClass17_0.<<BuildRequestFactoryForMethod>b__0>d.MoveNext() in d:\a\1\s\Refit\RequestBuilderImplementation.cs:line 594
--- End of stack trace from previous location where exception was thrown ---
   at Refit.RequestBuilderImplementation.<>c__DisplayClass14_0`2.<<BuildCancellableTaskFuncForMethod>b__0>d.MoveNext() in d:\a\1\s\Refit\RequestBuilderImplementation.cs:line 244

~I just noticed that it has an extra parameter left over from a previous version of code that's no longer used, which might be contributing to the problem.~ (Because it's a query string parameter 🤦‍♂)

[martincostello]

Explicitly adding the query string parameter to the URL template works around the issue.

[jamiehowarth0]

I think that because we added POCO support to [Query] params, the IndexOf function got modified, cause previously if you had an IEnumerable on the POCO, it threw index errors (if I recall my debugging sessions correctly).

I'll pick this up & look to try and fix before Tuesday, it's a 3-day weekend here in the UK.

Edit: once it is fixed, it'll be on Myget first pending an official release to Nuget, so you may want to try using the nightly on Myget if you urgently need this fixed.

[clairernovotny]

Thanks. The CI feed is now on Azure Artifacts now btw.

Let’s ensure we have test cases that cover these scenarios too.

[jamiehowarth0]

@onovotny picking this up today alongside #696 and #587, hopefully I can get a PR in by tomorrow

[CreepyGnome]

@benjaminhowarth1 did you happen to make any progress on this?

[CreepyGnome]

Just additional confirmation, that for me at least, in 4.7.51 it is the header attribute being first and moving it to after query string values it works, and making no changes and rolling back to 4.7.9 works.

I rolled back to 4.7.9 as this is a breaking change in a patch version which should never occur. Also I would argue this type of breaking change should happen in a minor version as well. Even a major version breaking change would be bad for this impact unless there was good justification for it.

Questions for those who maintain and release refit:

1. Why are there no release notes for 4.7.51?
2. Why was 4.7.51 released, what are the important changes?
3. Are there no tests around this functionality that triggered the breaking change?

I would recommend the release should be removed from GitHub and unlisting in nuget as this is is a huge breaking change. It killed just about all my refit calls and there is no way I am going to go back and refactor all of them to move the auth header from first to after the query string values.

[fouadroumieh]

This issue is not fixed in 4.7.51 and its quite critical issue, my first time using refit and its quite annoying!

[clairernovotny]

@benjaminhowarth1 any updates here?

[stoff99]

Hey @onovotny,

this is still not fixed. I get the same with the latest 5.0.15. Here is my api definition:

[Headers("Accept:application/json", "X-API-V: 125")] [Get("/api/someModule/deviceList?controlId={control_id}")] Task<DeviceListResponse> DeviceList([Header("Authorization")] string authorization, [Header("X-Lng")] string twoLetterLang, string search, [AliasAs("control_id")] string controlId, string secret);

We still get this error: "Specified argument was out of the range of valid values.\nParameter name: index"

When i set the header arguments to the end of the method its working. Example: [Headers("Accept:application/json", "X-API-V: 125")] [Get("/api/someModule/deviceList?controlId={control_id}")] Task<DeviceListResponse> DeviceList(string search, [AliasAs("control_id")] string controlId, string secret, [Header("X-Lng")] string twoLetterLang, [Header("Authorization")] string authorization);

To change this everywhere in all my libraries and apps is no possible solution. We sharing library codes across app and server in some projects. I rly cannot refactor all of this for sure. I revert back to 4.6.107 now. What is very important to know if you can provide a fix for this bug. Or isn't it a bug?

Thx a lot Stefan

[clairernovotny]

This should be finally fixed in 5.0.23, rolling out to NuGet now. If it crops up again, please open a new issue. The interface definitions have been helpful to validate these fixes and add additional test coverage.

[CreepyGnome]

@onovotny So it was broken in a patch update, but is being fixed in a major version update? Is there any way to fix it in at least a minor/patch so more people can upgrade to it if they have change controls going to major versions?

We had to roll back to the last working version (4.7.9) of refit as we have too many interfaces using refit and would be a large undertaking to refactor all that, and then do a full regression. We cannot just go from a 4.x to a 5.x without a lot of additional work and a full regression test because of a major version update in a library. This fix, as-is, will still be out of our reach in several of our larger and older projects.

[clairernovotny]

Refit has a policy of always taking the latest stable version version of its dependencies as they are available. That forced a major version bump to be in-line with SemVer. That said, we generally only release off of master and fix-forward.

[CreepyGnome]

I hope you understand the problem with that policy.

That a hotfix policy may be appropriate when you release an unexpected breaking change in a patch release so that you can fix/revert what is needed to release another patch release to address what was broken. Preferably quickly. Hotfix branches and CICD release pipelines are something that could be implemented to make it easy to manage a hotfix release.

[stoff99]

@onovotny the bug is fixed with 5.0.23 now. Thx a lot!