Rule fired for optional attribute which is missing

[mateusz-useo]

Description

Rule is getting fired for a required attribute which is nested within an optional attribute and when the whole optional hash is missing (which should be allowed) it reports an error causing the validation to fail.

To Reproduce

Create a contract with a rule for an optional hash attribute with a required key + a rule for the required key and run with an input not including that hash. I use custom macro and I don't know if it's related to the bug.

So given:

class SignUpContract < Dry::Validation::Contract
  params do
    required(:user).hash do
      optional(:id) { filled? & str? }
      optional(:first_name) { filled? & str? }
      optional(:last_name) { filled? & str? }
      required(:email) { filled? & str? }
      required(:password) { filled? & str? }
      required(:accepts_terms) { filled? & bool? }
      optional(:freelancer).hash do
        required(:id) { filled? & str? }
      end
      optional(:clients).array(:hash) do
        required(:id) { filled? & str? }
      end
    end
  end

  rule('user.id').validate(:uuid_format)
  rule('user.email').validate(:email_format)
  rule('user.password').validate(min_length: 8)

  rule(user: %i[freelancer clients]) do
    freelancer = values.dig(:user, :freelancer)
    clients = values.dig(:user, :clients)

    unless freelancer ^ (clients && clients.size == 1)
      key(:user).failure(:profile_missing)
    end
  end

  rule('user.freelancer.id').validate(:uuid_format)
  rule('user.clients') do
    have_valid_uuid = lambda { |client|
      client[:id].nil? || client[:id].match?(Macros::UUID_REGEXP)
    }

    unless value.all?(have_valid_uuid)
      key.failure('not a valid UUID format')
    end
  end
end

And params:

{
  "user": {
    "id": "d16dfef3-50f9-4588-b7d7-837801e403cd",
    "first_name": "Blah",
    "last_name": "Blah",
    "email": "blah@blah.blah",
    "password": "blahblahblah",
    "accepts_terms": true,
    "clients": [
      {
        "id": "d287fdff-78ac-45ed-871b-b1b5c9548399"
      }
    ]
  }
}

It produces:

#<Dry::Validation::Result{:user=>{:id=>"d16dfef3-50f9-4588-b7d7-837801e403cd", :first_name=>"Blah", :last_name=>"Blah", :email=>"blah@blah.blah", :password=>"blahblahblah", :accepts_terms=>true, :clients=>[{:id=>"d287fdff-78ac-45ed-871b-b1b5c9548399"}]}} errors={:user=>{:freelancer=>{:id=>["not a valid UUID format"]}}}>

Expected behavior

The validation rule for optional attribute should never be run when it's missing.
In the example rules related to freelancer attributes should not be run and the validation should pass

My environment

• Affects my production application: NO as I haven't upgraded yet
• Ruby version: 2.6.3
• OS: MacOS Mojave Version 10.14.5 (Darwin Mateuszs-MacBook-Pro.local 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64)

[solnic]

Please see #550 and let me know if this makes sense to you.

[solnic]

I'm gonna close this one since, after all, this is not a bug. See #550 where new features have been added to make it easier to work with optional keys and rules that depend on them.

[mateusz-useo]

It is awesome, it does add the condition here and then more often than not but I do get the idea behind it so thank you, I really appreciate your work :)

[solnic]

@mateusz-useo cool! I plan to add a bigger feature soon where you'll be able to use pattern matching to run rules conditionally, so ie this would work:

rule(:numbers).if(:key?).each do
  key.failure("must be even") if value.odd?
end

[flash-gordon]

for the record, this thing in PM is usually called "guard", just to save some time on coming with proper naming which is, you know, hard...

[mateusz-useo]

Maybe rule? instead of rule(...).if(:key?) as a shorthand?

[solnic]

@mateusz-useo oh that's a nice idea, thanks. I'll keep it in mind!

[shepmaster]

Please consider how this can work with .validate as well.

[solnic]

@shepmaster good point

[musaffa]

I've a contract like this:

params do
  optional(:per_page).filled(:integer)
  .....
end

rule(:per_page).validate(gt?: 0, lteq?: 20)
.....

If there's no per_page key, this code creates error with the predicate macros as defined in its rule. As an alternative, we can check for key? in a rule block. But doing the works of predicate macros and their associated errors manually inside a rule block will create a lot of boilerplate codes.

I've been trying to upgrade dry-validation from 0.13 to >= 1.0.0. Optional keys are very common and this appears to be a major blocker for me to upgrade.

We would love to see a solution implemented soon. Thank you.

@solnic @flash-gordon

[solnic]

@musaffa I'm not yet sure how to solve this. Adding key? check automatically may not be desired in various cases, see #550's description for more info.

[musaffa]

Exposing validate inside the rule block might solve the issue. For example:

params do
  optional(:per_page).filled(:integer)
end

rule(:per_page) do
  validate(gt?: 0, lteq?: 20) if key?
end

For maybe with optional:

params do
  optional(:per_page).maybe(:integer)
end

rule(:per_page) do
  validate(gt?: 0, lteq?: 20) if key? && value
end

[solnic]

@musaffa I reckon it'd be simpler (and better UX too) to extend rule DSL with pattern-matching-like API, starting with support for key? like I mentioned above, so:

params do
  optional(:per_page).maybe(:integer)
end

rule(:per_page).guard(:key?).validate(gt?: 0, lteq?: 20)

that would just work and you could also use it with block-based rules. WDYT?

[musaffa]

A guard can be a good shortcut. It's simpler and concise.

rule also supports multiple keys. How will guard(:key?) function in those cases?

guard(:key?) will only cover for optional key. For maybe, we will also need to guard the value before passing it to the validate function.

I still think that the ability to call validate inside the rule block can be a valuable addition. Not just for optional key or value but for the other use cases. For example, I would love to use registered macro inside a rule block.

[solnic]

guard(:key?) will only cover for optional key. For maybe, we will also need to guard the value before passing it to the validate function.

Yeah we can have probably if and unless instead of guard, then you could have:

# for optional
rule(:per_page).if(:key?).validate(gt?: 0, lteq?: 20)

# for maybe
rule(:per_page).unless(:nil?).validate(gt?: 0, lteq?: 20)

We could even go nuts a bit and provide a shortcut (but I've got mixed feelings about this):

rule(:per_page).if(:present?).validate(gt?: 0, lteq?: 20)

I still think that the ability to call validate inside the rule block can be a valuable addition. Not just for optional key or value but for the other use cases. For example, I would love to use registered macro inside a rule block.

Yes this is a really good point, thank you. I'll experiment with this and get back to you.

[musaffa]

rule(:per_page).unless(:nil?).validate(gt?: 0, lteq?: 20)

:nil? looks ambiguous. It may mean key or value. :value? may be more suitable:

# for optional and maybe
rule(:per_page).if(:key?).if(:value?).validate(gt?: 0, lteq?: 20)

# for optional and maybe (combined if)
rule(:per_page).if(:key?, :value?).validate(gt?: 0, lteq?: 20)

:present? -> I've got mixed feelings about this

I know where it's coming from. The spectre of ActiveModel validation haunts us all.

[solnic]

@musaffa I reported two feature requests:

• Support for pattern-matched rules #678
• Support for "validate" helper within rule blocks #679

It's tentatively scheduled to be part of 1.7.0 release. I marked both as help wanted for the time being though. I'll give this a shot during xmas break unless somebody beats me to it.