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

before filter for >= 0.10.0 #898

Closed
kushkella opened this issue Jan 16, 2015 · 7 comments
Closed

before filter for >= 0.10.0 #898

kushkella opened this issue Jan 16, 2015 · 7 comments
Labels

Comments

@kushkella
Copy link

As of 0.9.0 version of gem before filter was only called for the version block within which it was defined. But from 0.10.0 it is being called for all version blocks as long as we don't group them as a resource/group/namespace. e.g.

class Test < Grape::API
  resource :foo do
    version 'v1', :using => :path do
      before do
        puts 'foo'
      end
      get '/' do
        # something
      end
    end

    version 'v1', :using => :path do
      before do
        puts 'bar'
      end
      get '/:id' do
        # something
      end
    end
  end
end

when using 0.9.0 when I request GET /foo/1, the resource will only log "bar". But with 0.10.0 it will log both "foo" and "bar" as long as both version blocks are not individually wrapped inside a blank namespace/resource/group etc.

It would be great if you could explain if this is the desired behavior. Thanks!

@dblock
Copy link
Member

dblock commented Jan 16, 2015

Very interesting. If anything this is a regression and we should document it in UPGRADING.md - would appreciate a PR.

I am honestly not sure what we want here. The version block is not something that even has to be a block, it has scope for whatever follows it as well. I am inclined to say that this is what we want by design and the workaround is to use namespace. That said, I could easily argue the opposite.

Would love to hear what others have to say.

@dblock dblock added the bug? label Jan 16, 2015
@kushkella
Copy link
Author

Sure, I will create a PR.

I don't really like using version without a block. It is easy to read version of a specific resource-method if it is wrapped inside a version block. I started off with using version the way you described, but since we have so many methods/paths listed under a resource, it gets cumbersome to track versions without wrapping them in a version block.

And we use path for versioning, so we can easily switch to namespace/group etc. But I think we should treat version as something similar to namespace. The only difference being that version can be specified in path, header etc.

@dblock
Copy link
Member

dblock commented Jan 16, 2015

I think I agree. Lets treat it as a regression for now that we should be fixing, want to try to do that?

@kushkella
Copy link
Author

Sure, I'll give it a shot and keep you posted. Meanwhile I try that, I'll document this behavior and create a PR. Thanks!

@kushkella
Copy link
Author

If possible, please release a new version of Grape soon. I'll really appreciate that. Thanks!

@dblock
Copy link
Member

dblock commented Jan 18, 2015

Will do.

@dblock
Copy link
Member

dblock commented Jul 29, 2016

I believe this was fixed by #901, closing. Please reopen @kushkella if I misunderstood something.

@dblock dblock closed this as completed Jul 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants