actix / actix-web

Actix Web is a powerful, pragmatic, and extremely fast web framework for Rust.
https://actix.rs
Apache License 2.0
21.6k stars 1.67k forks source link

Overlapping scopes work differently than flat overlapping routes #2904

Open RReverser opened 2 years ago

RReverser commented 2 years ago

Expected Behavior

The docs seem to suggest that the web::scope(...) is a helper that groups different routes under the same prefix, but that otherwise the behaviour would be the same as with flattened routes. At least, I didn't see any mention of differences.

So, when I refactored my code from flat routes like in this minimal example:

use actix_web::{get, put, web, App, HttpServer, Responder};

#[get("/camera/dimensions")]
async fn camera_dimensions() -> impl Responder {
    "800x600"
}

#[get("/camera/owner")]
async fn camera_owner() -> impl Responder {
    "John Doe"
}

#[get("/focuser/distance")]
async fn focuser_focus() -> impl Responder {
    "42"
}

#[put("/focuser/distance")]
async fn focuser_focus_set(value: web::Path<f64>) -> impl Responder {
    format!("Set focus to {}", value)
}

#[get("/{device_type}/connected")]
async fn device_connected(device_type: web::Path<String>) -> impl Responder {
    format!("{} is connected", device_type)
}

#[put("/{device_type}/connect")]
async fn device_connect(device_type: web::Path<String>) -> impl Responder {
    format!("Connecting {}", device_type)
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| {
        App::new()
            .service(camera_dimensions)
            .service(camera_owner)
            .service(focuser_focus)
            .service(focuser_focus_set)
            .service(device_connected)
            .service(device_connect)
    })
    .bind(("127.0.0.1", 8080))?
    .run()
    .await
}

to scoped groups like in this one:

use actix_web::{get, put, web, App, HttpServer, Responder};

#[get("/dimensions")]
async fn camera_dimensions() -> impl Responder {
    "800x600"
}

#[get("/owner")]
async fn camera_owner() -> impl Responder {
    "John Doe"
}

#[get("/distance")]
async fn focuser_focus() -> impl Responder {
    "42"
}

#[put("/distance")]
async fn focuser_focus_set(value: web::Path<f64>) -> impl Responder {
    format!("Set focus to {}", value)
}

#[get("/connected")]
async fn device_connected(device_type: web::Path<String>) -> impl Responder {
    format!("{} is connected", device_type)
}

#[put("/connect")]
async fn device_connect(device_type: web::Path<String>) -> impl Responder {
    format!("Connecting {}", device_type)
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| {
        App::new()
            .service(
                web::scope("/camera")
                    .service(camera_dimensions)
                    .service(camera_owner),
            )
            .service(
                web::scope("/focuser")
                    .service(focuser_focus)
                    .service(focuser_focus_set),
            )
            .service(
                web::scope("/{device_type}")
                    .service(device_connected)
                    .service(device_connect),
            )
    })
    .bind(("127.0.0.1", 8080))?
    .run()
    .await
}

I expected all routes to continue working like before.

Current Behavior

Unfortunately, it seems that scopes are only matched against each other, and, once a scope is matched, no other scopes are tried even if it doesn't have the required method.

So, routes like /camera/connected and /focuser/connected worked before, but stop working after refactoring.

I thought reordering might help, putting the generic /{device_type} scope first, but that only seems to change which scope catches everything - now, routes like /*/connected work but specific device routes like /camera/dimensions don't.

Finally, I thought maybe I should use /{device_type} as app.default_service(), but that doesn't work either as default_service() seems to only accept a single handler and not a scope as its argument.

Possible Solution

Flatten all routes at the app level when scopes are registered, so that there is no observable change in behaviour after grouping.

Steps to Reproduce (for bugs)

Run the minimal examples provided above with suggested requests like http://localhost:8080/camera/connected and observe the working response "camera is connected" in the first implementation and 404 error in the 2nd one.

Context

I'm trying to refactor my complex list of routes into grouped scopes in separate functions to make code a bit more maintainable.

Your Environment

robjtede commented 2 years ago

As you've noticed, Scopes match paths in a fundamentally different way to a flat route structure. In general, once a service has been matched the router will never consider services registered after it. In your case the router would never match /camera/connected to the first two routes unless using Scope("/camera") because Scope captures everything with that prefix.

once a service has been matched the router will never consider services registered after it

I do believe this is worth spelling out in the documentation.

robjtede commented 2 years ago

Here's a reasonably clean solution that allows you to keep the scopes and also re-use the generic device routes: https://www.rustexplorer.com/b/6kr5zk


Using Scope::configure can help segment code in to modules the first, flat case, too, while keeping its behavior.

// camera.rs

#[get("/camera/dimensions")]
async fn camera_dimensions() -> impl Responder {
    "800x600"
}

#[get("/camera/owner")]
async fn camera_owner() -> impl Responder {
    "John Doe"
}

fn config_camera_routes(cfg: &mut web::ServiceConfig) -> {
  cfg
    .service(camera_dimensions)
    .service(camera_owner)
}

// focuser.rs
fn config_focuser_routes(cfg: &mut web::ServiceConfig) -> { ... }

// generic.rs
fn config_generic_routes(cfg: &mut web::ServiceConfig) -> { ... }

// app.rs
App::new()
  .configure(config_camera_routes)
  .configure(config_focuser_routes)
  .configure(config_generic_routes)
RReverser commented 2 years ago

can help segment code in to modules the first

Yeah but doesn't help eliminate the prefix unfortunately (in my case there's dozens of routes under each group).

But yeah, at least clearly documenting this, or, better yet, detecting unreachable routes at init time, like some routers do, would be already good to save time for others who run into this.

robjtede commented 2 years ago

I'll consider this particular issue closed when docs have satisfactory detail added though you may with to follow #414 and #2264 too.

Chu-4hun commented 1 year ago

I have a similar issue I want to add to ADMIN routes jwt auth middleware, but there is web::scope conflict

HttpServer::new(move || {
        let cors = Cors::permissive();
        App::new().wrap(cors).service(
            web::scope("api/v1")
                //ADMIN
                .service(
                    web::scope("/excursions")
                        .service(add_excursion)
                        .service(delete_excursion_by_id)
                        .service(update_excursion_by_id)
                        .service(
                            web::scope("/costs")
                                .service(add_customer_cost)
                                .service(delete_customer_cost_by_id)
                                .service(update_customer_cost_by_id)
                                .service(
                                    web::scope("/types")
                                        .service(add_customer_type)
                                        .service(delete_customer_type_by_id)
                                        .service(update_customer_type_by_id),
                                ),
                        ),
                )
                //CLIENT
                .service(
                    web::scope("/excursions")
                        .service(get_all_excursions)
                        .service(get_excursion_by_id)
                        .service(
                            web::scope("/costs")
                                .service(get_customer_cost_by_excursion_id)
                                .service(
                                    web::scope("/types")
                                        .service(get_all_customer_type)
                                        .service(get_customer_type_by_id),
                                ),
                        ),
                ), 
        )
    })
    .bind(("0.0.0.0", 8090))?
    .run()
    .await

all Client routes are not present (404)

tekeri commented 2 weeks ago

Another case I came across...

use actix_web::{
    http::StatusCode,
    test::{call_service, init_service, TestRequest},
    web::{resource, scope, ServiceConfig},
    App, HttpResponse,
};
#[actix_web::test]
async fn test_router() {
    fn routes(root: &mut ServiceConfig) {
        root.service(scope("").service(resource("/ok").get(|| HttpResponse::Ok())))
            .service(scope("").service(resource("/fail").get(|| HttpResponse::Ok())));
    }
    let app = init_service(App::new().configure(routes)).await;
    let res_ok = call_service(&app, TestRequest::get().uri("/ok").to_request()).await;
    let res_fail = call_service(&app, TestRequest::get().uri("/fail").to_request()).await;
    assert_eq!(res_ok.status(), StatusCode::OK);
    assert_eq!(res_fail.status(), StatusCode::NOT_FOUND);
}