Define contract instead of params about grape HOT 8 CLOSED

Sure, I'll give it a shot.

from grape.

It's a good idea. It could just be an extension to params that takes a Dry::Validation::Contract, and yes we could alias params to contract too. I'm thinking one would want to do things like optional :foo, contract: Dry::Validation::Contract as well.

from grape.

Hi folks! I've done a little write-up on ways to use dry-validation and grape together. Here's the repo with the example.

Note that this approach adds a new helper to be used instead of declared(params), and it doesn't support mixing contracts with regular params (see the blog for an explanation why). It lets you declare a contract inline, though, above the route. If people would like this in core Grape, I'm happy to prepare a PR.

A deeper integration seems more difficult. Also note that dry-schema doesn't support renaming/aliasing, so it's not a full superset in terms of features (though processor callbacks seem to fit as a workaround).

from grape.

I love the idea of contract do .... Do you think it's possible to refactor params do ... (ParamsScope) in a way that can be fully swapped by your implementation at runtime? Wouldn't worry about fewer features. We can dynamically load/configure which one is available, this is a small implementation detail.

from grape.

If we likewise dispatch declared to use the contract when set (or configured), I suppose it could work. I'm not sure about supporting the same lifecycle, though, where currently when you call params inside-route, they are all coerced and validated already--and declared only does the filtering and renaming. Or would we use another helper to get the validation's result?

Dispatching params (ParamsScope) to either of the implementations could work, but if you look at the example, the current usage looks like this (nested params is Dry::Validation::Contract's DSL for defining the schema):

contract do
  params do
    ...
  end
 
  rule(:abc) { ... }
  ...
end

If the first call in the above snippet is renamed to params do as well, that won't look right. We could drop the "contract" business, though, (and its "rules") and use dry-schema directly. Then the helper would create a Dry::Schema::Params instance. Less nesting, but one fewer feature.

from grape.

We should put the implementation aside for a second, and only talk DSL and user-facing behavior, it will make things easier.
I'd be curious to hear from @dnesteryuk (added dry-types) and possibly others who did a lot of this param implementation. Maybe go through some git blame and tag a few people that made big PRs in parameter coercion and validation.

On your comments specifically.

If we likewise dispatch declared to use the contract when set (or configured), I suppose it could work.

Yes, I think that declared(params) should return what it returns today, a validated set of parameters that satisfy the contract. We don't need contract_params or similar custom names, I think.

I'm not sure about supporting the same lifecycle, though, where currently when you call params inside-route, they are all coerced and validated already--and declared only does the filtering and renaming.

Why not? I think users would appreciate if contracts were to enforce the same behavior you'd expect from any parameter validation and coercion layer.

Or would we use another helper to get the validation's result?

If validation fails (contract not satisfied), you should get a validation error back on the client just like with params today and processing should stop. The body of the API should not be reached IMO.

Regarding the use of declared to filter params, I think it's sort of a hack waiting for an implementation for #810. You could enforce this for a contract by default (all params must satisfy the contract all the time, no other params), in which case declared(params) is a noop that returns all params.

Another thought. I am confused seeing required(:order) in params, because it feels like a field. Maybe I should be able to omit the name for the top-level param?

contract do
   params do
       filled(OrderSchema)
   end
   rule do
      next if value[:baskets].count < 10
      key.failure('contains too many baskets')
   end
   post do
      order = Order.create!(declared(params))
   end

Finally, on nesting/not nesting in contract, you could extend the DSL and have params in the contract sense, next to a new rules that is only available with contracts, but I think contract is cleaner.

from grape.

Why not? I think users would appreciate if contracts were to enforce the same behavior you'd expect from any parameter validation and coercion layer.

Sure. The question is whether they'd have to give something up as well, in performance or capabilities.

If validation fails (contract not satisfied), you should get a validation error back on the client just like with params today and processing should stop. The body of the API should not be reached IMO.

That definitely makes sense.

You could enforce this for a contract by default (all params must satisfy the contract all the time, no other params), in which case declared(params) is a noop that returns all params.

I was trying to remember when was it that I found it useful to have a different value in params, i.e. having access to the pre-filtered hash (though still validated). Guess it was examples like this:

    resource :bars do
      route_param :bar_id do
        resource :foos do
          params do
            ...
          end
          post do
            puts declared(params)
          end
        end
      end
    end

where :bar_id is not included in the set of declared params because it doesn't have the params block above it. But adding type: ... to the route_param call seems enough, so maybe that's fine. Another use for params returning something different is to check at runtime whether an optional parameter with default value was actually supplied. But I can't describe a scenario where that's useful offhand.

Anyway, the case of including the parent scopes' parameters in the declared set needs to be handled. My current implementation would filter them out, as long as they are not mentioned in the endpoint's schema directly.

Regarding the use of declared to filter params, I think it's sort of a hack waiting for an implementation for #810.

And dry-schema has a configuration attribute which disallows unknown attributes. It has a bunch of known bugs, though: https://github.com/dry-rb/dry-schema/issues?q=is%3Aissue+is%3Aopen+validate_keys

Another thought. I am confused seeing required(:order) in params, because it feels like a field. Maybe I should be able to omit the name for the top-level param?

In that example order is a wrapper attribute (used for the article's continuity). I'm not sure if I got your meaning right, but omitting the wrapper would either look like this:

contract do
  params do
    required(:baskets).array(BasketSchema)
  end
  
  rule(:baskets) ...

or the schema can be assigned this way:

contract do
  params(OrderSchema) # This actually creates a new schema inheriting from this one.

  rule(:baskets) ...
end

Finally, on nesting/not nesting in contract, you could extend the DSL and have params in the contract sense, next to a new rules that is only available with contracts, but I think contract is cleaner.

The point in having params as a separate nested call, is also that there are several types of schemas supported, such as params (which coerces), simply schema (which does not coerce, and is probably useful with pre-structured inputs, such as JSON post body) and json (which parses the JSON input itself; probably not very useful here). If we're okay with giving that up, then indeed params does not have to be specified explicitly, and rules could either be a sibling call, or it could be added to params with our custom translator, adapting the dsl. The downside is that then such inline contract definition would not be possible to simply extract into a separate file to create a named contract class -- it would have to be rewritten to account for the difference in syntax.

FWIW, I'm not sure if rules really have to be supported with the inline syntax -- it could just support defining a schema. But it would accept being passed an existing contract class that contains both schema and rules (and maybe macros).

from grape.

Good discussion and everything you said makes a lot of sense! Want to try a PR that introduces basic functionality that lets one switch between existing params and contracts? I think we could merge that and open issues for any gaps.

from grape.