Optional properties are treated as required

[ghost]

I was using Reform 2.2.1 until I needed to validate the presence of date_time fields. Didn't work. Found out my problem was covered by issue #380. Consequently, I updated my Gemfile to fetch this repo's master branch, as a temporary fix (August, 5, 2016). Now it works fine, but all optional(<property_name>).filled(...) statements in my code base are treated as if they were required(...).filled(...) statements.

Example:

optional(:gender).filled(included_in?: %w(male female))

yields

Trailblazer::Operation::InvalidContract:
        {:gender=>["must be filled", "must be one of: male, female"]}

[fran-worley]

In Reform if a property is defined the key will always be present due to the way we pass params to dry-validation.

Can you provide some background to your use case?

Due to the way reform works with dry it's not an easy fix and I was wondering if there is an alternative I can suggest.

[ghost]

My use case is fairly standard I suppose. Here is how it goes :

• make a JSON request to a Grape API
• parse the request from within the API and forward it to a Trailblazer Operation
• validate the params from the JSON request using Reform::Form::Dry

In my case, I want the : birthdate key to be optional, but if it is is present, then its value must not be nil, and, furthermore, it must be a date_time. Namely:

optional(:birthdate).filled(:date_time?)

From dry-validation documentation:

Optional Keys and Values We make a clear distinction between specifying an optional key and an optional value. This gives you a way of being very specific about validation rules. You can define a schema which can give you precise errors when a key was missing or key was present but the value was nil.

Let's observe the difference between dry-validation and reform:

require 'dry-validation'
require 'reform'
require 'reform/form/dry'

puts "\n\n--- With first Dry::Validation::Schema ---"
schema = Dry::Validation.Schema do
  required(:email).filled
  optional(:age).filled
end

errors = schema.call(email: 'jane@doe.org').messages
puts errors.inspect
# => {}

errors = schema.call(email: 'jane@doe.org', age: 32).messages
puts errors.inspect
# => {}

errors = schema.call(email: 'jane@doe.org', age: nil).messages
puts errors.inspect
# => {:age=>["must be filled"]}
# Conclusion : exactly what I expected. Nice !

puts "\n\n--- With second Dry::Validation::Schema ---"
schema = Dry::Validation.Schema do
  required(:email).filled
  optional(:birthdate).filled(:date_time?)
end

errors = schema.call(email: 'jane@doe.org').messages
puts errors.inspect
# => {}

errors = schema.call(email: 'jane@doe.org', birthdate: DateTime.now).messages
puts errors.inspect
# => {}

errors = schema.call(email: 'jane@doe.org', birthdate: nil).messages
puts errors.inspect
# => {:birthdate=>["must be filled"]}
# Conclusion : exactly what I expected. Nice !

puts "\n\n--- With a Reform::Form ---"
class FormA < Reform::Form
  feature Reform::Form::Dry

  property :email
  property :birthdate

  validation do
    required(:email).filled
    optional(:birthdate).filled(:date_time?)
  end
end

form = FormA.new(OpenStruct.new(email: nil, birthdate: nil))
form.validate(email: 'jane@doe.org')
puts form.errors
# reform 2.2.1  => {}
# github/master => {:birthdate=>["must be filled"]}

form = FormA.new(OpenStruct.new(email: nil, birthdate: nil))
form.validate(email: 'jane@doe.org', birthdate: DateTime.now)
puts form.errors
# reform 2.2.1  => {}
# github/master => {}

form = FormA.new(OpenStruct.new(email: nil, birthdate: nil))
form.validate(email: 'jane@doe.org', birthdate: nil)
puts form.errors
# reform 2.2.1  => {}
# github/master => {:birthdate=>["must be filled"]}

My two cents:

• The biggest pain point with the Trailblazer ecosystem is the documentation. See:
  • http://trailblazer.to/gems/representable/3.0/populator.html
  • http://trailblazer.to/gems/reform/#more (the full documentation link points to the very same page !)
• It's a big 'framework' (or a patchwork of independent libraries bundled as a big framework) that uses external gems such as dry-validation quite extensively, and if I want to know more about how to use some specific function, then I have to read the documentation of this external gem. This is not obviously stated from Trailblazer's documentation, and, as a newcomer, I have quite a hard time distinguishing between trailblazer's classes and methods from that of 'invited' gems.
• And now I learn that the behavior of these gem's methods vary depending on whether I use them vanilla-style or from within Trailblazer's ecosystem.

[fran-worley]

Hi @TristeFigure, thanks for coming back to me.

On your two cents:

• The docs are a continual work in progress. The repo for the site is here: https://github.com/trailblazer/trailblazer.github.io if you want to contribute to their development.
• One of the goals of the framework is to decouple each lib so that you can use as much or as little of the libs as possible. Where external gems exist to cover dedicated areas such as coercion and validation we provide support for those gems rather than building our own just for the sake of keeping it in the Trailblazer eco system. Rather than re-write documentation for external gems we provide documentation on how to use them within Trailblazer and then provide links to the full documentation from the gem provider. I take your point on it being confusing as to where 'trailblazer' ends and other gems begin, maybe this is something you could raise in the Docs?
• Dry-Validation is a very new and fast moving gem and was only incorporated into trailblazer relatively recently. There are a number of improvements we are making to how Reform works with dry-validation (marked with the dry-validation integration) label. These will be released as part of reform 2.3.0.

Now back to the issue at hand. There is a wider discussion to be had here on whether we are solely validating input or the object in its entirety. Take this example:

Your object has a birthdate of '' (empty string) or some other value. Your json input doesn't include the birthdate key. Should the Reform instance be valid because birthdate wasn't included in the original input and you've used optional or invalid because the object already has a value which is invalid?

[randomizor]

I found this to be an issue for myself as well. I understand the problem, but setting something as optional and then finding it's actually "required" is a bit of a subversion of expectation, especially since the other version of reform seem to handle this scenario.

If things were to stay the current way you are describing (with the form object being passed to the validation), is there even a use case for the optional method?

[fran-worley]

The way things are there is no use for an optional method. It's a method provided by dry-validation and not reform.

What we need to do is agree what the correct handling should be and that's something which should be discussed and agreed upon as it could lead to unexpected behaviour for others (see the example I described).

@apotonick do you have any thoughts here? Is validating just the input keys likely to cause confusion for others? Could we make it configurable (i.e. have an optional property setting which only includes the key if it was included in the input or it has a value)

[apotonick]

Is the problem that gender is nil on the form but is yet passed to the dry-validation schema because we don't know (yet) whether or not it has been modified?

If yes, we could check and only include changed values in the dry-v hash. Would that fix it?

[fran-worley]

Hi @apotonick. It's not that it's nil. The key doesn't exist in the input hash at all, but it is passed to dry as the property is defined on the form and we use an equivalent of 'form.to_nested_hash'. Optional(:key) in dry essentially means the key is optional but if present, run the validations. That's why we get the unexpected messages.

[pvmeerbe]

sorry to hijack this issue, but I think the conceptual issue/mismatch with dry-validation is not limited to 'optional' keyword, but also with the 'required' keyword:

Imagine a JSON API with following input data :

   {"first_key": "blabla","third_key":"hahaha"}

And using the following contract with dry-validation scheme

 class MyContract < Reform::Form
     feature Reform::Form::Dry

     property :first_key
     property :second_key
     property :third_key

     validation :default do
         required(:first_key)
         required(:second_key)
         required(:third_key)
      end
   end

And API Operation

    class Create < Trailblazer::Operation
     require 'ostruct'

       contract MyContract

       def model
         OpenStruct.new  # Note: same issue applies when using an ActiceRecord  model
       end

       def process(params)
         validate(params) do
           contract.save
         end
       end
    end

Then I would expect

  Create.({"first_key" =>  "blabla","third_key" => "hahaha"})

To raise a validation error on missing second_key

However this is not the case, since the contract will always have all keys available.

It does work when I rewrite my validation as required(:second_key).filled

But that is not the same validation + one of the ideas behind dry-validation to make a difference between missing key and missing value...

[fran-worley]

@pvmeerbe As we spoke about today what you're describing is actually an issue with dry see https://github.com/dry-rb/dry-validation/issues/251

[randomizor]

Has there been any headway made on this? The currently handling doesn't seem to fit with what one would expect from reading the dry-validation docs, which is fine, except that Reform specifically notes that dry validation is what is being used. If Reform was simply wrapping dry validation in it's own unique DSL I would think it would prove less troublesome, but would also be more work to maintain.

Here's the issue with the implementation in this release from using it with an API.

Let's say I have a User object:

class User < Disposable::Twin
  property :id
  property :name
  property :status
end

Now every time an API user wants to update a user, I want them to provide the status (because I'm weird), so I write my form:

class UpdateForm < Reform::Form
  feature Reform::Form::Dry

  property :name
  property :status

  validation :default do
    required(:status).filled(:str?)
    optional(:name).filled(:str?)
  end
end

Their request looks like follows:

{"id": 1, name: "Test"}

(I use the ID to get the object to pass through to the form)

Now I would expect this to fail, however, if the user already has a status, this will pass, correct?

What I almost need is to be able to do something like so:

class UpdateForm < Reform::Form
  feature Reform::Form::Dry

  property :name
  property :status

  validation :default, context: :object do
    required(:name).filled(:str?)
    required(:status).filled(:str?)
  end

  validation :default, context: :params do
     required(:status).filled(:str?)
     optional(:name).filled(:str?)
  end
end

[fran-worley]

@randomizor we've not made any progress on this because of the implications. What we need to do is something like you say where you can configure the scope/context of what dry validates. It will default to the complete object, but if you specify that you just want to validate the input then we will strip out keys which were included in your input.

So in your case you would be able to do something like:

validation do # validates the entire object
    required(:name).filled(:str?)
    required(:status).filled(:str?)
end

 validation(context: :input) do # only validate keys actually passed into the method
   required(:status).filled(:str?)
   optional(:name).filled(:str?)
 end

[randomizor]

@fran-worley Yeah that sounds perfect!

[pvmeerbe]

@fran-worley idd. Using

required(:second_key) { none? |  array? }

instead of

required(:second_key) 

works fine, I get :

 Create.({"first_key" =>  "blabla","third_key" => "hahaha"})
  --> {:second_key=>["is missing"]

However, when the second key was already set/loaded from the reform model it still doesn't behave as expected (i.e. no validation error if required key was not provided) as the validation is done after merging params with values from model : see http://pastebin.com/LPYG1dkh, more specifically the line

  OpenStruct.new(:second_key => 'value_set_by_reform_model')

This is in fact the same issue as @randomizor described above with the 'status' value loaded from the model behind the reform

So the proposed solution with the validation 'context' would also solve this issue.

I've currently solved my problem using a prevalidator as suggested by @ZimbiX on gitter : https://gitter.im/trailblazer/chat?at=57c5aa4289fabaea6bd6d9f5

The idea is the same, but your suggested implementation seems better/more clear

[pvmeerbe]

@fran-worley could it be that in contrast to what you mentioned in https://github.com/apotonick/reform/issues/390#issuecomment-237942594, keys are not always automatically present when defined as a property? It seems they are not added when a nil value was provided in the params and no value was available from the underlying model.

I've written some rspec tests comparing dry-validation with reform + dry-validation behavior for the "required" keyword as I didn't understand the behavior of reform (see http://pastebin.com/gmQXPa9e).

Apparently the 'to_nested_hash' call on Reform removes the keys with nil value, hence dry-validation never gets it ( FYI I'm using dry-validation 0.9.5).

  reform-2.2.1/lib/reform/validation.rb:36
  ....
  Groups::Result.new(self.class.validation_groups).(to_nested_hash, errors, self)
  ....

This results in the confusing/wrong error message on the key "is missing" instead of value "must be filled" (while dry-validation as stand-alone handles this case correctly).

Due to the same issue a weird "optional key" scenario is also present: http://pastebin.com/z73BsLpL In this case one would assume Reform would raise an invalid error on

 "{:serial_number=>["must be filled", "must be greater than 0"]}"

This doesn't happen since the serial_number key is never provided to dry-validation as its value was nil. (while dry-validation as stand-alone handles this case correctly).

So it seems there are a number of confusing differences when using dry-validation as stand-alone or within Reform. So far I've seen 2 root causes:

• Reform applies validation to the merged result of params + model values+ default values + populator values (one could mistakenly assume that only the params are checked)
• params are only considered for validation when their value is not set to nil ( as a result required keywords which are present in the params may not be present in the data send to dry-validation)

[randomizor]

The first root cause would be fix with the context functionality described above..

[fran-worley]

@pvmeerbe your point about nil values is actually bug which is filed under a seperate issue. And may have already been fixed in disposable.

Currently you cannot use optional from dry-v in reform as it will just treat it as required. We will include an option to enable param only validation to 2.3.0 but this wil start as an experimental feature whilst we consider whether there are deeper implications.

[pvmeerbe]

@fran-worley I guess you mean https://github.com/apotonick/reform/issues/362 & https://github.com/apotonick/disposable/pull/48 which is still open.

Not sure whether it is the same issue as it talks about syncing twin values to underlying models.

In this case we are converting a reform instance using 'to_nested_hash' which does use the Disposable::Twin::Sync::ToNestedHash module. Perhaps the same code is used for both purposes...

[fran-worley]

First draft of params context included in https://github.com/apotonick/reform/pull/388

Feedback would be great, then I can tick this off the list and we'll be one step closer to release.

[pvmeerbe]

FYI regarding nil values not forwarded to dry-v as mentioned in previous comment: this is idd fixed in disposable 4.0.0 (seen after using the new git tree from @fran-worley )

[blelump]

@fran-worley ,

adding the context option is probably the best solution for 2.3 and since dry-v grows in popularity, it seems it is essential to have such feature in Reform. Good job!

[pvmeerbe]

@fran-worley perhaps the new context option can also be added to the dry-validation config block, similar to the form option : https://github.com/apotonick/reform/blob/improve-dry-integration/lib/reform/form/dry.rb#L48

This way people may use it for example in custom predicates

[fran-worley]

I won't add it in by default as you can access the params via the form object, or use dry-v's high level rule feature to access other params.

[pvmeerbe]

@fran-worley that makes sense... thanks for all the work on dry-validation integration. it's much appreciated ;)

[fran-worley]

I'm going to close this as it's now in #388