Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate new dry-validation DSL #347

Closed
wants to merge 5 commits into from

Conversation

@blelump
Copy link
Contributor

commented Mar 10, 2016

Hi @apotonick ,

this PR is stil in very premature version since dry-v 0.7.0 haven't been released yet. However, it integrates the new DSL, which will be released with the upcoming version so let's test it! :-)

@blelump blelump force-pushed the blelump:new-dry-validation branch 4 times, most recently from 3cc240f to de6769e Mar 10, 2016

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2016

The build fails due to missing dependencies of dry-{validation, types, logic} gems, as I provided them only in gemfiles/Gemfile.rails-{3.1,4.2}. For these build passes, so I don't bother about the rest. Let's wait until @solnic releases new dry-v and I'll bump to new version in gemspec.

@blelump blelump changed the title Integrate new dry-validation Integrate new dry-validation DSL Mar 10, 2016

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2016

@apotonick , I've spoken with @solnic on gitter and he suggests to change the way form instance is currently passed through the new DSL, because every time it's set (and now it's set every time literally ;-), the rules have to be recompiled.

I've noticed that the form dependency exists from the beginning of the dry.rb file existence, introduced by @mrbongiolo . I've personally called this dependency a few times to get the model, but didn't get into any complex use case. If we would want to be flexible and let user set validation context per validation (there're edge cases for sure for such scenario), then probably validation method has to receive validation content, e.g:

validate(:default, with: {smth: model}) do
  key(:email).required
end

It seems API is almost ready for such approach. See here

ps. before saying anything, see how it works now in dry-v :-)

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

@blelump +1. We just had the form instance at the time because some scenarios needed access to the model or the form values. But just passing it whenever it's really needed seems as a better solution. Not sure if dry-v already has access to the "object" values, I remember at the time you couldn't do some validations without customizing them completely.

@solnic

This comment has been minimized.

Copy link

commented Mar 10, 2016

I remember at the time you couldn't do some validations without customizing them completely.

@mrbongiolo do you remember any specific examples?

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

@solnic can't remember right now, but if I'm not mistaken the key only had access to the current value, I couldn't compare it to another value from the form/model. Maybe that's not even needed. I had to stop refactoring, that's why I haven't used the dry-v lately also.
I guess that we should try out the simplest case and see which edge cases appears and what we can do about them.

Ah maybe that's one of the examples I was trying to use with form:

key(:email) do |email|
  email.filled? & email.email? & email.unique?(:email)
end

def unique?(field, value)
  return true if value == form.model.public_send(field)
  !form.model.class.exists?(field => value)
end

I wanted a method a bit more generic to check for unique values, but this can be done differently without the direct access to the form instance.

@solnic

This comment has been minimized.

Copy link

commented Mar 10, 2016

@mrbongiolo making a schema depend on a whole form just to do db-uniqueness validation is a code smell. I'll be adding a new interface for injecting dependencies from "the outside" so that you can have your model injected (or whatever that's needed for db interactions, ie a rom relation) when setting up a schema. With current approach I bet reform+dry-v is super slow :/

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

@solnic it surely is, but that was the easiest way to get it going at that time, we can always improve it. That's why I vote for the simplest solution, to just have the basic schema there as you would using bare dry-v, and just add more stuff to it if people start having too many edgy cases. But that's up to @apotonick also.
Injecting dependencies on the Schema setup seems fair enough, or even as @blelump suggested, have the reform validation block accept some options that I could pass down to the dry-v schema.
In pure dry-v it would be quite easy, since (if I'm not wrong) I can just create a custom method call whatever I need and just return the expected true/false. Sadly this gets a bit messy inside the validation block :/

@solnic

This comment has been minimized.

Copy link

commented Mar 11, 2016

You can now do this:

schema = Dry::Validation.Schema do
  configure do
    option :model, User

    def unique_email?(value)
      model.where(email: value).exists?
    end
  end

  key(:email).required(:unique_email?)
end
@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2016

Well, I thought about it for a while and I'd propose to slightly change the approach in case of dry-v validation. Obviously, we still could support the validation blocks, however as I suppose, the plan is to replace AM:V with dry-v sooner or later. Besides I think the DSL has changed significantly since 0.6 and actually everything what Reform supports natively, can be done in dry-v.

Version 0.7 extends schema definition so such approach is valid:

base_form = Dry::Validation.Form do
  key(:password).required
end

schema = Dry::Validation.Schema(base_form) do
  key(:email).required
end

p schema.({}).messages
# => {:email=>["is missing"], :password=>["is missing"]}

Another important thing is conditional validation, e.g:

form = Dry::Validation.Form do
  key(:password).required
  key(:email).required

  rule(password: [:email, :password]) do |email, password|
    # Validate password if email is valid
    email.email_valid?.then(password.valid?)
  end

  configure do
    def self.messages
      Dry::Validation::Messages.default.merge(
        en: { errors: { valid?: 'must be a valid ' }  }
      )
    end

    def email_valid?(email)
      email.include?('@')
    end

    def valid?(pass)
      false
    end
  end
end
p form.('email' => '5', 'password' => '123').messages
 # => {}

Essentially what I'd propose is not to wrap dry-v logic with validation blocks and provide new method that will handle pure dry-rb Schema object, e.g:

validates_with Dry::Validation.Schema {}

The validates_with could register dry-rb Schema objects and then just call validation with form data and deliver errors. If you guys think about any edge use case that wouldn't fit, please provide it here so we could investigate it.

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

Where would you define the validations Schema?

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2016

@mrbongiolo , we'd have at least 2 solutions:

validates_with Dry::Validation.Schema do
  key(:email).required
end

# or:

validates_with User::Schema::Validation
# user/schema.rb
module User::Schema
  Validation = Dry::Validation.Schema do
    key(:email).required
  end
end

ps. I've used validates_with accidentally, because now I see it's already defined. I wouldn't touch too much the AM:V related API and use some custom method.

@solnic

This comment has been minimized.

Copy link

commented Mar 11, 2016

I agree with @blelump. When using dry-v you should be able to do everything that reform validation "bonuses" provide, and if it's not the case I'll be happy to cover any missing features.

BTW one small nitpick - I don't like how the term "conditional validation" is being used. I understand where it's coming from but we should really think about validation logic as something that's always conditional. In example, you don't want to validate a value under a key when that key is not present, or you don't want to validate a nested hash when it's not really a hash, etc. This is something that's very explicit in dry-v, you can easily see it when you look at validation rule ast (assuming you have a big screen 😆). This idea is one of the core aspects of dry-v, right next to type-safety.

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2016

@blelump maybe we could stay with the validation but it could directly define Dry::Validation.Schema in the way that you suggested. After all the reform validation block is a concept to hold validation. Just not sure how this would play with AM:V.
@solnic AFAIK reform's validation does nothing special, actually it provides some conditional and group validations, but this is easily done when using dry-v instead.

What about we have the validation to work with dry-v, and let AM:V as it's, using validates :blargh?

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2016

TBH I would remove all group and conditional stuff from validation. That's not something that reform should really care about, I guess it came to be because AM:V doesn't have a good way to handle it. But it would be better overall for reform to just support basic AM:V validates :something and use validation as a dry-v Schema. But that requires @apotonick feedback.
I guess this could simplify a good amount of things, and in the future have the majority of users using dry-v.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2016

@mrbongiolo , I'm fine with validation, validates etc, however at first, I'd move all that AM:V related logic from Reform::Validation module. It's really coupled with AM:V, or to be more general, it's all designed to support block contexts, that fire somewhere else. I think, that with new dry-v DSL we don't need it at all, except a small piece of code, that will register Schema objects, perform validation against form data and track errors. I'd also move Reform::Validation::Groups, because it's related as well. Perhaps reform_am_v gem would be a good idea.

It's worth noting that if we would prepare such simple logic, it will work with any object that responds to call and messages. I mean, one would provide an arbitrary type, e.g:

class Validator
  def self.call(params)
    Result.new
  end

  class Result
    def messages
      {}
    end
  end
end

p Validator.call({}).messages
 # => {}

Isn't it cool? :-)

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2016

I knew that dry-v will provide features like "conditional" validations at some point, but until today, that wasn't available so I designed a generic interface which allows this pretty cool feature with AM:V, too.

Also, I am more than happy to remove things from Reform in favour of dry-v, but this will cause pain for people still using AM:V. We should find a nice transitional solution, even though I personally am zero interested in supporting anything Rails anymore (it is a waste of time). 😬

As for the form dependency - we should provide that per default using the new API, this is very commonly needed (at least that's what I remember). Will that still cause a recompilation of the rules? Excuse my ignorance, but any summarising pointer here highly appreciated!

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2016

@blelump Thanks so much for working on this, BTW! I am pretty confident that with yours and @solnic's help we can push a new Reform/Dry-v integration pretty easily.

Maybe we could really "deprecate" (move to separate gem) the validation groups, etc. since Dry-v will do this for us. People can then smoothly learn the new API without any real trouble.

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

@apotonick would you be up to have only dry-v as default option for Reform? And then the separate reform-am-v gem that could be used to add basic support for AM:V. I don't think we should support validation blocks for AM:V, if people really need conditional validation they can either hack it with if: or just use dry-v.

@solnic

This comment has been minimized.

Copy link

commented Mar 15, 2016

As for the form dependency - we should provide that per default using the new API, this is very commonly needed (at least that's what I remember).

Why is that? A schema, typically, should receive its external deps (if any are needed) during first initialization. There should be no need to inject form object into schema. In fact - schema should be called prior creating a form object...dry-v's philosophy is to validate (and potentially sanitize and/or coerce) input prior creating any objects using that input as the state.

Will that still cause a recompilation of the rules? Excuse my ignorance, but any summarising pointer here highly appreciated!

Rules are recompiled every time you do schema.with(...) to inject additional objects into a schema.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

Hi guys !

I think I'll handle it, however today I've got into an issue.
Before valid? is called (Reform::Validation), twins are getting populated and in case of dry-v I believe it should be the opposite. First, we perform validation and then Twin(s) population appears (see also summary of this post).

Essentially I've got into this issue because of nested data, e.g:

property :student do
    property :user do
      property :email
    end
  end

and it gets deserialized into {"phone_number"=>"+48 232343242234", "foreign_student"=>#<#<Class:0x000000039aae90>:0x00000004359b3... so it's not a params hash. I recall this, because here you explictily pass @fields (got in real troubles while trying to find where it's instantiated and finally used ObjectSpace to localize it – hello disposable 😄 ) and hence, dry-v crashes for nested schemas. Besides, we do not have any spec scenario for that 😄 . So I've used to_nested_hash and replaced the @fields variable. blelump@3f6744c#diff-6eacdc20d4fdded3689e3cf714969cbfR36

To sum up, I think before considering the topic of this PR, we need to deal with Reform internals and probably move Twin population after validation. Do you see any potential issues with this? I know that there're use cases where data validation is simply not enough, but then I believe, if user would need either a twin or a model, he can instantiate that on demand. Besides, how such changes are related to AM:V?

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

If I'm not wrong reform uses twins and disposable to coerce the values. This probably could be done with dry-v or one of the other dry-rb gems. But this would require some reworking on other core parts of Reform.

Right now nested hashes doesn't play nicely between reform and dry-v because each nesting is basically a new reform form object. Also reform still uses Virtus for coercion, which @solnic himself is not supporting anymore. We could try to use dry-values here also.

Or leave everything else as it is and have dry-v just do validation, even in a not so good manner.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2016

@mrbongiolo , yep, inferring type coercions is provided by Disposable::Twin and Virtus internally (currently it has a successor called dry-types), however I wouldn't touch it, at least for now.

Right now we should focus on dry-v and how to provide all of its features with minimal impact. Besides, we need to find out how to deliver dry-vlogic that allows one to inject an arbitrary dependency, e.g. having such schema:

validation Dry::Validation.Schema do
        key(:email).required(:email?)

        configure do
          option :email_check

          def email?(value)
            email_check.(value)
          end
        end
end

how to inject email_check. Here's a complete example.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

Hi guys,

@solnic just released dry-v 0.7 👏 , so we can start dealing with it officially.

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2016

@solnic I agree with you, but in real life, validations in a "Rails" application (let's call it "MVC") will often need dependencies, e.g. the current user, which is why I like your injection mechanism.

In my understanding, a validation verifies a deserialised object graph. In your philosophy it seems to be more of a "formal" validator, is that about right? The cool thing is our two different views on that still work together because dry-v kicks ass. 😉

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2016

@apotonick , @mrbongiolo ,

the more I'm using this, the more issues appear 😞 . In particular:

  • I cannot use dry-v coercions, because the validation result is not populated in twins.
  • I cannot use virtus coercions either, because it infers coercion right before dry-v performs and therefore dry-v complains about smth is not a String type. Quite obvious, but annoying...

My proposal is to re–orchestrate Reform so in the end:

  1. dry-v validation is performed first – no twins exist at this state.
  2. Twins are populated with the output data from dry-v. If one used dry-v coercions, twins are populated with appropriate types. Obviously, twins might be populated with invalid data, so that one is able to re–render the form with errors or perform any "rescue" operation is needed.

More TODOs:

  • improve dry-v error parsing, because now schema might contain another schema ([and another]), which in the end result with output smth like {a: {b: ['is missing']}}. This is currently not covered.
  • keep support for AM:V and move AM:V related logic somewhere (new gem would be a best fit?)

Do you guys have any caveats with that? In case of re–orchestration there's a lot work to do so any help is appreciated.

@solnic

This comment has been minimized.

Copy link

commented Mar 22, 2016

@blelump what's the responsibility of twins?

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

@solnic ,

the "twin" representation exists in Reform, because of these things:

  • The underlying Reform validation feature bases on AM:V. As we know, AM:V validates the objects itself so the purpose of the 'twin' representation is to validate possibly invalid objects against defined AM:V rules. If such representation is valid, you can persist its state to the original models (by simply calling twin.save). The twin idea is implemented in the disposable gem.

  • If validation fails, you're able to present invalid form, e.g. when provided data does not match validation schema. Most of the "popular" form renderers from the Rails world present an object graph ('twin" representation in Reform case). It would be diffcult to do that without such representation.

    note:
    even having dry-v validation step as the first step of the process, you'd need to create such twin representation to let one render invalid form. Another (but similar?) approach is presented in formalist gem, but I am not sure (at least by now), how one would persist the form data with the formalist approach (including collections, relations etc) without the deserialization process (in Reform case, deserialization creates twins of models).

Update

The 'twin' representation not only deals with persistence. Because every twin has explicit schema definition, one is able to decorate twin with a third party library, that does smth with the twin properties without touching the model at all, eg. consider a state machine that deals with state property. The model doesnt need to know any logic behind SM, it only stores the state whereas twin deals with the rest

@apotonick , please correct me if I missed something.

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

@apotonick can you accept this PR? It doesn't fix all things that we're discussing, but it adds the necessary support for dry-v 0.7+

After that we can work on another release that changes the way they both interact and hopefully remove and discard more active-stuff.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

@mrbongiolo , after a while, I personally don't feel comfortable with this PR (at least in the current form) :-) , even though it introduces 0.7 support.

First let me clarify where are we now:

  • This PR would introduce even more mess, because despite the new DSL works (partially), it completely doesn't address coercion problem. Moreover, virtus coercions will not work too, because we use DryV::Form which explictly expects string primitives. We could change it to a DryV::Schema, but then all keys need to be coerced into symbols. We could ask @solnic about another input sanitizer, but I feel this is just wrong, because we try to adjust something, that doesn't fit, because of how it's been designed.
  • Even though we would make the validation process first (before deserialization), it still needs to create invalid objects after validation (no matter if succeeded or failed), because most of the form renderer libraries expect object tree to render invalid form.

What can be done with minimum effort: # it also covers validate each form as a separate hash of primitives

Summary:
Minimum effort assumes that the whole process stays the same, which means that deserialization happens first. We also do not abandon the validation(&block) approach, because for Reform design it fits better.

Things to complete before switching to 0.7:

  • Ensure coercions work with dry-t (note I don't mention dry-v).
    TODO:
  • introduce dry-t and get rid of virtus for coercions.
  • ensure dry-v eats hash of primitives, even for complex object graphs (achievable with Twin#to_nested_hash method).
    Comment:
    You may think that's stupid, because first we deserialize and then serialize again, but still I vote for it, because as an alternative we've got validation of each nested structure as a separate invocation, which I discourage. Besides, it's more consistent to have validation schema in one place and for one object graph.
  • Improve error handling for nested schemas. Right now it works only for a flat schema (without any nested data).
    TODO:
    • parse error output and adjust to Reform needs.

What we get with such approach:

  • dry-v gets a hash of primitives (already coerced) and validates them against schema rules. As a result of dry-v job, we get only validation result (status + errors) without any coercions;
  • dry-v schema is fully supported (!), including with option, which might be helpful for users to inject extra deps;
  • cons: we introduce extra boilerplate, because coercions have to be defined in a twin schema, e.g: property :id, type: Types::Strict::Int

We get almost fully featured dry-v (without schema inheritance). Would you guys agree? // cc: @apotonick , @mrbongiolo , @solnic (?)

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

I was going to write something on why we could stick with flat schema and such for reform-v2.1.x and dry-validation-v0.7.x, but the changes required to better integrate both of them doesn't seem so hard and are good enough for a version bump on reform.

AFAIK coercion happens on disposable, so we should tackle there the use of dry-t instead of virtus.

What happens if a coercion fails when populating the twins? Should it go to the errors object?

If you only validate the data after creating the twins you can't enforce Strict Types, or am I wrong here?


On Reform itself we need to change the way on how all the validation occur and how it call each integration style (AM:V or dry-v). Reform already has a basic errors object, but we need to improve it a bit (it's required by those rails form rendering libs) so that Reform::Dry or Reform::AMV will in the end parse the errors from each validation library to Reform::Errors (not that hard to do).

This way we can simplify validation(&block), it should perform any group stuff, or anything related to that, it would just hold the form validations, dry-v or AM:V style, the user choose.

When using with dry-v we would have a single validation(&block) for the entire form (QUESTION: does this work for collections?).

When using with dry-v we would have one validation(&block) per nested property, it's ugly, but AM:V doesn't work other way.

I would also propose to remove all AM:V related sugar code from the Form itself and only set your validations inside the validation(&block). This way we can remove the :validate option from the property also. It's quite nice to have those things there, but the mess they create behind is not worth it. Better have everyone defining validations in a single point: validation(&block).

The entire Reform form would be simplified, as it doesn't have to worry about dumb AM:V messing around it scope.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

@mrbongiolo , let me clarify.

AFAIK coercion happens on disposable, so we should tackle there the use of dry-t instead of virtus.

👍

What happens if a coercion fails when populating the twins? Should it go to the errors object?

Nah, that would be a mess. We just need to infer a non strict coercions.

If you only validate the data after creating the twins you can't enforce Strict Types, or am I wrong here?

Yes, you're completely right!

When using with dry-v we would have a single validation(&block) for the entire form (QUESTION: does this work for collections?).

Yes and this is the way I'd like to handle it. I mean, the validation schema is consistent and defined in one place, instead of being scattered in every place of the twin schema.

When using with dry-v we would have one validation(&block) per nested property, it's ugly, but AM:V doesn't work other way.

We're able to have just one block for the entire form, by leveraging dry-v features, e.g:

params = {user: {name: "Bob", pets: [{name: :cat}, {name: :tiger}]}}


validator = DryV.Schema do
  key(:name).required
  key(:pets).each do
    key(:name).required
  end
end
validator.call(params[:user])

I would also propose to remove all AM:V related sugar code from the Form itself and only set your validations inside the validation(&block). This way we can remove the :validate option from the property also. It's quite nice to have those things there, but the mess they create behind is not worth it. Better have everyone defining validations in a single point: validation(&block).

The entire Reform form would be simplified, as it doesn't have to worry about dumb AM:V messing around it scope.

Sure, but would you agree to separate these tasks? Therefore we would be able to put 0.7 into production faster and then consider how much AM:V related logic is intented to be thrown out :-) .

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2016

@blelump I sure agree to separate those tasks.

@apotonick said on gitter chat if we could make dry-v work both ways, with a single schema or one per form. It shouldn't be that hard.

I already sent a PR migrating disposable to dry-types. Now we need a bit of a rework on Reform::Errors, so that dry-v can populate the nested errors correctly. Even using nested validations on a single schema, in the end you need to populate each form's twin @errors instance. Maybe we would like to provide nested @errors directly on the parent form also, what do you think?

After that we can discuss, if needed or wanted, further integration and bigger changes on Reform.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2016

@mrbongiolo , in case of "flat" and "global" approach, we need one "global" error object, so we would be able to put erros from within both validations. As a result, we're expecting errors gathered from all sub–validations.

@mrbongiolo , could you try to identify which parts of the Reform code is related somewhat to AM:V and hence we would start thinking what to throw out or at least prepare a list of tasks to do?

Thanks for helping with all of this ❤️

@mrbongiolo

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2016

@blelump I'll look at it today, lets see if we can come up with a task list to get this going.

@blelump blelump force-pushed the blelump:new-dry-validation branch from 3f6744c to 5401516 Apr 1, 2016

@blelump blelump force-pushed the blelump:new-dry-validation branch from 5401516 to 4c27de7 Apr 1, 2016

@blelump blelump force-pushed the blelump:new-dry-validation branch from 4c27de7 to f6ce2c0 Apr 1, 2016

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2016

I merged this into https://github.com/apotonick/reform/tree/2-2 - let's get it running now!

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2016

@apotonick , @fran-worley, @mrbongiolo , specs passes.

Please review 778a357 and drop me a line in case of any caveats.

Edit:

I've added support for dynamic dependencies to let user decide whether a dependency injection is needed. See f60f9d5 .

@blelump blelump force-pushed the blelump:new-dry-validation branch from 14acdcf to f60f9d5 Apr 2, 2016

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2016

@blelump Thanks, I am struggling to understand the intention of 778a357, though.

@@ -95,7 +97,7 @@ class AlbumForm < Reform::Form
end
end

validation :default do
validation :default, with: { title: -> { title }, guitar: Guitar } do

This comment has been minimized.

Copy link
@apotonick

apotonick Apr 2, 2016

Collaborator

Is that a dry-v feature? And where do you need this? Isn't that all nicely available via the form instance?

This comment has been minimized.

Copy link
@blelump

blelump Apr 3, 2016

Author Contributor

@apotonick , as @solnic said, injecting form instance every time costs us rebuilding the whole schema. Here's the implementation . That's why I've prepared a logic that let's one decide, whether an arbitrary dependency is needed.

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2016

Ok, everyone, I've played with this and came to the following conclusions (objections welcome!):

Nested Validations

We should allow users both styles of validations, one-for-all and per-form.

One-for-all

Here, all validations go into one schema.

    validation :default do
      key(:user_id).required

      key(:variants).schema do
        each do
          key(:id).required
        end
      end
    end

Per-form

Here, each nested form has its own schema, kinda the way it is now.

Errors

We then have to compile errors objects for each form, so the current error API remains the same (because it is super helpful).

This will probably imply some work, but I don't see why we wouldn't allow both (or am I wrong here?).

The speed problem when having to compile the schema every validation I gladly tolerate and leave it up to our smart @solnic - I'm sure we will find a way to speed this up. This shouldn't be a blocker!

Custom High-Level Predicates

I have one problem with dry-v, though, I couldn't figure out how to have high-level predicates that are unrelated to any input. For example, I want to make sure the products array is max. 5 items when user not logged in. We really have to find a way for those validations as they are a fundamental concept of Trailblazer and validating a deserialized object graph.

AM::Stuff and Groups

For now, we can leave Reform's groups as they can be helpful. I find them less clumsy than the way dry-v allows "chained validations". Once this is simpler in dry-v, we can remove this feature.

Speaking of removing, I would remove all AM::V related code and throw it into reform-active.

@blelump

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2016

We then have to compile errors objects for each form, so the current error API remains the same (because it is super helpful).

This will probably imply some work, but I don't see why we wouldn't allow both (or am I wrong here?).

Yep, I agree and that's the intention. I mean, provide errors for each declared validation block, so either a flat or nested validation style is used, they both work.

The speed problem when having to compile the schema every validation I gladly tolerate and leave it up to our smart @solnic - I'm sure we will find a way to speed this up. This shouldn't be a blocker!

Uhm, according to the whole discussion above, the only one speed issue is rebuilding schema each time, when using with method. All the remaining cases we cover here, is just using plain old dry-v features or am I wrong here?

@solnic

This comment has been minimized.

Copy link

commented Apr 3, 2016

I have one problem with dry-v, though, I couldn't figure out how to have high-level predicates that are unrelated to any input. For example, I want to make sure the products array is max. 5 items when user not logged in. We really have to find a way for those validations as they are a fundamental concept of Trailblazer and validating a deserialized object graph.
AM::Stuff and Groups

This is a missing feature in dry-v, specifically the ability to use a predicate which doesn't rely on any input. I'll add this in next release, so you'll be able to do:

rule(min_products: [:products]) do |products|
  not_logged_in?.then(products.max_size?(5))
end

For now, we can leave Reform's groups as they can be helpful. I find them less clumsy than the way dry-v allows "chained validations". Once this is simpler in dry-v, we can remove this feature.

We don't have an equivalent of reform's groups. The high-level rules feature is something that could be used as the base for grouping things, I plan to add this feature under the name of "safe-points" or maybe "groups" too.

As far as clumsiness goes - there are a couple of improvements I can make to make rule API less verbose, I was more focused on its behavior than ease-of-use in terms of syntax. This is a very powerful feature, being able to explicitly define which values need to be valid for a given rule to be applied is extremely useful.

@solnic

This comment has been minimized.

Copy link

commented Apr 3, 2016

Uhm, according to the whole discussion above, the only one speed issue is rebuilding schema each time, when using with method. All the remaining cases we cover here, is just using plain old dry-v features or am I wrong here?

Please just ignore this for now. I will make it better somehow and you should not be blocked by this in any way.

@apotonick

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2016

Oh yeah, I love the rules feature in dry-v, @solnic, I just find it a bit verbose, but I prefer having a clear and verbose API before abstracting that to a nicer DSL, so, all good.

Cool, I can see light at the end of the tunnel, and how I will never ever have to use Active* again! 😬

@fran-worley

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2016

Closing as a merge was finally agreed upon cd7c3b3

Not everything discussed here has been fully addressed but if these are still a problem then they should be raised as separate issues.

@fran-worley fran-worley closed this Jul 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.