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

Make JSON schema resilient to any JSON-compatible value as input

Open flash-gordon opened this issue 6 years ago • 16 comments

The JSON standard defines the following data types

  • number (WRT ruby both float and integer shall be supported)
  • string
  • boolean
  • array
  • object (represented as Hash in ruby)
  • null (= nil)

If any JSON schema receives any value of the types above it shouldn't raise an exception at least. Better yet it should provide a helpful error message.

flash-gordon avatar Feb 06 '19 11:02 flash-gordon

We want this for 1.0.0?

solnic avatar Feb 16 '19 19:02 solnic

I guess so, it's just about having a sanity check at the beginning of validation, no?

flash-gordon avatar Feb 17 '19 08:02 flash-gordon

@flash-gordon yes, JSON is a Processor specialization, it supports adding arbitrary steps, it simply passes result-like objects from step to step, and can aggregate errors in this process. This means you should be able to add a special validation step to do your checks. I suspect we'll want to make step API nicer to work with though, but its core functionality is implemented already.

solnic avatar Feb 17 '19 09:02 solnic

Can we make :float values in JSON schema to be coercible from integers? Javascript from {a: 1.0} generates JSON {"a": 1}, and then ruby parses this 1 to integer value. And json schema returns error that value must be float. Looks like a bug

andreyruby avatar Jul 05 '19 12:07 andreyruby

@aglushkov we can, but it looks like it would be a breaking change. I think it should be possible to use a custom type container to override json's float as a workaround for the time being.

solnic avatar Jul 06 '19 15:07 solnic

I reckon we could have something like this @flash-gordon :

Dry::Schema.JSON(Array) do
   # do your thing
end

Dry::Schema.JSON do
   # do your thing
end

solnic avatar Jul 10 '19 11:07 solnic

API-wise it's not a big deal, though I would expect .JSON and .JSON(Hash) to work the same way, just in case

flash-gordon avatar Jul 11 '19 17:07 flash-gordon

I mean, no matter how we do this on the user side, dealing with arrays inside dry-schema will be non-trivial

flash-gordon avatar Jul 11 '19 17:07 flash-gordon

I would propose to introduce the root keyword:

Dry::Schema.JSON do
  root.array(:integer)
end

To implement this I would:

  • add a special __root__ key to the schema
  • wrap input with hash before passing to the steps
  • filter __root__ out from the result
  • return an error on "root" schema nesting
  • handle a special case on passing "root" schema as value

It's a bit hacky, but still, wdyt? @solnic @flash-gordon

skryukov avatar Dec 24 '19 08:12 skryukov

Why not having something like .JSONArray instead? It would be easier to implement IMO

flash-gordon avatar Dec 24 '19 08:12 flash-gordon

besides, the biggest problem I think is producing error messages, not the DSL part

flash-gordon avatar Dec 24 '19 08:12 flash-gordon

besides, the biggest problem I think is producing error messages, not the DSL part

I think you can pass a special option to the message_compiler to skip the first node of the ast. Still don't know how this will play with dry-v though.

skryukov avatar Dec 24 '19 09:12 skryukov

Here's some food for thought:

require 'dry/schema'

Dry::Schema::ProcessorSteps.prepend(Module.new {
  def call(result)
    catch(:halt) { super }
    result
  end
})

class Dry::Schema::MessageCompiler
  def visit_unexpected_input(node, opts)
    input, * = node

    Dry::Schema::Message.new(
      path: [nil],
      predicate: nil,
      text: "we did not expect this",
      input: input
    )
  end
end

schema = Dry::Schema.JSON do
  required(:name).filled(:string)

  before(:key_coercer) do |result|
    unless result.output.is_a?(Hash)
      result.add_error([:unexpected_input, [result.output]])
      throw :halt
    end
  end
end

puts schema.("oops").errors.to_h.inspect
# {nil=>["we did not expect this"]}
#
# `nil` represents input itself in error hash

solnic avatar Mar 12 '20 11:03 solnic

For anyone in the future, before using catch/throw pls consult me :) Maybe we'll be able to find a better solution.

flash-gordon avatar Mar 12 '20 18:03 flash-gordon

@flash-gordon we can have result.halt that just sets it internally to a failure state and further processing won't happen

solnic avatar Mar 12 '20 19:03 solnic

@solnic yeahyeah sure, I just mean using catch/throw without a real need may have undesirable consequences. And even though this doesn't seem to be the case here, I'd like to try other options if available

flash-gordon avatar Mar 12 '20 19:03 flash-gordon