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

Composable contracts

Open ianwhite opened this issue 6 years ago • 22 comments
trafficstars

This feature will allow contracts to be composed of other contracts, optionally mounted at input paths.

Example

# this contract defines its own params and rules, but also uses the
# schema/rules of CustomerContract, and AddressContract on the input at :address
# via the #contract and #path methods
class OrderContract < ApplicationContract
  params do
    required(:accept_terms).value(:bool)
  end

  rule(:accept_terms).validate(:acceptance)

  contract CustomerContract

  path :address do
    contract AddressContract
  end
end

Implementation

  • Initial on branch composable-contracts

    • [x] Create a :composable extension
    • [x] implement new #contract feature in the :composable extension
    • [x] Add #path block DSL
    • [ ] Rewrite ResultSet as extension to Result
    • [ ] Rewrite the contract/call_spec integration test using the desired new syntax
    • [ ] Add an example
    • [ ] Document the changes in docsite
  • Future work

    • Decide on exact semantics for merging result values/errors if its unclear
    • Ensure it works wrt: concurrency
    • Figure out if we should integrate with other extensions in the short term
    • Decide on a syntax so contracts can be applied to nested arrays of input
    • Decide on a syntax so contracts can be conditionally be applied depending on the input
    • If consensus that the API is good, remove the :composable extension and rewrite contract and result to include the new functionality by default

Resources

ianwhite avatar Oct 06 '19 20:10 ianwhite

Feel free to open a draft PR early if you'd like to get some feedback sooner.

solnic avatar Oct 08 '19 08:10 solnic

Will do, when the next item (simplify ResultSet) is checked off.

ianwhite avatar Oct 08 '19 09:10 ianwhite

Closing in favour of #595

ianwhite avatar Oct 10 '19 09:10 ianwhite

I think this issue should stay open until it's resolved, because I believe other people would also like to get notified whenever it gets solved and if it's open it won't be forgotten (hopefully).

cgeorgii avatar Dec 13 '19 14:12 cgeorgii

Hi, I'm happy to do that. I'm hoping to get some time to work on the new plan outlined in #595 over the Xmas break.

ianwhite avatar Dec 13 '19 14:12 ianwhite

Hi @ianwhite and @solnic! First I just wanted to take a moment and thank you and the rest of the devs involved in this project for all of your efforts. It's really appreciated, and I know how hard it is to maintain projects like these.

That said I was hoping to find out what the state of this feature request is? I see from the threads that some of the work has shifted to dry-struct and dry-schema, and that work appears to have been completed, at least in part with the changes from https://github.com/dry-rb/dry-struct/pull/139 and https://github.com/dry-rb/dry-schema/pull/256. I also see that https://github.com/dry-rb/dry-validation/pull/595 has been closed, presumably with the intention of extracting some of the logic from dry-schema and dry-struct. Is that still the plan, and is this feature still viable?

I'd be willing to take a look and see what needs to be be changed, but I might need some direction. Let me know when you get a chance and thanks again for your work here!

jameskbride avatar Mar 25 '20 14:03 jameskbride

@jameskbride hey 😄 this is on hold but I think @ianwhite is still interested in working on this, so please try to sync up with him first.

solnic avatar Mar 26 '20 09:03 solnic

Hey, I’ve had a bunch of life issues recently interfering with my available time, but am still keen to work on this. I also want to grok the recent changes to dry-schema to make sure the approach still holds. Happy to discuss @jameskbride

On 26 Mar 2020, at 20:26, Piotr Solnica [email protected] wrote:

@jameskbride https://github.com/jameskbride hey 😄 this is on hold but I think @ianwhite https://github.com/ianwhite is still interested in working on this, so please try to sync up with him first.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dry-rb/dry-validation/issues/593#issuecomment-604321050, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABOXB3ADBLP4XF7A7YEMTRJMNUPANCNFSM4I55QWAA.

ianwhite avatar Mar 26 '20 09:03 ianwhite

@ianwhite @jameskbride I'm really interested in this feature as well and probably will have some bandwidth to help on this!

bilby91 avatar Apr 16 '20 01:04 bilby91

I am composing contracts in this way. What do you think?

class EmployeeContract < Dry::Validation::Contract
  json do
    required(:concepts).array(:hash)
  end

  rule(:concepts).each do
    result = ConceptContract.new.call(value)
    unless result.success?
      meta_hash = { text: 'invalid concept' }.merge(result.errors.to_h)
      key.failure(meta_hash)
    end
  end
end

anmacagno avatar May 20 '20 16:05 anmacagno

@anmacagno I think it's perfectly fine to do it like that for the time being, but eventually we definitely want a dedicated feature that would make it easier

solnic avatar May 21 '20 09:05 solnic

I’m going to have some time to work on this tomorrow (Australian time). I’ll jump on Zulip from about 9am Australian Eastern Time.

On 21 May 2020, at 19:12, Piotr Solnica [email protected] wrote:

@anmacagno https://github.com/anmacagno I think it's perfectly fine to do it like that for the time being, but eventually we definitely want a dedicated feature that would make it easier

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dry-rb/dry-validation/issues/593#issuecomment-631977236, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABOXB3K4LHQLP5BNH5ESTRSTV6FANCNFSM4I55QWAA.

ianwhite avatar May 21 '20 13:05 ianwhite

@anmacagno thx for ur snippet. Just a short extension, when using array(:hash) any key coming inside concepts json will go through. Therefore we use the schema of the child contract in the parent contract schema.

class EmployeeContract < Dry::Validation::Contract
  json do
    required(:concepts).array(ConceptContract.schema)
  end

  rule(:concepts).each do
    result = ConceptContract.new.call(value)
    unless result.success?
      meta_hash = { text: 'invalid concept' }.merge(result.errors.to_h)
      key.failure(meta_hash)
    end
  end
end

tak1n avatar Jan 28 '21 19:01 tak1n

@solnic This feature would much improve this already great library!

tbsvttr avatar Feb 17 '21 18:02 tbsvttr

@tbsvttr ah thanks, I know it's long-time coming but we'll get to implement it eventually 🙂

solnic avatar Feb 18 '21 05:02 solnic

@tak1n While your solution works very well it has the downside that now each error hash has this text key value pair added which neither adds very useful additional information and also could lead to the confusion that the validated field in a hash has a field named text. Is there a way to get rid of it?

tbsvttr avatar Feb 22 '21 13:02 tbsvttr

Hi- are we getting closer to allowing contracts to be reused within another contract?

jacobtani avatar Apr 19 '21 12:04 jacobtani

@jacobtani not really :( currently it doesn't seem like there's anybody who'd have time to work on this feature. Like I mentioned, we'll get to this eventually. It's on my radar and it's going to be my priority after we're done with Hanami 2.0 (FWIW).

solnic avatar Apr 19 '21 13:04 solnic

This is my improvement to @tak1n's and @anmacagno's solutions:

Dry::Validation.register_macro(:contract) do |macro:|
  contract_instance = macro.args[0]
  contract_result = contract_instance.(value)
  unless contract_result.success?
    # Generate a message string instead of adding a
    # dummy 'text' value.  I don't love this, and I
    # wonder if it's possible to improve this.
    msg = flattened_error_messages(
      contract_result.errors.to_h
    ).join(', ')
    key.failure(msg)
  end
end

def flattened_error_messages(errors, path = [])
  errors.each_with_object([]) do |(key, value), msgs|
    case value
    when Array
      value.each { |v| msgs << "#{(path + [key]).join('.')} #{v}" }
    when Hash
      msgs.concat(flattened_error_messages(value, path + [key]))
    end
  end
end

# ...

class MyContract < Dry::Validation::Contract
  params do
    # hash:
    required(:a).hash(A.schema)
    # array of hashes:
    required(:b).array(:hash, B.schema)
  end

  rule(:a).validate(contract: A)
  rule(:b).each(contract: B)
end

johanlunds avatar Oct 25 '21 22:10 johanlunds

Updated variant, now with proper error message nesting. Full solution:

# Use it like this:
#
#     MyContract = Dry::Validation.Contract do
#       params do
#         # for a hash:
#         required(:a).hash(OtherContract.schema)
#
#         # for an array of hashes:
#         required(:b).array(:hash, OtherContract.schema)
#       end
#
#       rule(:a).validate(contract: OtherContract)
#       rule(:b).each(contract: OtherContract)
#     end
#
Dry::Validation.register_macro(:contract) do |macro:|
  contract_instance = macro.args[0]
  contract_result = contract_instance.(value)
  unless contract_result.success?
    errors = contract_result.errors
    errors.each do |error|
      key(key.path.to_a + error.path).failure(error.text)
    end
  end
end

johanlunds avatar Oct 25 '21 22:10 johanlunds

If you're looking to just merge schemas at the top level like myself, looks like you can do this:

class MyContract < Dry::Validation::Contract
  params(MyOtherContract.schema, MyThirdContract.new(args).schema) do
    required(:a).value(:string)
  end
end

This is in the docs, but I had missed it.

If you need the rules you can copy them over too:

my_contract.rules = my_other_contract.rules + my_third_contract.rules

zavan avatar Nov 16 '22 19:11 zavan

Updated variant, now with proper error message nesting. Full solution:

# Use it like this:
#
#     MyContract = Dry::Validation.Contract do
#       params do
#         # for a hash:
#         required(:a).hash(OtherContract.schema)
#
#         # for an array of hashes:
#         required(:b).array(:hash, OtherContract.schema)
#       end
#
#       rule(:a).validate(contract: OtherContract)
#       rule(:b).each(contract: OtherContract)
#     end
#
Dry::Validation.register_macro(:contract) do |macro:|
  contract_instance = macro.args[0]
  contract_result = contract_instance.(value)
  unless contract_result.success?
    errors = contract_result.errors
    errors.each do |error|
      key(key.path.to_a + error.path).failure(error.text)
    end
  end
end

This should be a built-in feature

ssoulless avatar Feb 22 '23 20:02 ssoulless