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

actix-web returns 404 instead of 405 for unsupported verbs on resources #1256

Closed
vmalloc opened this issue Jan 5, 2020 · 8 comments
Closed

Comments

@vmalloc
Copy link
Contributor

vmalloc commented Jan 5, 2020

The following example illustrates the issue:

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

async fn index() -> impl Responder {
    ...
}

#[actix_rt::main]
async fn main() -> std::io::Result<()> {
    HttpServer::new(|| {
        App::new()
            .route("/index", web::get().to(index))
    })
    .bind("127.0.0.1:8000")?
    .run()
    .await
}

Attempting to send a POST /index will fail with 404, where 405 is actually the appropriate error code for such a case.

This is a regression compared to 1.x, as I previously reported the same issue...

@fafhrd91
Copy link
Member

fafhrd91 commented Jan 5, 2020

Could you submit pr with unit test?

@vmalloc
Copy link
Contributor Author

vmalloc commented Jan 5, 2020

Sure. You mean submit the fix as well? I'm not sure I know where the relevant code is

@fafhrd91
Copy link
Member

fafhrd91 commented Jan 5, 2020

Fix would good as well

@vmalloc
Copy link
Contributor Author

vmalloc commented Jan 5, 2020

Ok, I'll try...

@vmalloc vmalloc self-assigned this Jan 5, 2020
@vmalloc
Copy link
Contributor Author

vmalloc commented Jan 5, 2020

@fafhrd91 I have the unit test written down, but I don't understand how to fix https://github.com/actix/actix-web/blob/9eedd010516dbc56a218e9d8c34f3a2e13c6faec/src/app_service.rs#L73 so that if any paths matched with incorrect methods it should selectively return 405... Just changing it to return 405 breaks other behaviors, and I couldn't figure out how to apply the fix you made in ffb3324 to this scenario....

@vmalloc vmalloc removed their assignment Jan 6, 2020
@yim7
Copy link

yim7 commented Jan 6, 2020

That is because App::route() adds all guards to the Resource, so resouce check guards failed and the app's default device is actually called. If we can combine resource with same path into one, there is no need to add guards in routes to resource, just like registering routes with resouce directly.

    pub fn route(self, path: &str, mut route: Route) -> Self {
        self.service(
            Resource::new(path)
                .add_guards(route.take_guards())
                .route(route),
        )
    }

@robjtede
Copy link
Member

I've run across this and feel like its intended behavior. There should be a way to "hide" verbs on routes. If you want 405s use a web::resource instead. Is this reasonable?

@vmalloc
Copy link
Contributor Author

vmalloc commented Jan 21, 2020

@robjtede that seems like a reasonable compromise to me. Thanks!

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

No branches or pull requests

4 participants