Skip to content

Commit

Permalink
Ensure application's middleware stack to mount ordered, unique Rack m…
Browse files Browse the repository at this point in the history
…iddleware
  • Loading branch information
jodosha committed Oct 22, 2016
1 parent 4de8fd5 commit 115b330
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 2 deletions.
2 changes: 1 addition & 1 deletion Gemfile
Expand Up @@ -13,7 +13,7 @@ gem 'hanami-router', '~> 0.7', require: false, github: 'hanami/router',
gem 'hanami-controller', '~> 0.7', require: false, github: 'hanami/controller', branch: '0.7.x'
gem 'hanami-view', '~> 0.7', require: false, github: 'hanami/view', branch: '0.7.x'
gem 'hanami-model', '~> 0.7', require: false, github: 'hanami/model', branch: '0.7.x'
gem 'hanami-helpers', '~> 0.5', require: false, github: 'hanami/helpers', branch: 'new-routes-factory'
gem 'hanami-helpers', '~> 0.5', require: false, github: 'hanami/helpers', branch: 'master'
gem 'hanami-mailer', '~> 0.3', require: false, github: 'hanami/mailer', branch: '0.3.x'
gem 'hanami-assets', '~> 0.4', require: false, github: 'hanami/assets', branch: 'configuration'

Expand Down
2 changes: 2 additions & 0 deletions lib/hanami/middleware.rb
Expand Up @@ -67,6 +67,7 @@ def call(env)
# @see Hanami::Middleware#prepend
def use(middleware, *args, &blk)
stack.push [middleware, args, blk]
stack.uniq!
end

# Prepend a middleware to the stack.
Expand All @@ -82,6 +83,7 @@ def use(middleware, *args, &blk)
# @see Hanami::Middleware#use
def prepend(middleware, *args, &blk)
stack.unshift [middleware, args, blk]
stack.uniq!
end

private
Expand Down
56 changes: 56 additions & 0 deletions spec/isolation/middleware/ensure_unique_stack_spec.rb
@@ -0,0 +1,56 @@
RSpec.describe Hanami::Middleware, type: :cli do
describe "#load!" do
it "loads the middleware stack without duplicates" do
with_project do
generate "action web home#index"
replace "apps/web/application.rb", "Application < Hanami::Application", <<-END
class RackApp
def initialize(app, options = {}, &blk)
@app = app
@options = options
@blk = blk
end
def call(env)
@app.call(env)
end
end
class Application < Hanami::Application
END

replace "apps/web/application.rb", "configure do", <<-END
configure do
block1 = ->() {}
block2 = ->() {}
block3 = ->() {}
middleware.use Web::RackApp, foo: :bar
middleware.use Web::RackApp, foo: :bar # this is a duplicate and it shouldn't be included
middleware.use Web::RackApp, baz: :bat
middleware.use Web::RackApp, &block1
middleware.use Web::RackApp, &block1 # this is a duplicate and it shouldn't be included
middleware.use Web::RackApp, &block2
middleware.prepend Web::RackApp, foo: :bar # this is a duplicate and it shouldn't be included
middleware.prepend Web::RackApp, cap: :tain
middleware.prepend Web::RackApp, &block1 # this is a duplicate and it shouldn't be included
middleware.prepend Web::RackApp, &block2 # this is a duplicate and it shouldn't be included
middleware.prepend Web::RackApp, &block3
END

require Pathname.new(Dir.pwd).join("config", "environment")
Hanami::Components.resolve('all')

middleware = Web::Application.new.__send__(:middleware)

# We tried to mount Web::RackApp 11 times.
#
# We have 5 duplicates marked inline.
#
# That leads us to 6 (11 - 5) mounted middleware.
expect(middleware.__send__(:stack).count).to be(6)
end
end
end
end
2 changes: 1 addition & 1 deletion spec/support/fixtures.rb
@@ -1,6 +1,6 @@
# Please use this app only for:
#
# * spec/application_spec.rb
# * spec/unit/application_spec.rb
# * spec/isolation/application_configure_spec.rb
module UnitTesting
class Application < Hanami::Application
Expand Down

6 comments on commit 115b330

@beauby
Copy link

@beauby beauby commented on 115b330 Oct 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a bandaid more than a fix? It does not address the reason why a given middleware would be pushed more than once on the stack.

@jodosha
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beauby Hi! If you look at the parent commit, that ensures configurations are evaluated only once.


This commit introduces a secondary mechanism of defense and unit tests to prevent regressions.

@beauby
Copy link

@beauby beauby commented on 115b330 Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry – I had assumed that the base issue hadn't been fixed. However, I am not convinced that silently suppressing duplicate middlewares makes sense then. Regression tests on the other hand are super helpful 👍.

@jodosha
Copy link
Member Author

@jodosha jodosha commented on 115b330 Oct 25, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beauby
Copy link

@beauby beauby commented on 115b330 Oct 25, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beauby
Copy link

@beauby beauby commented on 115b330 Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize my wording was confusing. What I meant was:

  • I understand the bug causing middlewares to be loaded more than once was fixed in the parent commit.
  • Since it is fixed, calling uniq! on the middlewares stack is now useless. It is intended as a "second mechanism of defence", but all it will do is make the issue unnoticeable should a new bug that loads components multiple times arise.

Please sign in to comment.