voxpupuli / json-schema

Ruby JSON Schema Validator
MIT License
1.52k stars 242 forks source link

Validations always return true with ActionController::Parameters in rails 5 #371

Open alisaifee opened 7 years ago

alisaifee commented 7 years ago

With Rails 5 ActionController::Parameters no longer inherits from Hash. Using the controller params with json-schema therefore results in the validations always returning true

Example:

Test [11] > schema = {properties: {a: {type: "integer"}}}
{
    :properties => {
        :a => {
            :type => "integer"
        }
    }
}
Test [12] > props = ActionController::Parameters.new a: 'not-int'
<ActionController::Parameters {"a"=>"not-int"} permitted: false>
Test [13] > JSON::Validator.validate! schema, props
true
Test [14] > JSON::Validator.validate! schema, props.to_unsafe_h
JSON::Schema::ValidationError: The property '#/a' of type String did not match the following type: integer
iainbeeston commented 7 years ago

Good point. Looks like we should be calling "to_h" on input data. It'd be good practice to do this anyway

alisaifee commented 7 years ago

I guess its debatable whether an error should be raised if anything that is !is_a? Hash is passed in or an implicit attempt to convert using to_h should be done. Personally I don't have an issue with having to do the conversion myself.

iainbeeston commented 7 years ago

Yes I think for now the workaround is to call to_h in your own code before passing to json-schema

RST-J commented 7 years ago

In terms of predictability I think it would be clearer/easier to identify the source of errors if we rejected everything that returns false for is_a? Hash - exactly as @alisaifee suggested.

On the other side it would be convenient and compliant to what I understand by "the Ruby way" if we just sent to_h to every given input - no matter what (not even check for is_a? Hash). Or is that a mistake in my notion?

iainbeeston commented 7 years ago

Ok, just picking up on this now. I believe "the Ruby way" is not to perform type checking, but use duck typing instead. What the ruby core library does in instances like this is to check respond_to?(:to_hash), and then (if we wanted to) we'd raise an error if it doesn't support it, or if it does support it, call to_hash on the input and carry on as normal

iainbeeston commented 7 years ago

I've been thinking about this some more. I think the issue is a lot more complex than simply checking if the data is a hash or calling to_hash because:

  1. The data given to json schema can be any valid json text - could be an array or a number, not just a hash
  2. The insert_defaults option needs to be able to write to the original object after validation. That means we can't (easily) convert it from a parameters object to a hash.

I think the solution is to start removing all the explicit type checking and make the codebase duck type with confidence

datibbaw commented 5 years ago

bump