-
Notifications
You must be signed in to change notification settings - Fork 914
[WIP] Add Attribute Constraints (validation) support for GenericObjectDefinition #23654
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
base: master
Are you sure you want to change the base?
Conversation
| props = properties.symbolize_keys | ||
|
|
||
| properties[:attribute_constraints] = props[:attribute_constraints].each_with_object({}) do |(name, constraints), hash| | ||
| hash[name.to_s] = constraints | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just properties.deep_symbolize_keys!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, stringify_keys!, but yes.
| end | ||
|
|
||
| def validate_property_attribute_constraints | ||
| properties[:attribute_constraints].each do |attr_name, constraints| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for properties[:attribute_constraints] to be nil here for existing generic_object_definitions unless we're also doing a data migration?
| errors.add(:properties, "constraint 'enum' contains duplicate values for attribute [#{attr_name}]") | ||
| end | ||
|
|
||
| # Existing type validation continues... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not a necessary comment
| before do | ||
| @god = FactoryBot.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use let(:generic_object_definition) (or let!(:...) if you need it to be eagerly defined) rather than introducing an instance variable in the spec context.
| :attribute_constraints => {}, | ||
| :associations => {'cloud_tenant' => 'CloudTenant'}, | ||
| :methods => ['kick', 'laugh_at', 'punch', 'parseltongue'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to want to align the => here (assume rubocop will warn about it if it hasn't already)
|
This looks great @putmanoj safe to say you can take it out of Draft and let specs run I think unless you are waiting on something else. |
|
The mixture of strings and symbols looks weird and pretty fragile, but it looks like that is the way the existing code is as well and is required to be that way |
| end | ||
|
|
||
| def apply_default_values | ||
| return unless generic_object_definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this line can happen, because you have to create a generic object from a generic object definition. I think we can remove it.
| if properties[attr_name].nil? && attr_constraints.key?(:default) | ||
| default_value = attr_constraints[:default] | ||
| # Type cast the default value | ||
| if property_attribute_defined?(attr_name) | ||
| properties[attr_name] = type_cast(attr_name, default_value) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a way to dry this up relative to _property_setter.
| :min_length => [:string], | ||
| :max_length => [:string], | ||
| :enum => [:integer, :string], | ||
| :format => [:string], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format is a weird name for a regex type. can we call it regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, in context it looks fine, I think. WDYT, @agrare ?
| end | ||
|
|
||
| def validate_enum_constraint(attr_name, attr_type, value) | ||
| unless value.is_a?(Array) && value.any? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but this is hard to read. Prefer:
| unless value.is_a?(Array) && value.any? | |
| if value.is_a?(Array) && value.empty? |
| - apply_discount | ||
| - validate_stock | ||
|
|
||
| # Made with Bob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this :)
| expect(god2_yaml.first["GenericObjectDefinition"]["description"]).to eq(nil) | ||
| expect(god2_yaml.first["GenericObjectDefinition"]["properties"]).to eq( | ||
| :attributes => {}, :associations => {}, :methods => [] | ||
| :attributes => {}, :attribute_constraints=>{}, :associations => {}, :methods => [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit
| :attributes => {}, :attribute_constraints=>{}, :associations => {}, :methods => [] | |
| :attributes => {}, :attribute_constraints => {}, :associations => {}, :methods => [] |
| :attribute_constraints => {:name => "invalid"} | ||
| } | ||
| ) | ||
| expect { testdef.save! }.to raise_error(ActiveRecord::RecordInvalid, /constraints for attribute .* must be a hash/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this error message test could be more explicit...I would expect something like
| expect { testdef.save! }.to raise_error(ActiveRecord::RecordInvalid, /constraints for attribute .* must be a hash/) | |
| expect { testdef.save! }.to raise_error(ActiveRecord::RecordInvalid, /constraints for attribute "name" must be a hash/) |
| :attribute_constraints => {:flag => {:min_length => 5}} | ||
| } | ||
| ) | ||
| expect { testdef.save! }.to raise_error(ActiveRecord::RecordInvalid, /constraint .* is not applicable to attribute type/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here with the error message.
| :attribute_constraints => {:name => {:invalid_constraint => true}} | ||
| } | ||
| ) | ||
| expect { testdef.save! }.to raise_error(ActiveRecord::RecordInvalid, /invalid constraint type/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably list for which property the constraint is invalid.
Summary
This PR implements comprehensive attribute constraint validation for GenericObjectDefinition, enabling developers to define validation rules for property attributes. The implementation supports all existing attribute types (boolean, integer, float, string, datetime, time) plus newly added Hash and Array types.
Changes
Core Features
Constraint Types - 12 validation constraint types:
Hash and Array Types - New attribute types with JSON storage:
Default Values - Automatic default value application:
Import/Export - Full YAML serialization support:
Usage Example
API Changes
GenericObjectDefinition
New Methods:
Enhanced Methods:
Example:
GenericObject
New Validations:
Constraint Reference
Backward Compatibility
✅ Fully backward compatible - Existing GenericObjectDefinitions without constraints continue to work unchanged. Constraints are optional and only enforced when defined.
Benefits
Checklist
Issues : #23625