Provide attribute macro for multiple HTTP methods

[mattgathu]

PR Type

Feature

PR Checklist

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt

Overview

Provide attribute macro for multiple HTTP methods

Define a new route attribute macro that supports defining multiple
HTTP methods to be routed to (handled by) a single handler.

The attribute macro syntax looks like this

use actix_web::route;

#[route("/multi", method = "GET", method = "POST")]
async fn multi_methods() -> &'static str {
    "Hello world!\r\n"
}

This implementation extends the GuardType enum in actix-web-codegen to have a
new GuardType::Multi variant that denotes when multiple method guards
are used.

A new method attribute in the route attribute macro provides an HTTP method to provide a guard for.
This attribute can be used multiple times for multiple HTTP methods.

The code matches the methods to the respective
GuardType and uses the AnyGuard struct to combine them together.

The generated code looks like this:

pub struct multi_methods;
impl actix_web::dev::HttpServiceFactory for multi_methods {
    fn register(self, __config: &mut actix_web::dev::AppService) {
        async fn multi_methods() -> &'static str {
            "Hello world!\r\n"
        }
        let __resource = actix_web::Resource::new("/multi")
            .name("multi_methods")
            .guard(actix_web::guard::Any(actix_web::guard::Get()).or(actix_web::guard::Post()))
            .to(multi_methods);
        actix_web::dev::HttpServiceFactory::register(__resource, __config)
    }
}

This Closes #1360

[codecov-commenter]

Codecov Report

Merging #1674 into master will decrease coverage by 0.19%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
- Coverage   53.51%   53.31%   -0.20%     
==========================================
  Files         123      123              
  Lines       11876    11919      +43     
==========================================
  Hits         6355     6355              
- Misses       5521     5564      +43     

Impacted Files	Coverage Δ
actix-web-codegen/src/route.rs	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d707704...64a0339. Read the comment docs.

[Silentdoer]

I think this api should be:

#[route("/multi", method = "GET", method = "POST")]
async fn multi_methods() -> &'static str {
    "Hello world!\r\n"
}

otherwise it's not consistent with:

#[get("/index2", wrap = "middleware::SayHi", wrap = "middleware::SayHi2")]

[Silentdoer]

or may be:

#[route("/multi", methods = ["GET","POST"])]
async fn multi_methods() -> &'static str {
    "Hello world!\r\n"
}

[mattgathu]

Thanks for the feedback @Silentdoer

My initial approach was #[route("/multi", methods = ["GET","POST"])] but unfortunately only literal arguments are allowed in this position, and that ruled out the [...] syntax.

I can reimplement the macro to have the #[route("/multi", method = "GET", method = "POST")] syntax. If the core contributor(s) can affirm that this is the preferred syntax. maybe @robjtede or @JohnTitor

[mattgathu]

I've updated the syntax to be #[route("/multi", method = "GET", method = "POST")]

[robjtede]

Hey @mattgathu, it's great to see you tackling this!

I'd like to land #1677 first so we can test some failure cases in CI and make sure the compile error outputs make sense.

[popzxc]

Looks really nice. Thanks!

[robjtede]

@mattgathu #1677 is merged so it'd be great to get some trybuild tests added as part of this. Try to get in as many weird edge case failures as you can think of.

[mattgathu]

@robjtede I got an issue with tests passing on rustc 1.42.0 but failing on the current stable.
Here is the diff:

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: The #[route(..)] macro requires at least one `method` attribute
##[error] --> $DIR/route-missing-method-fail.rs:3:1
  |
3 | #[route("/")]
  | ^^^^^^^^^^^^^

error[E0425]: cannot find value `index` in this scope
##[error]  --> $DIR/route-missing-method-fail.rs:10:49
   |
10 |     let srv = test::start(|| App::new().service(index));
   |                                                 ^^^^^ not found in this scope
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: The #[route(..)] macro requires at least one `method` attribute
##[error] --> $DIR/route-missing-method-fail.rs:3:1
  |
3 | #[route("/")]
  | ^^^^^^^^^^^^^
  |
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0425]: cannot find value `index` in this scope
##[error]  --> $DIR/route-missing-method-fail.rs:10:49
   |
10 |     let srv = test::start(|| App::new().service(index));
   |                                                 ^^^^^ not found in this scope
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
note: If the actual output is the correct output you can bless it by rerunning
      your test with the environment variable TRYBUILD=overwrite

note the = note: this error originates in an attribute macro... line.

Do you have any suggestions on how to handle this?

[robjtede]

Do you have any suggestions on how to handle this?

Hmm... I'll have a look.

In the mean time, I feel that the macro should be called "resource" as it is more consistent with the manual routing methods. A resource can define multiple methods on a path where a route is a single method.

[mattgathu]

In the mean time, I feel that the macro should be called "resource" as it is more consistent with the manual routing methods. A resource can define multiple methods on a path where a route is a single method.

I would argue that route is more fitting as it expresses the intention: to route one or more http methods to the same handler. It is also, I would argue, more consistent with the HTTP verbs get, put etc as it is also a verb.

resource is a noun and doesn't capture/express the user's intent.

I'm viewing route as a verb and not its noun meaning

[robjtede]

Alright, fair argument. Would like to get more opinons from @actix/contributors then.

When you read it as a verb it makes sense even though you'd have to sort of read it as #[route_to("/abc")]. I think there's value in keeping the nomenclature consitent with existing routing methods. But, again, more input here will be good.

[robjtede]

I'm coming around to #[route(...)] for brevity sake.

In other news, I think I've found a solution to the compile error difference while letting us test on stable too. Great find with that rustversion macro; using that you can do this:

#[test]
fn compile_macros() {
    let t = trybuild::TestCases::new();

    t.pass("tests/trybuild/simple.rs");
    t.compile_fail("tests/trybuild/simple-fail.rs");

    t.pass("tests/trybuild/route-ok.rs");
    t.compile_fail("tests/trybuild/route-duplicate-method-fail.rs");
    t.compile_fail("tests/trybuild/route-unexpected-method-fail.rs");
    missing_method_fail_hack(&t);
}

#[rustversion::stable(1.42)]
fn missing_method_fail_hack(t: &trybuild::TestCases) {
    t.compile_fail("tests/trybuild/route-missing-method-fail-msrv.rs");
}

#[rustversion::not(stable(1.42))]
fn missing_method_fail_hack(t: &trybuild::TestCases) {
    t.compile_fail("tests/trybuild/route-missing-method-fail.rs");
}

Then have two versions of the test file...:

• route-missing-method-fail.rs
• route-missing-method-fail-msrv.rs

(Even cleaner, you can symlink the -msrv version to the original.)

...and two versions of the compiler output:

• route-missing-method-fail.stderr
• route-missing-method-fail-msrv.stderr

[fakeshadow]

I'm not against this poll but would like to share my two cents.

I feel attributes should follow the rust std style. For example:
#[route(method = "")]
#[route(any(method = "", method = "" ))]
#[route(not(method = ""))]

[mattgathu]

@robjtede regarding the trybuild tests, the Actix-web's CI matrix has 1.42, current stable and nightly tests with the compilers never guaranteed to produce similar outputs. There will be a probability that each time nightly or the current stable is updated the tests will break. Pinning the tests to 1.42 avoids this likely breakage.

Having multiple tests for different compilers is great but might not be maintainable IMHO.

[robjtede]

I think the maintenance cost is small enough using this approach, testing for MSRV and latest stable is enough; we should not run any trybuild tests on nightly.

Worst case we will need to update the stderr files every 6 weeks. But I think theres value in knowing when compiler output changes for our macro errors, anyway.

[mattgathu]

@robjtede I haven't found a way to differentiate between the 1.42 stable and other stable versions using the rustversion macros :-(

You have #[rustversion::not(stable(1.42))] in your example, but it doesn't eliminate nightly runs.

Edit: I ended up building a dummy shim and stacking the macros 🤞

[robjtede]

Great! I'm happy to go ahead with that test set up.

Check out this commit (02e16c6) for how to do the multi guard without requiring changes to actix-web. The generated code looks like this for the example in that commit:

let __resource = actix_web::Resource::new("/")
    .name("index")
    .guard(
        actix_web::guard::Any(actix_web::guard::Get())
            .or(actix_web::guard::Trace())
            .or(actix_web::guard::Post())
            .or(actix_web::guard::Head()),
    )
    .to(index);

[robjtede]

Thanks so much for working on this 👍 great job!

[robjtede]

@fakeshadow RE your comment on attribute style: I don't think there's a use case for not(...) here and I think it's safe to assume the syntax is any(...) without having to explicitly write that out. This is also inkeeping with current wrap="..." style.

Speak now if you still feel strongly on it.

[fakeshadow]

I'm fine with the current attributes. You guys did a good job.

[robjtede]

And awaaayyyyy we go...