Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documented but not implemented feature so a user can insert a middleware at a specific point in the stack #1787

Merged
merged 3 commits into from
Sep 4, 2018

Conversation

michaellennox
Copy link
Contributor

@michaellennox michaellennox commented Sep 4, 2018

What does this PR do?

Adds the ability for a user to insert a middleware at a specific point in the middleware stack using the insert method.

Where should the reviewer start?

First commit demonstrates failing integration test case. Second fixes it. Recommended to step through.

Any background context to be provided?

In the README of this project it is documented that a user can insert a middleware at a specific point in the stack using the .insert method.

Notable passage:

You can add your custom middleware with use, that push the middleware onto the stack, and you can also control where the middleware is inserted using insert, insert_before and insert_after.

Notable example:

class CustomOverwriter < Grape::Middleware::Base
  def after
    [200, { 'Content-Type' => 'text/plain' }, [@options[:message]]]
  end
end

class API < Grape::API
  use Overwriter
  insert_before Overwriter, CustomOverwriter, message: 'Overwritten again.'
  insert 0, CustomOverwriter, message: 'Overwrites all other middleware.'

  get '/' do
  end
end

This was added in c2e41f7 but it appears that the insert method was missed from the public API and only exposed on the internal middleware stack class.

Any specific implementation details?

I added the CHANGELOG entry under Fixes as it is 'fixing' a discrepancy between the documentation and the code but unsure whether it belongs best under this or under Features.

It is documented in the README for this project that there is a DSL method (`.insert`) that can be utilised but this is not the case.

The notable passage is:

```
You can add your custom middleware with use, that push the middleware onto the stack, and you can also control where the middleware is inserted using insert, insert_before and insert_after.

```

The notable example of this is:

```
class CustomOverwriter < Grape::Middleware::Base
  def after
    [200, { 'Content-Type' => 'text/plain' }, [@options[:message]]]
  end
end

class API < Grape::API
  use Overwriter
  insert_before Overwriter, CustomOverwriter, message: 'Overwritten again.'
  insert 0, CustomOverwriter, message: 'Overwrites all other middleware.'

  get '/' do
  end
end
```

This method is not actually available though and when invoked will raise a NoMethodError. This appears to have just been an error of omission in the original commit adding the middleware stack and extending the DSL to include the ability to interact with it in a more granular fashion (ruby-grape@c2e41f7).

This commit adds a failing test case which attempts to utilise this documented feature.
This commit adds a documented but not implemented feature to allow a user to insert a middleware at a specific point in the stack. It does this by extending the Grape::DSL::Middleware class with a new method `.insert` which delegates down to the already existing methods in existance on the Grape::Middleware::Stack class.
@dblock dblock merged commit 5ae64f0 into ruby-grape:master Sep 4, 2018
@dblock
Copy link
Member

dblock commented Sep 4, 2018

Perfect, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants