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

Option guards should return None only if the value is empty. #2831

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

the10thWiz
Copy link
Collaborator

@the10thWiz the10thWiz commented Jul 24, 2024

Based on #2543

Option is usable as pretty much any kind of guard. Right now, it's always infallible, and simply ignored errors. This may be somewhat counter-intuitive for newcomers (and old hats such as myself), and may have some other negative implications. This PR changes the behavior of Option to match what one might expect - None represents the empty case, as it makes sense for each type of guard.

The Negative Implications

There are a couple of issues I can see with the current behavior. First, and most obvious, it's likely not what a newcomer to Rocket would expect. We should avoid sweeping errors under the rug, so to speak, and instead leave that choice fully up to the user. Second, there may be a non-trivial overhead to simply discarding data in the case we fail to parse. This is generally mitigated by data limits, but I don't think they represent a full solution. Finally, the current behavior may actually impede debugging efforts. To see how, consider the following route:

#[get("/?<age>")]
fn r(age: Option<u8>) -> ...

Right now, a request to /?age=a will call r(None) - with no mention (even in the logs) that the guard failed. With this PR, the above request will instead return an Error (which, with typed-catchers, could produce a nice error message for the consumer). If age had a more complex parser, this would make debugging much harder, since the error is just being silently ignored.

New behavior

In general, the old behavior can be emulated by using Result, and ignoring the Err value. We could provide a separate Option-like enum that had the old behavior, but it might be easier to simply provide a set of type aliases that fill in the error type automatically, e.g. type DataResult<T: FromData> = Result<T, T::Error>;

FromData

FromData now uses peek to check whether the data contains any bytes. Iff it is empty, it will succeed with None.

FromParam

This implementation has been removed. This is a more obviously breaking change, but since Rocket ignores empty segments, we can't meaningfully provide this implementation anymore.

FromSegements

This implementation now only succeeds with None if the Segments is empty.

FromForm

This one has different behavior depending on the current strictness. In strict mode, it succeeds with None iff there were no values passed to the FromForm implementation. Otherwise, it also succeeds with None if all the ErrorKinds were Missing.

See the diff of core/lib/src/form/tests.rs for the changes to how Option parses.

FromRequest

There isn't really an easy way to check whether a request guard failed or was simply missing, but now Option only catches Forwards. In this case, the old behavior can be emulated by Option<Result>, Result<Option>, or Outcome.

TODO

  • Update documentation
    • FromData
    • FromParam
    • FromSegments
    • FromForm
    • FromRequest
  • FromRequest implementation

@the10thWiz
Copy link
Collaborator Author

@SergioBenitez This is a breaking change, so it needs to land before 0.6 (or be rejected).

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.

1 participant