Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change StatusCode to handle unknown status codes #230

Merged
merged 3 commits into from
Jun 5, 2024
Merged

Conversation

jbearer
Copy link
Member

@jbearer jbearer commented Jun 5, 2024

Closes #229

This PR:

Tide's design of their status code type (which we copied) as an enum, is not resilient against new status codes or status codes unknown to the server framework (such as ELB-specific status codes). This caused a panic in a Cappuccino node when it received a response with a status code it did not recognize.

This changes StatusCode from an enum to a wrapper around reqwest::StatusCode, which is itself a wrapper around a u16. As such, it can represent any status code in the allowed range without necessarily knowing what a certain status represents.

We keep most of the same interface by reimplementing all the appropriate and conversion traits. However, there are a couple of breaking changes:

  • We no longer implement From<StatusCode> for tide::StatusCode; instead we implement TryFrom, so we can return an error instead of panicking if it is a status code that tide cannot handle
  • Since our status code is no longer an enum, we replace enum variant constructors with associated constants, which is the same as what reqwest does

Tide's design of their status code type (which we copied) as an enum,
is not resilient against new status codes or status codes unknown to
the server framework (such as ELB-specific status codes). This caused
a panic in a Cappuccino node when it received a response with a status
code it did not recognize.

This changes `StatusCode` from an enum to a wrapper around `reqwest::StatusCode`,
which is itself a wrapper around a `u16`. As such, it can represent any status
code in the allowed range without necessarily knowing what a certain status
represents.

We keep most of the same interface by reimplementing all the appropriate and
conversion traits. However, there are a couple of breaking changes:
* We no longer implement `From<StatusCode> for tide::StatusCode`; instead we
  implement `TryFrom`, so we can return an error instead of panicking if it
  is a status code that `tide` cannot handle
* Since our status code is no longer an enum, we replace enum variant
  constructors with associated constants, which is the same as what `reqwest`
  does
@jbearer jbearer merged commit c99d61d into main Jun 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StatusCode does not support all possible status codes
3 participants