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

Type safe routing #756

Merged
merged 28 commits into from
Feb 18, 2022
Merged

Type safe routing #756

merged 28 commits into from
Feb 18, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Feb 13, 2022

Some things thats been bugging me about axum's routing for the longest time:

  • There is no connection between the route you use in Router::router and the path you try and extract with Path. Requires you to make changes in multiple places.
  • Path might try and extract captures that don't exist on the route
  • By looking at just the handler function you don't know what the route will be. You have to find the places where its added to a router.

Attributes like #[get("/users/:id")] can fix that but they require some pretty gnarly parsing of the function arguments to make sure things are aligned. RA also isn't great with attributes so I'd like to avoid them.

I've been experimenting with another approach that I think is interesting:

use axum::{response::IntoResponse, Router};
use axum_extra::routing::{typed_path, RouterExt};
use axum_macros::TypedPath;
use serde::Deserialize;

// new `TypedPath` trait in axum_extra:
//
// pub trait TypedPath {
//     // the path with captures such as `/users/:id`
//     const PATH: &'static str;
// }

#[tokio::main]
async fn main() {
    let app = Router::new()
        // `typed_path::get` requires a handler who's first argument implements `TypedPath`
        // so it can acces it's `PATH` const
        .typed_get(users_index)
        .typed_post(users_create)
        .typed_get(users_show);

    axum::Server::bind(&"0.0.0.0:3000".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}

// `#[derive(TypedPath)]` derives `TypedPath` and `FromRequest`
#[derive(TypedPath)]
#[typed_path("/users")]
struct UsersCollection;

// for paths with fields `TypedPath` implements `FromRequest` via `Path`
// which requires `Deserialize`
#[derive(Deserialize, TypedPath)]
#[typed_path("/users/:id")]
struct UsersMember {
    id: u32,
}

// regular handlers that take a `TypedPath` as their first argument
async fn users_index(_: UsersCollection) -> impl IntoResponse {
    "users#index"
}

async fn users_create(_: UsersCollection, _payload: String) -> impl IntoResponse {
    "users#create"
}

async fn users_show(UsersMember { id }: UsersMember) -> impl IntoResponse {
    format!("users#show: {}", id)
}

Advantages:

  • The only macro is a #[derive(_)] of a known trait which RA tends to work well with.
  • No macros on the handlers so RA still works great there.
  • You get typed connection between the route and the handler.

@jplatte @programatik29 what do you think about this?

axum-extra/src/routing/typed_path.rs Outdated Show resolved Hide resolved
axum-extra/src/routing/typed_path.rs Outdated Show resolved Hide resolved
axum-macros/src/lib.rs Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Feb 13, 2022

Overall, I love the way you solved this! When I saw the PR title I first thought it would be a solution to the context / Extension problem, but that might be better solved by a language feature given it's not really a problem specific to axum.

@programatik29
Copy link
Contributor

I think this is a nice addition. Empowers users to inverse the logic so that instead of routes having handlers, now handlers have routes.

@davidpdrsn
Copy link
Member Author

davidpdrsn commented Feb 13, 2022

Realized that by adding typed_* methods to RouterExt we can avoid having the TypedPathRouter entirely:

let app = Router::new()
    .typed_get(users_index)
    .typed_post(users_create)
    .typed_get(users_show)
    .typed_get(users_edit);

I think thats way nicer because you only have to use and learn Router and can add middleware/fallbacks in the same way.

Edit: One could even do

let app = Router::new()
    .get(users_index)
    .post(users_create)
    .get(users_show)
    .get(users_edit);

But maybe thats one step too far 😅

@davidpdrsn davidpdrsn changed the title WIP: Type safe routing Type safe routing Feb 14, 2022
@davidpdrsn davidpdrsn marked this pull request as ready for review February 14, 2022 08:58
@davidpdrsn
Copy link
Member Author

This is ready for review now :)

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting issue I see with this is nested routers. Nesting a router that uses the new typed routes under a route that has its own captures wouldn't work.

axum-extra/Cargo.toml Show resolved Hide resolved
axum-extra/src/routing/mod.rs Outdated Show resolved Hide resolved
axum-extra/src/routing/typed.rs Show resolved Hide resolved
axum-extra/src/routing/typed.rs Outdated Show resolved Hide resolved
axum-extra/src/routing/typed.rs Outdated Show resolved Hide resolved
axum/src/routing/mod.rs Show resolved Hide resolved
davidpdrsn and others added 2 commits February 14, 2022 15:27
Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>
@davidpdrsn
Copy link
Member Author

Tell me if this is too much but I had a thought this morning. Why only make the paths types, why not the http methods as well? Basically:

use axum::{response::IntoResponse, Router};
use axum_extra::routing::{Any, Get, OneOf, Post, RouterExt, TypedPath};
use serde::Deserialize;

#[tokio::main]
async fn main() {
    let app = Router::new()
        .typed_route(users_index)
        .typed_route(users_create)
        .typed_route(users_show)
        .typed_route(users_edit);

    axum::Server::bind(&"0.0.0.0:3000".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}

#[derive(TypedPath)]
#[typed_path("/users")]
struct UsersCollection;

#[derive(Deserialize, TypedPath)]
#[typed_path("/users/:id")]
struct UsersMember {
    id: u32,
}

#[derive(Deserialize, TypedPath)]
#[typed_path("/users/:id/edit")]
struct UsersEdit(u32);

async fn users_index(_: Get, _: UsersCollection) -> impl IntoResponse {
    "users#index"
}

async fn users_create(_: Post, _: UsersCollection, _payload: String) -> impl IntoResponse {
    "users#create"
}

async fn users_show(_: Any, UsersMember { id }: UsersMember) -> impl IntoResponse {
    format!("users#show: {}", id)
}

async fn users_edit(_: OneOf<(Get, Post)>, UsersEdit(id): UsersEdit) -> impl IntoResponse {
    format!("users#edit: {}", id)
}

I think this is pretty interesting but its probably also gonna be a little foreign to some people. Too much advanced type trickery?

A hunch I have is that it might be convenient for openapi spec generation since more of the data is in the handler itself, but thats just a hunch.

@SpyrosRoum
Copy link
Contributor

I think on a user's perspective it the signatures might start getting a bit too noise with both the http method and the typed path.
Without knowing much on the internals would it be possible to combine the two on something like a Metadata struct that would contain both the http method, the path, and any arguments from it?

@niklasf
Copy link

niklasf commented Feb 17, 2022

Regarding the general idea: It looks like this approach could also enable typesafe reverse routing. Safely generating URLs that are guaranteed to be accepted by the router is a useful feature. Changed the path in some way? All links created using reverse routing are automatically kept in sync.

@davidpdrsn
Copy link
Member Author

I think on a user's perspective it the signatures might start getting a bit too noise with both the http method and the typed path.

I think that's a valid concern. Question is whether you think it's a worthy trade off.

Without knowing much on the internals would it be possible to combine the two on something like a Metadata struct that would contain both the http method, the path, and any arguments from it?

Do you have an example of what that might look like?

All links created using reverse routing are automatically kept in sync.

Yep that's part of the idea. Typed paths will implement Display and return a path that you can use with a tags and such. It doesn't guarantee that you've actually added the route to the router. Don't think that's possible without going full on Haskell.

@jplatte
Copy link
Member

jplatte commented Feb 18, 2022

After thinking about it for the last days, I don't think the typed method stuff makes that much sense. In theory it could be nice for generating redirects but only with a decently more complex API (I really don't like the _: Get params).

I think typed paths are worthwhile because they not only solve the problem of parameter mismatches (by the way: what about *rest params?), but also the problem of forgetting to percent-encode parameters in redirects¹. The same doesn't seem to apply for methods.

¹ or even requests? there's some server-server APIs like Matrix federation where you could use the same path type for receiving and sending requests

@davidpdrsn
Copy link
Member Author

Thats fair. I've reverted the typed method stuff.

but also the problem of forgetting to percent-encode parameters in redirects

Oh I had forgotten about that. Currently the Display impl it generates doesn't URL the segments. Should it? 🤔

re /*wildcards I've made that an error in the derive macro. Not sure how to handle them but can figure that out later.

@jplatte
Copy link
Member

jplatte commented Feb 18, 2022

Oh I had forgotten about that. Currently the Display impl it generates doesn't URL the segments. Should it? 🤔

I think it should, yes. Why not?

@davidpdrsn
Copy link
Member Author

@jplatte done! Its not the most efficient implementation but probably good enough.

davidpdrsn and others added 2 commits February 18, 2022 13:48
Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>
@davidpdrsn davidpdrsn merged commit 7a228a5 into main Feb 18, 2022
@davidpdrsn davidpdrsn deleted the route-macro branch February 18, 2022 13:13
davidpdrsn added a commit that referenced this pull request Feb 22, 2022
* wip

* wip

* make macro implement trait

* checkpoint

* checkpoint

* Simplify things quite a bit

* re-export `axum_macros::TypedPath` from `axum_extra`

* docs

* add missing feature

* fix docs link

* fix features

* fix missing imports

* make serde an optional dep again

* ui tests

* Break things up a bit

* Update span for `FromRequest` impls to point to callsite

* make docs feature labels show up automatically

* Apply suggestions from code review

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* add note about Display/Serialize being compatible

* Update axum-extra/src/routing/typed.rs

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* fix missing docs link

* what about typed methods?

* Revert "what about typed methods?"

This reverts commit cc1f989.

* don't allow wildcards for now

* percent encode params

* Update axum-extra/src/routing/typed.rs

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* rephrase args

* changelog

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>
davidpdrsn added a commit that referenced this pull request Feb 22, 2022
* wip

* wip

* make macro implement trait

* checkpoint

* checkpoint

* Simplify things quite a bit

* re-export `axum_macros::TypedPath` from `axum_extra`

* docs

* add missing feature

* fix docs link

* fix features

* fix missing imports

* make serde an optional dep again

* ui tests

* Break things up a bit

* Update span for `FromRequest` impls to point to callsite

* make docs feature labels show up automatically

* Apply suggestions from code review

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* add note about Display/Serialize being compatible

* Update axum-extra/src/routing/typed.rs

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* fix missing docs link

* what about typed methods?

* Revert "what about typed methods?"

This reverts commit cc1f989.

* don't allow wildcards for now

* percent encode params

* Update axum-extra/src/routing/typed.rs

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* rephrase args

* changelog

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>
davidpdrsn added a commit that referenced this pull request Feb 22, 2022
* wip

* wip

* make macro implement trait

* checkpoint

* checkpoint

* Simplify things quite a bit

* re-export `axum_macros::TypedPath` from `axum_extra`

* docs

* add missing feature

* fix docs link

* fix features

* fix missing imports

* make serde an optional dep again

* ui tests

* Break things up a bit

* Update span for `FromRequest` impls to point to callsite

* make docs feature labels show up automatically

* Apply suggestions from code review

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* add note about Display/Serialize being compatible

* Update axum-extra/src/routing/typed.rs

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* fix missing docs link

* what about typed methods?

* Revert "what about typed methods?"

This reverts commit cc1f989.

* don't allow wildcards for now

* percent encode params

* Update axum-extra/src/routing/typed.rs

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>

* rephrase args

* changelog

Co-authored-by: Jonas Platte <jplatte@users.noreply.github.com>
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.

None yet

5 participants