Dj/validation using blocks and procs

[drujensen]

An alternative approach to support Blocks and Procs.

The main difference is that you need to pass in a parameter to the Block:

validate :name, "can't be blank" do |user|
  !user.name.to_s.blank?
end

validate :name, "can't be blank", -> (user : User) do
  !user.name.to_s.blank?
end

This also removes the majority of the macro definitions.

If we agree on this approach, I will add separate validation specs.

[robacarp]

There's a lot of code flying around today regarding validators, so I want to pitch in what I have been using. The block/proc thing wasn't working for me and I didn't want to take the time to look into it so this is what I came up with. It's not always concise but it does have some things going for it.

I'm not proposing this is what is implemented, just showing some code that could be used as an example.

class Granite::ORM::Base
  def validate : Nil
  end

  def valid? : Bool
    clean_errors
    validate
    errors.none?
  end

  def clean_errors
    errors = [] of Error
  end

  def add_error(field : Symbol, message : String)
    errors << Error.new(field, message)
  end
end

class Domain < Granite::ORM::Base
  def validate : Nil
    blank_name = true
    if name = @name
      blank_name = name.blank?
    end

    add_error :name, "cannot be blank" if blank_name
    return if blank_name

    malformed_name = false
    if name = @name
      malformed_name = ! name.index("/").nil?
      malformed_name ||= name[0...4] == "http"
    end

    if malformed_name
      add_error :name, "should be the DNS name to be checked. For example: google.com instead of http://google.com/gmail"
    end
  end
end

As you can see I'm running several checks on the same field.

The major pain point here is the need need to check if the field exists manually name = @name before asking about it's formatting because Crystal doesn't like it if you call #index on nil. Perhaps someone with more crystal typing chops can do some magic for a not_nil! getter. Perhaps #91 would solve that.

However, it also has the benefit that the validate method lives in instance-space instead of macro-space. So it's possible to do things like this admittedly contrived example:

def validate : Nil
  case type
  when "subdomain":
    validate_subdomain
  when "ip_address":
    validate_ip_address
  else
    validate_domain
  end
end

The Rails model validations have if: and unless: predicates stuffed into them and it can be hard to write maintainable validations out of that stuff. It's really easy to make a mess.

[faustinoaq]

@eliasjpr Should we implement this on param validators too?

[drujensen]

@robacarp thanks for the information. Hopefully the changes we are making will help.

@amberframework/core-team @amberframework/contributors This is ready for review. This will address the breaking change introduced and fix Amber specs.

This also removes the macros for validation and uses methods instead.

[robacarp]

doesn't this recreate the "Error on one instance, error on all instances" bug?

[faustinoaq]

I tested it and surprisingly works without that issue 😅

[eliasjpr]

@eliasjpr Should we implement this on param validators too?

We can extend the params validators to support validation for a Granite::ORM::Model, I don't think it will take a lot of effort from what I can see. We did discuss last night of the possibility of normalizing validators

[marksiemers]

@drujensen - Since this fixes the currently failing amber specs, as long as it doesn't introduce a new bug, I say merge it and release.

@robacarp

The major pain point here is the need to check if the field exists manually name = @name before asking about its formatting because Crystal doesn't like it if you call #index on nil. Perhaps someone with more crystal typing chops can do some magic for a not_nil! getter.

There is a try method in crystal, and it is not frowned upon using it (at least not by RX14). When called on nil, it just doesn't yield to the block.

Perhaps this would clean things up a little bit:

class Domain < Granite::ORM::Base
  ERROR_MSG = {
    dns: "should be the DNS name to be checked. For example: google.com instead of http://google.com/gmail",
    blank: "cannot be blank",
  }

  def validate : Nil
    (add_error :name, ERROR_MSG[:blank]; return) if @name.try &.blank?
    (add_error :name, ERROR_MSG[:dns]) if name.try { |name| !name.index("/").nil? || name[0...4] == "http" }
  end
end