Fix CORSMiddleware configuration initializer parameter

[grosch]

Updates CORSMiddleware.Configuration's exposedHeaders parameter to accept [HTTPHeader.Name] instead of [String] like the other parameters do (#2314).

[0xTim]

This was discussed in #2295 and there are multiple cases where you may want a certain type of middleware before the error middleware. Having a special case just for CORS seems wrong to be and the way it's now documented in the docs where you create a new Middlewares type seems the best option to me

[grosch]

I disagree. CORS has to be before the error middleware, and it's a bad idea to force people to find something in the docs like this when we can easily resolve the issue for them. I think the most common case would be a person just adding CORS and nothing else.

[grundoon]

100% with @0xTim on this.

Broken scenario: CORSMiddleware -> DogCowMiddleware -> ErrorMiddleware
Ineffective scenario: CORSMiddleware -> MyErrorhandlerThatOnlyConformsToMiddleware

The docs clearly address the importance of ordering – if one is going to alter the default middlewares configuration, an expectation that they first read the essential docs is reasonable. While the intent is good, hard-coding is limiting and rarely the best solution.

[grundoon]

Oh, and just for sake of completeness, I don't have a strong opinion on the other proposed change in here – though do note that this would now require extending HTTPHeaders if there are any custom ones which need exposing.

[tanner0101]

though do note that this would now require extending HTTPHeaders if there are any custom ones which need exposing.

You can init HTTPHeaders.Name with an arbitrary string. Though adding an extension definitely makes for more readable code. I looked into making HTTPHeaders.Name expressible by string literal but it caused ambiguity. Not sure if that's still the case...it might be worth trying again.

[tanner0101]

I agree that there should be an easier way to add CORS middleware. Needing to first empty the middleware then re-add CORS and error middleware in the correct order is not intuitive.

That said, I don't think this is the right approach as it has just as much potential to cause problems as the current API.

What if we instead expose collection-like APIs on app.middleware? This would let you do:

app.middleware.insert(cors, at: 0)

We could copy collection directly here or add parameters to use:

app.middleware.use(cors, .first)

This prevents the need for resetting the middleware config / re-adding error middleware. It also makes CORS configuration more easily copy-pastable. Most importantly though, it still gives the developer full control over how middleware are added. If they want something to go before CORS middleware, they can easily do it.

[0xTim]

Another issue I didn't spot, changing the type of exposedHeaders is a breaking change which can't be done until Vapor 5

[grosch]

@0xTim Yup, it's a breaking change. I had mentioned that to Tanner in Discord. My vote would be that we go ahead and do it anyways based on:

• We are only like a week or so into the 4.0 version being released, and not all packages actually are yet.
• This is a super minor change that's a single line of code to fix, not having to "rework" anything
• It's likely not used by very many people at all, so the impact would be quite low.

That said, it's not that a big a deal if we decide not to :)

@Tanner I don't think that's a "clean" API for people. What about, instead, if we allowed something more like

app.middleware.use(cors, before: ErrorMiddleware.self)

[0xTim]

Going to have to say no to the breaking change - we're stable and we can't just break SemVer like that just because someone might not be using it. We've avoided smaller changes than this in the past and we should stick to it. It sets a dangerous precedent and destroys trust in Vapor.

As for the API, I think having a finer grained control over the order is clearer:

app.middleware.use(cors, before: ErrorMiddleware.self)

Is that the slot before ErrorMiddleware or any place before it (you could have several middlewares sitting before it by the time you add CORS). The other thing to consider with app.middleware.insert(cors, at: 0) is what happens when two places try to insert something at position 0? (Especially now that 'services' can add middlewares in willBoot

[tanner0101]

As for the API, I think having a finer grained control over the order is clearer:

I agree, I also considered an API like before: ErrorMiddleware.self but I think that is more likely to break since it assumes ErrorMiddleware is always the first one which is not true in all cases. insert(cors, at:0) doesn't make any assumptions about current middleware ordering and just always puts it first.

The other thing to consider with app.middleware.insert(cors, at: 0) is what happens when two places try to insert something at position 0?

It would work just like array, the item currently at 0 would move to 1.

[tanner0101]

Going to have to say no to the breaking change

Agreed. Semver must be followed as our docs promise:

At no point can existing API be removed or broken during an active version. Semver is strictly followed and tested.

In this case we can add a new init overload and deprecate the old one though. Win win.

[grosch]

Great points, Tim. I pushed a new version (I had to do a force push of this branch) which deprecates the old one. I also removed all the stuff related to the location of the middleware as it seems that still needs some architecture work done.

[tanner0101]

+1 to the overall changes.

[tanner0101]

These changes are now available in 4.3.0