Allow using `Option<Middleware>` to enable/disable a middlware

[ctron]

PR Type

Feature

PR Checklist

• [x] Tests for the changes have been added / updated.
• [x] Documentation comments have been added / updated.
• [x] A changelog entry has been made for the appropriate packages.
• [x] Format code with the latest stable rustfmt.
• [x] (Team) Label with affected crates and semver status.

Overview

Currently, there is Condition, which accepts a boolean (to enable/disable) and an instance to the actual middleware. The downside of that is, that such a middlware needs to be constructed in any case. Even if the middleware is used or not.

However, the middleware is not used when it is disabled. Only the type seems required. So this Optional middleware allows passing in an Option instead of boolean and instance. If the option "is some" it is enabled. Otherwise not.

Improves #934

[aliemjay]

I like this and I think it is cleaner for Condition::new to already accept Option. But since this would arguably be an unneccessary breaking change, I think it is better to be implemented as Condition::from_option or impl From<Option<T>> for Condition<T>.

And thanks again for your contribution :)

[ctron]

But since this would arguably be an unneccessary breaking change, …

Why would that be a breaking change? It only adds a new struct.

[aliemjay]

Why would that be a breaking change?

I meant changing Condition::new to accept Option would be breaking. Sorry for the confusion.

I prefer having the implementation on Condition because it is less api suface area and thus less maintenance burden than adding a new struct. You can change Condition internals to store an option and that would make using it either way possible Condition::{new, from_option}

[ctron]

Thanks @aliemjay … that makes sense.

I rebased the PR and made the suggested changes.

[fakeshadow]

No Condition not always produce middleware service. See: https://github.com/actix/actix-web/blob/5ca42df89ae4c4eda46fcb408ba5b13549f09ad7/actix-web/src/middleware/condition.rs#L55

Condition only call new_transform and produce middleware service when you pass true to it. You always provide the factory type for the middleware but it's not used in anyway when disabled by condition.

I don't see the point of this PR. It's like a misunderstanding of the service initialization to me.

[aliemjay]

I don't see the point of this PR. It's like a misunderstanding of the service initialization to me.

It is pretty useful for middlewares that doesn't provide an easy default. Consider something like Condition::from(std::env::var("ARG").map(|arg| MyMiddleware::config(arg)))

[fakeshadow]

MyMiddleware is just a constructor for the real middleware. Why you make it not have an easy constructor based on condition? If not how were you using Condition?

[ctron]

If not how were you using Condition?

Not at all. That is the point of this PR to enable this.

[ppamorim]

@ctron HttpAuthentication is broken, I cannot use it. It's giving this error:

   --> src/bin/main.rs:116:13
    |
116 |       .wrap(Condition::new(env.as_str() == "production", HttpAuthentication::bearer(validator)))
    |        ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `BoxBody`, found enum `EitherBody`
    |        |
    |        required by a bound introduced by this call
    |

[ctron]

I moved this into an "extras" crate, so that it can be used in the meantime: https://crates.io/crates/actix-web-extras

[ctron]

I rebased the PR and added an entry in the changelog. The documentation was already there.