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

Possible to disable catchers? #1040

Closed
philbooth opened this issue Jun 25, 2019 · 4 comments
Closed

Possible to disable catchers? #1040

philbooth opened this issue Jun 25, 2019 · 4 comments
Labels
no bug The reported bug was confirmed nonexistent question A question (converts to discussion)

Comments

@philbooth
Copy link

I feel like there's probably a straightforward answer to this staring me in the face, but I've not been able to find it, sorry.

Anyway, I'm updating an old codebase from 0.3.17 to 0.4.1. It contains an error type that impls Responder like so (it also impls Serialize):

impl<'r> Responder<'r> for AppError {
    fn respond_to(self, request: &Request) -> response::Result<'r> {
        let status = self.inner.get_context().http_status();
        let json = Json(self);
        let mut builder = Response::build_from(json.respond_to(request)?);
        builder.status(status).ok()
    }
}

There are some tests for my API that look like this:

#[test]
fn missing_to_field() {
    let server = rocket::ignite()
        .mount("/", routes![super::handler]);
    let client = Client::new(server).unwrap();

    let mut response = client
        .post("/send")
        .header(ContentType::JSON)
        .body(
            r#"{
      "subject": "bar",
      "body": {
        "text": "baz"
      },
      "provider": "mock"
    }"#,
        )
        .dispatch();

    assert_eq!(response.status(), Status::BadRequest);

    let body = response.body().unwrap().into_string().unwrap();
    let error: AppError =
        AppErrorKind::InvalidPayload(String::from("missing field `to` at line 7 column 5")).into();
    let expected = serde_json::to_string(&error).unwrap();
    assert_eq!(body, expected);
}

Before updating to 0.4 these succeed, but now they fail and the output indicates the default 422 catcher is taking control of the response:

---- api::send::test::missing_to_field stdout ----
🔧 Configured for development.
    => address: localhost
    => port: 8000
    => log: normal
    => workers: 16
    => secret key: generated
    => limits: forms = 32KiB
    => keep-alive: 5s
    => tls: disabled
👾 Catchers:
🛰  Mounting /:
    => POST /send application/json (handler)
POST /send application/json:
    => Matched: POST /send application/json (handler)
    => Error: Couldn't parse JSON body: Error("missing field `to`", line: 7, column: 5)
    => Outcome: Failure
    => Warning: Responding with 422 Unprocessable Entity catcher.
thread 'api::send::test::missing_to_field' panicked at 'assertion failed: `(left == right)`
  left: `Status { code: 422, reason: "Unprocessable Entity" }`,
 right: `Status { code: 400, reason: "Bad Request" }`', src/api/send/test.rs:186:5

What should I do to stop the catcher kicking in?

@jebrosen
Copy link
Collaborator

As far as I can tell the change here was not in Rocket, but in the route (AppResult<Email> paired with impl FromData for Email -> Json<Email>). Json<Email> responds immediately with a 422 if there is an error, where impl FromData for Email was responsible for responding with a 400 and the error details.

If you removed impl FromData for Email due to the complexity of FromData in 0.4, you likely want to implement FromDataSimple now instead.

@philbooth
Copy link
Author

Ah, great, thank you!

@jebrosen jebrosen added no bug The reported bug was confirmed nonexistent question A question (converts to discussion) labels Jun 25, 2019
@philbooth
Copy link
Author

@jebrosen, sorry to bug you again, I'm still struggling with this.

In addition to the Responder impl on AppError above, I added this impl of FromDataSimple to my incoming data type:

impl FromDataSimple for Email {
    type Error = AppError;

    fn from_data(request: &Request, data: Data) -> Outcome<Self, Self::Error> {
        let mut email_address = String::new();
        if let Err(error) = data.open().take(1024).read_to_string(&mut email_address) {
            return Outcome::Failure((
                Status::InternalServerError,
                AppErrorKind::Internal(error.to_string()).into(),
            ));
        }

        Json::<Email>::from_data(
            request,
            Transform::Borrowed(Outcome::Success(&email_address)),
        )
        .map_failure(|(_status, error)| {
            match error {
                JsonError::Io(ref inner) => (
                    Status::InternalServerError,
                    AppErrorKind::Internal(inner.to_string()).into(),
                ),
                JsonError::Parse(_, ref inner) => (
                    Status::BadRequest,
                    AppErrorKind::InvalidPayload(inner.to_string()).into(),
                ),
            }
        })
        .map(|json| json.into_inner())
    }
}

I can see the appropriate map_failure match arm is being entered correctly here, but my Responder::respond_to implementation isn't called. When I run the test the default 400 catcher kicks in.

Any ideas what I'm doing wrong?

@philbooth
Copy link
Author

Any ideas what I'm doing wrong?

Ignore me sorry, just realised I needed to wrap the argument in an AppResult in my route handler too. All good now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no bug The reported bug was confirmed nonexistent question A question (converts to discussion)
Projects
None yet
Development

No branches or pull requests

2 participants