dry-validation icon indicating copy to clipboard operation
dry-validation copied to clipboard

import_predicates_as_macros: included_in?, format?, size? issues

Open aleksandra-stolyar opened this issue 4 years ago • 9 comments

class MyContract < Dry::Validation::Contract
  params do
    required(:hogwarts_house).hash do
      required(:head).filled(:string)
      required(:common_room).hash do
        required(:name).filled(:string)
        required(:location).filled(:string)
      end
    end
  end

  rule('hogwarts_house.head').validate(size?: 1..255)
  rule('hogwarts_house.head').validate(format?: SOME_REGEX)
  rule('hogwarts_house.common_room.location').validate(included_in?: ['tower', 'underground'])
end

For included_in? and size? passing an array of integers/strings not working but it works when just one value passed or with predicates which do not require arrays) included_in?

rule('hogwarts_house.common_room.location').validate(included_in?: ['tower', 'underground'])

size?

rule('hogwarts_house.head').validate(size?: 1..255) ArgumentError (wrong number of arguments (given 256, expected 2))

This predicate is just returning error. format?

rule('hogwarts_house.head').validate(format?: SOME_REGEX) Dry::Container::Error: Nothing registered with the key :format?

aleksandra-stolyar avatar Dec 19 '19 15:12 aleksandra-stolyar

Just ran into these same issues with included_in? and format?

lromeo avatar Dec 30 '19 19:12 lromeo

#610 adds missing :format? predicate.

About the other two, the issue is that they are related to predicates expecting an array (or range) as argument. It hasn't straightforward solution. Macro argument(s) are forwarded to the predicate method, and we don't have any way to tell whether it expects an array or an unitary item. I see three options:

  • Force the user to be explicit. For example, currently the following works (@aleksandra-stolyar @lromeo take note for now):
rule('hogwarts_house.head').validate(size?: [1..255])
rule('hogwarts_house.common_room.location').validate(included_in?: [['tower', 'underground']])
  • Manually keep record of what predicates expect what.

  • Modify dry-logic so that the predicates with array or range arguments also work with splat arguments.

waiting-for-dev avatar Jan 01 '20 21:01 waiting-for-dev

@waiting-for-dev that's OK, it's always worked like that, going back to dry-validation 0.x. We gotta make sure to document it properly.

solnic avatar Jan 03 '20 10:01 solnic

@waiting-for-dev ...no wait, sorry - it did not work like that in dry-validation 0.x! dooh. OK this needs to be investigated because I can't say why it's not working as expected in case of predicate extension :(

solnic avatar Jan 03 '20 10:01 solnic

Currently, a predicate can be of three kinds:

  • #method(input), as in #empty?([1, 2])
  • #method(value, input), as in #gt?(1, 2)
  • #method(list_value, input), as in #included_in?([1, 2], 1)

The last two are problematic when used with import_predicates_as_macros extension. Macro arguments are always collected as an array:

rule(:age).validate(gt: 18) # => `macro.args` is `[18]`
rule(:age).validate(included_in: [18, 21]) # => `macro.args` is `[18, 21]`

In the #included_in? example, of course the user should provide a nested array [[18, 21]], because they want to provide an array argument and not two arguments. But as the predicate method is "hidden", there is the concern whether it means expecting too much from them.

As ruby is a dynamically typed language, there is no way to know whether the expected argument is an array or not. Currently, when arguments are dispatched they are splatted, so when you provide an array like in included_in it results in a wrong number of arguments error.

waiting-for-dev avatar Jan 04 '20 17:01 waiting-for-dev

In the case of #size? the user is in fact providing what the predicate method expects, but dry-validation coerces arguments with Array() so the information is lost.

waiting-for-dev avatar Jan 04 '20 17:01 waiting-for-dev

Maybe we should:

  • Don't coerce with Array(). Just create a singleton array when provided macro arguments returns false with #is_a?(Array).
  • Document properly the situation for array arguments. Alternatively, change dry-logic to accept splatted arguments, but I don't like moving the complexity to the fundamental piece...

waiting-for-dev avatar Jan 04 '20 17:01 waiting-for-dev

Don't coerce with Array()

I bet that is the solution we're looking for

solnic avatar Jan 08 '20 09:01 solnic

The referenced PR should fix the issue here with #size? predicate usage with a range.

About #included_in?, the predicate expects an array so the correct way to specify it is included_in?: [[18, 21]]. Saying [18, 21] would mean that the predicate expected two arguments. I understand this may seem confusing at first sight. As I said, an option would be modifying dry-logic to also accept splatted arguments, but I don't like it very much.

waiting-for-dev avatar Jan 15 '20 17:01 waiting-for-dev