reactiveui / refit

The automatic type-safe REST library for .NET Core, Xamarin and .NET. Heavily inspired by Square's Retrofit library, Refit turns your REST API into a live interface.
https://reactiveui.github.io/refit/
MIT License
8.65k stars 744 forks source link

feat: prevent serialization of `CancellationToken?` #1917

Closed TimothyMakkison closed 2 weeks ago

TimothyMakkison commented 2 weeks ago

I'm not sure how Refit should handle nullable Cancellation token, it's a struct so it isn't nullable by default. My two ideas:

See #1915

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.94%. Comparing base (6ebeda5) to head (9cec517). Report is 163 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1917 +/- ## ========================================== - Coverage 87.73% 84.94% -2.80% ========================================== Files 33 36 +3 Lines 2348 2504 +156 Branches 294 364 +70 ========================================== + Hits 2060 2127 +67 - Misses 208 299 +91 + Partials 80 78 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ChrisPulman commented 2 weeks ago

I am thinking that raising a diagnostic would be the best, accepting a null token may cause more harm further down the line.

TimothyMakkison commented 2 weeks ago

I am thinking that raising a diagnostic would be the best, accepting a null token may cause more harm further down the line.

👍 What severity level should the diagnostic be? Should I also add a runtime exception if it a warning?

ChrisPulman commented 2 weeks ago

So it seems that CancellationToken = default gets turned into CancellationToken.None by the internal workings. However a completely unset value would result in a null reference exception. So I am thinking that a nullable unset token should raise a Error level diagnostic and the use of nullable Tokens should have a warning diagnostic. I have not run any tests on these scenarios yet, and there may be some checks within HttpClient to replace the null value with None.

github-actions[bot] commented 2 days ago

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.