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

[iangreenleaf]

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]

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]

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]

@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]

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

[jrforrest]

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]

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

[towanda]

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]

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]

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]

I think what you propose is a fine solution IMHO.

[jrforrest]

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.

[towanda]

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

[dblock]

Thanks @towanda, closing.