feat: prevent serialization of `CancellationToken?` by TimothyMakkison · Pull Request #1917 · reactiveui/refit

@TimothyMakkison

  • Prevent CancellationToken? being serialized.
  • Add cancellation tests to Tests/RestService, including NullableCancellationTokenShouldBeIgnored

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:

  • Refit functions containing a CancellationToken parameter should throw an exception or emit a diagnostic.
  • Accept CancellationToken?, check these tokens for nullability. If they are null then substitute it with a default token.

See #1915

@codecov

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TimothyMakkison

ChrisPulman

@ChrisPulman

@ChrisPulman

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

@TimothyMakkison

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

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

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators

Nov 21, 2024