Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ActionDispatch::MiddlewareStack does not allow swapping middleware of same class #4873

Closed
jonahb opened this Issue Feb 3, 2012 · 6 comments

Comments

Projects
None yet
2 participants
Contributor

jonahb commented Feb 3, 2012

Ruby 1.9.2p290
Rails 3.2.1

config.middleware.swap ActionDispatch::ShowExceptions,
  ActionDispatch::ShowExceptions,
  lambda { |env| [ 500, {}, ['error!'] ] }

Expected: Middleware stack contains an instance of ActionDispatch::ShowExceptions with the new exceptions app.

Observed: ActionDispatch::ShowExceptions has been deleted from the middleware stack.

The problem is ActionDispatch::MiddlewareStack#swap:

def swap(target, *args, &block)
  insert_before(target, *args, &block)
  delete(target)
end

The delete call deletes all middlewares of the given class, including the one that was just inserted.

The workaround is to use #delete and #insert, but still, #swap should work as expected.

Contributor

kennyj commented Feb 4, 2012

Hi @jonahb

I can reproduce this issue, and I think that I fixed it.
Please review my commit !

Contributor

kennyj commented Feb 4, 2012

I modified the testcase a little.

kennyj added a commit to kennyj/rails that referenced this issue Feb 4, 2012

Contributor

kennyj commented Feb 4, 2012

I rebased my commit.

Contributor

kennyj commented Feb 4, 2012

Anyway, I send pull request :-)

josevalim added a commit that referenced this issue Feb 4, 2012

Merge pull request #4879 from kennyj/fix_4873
Fix GH #4873. Allow swapping same class middleware.

josevalim added a commit that referenced this issue Feb 4, 2012

Merge pull request #4879 from kennyj/fix_4873
Fix GH #4873. Allow swapping same class middleware.
Contributor

kennyj commented Feb 4, 2012

@jonahb
The above PR is merge into master and 3-2-stable.
I'm closing this issue. Thanks !

@kennyj kennyj closed this Feb 4, 2012

Contributor

jonahb commented Feb 4, 2012

Thanks, @kennyj. The fix looks good.

One question: It seems odd that middlewares are tested for equality by class. IMO, two ShowException middlewares with different exception apps are not "equal." Might a better fix be to change the definition of equality or to override == in ShowException? This would fix the original and very natural implementation of #insert. It would also allow the stack to contain two middlewares of the same class configured differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment