Pass request to validators along with params.

[glennpratt]

Addresses #986

[dblock]

I think we want to build request into the base class of Validator and not change the method signature. Something like in formatter. Give it a try?

[glennpratt]

@dblock the validator is per Endpoint, not per request, so making it a method of the Validator class seems odd to me.

[dblock]

I see your point, but I politely disagree :) The API is all singletons, and there're methods all over the place such as request, so my suggestion just keeps things consistent and easier to use. Furthermore, the fact that we need to check for arity and what not is definitely telling us something.

Give it a try, you'll find the code much easier to grok.

[glennpratt]

Either we change the method args, the constructor args or add a setter. All the same to me.

Is there no need for thread safety here? I wrote a test to check thread safety with @request in the Validator class and it immediately failed. It's quite possible I don't understand how this is supposed to work.

[dblock]

I think you shouldn't memoize anything or it should be thread-safe. I vote for the former to start.

[glennpratt]

Huh, I'm not sure how to do this without attaching something to the instance - memoizing or whatever.

This is one way I got it to work that passed my thread safe test - duplicating the validator objects:

        route_setting(:saved_validations).map(&:dup).each do |validator|
          req = @request
          validator.define_singleton_method(:request) { req }
          begin
            validator.validate!(params)
          rescue Grape::Exceptions::Validation => e
            validation_errors << e
          end
        end

define_singleton_method could just as well be a setter - this was just quicker.

It's not clear to me how @request and other instance vars in Endpoint instances are thread safe - perhaps some magic with the binding (generate_api_method?). It passes my threading tests, so it seems they are. Whatever magic that is doesn't happen when calling the Validator.

[dblock]

Not sure why/what/etc., I am speaking without looking at the code in depth, so I'll take a look soon.

I think all I would want here is to be able to have request inside the validator context without having to change any function interfaces. Hope that makes sense? Thinking of binding context or just a definition ala what you're proposing but not that esoteric ;)

[glennpratt]

@dblock updated.

[glennpratt]

This change is to make sure @converter is only created once, rather than in each validator dup.

coerce_spec.rb is expecting this behavior.

[dblock]

Calling dup so many times makes me nervous. @dm1try I could use another opinion on this thread, please

Is https://github.com/ruby-grape/grape/blob/master/lib/grape/validations/validators/multiple_params_base.rb#L7 saying that we shouldn't worry about thread-safety in any of these and therefore don't need a dup but can just make @request appear in there by either implementing another validate (like you did) or possibly deprecating the signature like here?

I am proposing dblock@7815ad0 basically on top of your change. Either way we don't need to pass both the request and params into validate since params is request.params. Since all tests pass and this is single-threaded Rack I think this is OK, @dm1try?

[glennpratt]

If I use threads to make requests to the same app, that @request instance variable in dblock/grape@7815ad0 won't be thread safe - @request can be overwritten in another thread.

I haven't tested it in a common threaded app server, but I could if that would help.

[dm1try]

It's not clear to me how @request and other instance vars in Endpoint instances are thread safe - perhaps some magic with the binding (generate_api_method?).

No magic. Just dup on call: 1, 2, 3.

@glennpratt has the good point: all validators for a specific endpoint share the same state; they are saved in route_setting on the initialization and they are not cloned on call/run.

Is https://github.com/ruby-grape/grape/blob/master/lib/grape/validations/validators/multiple_params_base.rb#L7 saying that we shouldn't worry about thread-safety in any of these and therefore

This line seems unsafe (multiple requests for the same endpoint with shared state 😟)

In short, don't provide any "per-request" state to validators because the state is shared across all endpoint instances(add to README) or copy them.

I'll have a deeper look at this later.

[glennpratt]

No magic. Just dup on call: 1, 2, 3.

@dm1try doh, thank you. When I was poking at it I could have sworn multiple requests were in the same Endpoint object, but I must have been wrong.

I'm thinking Validator#dup probably isn't that expensive and probably prevents future mistakes. Would probably require benchmarks to say.

If #dup doesn't work, then we could use #validate(request) as the new method with #validate!(params) for backwards compatibility with NO access to the request.

[dblock]

Ok so we're saying that https://github.com/ruby-grape/grape/blob/master/lib/grape/validations/validators/multiple_params_base.rb#L7 is a bug.

@dm1try Should we merge this PR as is giving validators a dup? I would want to simplify the interface and not pass both request and params and do what @glennpratt suggests above for backwards compat.

[dm1try]

@dm1try Should we merge this PR as is giving validators a dup?

As mentioned before, we might do some benchmarks.
I agree on the extending a scope of a validation object(params => request) without providing any additional state to validators and do not dup for now. It will be work as before.

I would want to simplify the interface and not pass both request and params and do what @glennpratt suggests above for backwards compat.

Sounds good to me!

[glennpratt]

Super Scientific Benchmarks

Calculating -------------------------------------
#validate(request)
                        20.840k i/100ms
#validate_no_attr(request)
                        20.409k i/100ms
#validate_no_dup(request)
                        22.787k i/100ms
#validate!(params)
                        23.616k i/100ms
-------------------------------------------------
#validate(request)
                        245.524k (± 7.2%) i/s -      2.459M
#validate_no_attr(request)
                        245.349k (± 7.4%) i/s -      2.449M
#validate_no_dup(request)
                        293.073k (± 7.9%) i/s -      2.917M
#validate!(params)
                        318.294k (± 7.1%) i/s -      3.165M

[dblock]

Looks like dup isn't that big of a deal, but we still don't have to if we just do dblock@7815ad0 (minus the @request assignment).

To summarize:

• Add validate that takes a request instead of params (not both)
• Call validate(request.params) by default

This will let you override validate in a custom validator.

We still need to fix the existing problem in MultipleParams::Base.

[dblock]

@glennpratt Want to give above a try? I am interested in getting this merged, I think we're close.

[glennpratt]

@dblock sure, I have a moment now.

[glennpratt]

@dblock The cleanest, backwards compatible fix for #scoped_params I see is:

      def validate(request)
        dup.validate!(request.params)
      end

Assuming external users could be depending on the #scoped_params method.

[glennpratt]

@dblock changes pushed. Do you want me to push that fix for MultipleParams::Base now or add a todo?

[dblock]

Ok, this change looks good, merging. Make a separate PR for the MultipleParams fix, please, and much much appreciated!

[magni-]

#986 mentioned being able to "use helpers such as current_user."

How do the changes from this PR let us do that?

[dblock]

This PR passes request along with params, which is a step closer, but I think more work is needed for #986