Update attribute validations

[alimi]

This PR addresses the first few things outlined in #126.

• [x] Rename Magma::Attribute#match to validation
• [x] Create validation classes for IntegerAttribute, FloatAttribute, DateTimeAttribute which use the @validation parameter to validate ranges for these values.
• [x] Move Magma::Attribute::Validation to Magma::StringAttribute::Validation
• [ ] Investigate if other validations make sense for other types.
• [ ] Add a validation json column to populate the attribute.

It doesn't add validation to the attributes table, but it takes a step in that direction by making the Ruby validation definitions hashes/JSON. For example, regex validation on a string attribute looks like

https://github.com/mountetna/magma/blob/422807062c90a2bea248a3d48b8ef26468832c8b/spec/labors/models/monster.rb#L6

I did this to lay the foundation for moving it to the attributes table. I also wanted to get feedback on the way things look now before we take that next step.

I also haven't looked to see if we should add more attribute validations.

[graft]

This is some nice work. Two remarks:

1) Since there is so much type checking of validation_object in the Validation class, you could pass the entire @validation parameter into the validation class instead of using validation_arguments and validation_object in Magma::Attribute.

2) There is a nearly-identical usage of this validation format here: https://github.com/mountetna/magma/blob/b9ce3a1028847ece9239d841e5fe0236a0ee8024/lib/magma/dictionary.rb#L46 - it might be nice to use a single class for both of these formats.

[alimi]

1) Since there is so much type checking of validation_object in the Validation class, you could pass the entire @validation parameter into the validation class instead of using validation_arguments and validation_object in Magma::Attribute.

I like this idea, but there are a few places outside the validation classes that need to know what the validation object is:

https://github.com/mountetna/magma/blob/422807062c90a2bea248a3d48b8ef26468832c8b/lib/magma/attribute.rb#L42-L43

https://github.com/mountetna/magma/blob/422807062c90a2bea248a3d48b8ef26468832c8b/lib/magma/attributes/matrix.rb#L74-L82

https://github.com/mountetna/magma/blob/422807062c90a2bea248a3d48b8ef26468832c8b/lib/magma/query/predicate/matrix.rb#L23-L25

https://github.com/mountetna/magma/blob/422807062c90a2bea248a3d48b8ef26468832c8b/lib/magma/query/predicate/matrix.rb#L36

I'm not sure it makes sense to push that logic into the validation classes when we need it elsewhere.

[graft]

I think you are right about not hiding the object inside the validation class. How about a ValidationMatcher class to wrap that { type, value } format? So you could do:

 options: validation_object.options, 
 match: validation_object.match, 

and

validation_object.matches?(value)

and probably reuse this in MatchAttribute.

[alimi]

Magma::MatchAttributes have to have a type, right?

The validation will error for Magma::MatchAttributes that don't have type.

https://github.com/mountetna/magma/blob/91f8d3a0056a4354958b7a4af9ff308b7905c37a/lib/magma/attributes/match.rb#L11

But Magma::Dictionary#json_match has a fallback in case type isn't set.

https://github.com/mountetna/magma/blob/91f8d3a0056a4354958b7a4af9ff308b7905c37a/lib/magma/dictionary.rb#L49-L58

I'm guessing the else clause doesn't ever get executed because the validation won't allow type to be nil. Is that true?

[alimi]

Actually, it looks like the else clause catches strings?

[graft]

You seem to be correct about { type: 'String' }. However, this makes me cognizant of how I am probably leading you astray sending you after the MatchAttribute, because I don't really know the answer! I implemented this and it is available, but we never wrote a dictionary for the ipi project, so it's difficult to know what the actual usage would look like. Since it's not being used in production it seems low-value to work on it; also, we'll be forced to re-encounter this if we actually want to add dictionary validation back to the models via the API, at which point we might unify MatchAttribute with the attribute validation interface.

[alimi]

Yeah, the else clause catches strings and numerics. I came across them in spec/validation_spec.rb.

Is there anything else to work on for this PR or can we ship it? I'm thinking to do add the json column to the attributes in another PR so #128 doesn't get blocked.

[graft]

Looks like it needs a rebase; i still think a matcher class to wrap the validation format has value for this PR, but we should restrict scope as much as possible, I'm getting frightened by all the feature dev creeping into the migration roadmap.

[alimi]

Ok, I added Magma::ValidationObject in https://github.com/mountetna/magma/pull/135/commits/5dac6ac2a50adb983d3f958328a6722057af4baf. There's a different Magma::ValidationObject class for each of the validation types (Magma::ArrayValidationObject, Magma::RangeValidationObject, etc.).

https://github.com/mountetna/magma/blob/5dac6ac2a50adb983d3f958328a6722057af4baf/lib/magma/validation_object.rb#L1-L92

It's nice to have all that logic in one place. It also helps us eliminate a lot of the type checking that was going on. I added Magma::NullValidationObject to get us away from some nil checks too.

I think it makes sense to leave MatchAttribute as is for now.

[alimi]

I also brought back proc support for regex validations in https://github.com/mountetna/magma/pull/135/commits/9a241a0019988f374845e508ecbeaab6c20aded3 so the IPI project can continue to build regexes with database values.

[graft]

Looks nice! Is it possible to remove the empty class Validation < BaseAttributeValidation now (since I think it should default to BaseAttributeValidation)? Otherwise seems fine to :shipit:

The proc support is nice since otherwise we would be breaking the validation in production and would have to amend it. Going forward (adding the json column to the db) I think we'd have to abandon this, though?

[alimi]

Ah, good call! I removed the empty validation classes in https://github.com/mountetna/magma/pull/135/commits/1e33bbe60bed584a8e03acb9e0248030f7420348.

Yeah, I don't think we'll be able to support procs in the attributes table but we can deal with it when we get there.

[alimi]

I pushed up a test fix in https://github.com/mountetna/magma/pull/135/commits/5425659434673d108956f1ed1abee71fc6594226. I accidentally made the validation spec flaky, but it's fixed now.