[Bug]: "CancellationToken?" mishandling in refit requests

[AFract]

Describe the bug 🐞

Hello,

We are happily using Refit in a project with a lot of calls like :

    [Post("SomeRoute)]
    public Task<ApiResponse<SomeDto>> DoSomething(CancellationToken? cancellationToken = null);

(this example is for POST, but PUT or GET does the same)

We have noticed that when a CancellationToken is provided, it is handled as a query string parameter :

(note the IsCancellationRequested, CanBeCancelled... Appearing in the query string).

If the signature is updated as below, the CancellationToken is handled as expected:

    [Post("SomeRoute)]
    public Task<ApiResponse<SomeDto>> DoSomething(CancellationToken cancellationToken);

But we were willing to make the CancellationToken optional, hence the "?" and "= null". We didn't figure it won't work.

One workaround would be to change the signature for

    [Post("SomeRoute)]
    public Task<ApiResponse<SomeDto>> DoSomething(CancellationToken cancellationToken = default);

to make the parameter both non nullable and optional, "default" does exactly what a CancellationToken.None does:

Do you have a suggestion regarding my workaround above, is it good for you ? Do you plan to improve this to handle "nullable CancellationToken" ?

Thank you very much !

Step to reproduce

Write a refit method like
[Post("SomeRoute)]
public Task<ApiResponse> DoSomething(CancellationToken? cancellationToken = null);
and see query string.

Expected behavior

It would be nice and safer if nullable "CancellationToken?" methods signatures could be handled as non nullable "CancellationToken" instead of to be processed as query string.

IDE

Visual Studio 2022

Operating system

Windows

Refit Version

Refit.Newtonsoft.Json (and refit) in version 7.2.1 with .Net 8. Same behavior with Refit.Newtonsoft.Json in version 8.

[TimothyMakkison]

Hey, thanks for the bug report. I'm shocked this wasn't discovered earlier 😄.

Thankfully, I don't think many people are using nullable cancellation tokens as they are a struct type. I don't think many apis accept a nullable token. I'll see if I can fix this or make refit throw an exception on nullable tokens.

[AFract]

Thank you for your quick answer ! If it's not possible to use the nullable cancellation token, at least throwing an exception as you have suggested rather than serializing it in query would avoid to end up with our situation :).
Please let me know if you think you'll be able to address the issue by "properly" handling nullable cancellationtokens as in my example, it will make us know if we should apply the workaround I have suggested or if it's relevant to wait for the fix :).

[TimothyMakkison]

I'm leaning towards throwing an exception and raising a diagnostic.
I did contemplate adding a case where we check for null and pass a default token instead. Not sure which one follows the law of least surprise the most

[AFract]

I guess if somebody pass a nullable cancellation token with a null value, it's because he wants a cancellation token for his query, not some bits of cancellation token in its url :D. So I don't think anybody would be surprised if the "null" CT was suddenly properly handled as a CancellationToken.None, neither if the nullable argument itself was correctly processed :p.

Actually, regarding existing usages of the library, an exception coming from nowhere in future releases is an higher risk, as before it was just an "almost silent error" that nobody has never noticed.

Thank you again :)

[AFract]

Hello,
I had a look on the related pull requests, if I understand well you just prevent to serialize the "CancellationToken?" in the query, but you don't do anything with it ? (no usage, no diagnostic message, no exception, etc) ?
Note that I am not complaining at all, it's just to define how to deal with our error on our side.
I guess we'll go for the CancellationToken cancellationToken = default :).

[ChrisPulman]

We need to do a few tests on this, I would certainly agree with CancellationToken cancellationToken = default being a good choice, which if it remains with this value as it's consumed will result in CancellationToken.None due to it being a Struct.