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

Correctly handle HEAD requests #129

Merged
merged 2 commits into from
Aug 15, 2021
Merged

Correctly handle HEAD requests #129

merged 2 commits into from
Aug 15, 2021

Conversation

davidpdrsn
Copy link
Member

HEAD requests will be routed to GET handlers, unless an explicit HEAD handler exists:

// this will receive `GET /` and `HEAD /` and send both to `handler`
route("/", get(handler));

// `GET /` goes to `handler`, `HEAD /` goes to `other_handler`
route("/", get(handler).head(other_handler));

Reponses to HEAD requests will always have the body stripped.

Fixes #84

@davidpdrsn davidpdrsn added C-enhancement Category: A PR with an enhancement breaking change A PR that makes a breaking change. labels Aug 5, 2021
@davidpdrsn davidpdrsn added this to the 0.2 milestone Aug 5, 2021
@davidpdrsn
Copy link
Member Author

@jplatte @songww I looked a bit more into #84 and think I found a fix. Are you able to test this and see if its working correctly for you?

It unfortunately required some breaking changes so will be released in 0.2.

davidpdrsn added a commit that referenced this pull request Aug 5, 2021
#129 was a breaking change, in part
because I had to remove the `Copy` impl from `OnMethod`.

For the sake of future proofing I think we should remove other `Copy`
impls from services as well. We can always bring them back once things
have matured more.

These types no longer implement `Copy`:

- `EmptyRouter`
- `ExtractorMiddleware`
- `ExtractorMiddlewareLayer`
@davidpdrsn davidpdrsn mentioned this pull request Aug 5, 2021
src/service/future.rs Outdated Show resolved Hide resolved
davidpdrsn added a commit that referenced this pull request Aug 7, 2021
#129 was a breaking change, in part
because I had to remove the `Copy` impl from `OnMethod`.

For the sake of future proofing I think we should remove other `Copy`
impls from services as well. We can always bring them back once things
have matured more.

These types no longer implement `Copy`:

- `EmptyRouter`
- `ExtractorMiddleware`
- `ExtractorMiddlewareLayer`
@davidpdrsn
Copy link
Member Author

@jplatte would you be able to give this a try?

@jplatte
Copy link
Member

jplatte commented Aug 9, 2021

Nope, sorry. I'm not actually using axum in any projects yet 😄

@davidpdrsn
Copy link
Member Author

Ah okay. Fair enough 😊

@SabrinaJewson
Copy link
Contributor

I don't like how magic this solution feels. For simplicity's sake, I think we would be better off making get accept both GET and HEAD, and head(handler).get(other_handler) not being supported. If users really wanted to list their routes in that order, we maybe could support head(handler).or(get(other_handler)) or something.

@davidpdrsn
Copy link
Member Author

What about this solution is too magical?

I think we would be better off making get accept both GET and HEAD

That's basically what this does except it also removes response bodies from HEAD requests, as per the spec.

@SabrinaJewson
Copy link
Contributor

What about this solution is too magical?

It automatically disables the head-matching part of get when it detects a head filter present. This is kind of opaque to the user, and somewhat counterintuitive if they expected each OnMethod to be fully modular.

That's basically what this does except it also removes response bodies from HEAD requests, as per the spec.

I definitely agree that response bodies should be removed from HEAD requests. My suggestion is to keep get fully consistent in its behaviour, so that it always matches both GET and HEAD even if there are other HEAD handlers. Users can handle HEADs explicitly by checking for HEAD before the get filter: get(handler).head(other_handler).

@davidpdrsn
Copy link
Member Author

It automatically disables the head-matching part of get when it detects a head filter present. This is kind of opaque to the user, and somewhat counterintuitive if they expected each OnMethod to be fully modular.

I agree with that actually 🤔 I'm gonna change it to work like that when getting up to date with main I think.

@davidpdrsn davidpdrsn merged commit 995ffc1 into main Aug 15, 2021
@davidpdrsn davidpdrsn deleted the get-head branch August 15, 2021 18:27
@davidpdrsn davidpdrsn mentioned this pull request Aug 23, 2021
davidpdrsn added a commit that referenced this pull request Aug 23, 2021
- Overall:
  - **fixed:** Overall compile time improvements. If you're having issues with compile time
    please file an issue! ([#184](#184)) ([#198](#198)) ([#220](#220))
  - **changed:** Remove `prelude`. Explicit imports are now required ([#195](#195))
- Routing:
  - **added:** Add dedicated `Router` to replace the `RoutingDsl` trait ([#214](#214))
  - **added:** Add `Router::or` for combining routes ([#108](#108))
  - **fixed:** Support matching different HTTP methods for the same route that aren't defined
    together. So `Router::new().route("/", get(...)).route("/", post(...))` now
    accepts both `GET` and `POST`. Previously only `POST` would be accepted ([#224](#224))
  - **fixed:** `get` routes will now also be called for `HEAD` requests but will always have
    the response body removed ([#129](#129))
  - **changed:** Replace `axum::route(...)` with `axum::Router::new().route(...)`. This means
    there is now only one way to create a new router. Same goes for
    `axum::routing::nest`. ([#215](#215))
  - **changed:** Implement `routing::MethodFilter` via [`bitflags`](https://crates.io/crates/bitflags) ([#158](#158))
  - **changed:** Move `handle_error` from `ServiceExt` to `service::OnMethod` ([#160](#160))

  With these changes this app using 0.1:

  ```rust
  use axum::{extract::Extension, prelude::*, routing::BoxRoute, AddExtensionLayer};

  let app = route("/", get(|| async { "hi" }))
      .nest("/api", api_routes())
      .layer(AddExtensionLayer::new(state));

  fn api_routes() -> BoxRoute<Body> {
      route(
          "/users",
          post(|Extension(state): Extension<State>| async { "hi from nested" }),
      )
      .boxed()
  }
  ```

  Becomes this in 0.2:

  ```rust
  use axum::{
      extract::Extension,
      handler::{get, post},
      routing::BoxRoute,
      Router,
  };

  let app = Router::new()
      .route("/", get(|| async { "hi" }))
      .nest("/api", api_routes());

  fn api_routes() -> Router<BoxRoute> {
      Router::new()
          .route(
              "/users",
              post(|Extension(state): Extension<State>| async { "hi from nested" }),
          )
          .boxed()
  }
  ```
- Extractors:
  - **added:** Make `FromRequest` default to being generic over `body::Body` ([#146](#146))
  - **added:** Implement `std::error::Error` for all rejections ([#153](#153))
  - **added:** Add `OriginalUri` for extracting original request URI in nested services ([#197](#197))
  - **added:** Implement `FromRequest` for `http::Extensions` ([#169](#169))
  - **added:** Make `RequestParts::{new, try_into_request}` public so extractors can be used outside axum ([#194](#194))
  - **added:** Implement `FromRequest` for `axum::body::Body` ([#241](#241))
  - **changed:** Removed `extract::UrlParams` and `extract::UrlParamsMap`. Use `extract::Path` instead ([#154](#154))
  - **changed:** `extractor_middleware` now requires `RequestBody: Default` ([#167](#167))
  - **changed:** Convert `RequestAlreadyExtracted` to an enum with each possible error variant ([#167](#167))
  - **changed:** `extract::BodyStream` is no longer generic over the request body ([#234](#234))
  - **changed:** `extract::Body` has been renamed to `extract::RawBody` to avoid conflicting with `body::Body` ([#233](#233))
  - **changed:** `RequestParts` changes ([#153](#153))
      - `method` new returns an `&http::Method`
      - `method_mut` new returns an `&mut http::Method`
      - `take_method` has been removed
      - `uri` new returns an `&http::Uri`
      - `uri_mut` new returns an `&mut http::Uri`
      - `take_uri` has been removed
  - **changed:** Remove several rejection types that were no longer used ([#153](#153)) ([#154](#154))
- Responses:
  - **added:** Add `Headers` for easily customizing headers on a response ([#193](#193))
  - **added:** Add `Redirect` response ([#192](#192))
  - **added:** Add `body::StreamBody` for easily responding with a stream of byte chunks ([#237](#237))
  - **changed:** Add associated `Body` and `BodyError` types to `IntoResponse`. This is
    required for returning responses with bodies other than `hyper::Body` from
    handlers. See the docs for advice on how to implement `IntoResponse` ([#86](#86))
  - **changed:** `tower::util::Either` no longer implements `IntoResponse` ([#229](#229))

  This `IntoResponse` from 0.1:
  ```rust
  use axum::{http::Response, prelude::*, response::IntoResponse};

  struct MyResponse;

  impl IntoResponse for MyResponse {
      fn into_response(self) -> Response<Body> {
          Response::new(Body::empty())
      }
  }
  ```

  Becomes this in 0.2:
  ```rust
  use axum::{body::Body, http::Response, response::IntoResponse};

  struct MyResponse;

  impl IntoResponse for MyResponse {
      type Body = Body;
      type BodyError = <Self::Body as axum::body::HttpBody>::Error;

      fn into_response(self) -> Response<Self::Body> {
          Response::new(Body::empty())
      }
  }
  ```
- SSE:
  - **added:** Add `response::sse::Sse`. This implements SSE using a response rather than a service ([#98](#98))
  - **changed:** Remove `axum::sse`. Its been replaced by `axum::response::sse` ([#98](#98))

  Handler using SSE in 0.1:
  ```rust
  use axum::{
      prelude::*,
      sse::{sse, Event},
  };
  use std::convert::Infallible;

  let app = route(
      "/",
      sse(|| async {
          let stream = futures::stream::iter(vec![Ok::<_, Infallible>(
              Event::default().data("hi there!"),
          )]);
          Ok::<_, Infallible>(stream)
      }),
  );
  ```

  Becomes this in 0.2:

  ```rust
  use axum::{
      handler::get,
      response::sse::{Event, Sse},
      Router,
  };
  use std::convert::Infallible;

  let app = Router::new().route(
      "/",
      get(|| async {
          let stream = futures::stream::iter(vec![Ok::<_, Infallible>(
              Event::default().data("hi there!"),
          )]);
          Sse::new(stream)
      }),
  );
  ```
- WebSockets:
  - **changed:** Change WebSocket API to use an extractor plus a response ([#121](#121))
  - **changed:** Make WebSocket `Message` an enum ([#116](#116))
  - **changed:** `WebSocket` now uses `Error` as its error type ([#150](#150))

  Handler using WebSockets in 0.1:

  ```rust
  use axum::{
      prelude::*,
      ws::{ws, WebSocket},
  };

  let app = route(
      "/",
      ws(|socket: WebSocket| async move {
          // do stuff with socket
      }),
  );
  ```

  Becomes this in 0.2:

  ```rust
  use axum::{
      extract::ws::{WebSocket, WebSocketUpgrade},
      handler::get,
      Router,
  };

  let app = Router::new().route(
      "/",
      get(|ws: WebSocketUpgrade| async move {
          ws.on_upgrade(|socket: WebSocket| async move {
              // do stuff with socket
          })
      }),
  );
  ```
- Misc
  - **added:** Add default feature `tower-log` which exposes `tower`'s `log` feature. ([#218](#218))
  - **changed:** Replace `body::BoxStdError` with `axum::Error`, which supports downcasting ([#150](#150))
  - **changed:** `EmptyRouter` now requires the response body to implement `Send + Sync + 'static'` ([#108](#108))
  - **changed:** `Router::check_infallible` now returns a `CheckInfallible` service. This
    is to improve compile times ([#198](#198))
  - **changed:** `Router::into_make_service` now returns `routing::IntoMakeService` rather than
    `tower::make::Shared` ([#229](#229))
  - **changed:** All usage of `tower::BoxError` has been replaced with `axum::BoxError` ([#229](#229))
  - **changed:** Several response future types have been moved into dedicated
    `future` modules ([#133](#133))
  - **changed:** `EmptyRouter`, `ExtractorMiddleware`, `ExtractorMiddlewareLayer`,
    and `QueryStringMissing` no longer implement `Copy` ([#132](#132))
  - **changed:** `service::OnMethod`, `handler::OnMethod`, and `routing::Nested` have new response future types ([#157](#157))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change. C-enhancement Category: A PR with an enhancement
Projects
None yet
4 participants