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

declared(params) doesn't coerce types in before blocks #1074

Closed
iangreenleaf opened this issue Jul 20, 2015 · 13 comments
Closed

declared(params) doesn't coerce types in before blocks #1074

iangreenleaf opened this issue Jul 20, 2015 · 13 comments

Comments

@iangreenleaf
Copy link

In before blocks, declared(params) doesn't seem to perform the usual type coercion. For example:

      params do
        requires :id, type: Integer
      end
      group "foo/:id" do
        before do
          puts declared(params)[:id].inspect # this is a string
       end
     end

Strangely, it does still seem to validate in the blocks, as undeclared params are still filtered out. It just doesn't convert the types.

@rnubel
Copy link
Contributor

rnubel commented Jul 21, 2015

I believe this is the expected behavior, actually. Type coercion is really a type of validation, so if you want a given block to run after coercion has happened, you'd want it to be in the after_validations hook.

Granted, that may violate the Principle of Least Surprise a bit, but changing this behavior would be a breaking change.

@iangreenleaf
Copy link
Author

Thanks for the tip, I'll change my callbacks. It's reasonable to consider this as part of validation, but there's a couple things about this I'd rate as especially surprising:

  1. That declared still runs in this context, instead of erroring out because I'm trying something that doesn't make sense.
  2. That the validations are incompletely applied at this point - undeclared params are already filtered out, but types are not yet coerced.

@dblock
Copy link
Member

dblock commented Jul 21, 2015

@iangreenleaf I think you are onto something here, maybe this should be fixed? I mean we should prevent declared from running or something like that.

@iangreenleaf
Copy link
Author

Agreed! Either make the method unavailable in this scope, or have it raise an error if run.

@jrforrest
Copy link
Contributor

I ran into this issue a bit ago and put together this somewhat sloppy solution for my own fork since I hit this pitfall enough for it to annoy me (I am not a smart guy.)

diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb
index 2b8ee61..4f8fc9b 100644
--- a/lib/grape/endpoint.rb
+++ b/lib/grape/endpoint.rb
@@ -73,6 +73,7 @@ module Grape

       @options[:method] = Array(options[:method])
       @options[:route_options] ||= {}
+      @run_filters = Hash.new

       return unless block_given?

@@ -319,6 +320,23 @@ module Grape
           instance_eval(&filter)
         end
       end
+      filters_dun_been_run(type)
+    end
+
+    def filters_dun_been_run(type)
+      @run_filters[type] = true
+    end
+
+    def filters_dun_been_run?(type)
+      @run_filters[type] || false
+    end
+
+    def declared(*args)
+      unless filters_dun_been_run?(:before)
+        raise ScriptError, "Oh man you goofed bad"
+      else
+        super(*args)
+      end
     end

     def befores

I don't know the Grape codebase at all, but if anyone more savvy than I feels that this solution is in the general direction of correct, I'll go ahead and fix variable/method names, catch all problem cases (not just the general before, but whatever other callbacks the declared method should fail on), raise the proper exception type, fix the exception message, put a spec around this and open a PR, following whatever other steps the CONTRIBUTING guide requires.

@dblock
Copy link
Member

dblock commented Aug 27, 2015

I think a much simpler solution would be to declare declared in the before scope and have it raise an exception.

@towanda
Copy link
Contributor

towanda commented Aug 30, 2015

I have been taking a look at this, and I can't think of any solution better than the one @jrforrest proposes.

@dblock which before scope are you mentioning? I first tried to re-define declare in the before method in callbacks.rb, https://github.com/ruby-grape/grape/blob/master/lib/grape/dsl/callbacks.rb#L26, but they are class methods, and declared is an instance method, and also is loaded after.

@dblock
Copy link
Member

dblock commented Aug 30, 2015

There has to be a way to introduce scope into that execution that overrides a method. Probably dynamically. I might get to look into this :)

@jrforrest
Copy link
Contributor

Well, I suppose we could further break InsideRoute into a couple of different modules that should be extended by the Endpoint after certain filters are called.

Endpoint#run_filters could be modified to do something like extend Kernel.const_get("Grape::DSL::InsideRoute::Post#{type.camelize}") so those methods aren't available to the filters being instance_eval'd until they're meant to be.

This would, of course, raise a NoMethodError, but I suppose you could do something like

def declared
  raise "This method isn't available until..."
end

as a default in InsideRoute.

I'm personally really not a fan of this approach, but it might be acceptable here since there's already a lot of metaprogramming going on in this code. I don't like extending modules on the fly, but I do agree after reading through Endpoint a little more that defining declared there is pretty dirty.

I would like to point out that I think this pain is somewhat due to Endpoint both being responsible for the instance_eval context for filters and the route, as well as managing the configuration, middleware assembly, and other concerns. It may simplify things if the context in which these blocks are evaluated could be isolated from the other concerns into its own class at some point.

Anyway just let me know what I should do in the short term for this. If the approach above sounds acceptable I don't mind coding it and submitting a PR.

@dblock
Copy link
Member

dblock commented Sep 1, 2015

I think what you propose is a fine solution IMHO.

jrforrest added a commit to jrforrest/grape that referenced this issue Sep 1, 2015
In response to issue ruby-grape#1074

This adds a mechanism by which Grape::DSL::InsideRoute methods may be
overriden after certain filters are run.

The first problem case of a filter being utilized before it should that
necessitated this enhancement is the `#declared` method, which was
returning un-coerced params in `before` filters.

There may be other problem cases, which may be rectified using this same
mechanism.
jrforrest added a commit to jrforrest/grape that referenced this issue Sep 1, 2015
In response to issue ruby-grape#1074

This adds a mechanism by which Grape::DSL::InsideRoute methods may be
overriden after certain filters are run.

The first problem case of a filter being utilized before it should that
necessitated this enhancement is the `#declared` method, which was
returning un-coerced params in `before` filters.

There may be other problem cases, which may be rectified using this same
mechanism.
jrforrest added a commit to jrforrest/grape that referenced this issue Sep 1, 2015
In response to issue ruby-grape#1074

This adds a mechanism by which Grape::DSL::InsideRoute methods may be
overriden after certain filters are run.

The first problem case of a filter being utilized before it should that
necessitated this enhancement is the `#declared` method, which was
returning un-coerced params in `before` filters.

There may be other problem cases, which may be rectified using this same
mechanism.
@jrforrest
Copy link
Contributor

Sorry 'bout the reference spam. Didn't realize issue references would get picked up on fork and I was force-pushing rebases like mad over there.

That PR aught to do it. I would like to know if there's related issues for other filters though so I can address those while I'm at it.

jrforrest added a commit to jrforrest/grape that referenced this issue Sep 2, 2015
In response to issue ruby-grape#1074

This adds a mechanism by which Grape::DSL::InsideRoute methods may be
overriden after certain filters are run.

The first problem case of a filter being utilized before it should that
necessitated this enhancement is the `#declared` method, which was
returning un-coerced params in `before` filters.

There may be other problem cases, which may be rectified using this same
mechanism.
jrforrest added a commit to jrforrest/grape that referenced this issue Sep 2, 2015
In response to issue ruby-grape#1074

This adds a mechanism by which Grape::DSL::InsideRoute methods may be
overriden after certain filters are run.

The first problem case of a filter being utilized before it should that
necessitated this enhancement is the `#declared` method, which was
returning un-coerced params in `before` filters.

There may be other problem cases, which may be rectified using this same
mechanism.
jrforrest added a commit to jrforrest/grape that referenced this issue Sep 2, 2015
In response to issue ruby-grape#1074

This adds a mechanism by which Grape::DSL::InsideRoute methods may be
overriden after certain filters are run.

The first problem case of a filter being utilized before it should that
necessitated this enhancement is the `#declared` method, which was
returning un-coerced params in `before` filters.

There may be other problem cases, which may be rectified using this same
mechanism.
@towanda
Copy link
Contributor

towanda commented Sep 17, 2015

@dblock this can be closed, since It's resolved here: #1142

@dblock
Copy link
Member

dblock commented Sep 17, 2015

Thanks @towanda, closing.

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

No branches or pull requests

5 participants