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

add web Router service to rama-http #396

Open
GlenDC opened this issue Jan 15, 2025 · 12 comments
Open

add web Router service to rama-http #396

GlenDC opened this issue Jan 15, 2025 · 12 comments
Labels
low prio Low priority item. mentor available A mentor is available to help you through the issue.
Milestone

Comments

@GlenDC
Copy link
Member

GlenDC commented Jan 15, 2025

There's already a Matcher approach in https://docs.rs/rama/latest/rama/http/service/web/index.html, in two ways:

  • WebService: this is using boxed handlers and comes with the benefits of nice regular Rust
  • match_service! macro: this is same-same but using tuples and thus no need to Box. It is however a bit less easy to use for first time users

Both are very flexible in that they allow you to match on pretty much anything. However for a traditional web server it is a lot nicer to work with a trie-like structure to match purely on paths. You could still mix-and-match that with the two approaches above in case you want to specialise further (e.g. method) once you are in a path. This is possible as the two above still result in a Service (https://docs.rs/rama/latest/rama/trait.Service.html), which is also an EndpointService (https://docs.rs/rama/latest/rama/http/service/web/trait.IntoEndpointService.html)

The Router has to implement the Service trait (https://docs.rs/rama/latest/rama/trait.Service.html), which will be very similar to how https://docs.rs/rama/latest/rama/http/service/web/struct.WebService.html#impl-Service%3CState,+Request%3CBody%3E%3E-for-WebService%3CState%3E does it, except that you would do it using an approach focussed on paths. These paths should support extraction of segment data and wildcards out of the box and inject them similar to how a PatchMatcher (https://docs.rs/rama/latest/rama/http/matcher/struct.PathMatcher.html) does it.

API interface could look like:

use rama::http::service::web::Router

let app = Router::new()
    .get("/", root)
    .get("/users", list_users)
    .post("/users", create_user)
    .get("/users/{id}", show_user)
    .get("/assets/{*path}", serve_asset);

As part of implementing this Router you can add matchit as a dependency: https://docs.rs/matchit/latest/matchit/

You could also go fancier and create also some kind of RoutingService which could wrap the WebService and makes it a lot easier to chain based on common matchers such as methods etc, for same path. But that's more of an extra.

Which would allow you to make it look more like:

use rama::http::service::web::{Router, routing::get};

let app = Router::new()
    .route("/", get(root))
    .route("/users", get(list_users).post(create_user))
    .route("/users", get(create_user))
    .route("/users/{id}", get(show_user))
    .route("/assets/{*path}", get(serve_asset));

Might be nicer and easier to implement, something to think about.

@GlenDC GlenDC added this to the v0.3 milestone Jan 15, 2025
@GlenDC GlenDC added low prio Low priority item. mentor available A mentor is available to help you through the issue. labels Jan 15, 2025
@nagxsan
Copy link

nagxsan commented Feb 2, 2025

Hey! I came across this issue in This Week In Rust newsletter and I would love to have a crack at it if possible!

@GlenDC
Copy link
Member Author

GlenDC commented Feb 2, 2025

All yours @nagxsan . Let me know if something is not clear, you want to discuss something or need help / have questions.

Also feel free to open a PR if you want feedback or are stuck, have questions, etc.

Thank you for your interest and take care!

@GlenDC GlenDC modified the milestones: v0.3, v0.2 Feb 2, 2025
@nagxsan
Copy link

nagxsan commented Feb 5, 2025

Thanks for assigning this task to me!

I had a look at the two methods, using WebService boxed handlers and the match_service! macro. As of now, I will start building Router and get that done as a v1 task. Once I am satisfied with those results, I will also have a crack at the RoutingService way (that feels more intuitive so would be nice to have that solution as well).

@GlenDC
Copy link
Member Author

GlenDC commented Feb 5, 2025

Sounds good. Do let me know if you need feedback, have questions or want to bounce some ideas around. Do also feel free to open a draft PR if you are stuck or require (early) feedback, all good.

@nagxsan
Copy link

nagxsan commented Feb 10, 2025

Hey @GlenDC , so this implementation would basically be a wrapper around matchit right?
Also maybe a silly doubt, but does the current implementation handle the params feature in a route? Because if it does not, then we would have to write a separate matcher service as well right which would provide a function to extract params from the route? Can you please tell me if this is right or I am going wrong somewhere?

@GlenDC
Copy link
Member Author

GlenDC commented Feb 10, 2025

we would have to write a separate matcher service as well right which would provide a function to extract params from the route? Can you please tell me if this is right or I am going wrong somewhere?

Correct, you will have to write a separate service, which you can call Router as mentioned in the issue description.

the matchit's Matched params would then need to consumed as https://docs.rs/rama/latest/rama/http/matcher/struct.UriParams.html, using https://docs.rs/matchit/latest/matchit/struct.Params.html#method.iter, so that you can inject that UriParams into the context for other services to make use of. As such the usage of matchit is a detail only known to Router.

@GlenDC
Copy link
Member Author

GlenDC commented Feb 10, 2025

this implementation would basically be a wrapper around matchit right?

You can see it like that, yes.

The service would wrap https://docs.rs/matchit/latest/matchit/struct.Router.html and store services similar to WebService as Arc<BoxService<State, Request, Response, Infallible>> (created via https://docs.rs/rama/latest/rama/http/service/web/trait.IntoEndpointService.html)

@nagxsan
Copy link

nagxsan commented Feb 16, 2025

I am going with a trie based approach with the following state that each trie node will hold:

  • children: this stores the static child nodes
  • param_child: this will store the path parameter node (for example {id})
  • wildcard_child: this will store the wildcard node (for example {*path})
  • param_name: this will store the name of the parameter at this point in the path
  • handlers: this is a HashMap from Method (GET, POST, and so on) to the actual handler function that maps each method type to the respective handler function.

I know this is slow and I am very sorry but I am kind of new to Rust so it is taking some time getting used to it.

@GlenDC
Copy link
Member Author

GlenDC commented Feb 16, 2025

It's okay, also okay if you want to stop working on the issue as it might perhaps be a bit too much for where you are right now at your journey, no shame in that.

That said, https://github.com/plabayo/rama/pull/423/files is not what was requested for this issue.

The goal is really to make use of matchit internally of the service to be written for this service and be able to route it to endpoint services. No need to reinvent the wheel for this one. In case you have questions about this or anything that was previously discussed do let me know. And of course do let me know in case you feel this might be a bit too much for you. It happens, sometimes we chew more than we can handle. Again no shame if that would be the case.

@nagxsan
Copy link

nagxsan commented Feb 17, 2025

Hey, so sorry, I got a bit confused with the approaches. I have updated the code with the matchit implementation. I am facing this issue though, can you please explain the problem here?

async fn serve(
        &self,
        mut ctx: Context<State>,
        req: Request<>,
    ) -> Result<Self::Response, Self::Error> {
        let uri_string = req.uri().to_string();
        match &self.routes.at(uri_string.as_str()) {
            Ok(matched) => {
                let params: HashMap<String, String> = matched.params.clone().iter().map(|(k, v)| (k.to_string(), v.to_string())).collect();
                ctx.insert(params);
                if let Some(service) = matched.value.get(&req.method()) {
                    service.boxed().serve(ctx, req).await
                } else {
                    self.not_found.serve(ctx, req).await
                }
            },
            Err(_err) => {
                self.not_found.serve(ctx, req).await
            }
        }
    }

In this, at this line: &self.routes.at(uri_string.as_str()), it shows the error:

borrowed data escapes outside of method [E0521] `self` escapes the method body here

Why is this happening?

Also I will probably give it a try for a couple of more days and if I cannot make any further headway, it would probably be good if someone else picks it up. I am sorry for wasting your time as well. I felt that maybe a more hands-on task would help me practice my skills better.

@GlenDC
Copy link
Member Author

GlenDC commented Feb 17, 2025

It's okay, it isn't an easy issue by any means, that's why I didn't add the label easy. There's a lot of moving parts here, and some of it might also mean we might need to adapt code elsewhere to improve the system overall. You are right that hands-on task is required to get better, but this might have been a jump too high, where you might need some steps in between first before a task of this complexity is do-able.

Why is this happening?

Would need to see the entire error message to be sure, but you are doing some weird things there.

For sure service.boxed() should not be needed, if you would do it you would need to clone first as https://docs.rs/rama/latest/rama/trait.Service.html#method.boxed consumes self to produce a new representation. But again, there should be no need to box on the fly like that.

@GlenDC
Copy link
Member Author

GlenDC commented Feb 17, 2025

Issue available once again. @nagxsan thank you for the interest and enthusiasm though, greatly appreciated and respected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low prio Low priority item. mentor available A mentor is available to help you through the issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants