Refactor middleware stack to track delete middlewares

[rafaelfranca]

Right now it is hard to re-insert delete middleware in the stack. The reason is that we only apply the delete operations after the insert operations and this will remove the middleware in the end.

To fix this instead of recording the delete operations separately in the MiddlewareStackProxy we keep track of deleted middleware so it is possible to add after and before them.

Fixes #26303 without reverting #16433.

cc @naw @sirupsen @tgxworld.

[naw]

@rafaelfranca thank you for working on this. I appreciate the time you've put into it.

Overall, it seems this approach will work; however, I do have a couple of concerns:

First, suppose you have the following middleware stack:

Alpha
Bravo

Now suppose I delete Bravo and add Delta after Alpha, resulting in this:

Alpha
Delta

Finally, if I add Echo after Bravo (which was deleted), your implementation will add Echo immediately after Alpha (which is what Bravo in the deleted middleware tracker is pointing at), resulting in this:

Alpha
Echo
Delta

However, I would have expected Echo to be added after Delta (not inbetween Alpha and Delta), since the original Bravo would have been after Delta had it not been deleted.

One way to solve this is as follows: Rather than storing a next and previous for each deleted middleware, just insert a Placeholder class into the real middleware stack in the exact place of the deleted middleware. Then, when you check for deleted middlewares, just check for any Placeholder's that reference the intended (deleted) class and return the index of the placeholder (i.e. no need to reference a next or previous). Finally, at the end, you can remove all Placeholders from the stack.

[naw]

My second concern is that if you delete a middleware and add it back into the stack in a different location, and then later you reference that middleware class when adding some other middleware, it's not obvious whether you intend to add relative to the deleted location or relative to the new location. I can see valid use cases for both. I think using the new location is slightly more intuitive, but either way seems that it would surprise one group of people or another.

Edit To clarify, current Rails behavior uses whichever location is first in the stack (either the old deleted location, or the new location). This PR changes that behavior to always use the old deleted location.

I understand you want a way to address #16433, but overall, I think the idea of handling middleware deletes as a special case leads to unintended surprises.

Conceptually, the whole idea of adding middleware relative to other middleware (which may or may not be present in the stack) is intrinsically fragile, especially when 3rd parties are involved. Rather than Rails trying to "guess" what the user is trying to do (i.e. by handling deletes as a special case), perhaps it would be better to enhance the middleware API to allow 3rd parties to handle the fragility themselves. For example, insert_before could take an array of middlewares, so that if the given middleware isn't present, it can try to insert before something else instead. A fallback, if you will.

If Rack::Runtime is truly optional, other pieces of Rails shouldn't be inserted relative to its location. If Rack::Runtime isn't truly optional, then the solution to #16433 is to tell people not to delete it.

[naw]

This doesn't work well if you delete the very first middleware. In such a case, the previous pointer will point to the last middleware in the stack, so subsequent calls to insert relative to that pointer will put things near the end rather than near the beginning, for example.

[rafaelfranca]

Thank you for the feedback. You concerns are all valid. I'm not sure there is a way to fix both issues.

Any middleware is optional so we need to have a way to handle changing the stack relative to other middleware.

But at same time, changing the stack relative to other middleware is not intuitive as you pointed in both examples.

I believe my solution is closer to the behavior we have since Rails 3.0 so even that is not intuitive to some people, it is already being here for more than 5 years and people is already familiar with it.

[naw]

@rafaelfranca thank you for your response.

For my second example, I agree there really isn't a way to fix it, and there isn't a clear "best" way to pick which position to use.

However, for my first example, I'm not sure I understand why you prefer the behavior in this PR over the existing behavior.

In other words, why do you prefer Echo to end up before Delta?

Alpha
Echo
Delta

instead of

Alpha
Delta
Echo

The latter matches the current Rails behavior (as well as intuition), but this PR changes it.

Perhaps I misunderstood your reasoning as it applies to my first example.

Thanks!

[kaspth]

assert_not_includes?

[rafaelfranca]

The latter matches the current Rails behavior (as well as intuition), but this PR changes it.

Yeah, I was only talking about the second problem. The first one I need to fix.

[naw]

I would be happy to work on a fix for the the first one, if you would like.

[rafaelfranca]

Sure. Feel free to open a pr against this branch

…

On Wed, Feb 15, 2017 at 9:31 PM Nathan ***@***.***> wrote: I would be happy to work on a fix for the the first one, if you would like. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27936 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC66GBiqaMO__TRhB_Va0Y3v-EDadP9ks5rc4rBgaJpZM4L54W6> .

[matthewd]

👍🏻

I like this because it retains the current simple, intuitive-most-of-the-time API.

We could replace the middleware management API entirely, and switch to full explicit dependencies -- essentially promoting 6b126ff to runtime. But I think I still prefer the explicitness of a static list.

[naw]

FYI: I wrote code for this a few months ago in a PR into this branch here: rafaelfranca#2

Please let me know if there is anything else I can do to move this forward.

The basic approach of my PR is this: Deleting a middleware doesn't actually remove the middleware; instead, it replaces it with an instance of a wrapper class that knows about the original. When enumerating over the middleware list, these wrappers are ignored (effectively treating them as if they aren't there), but they maintain the place of the deleted middleware so that future inserts can reference the deleted middleware and be placed in exactly the right place relative to where the deleted middleware would have been if it hadn't been deleted.

[benlangfeld]

@rafaelfranca Is this likely to be included in the next release? Currently the published work-arounds do not work and there is effectively no way to move an entry in the set.

[camertron]

It may not cover all use cases, but I recently released a gem called rails-middleware-extensions that could help with this issue. Specifically it adds middleware.insert_unless_exists and middleware.move.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[benlangfeld]

@rafaelfranca Did this get blocked on something?

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[jeremy]

Still lgtm. Worth reviving @rafaelfranca?

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.