[RFC] Remove validations

[Blacksmoke16]

The current implementation of model validations is less than ideal. I.e. #123, #238. As I see it we have two options:

1. Try to come up with a similar API to the current validation implementation that avoids the issue with inheritance
2. Remove validations, either in favor of another shard or a reimplementation of it in a better way.

Based on the start of some of the conversation around amberframework/amber#698 a possible solution would be to use https://github.com/Blacksmoke16/assert https://github.com/athena-framework/validator as an optional/recommended dependency. Granite itself could extend assert in order to provide built in reusable assertions specific to data model validations, for example:

NOTE: This example is for the legacy version of Athena Validator, but the concept is the same.

# exists_assertion.cr
@[Assert::Assertions::Register(annotation: Assert::Exists)]
class Exists(PropertyType, Model) < Assert::Assertions::Assertion
  initializer(
    actual: PropertyType
  )

  # :inherit:
  def default_message_template : String
    "'%{actual}' is not a valid %{property_name}."
  end

  # :inherit:
  def valid? : Bool
    Model.exists? @actual
  end
end

# post.cr
class Post < Granite::Base
  connection pg
  table posts

  ...

  @[Assert::Exists(User)]
  column author_id : Int64
end

Which would call the .exists? method (#343) on the given type and would only be valid if there is a user with the value of author_id; which would be useful when, for example, creating a post from form data or JSON.

The other benefit would be enabling inheritance to be used. I would have to do some testing to see how it actually would work (I imagine there will be some work required to make MTI/STI possible), but would at least prevent compile errors until then.

The main con is of course it would break existing validations and would prevent using Amber with ORMs from using assert assertions, if the other ORM does not support applying annotations to their columns.

Thoughts?

[Blacksmoke16]

Any thoughts on this @robacarp @drujensen ?

[robacarp]

I’m ... still, I guess... not really warm to the idea of annotations as a mechanism for general development.

More on topic, fixing the inheritance problem is certainly important, but I think it’ll need more than just Validations, it’ll probably require pulling out of depending on hooks entirely... both finished and inherited

I think it’s important for Granite to ship with validations built in, but I agree that a substantial refactor is needed to get something which is scaleable in its implementation.

[drujensen]

so I am wondering if @Blacksmoke16 can do his magic and support both the assert gem and then add macros to inject the annotation. This is how the mapping is working. That way you can use the annotation if you want or you can use the macros that wrap them. Can this be done?

[Blacksmoke16]

but I think it’ll need more than just Validations, it’ll probably require pulling out of depending on hooks entirely... both finished and inherited

We don't have any finished macro hooks left. The only inherited hooks left are for validations, callbacks, and base. However with the direction it seems we're going in #370, we probably won't need the macro inherited in base anymore anyway.

I think it’s important for Granite to ship with validations built in

support both the assert gem and then add macros to inject the annotation

I think ideally the interface that granite uses should be generic enough to allow for using whatever you want to use, be that assert, the built in Granite validations, or another shard; assuming it implements the same interface.

I'm pretty sure assert would already just inherently work since the current implementation uses valid? which it would override.

But, maybe? The main question would be if you do like

@[Column]
property email : String

@[Assert::Email]
@email : String

where the ivar would have each annotation applied. However, IMO it would be easier to just tell people to add the annotation.

@[Assert::Email]
@[Assert::NotBlank]
column email : String

vs

column email : String

assert Email, :email
assert NotBlank, :email

It also would be less than ideal that granite would have a custom wrapper to a third party shard.

[drujensen]

I prefer something a little more rails like:

column email : String

validate :email, format: email, presence: true, allow_blank: false

[damianham]

Which would call the .exists? method (#343) on the given type and would only be valid if there is a user with the value of author_id; which would be useful when, for example, creating a post from form data or JSON.

This is a referential integrity constraint and that is already available in mysql, postgres and sqlite as foreign key constraint.

@drujensen I prefer something a little more rails like too and would prefer something that didn't rely on annotations.

[drujensen]

@damianham It looks like crystal is embracing annotations so not sure we have many options. The inherited or finished hooks cause other issues like you can't inherit from a Granite Model and they seem to be frowned upon.

I was just hoping to put a little lipstick on the pig.

[Blacksmoke16]

@damianham

This is a referential integrity constraint and that is already available in mysql, postgres and sqlite as foreign key constraint.

It was more so meant to be an example of how the shard could be easily extended to support custom assertions. The point I was trying to show was related to amberframework/amber#698. I.e. it would allow you to share validations of when you go to save a model AND when validating input from a user; say when deserializing a JSON POST body, or when the form data is submitted.

Currently Granite and Amber use two separate validations systems when it could all be tied into one.

I prefer something a little more rails like too and would prefer something that didn't rely on annotations.

It looks like crystal is embracing annotations so not sure we have many options.

Again, I'm not trying to say use my library because I said so. The options are:

1. Keep the current validation implementation but refactor it to support inheritance and possibly tie in with Amber validations.
2. Drop the current implementation to unblock progress on inheritance. Either adopting an already made shard, or reimplementing the current DSL in a different way.

[drujensen]

@Blacksmoke16 Just to clarify, I wasn't calling your Assert library a pig, I was calling the annotations syntax a pig. I think your code is absolutely beautiful and would love it if we could leverage the Assert shard.

Option 2 is where we want to go. Just wondering if we can do it without forcing the user to use the annotation syntax.

[Blacksmoke16]

Just to clarify, I wasn't calling your Assert library a pig

I know, all good :P...oink.

Just wondering if we can do it without forcing the user to use the annotation syntax.

I'm sure it's possible, I just don't think it would be trivial. If someone wants to take a stab at it and gets something working id be happy to review it.

[eliasjpr]

See https://github.com/eliasjpr/schema as a possible approach that would not require annotations