validates_serialized icon indicating copy to clipboard operation
validates_serialized copied to clipboard

embedded validates with "if"-Proc don't receive model_instance

Open mjrk opened this issue 11 years ago • 6 comments

If I have a nested validation:

serialize :data, Hash
validates_hash_keys :data do
  validates :url, presence: true, if: Proc.new{|f| f.required_data_field?(:url) }
end

The f in the Proc won't be the model instance as usual, but a ActiveModel::Validations::HashBlockValidator::TempValidateableHash_. I could not find a way in the source to access the model_instance form this block validator, however, usually you will need to access the model for conditional validations.

mjrk avatar Jan 27 '15 16:01 mjrk

Btw. thanks for you great work!

Edit: I could work around the problem by modifying ActiveModel::Validations::HashBlockValidator and the ValidateableHash (adding attr_accessor :record, passing record to it in initialize), so that the record is passed to the ValidateableHash. However, maybe there is a more common solution.

mjrk avatar Jan 27 '15 16:01 mjrk

Hrm. I'll take a look at this this afternoon; That TempValidateableHash class was a sloppy workaround for the fact that ActiveModel 3.2 doesn't have some of the functionality of Activemodel 4.0.

brycesenz avatar Jan 27 '15 17:01 brycesenz

@mjrk - Actually, upon re-reading your question, I think I disagree with you on what the behavior of your syntax even should be. I would expect in the code you provided -

serialize :data, Hash
validates_hash_keys :data do
  validates :url, presence: true, if: Proc.new{|f| f.required_data_field?(:url) }
end
  • for the f in the Proc to actually refer to the serialized Hash that you've named "data". That hash is the object being acted upon in that block, so in this case, you'd be calling required_data_field?(:url) on that Hash object.

Do you agree with that assessment? If so, then we need to figure out what syntax makes the most sense for passing in the original model and referencing that model within the block

I would think that we'd want to allow a syntax like:

serialize :data, Hash
validates_hash_keys :data do |model|
  validates :url, presence: true, if: Proc.new{ |model| model.required_data_field?(:url) }
end

But I'm curious to get your thoughts.

brycesenz avatar Jan 27 '15 17:01 brycesenz

Well, actually I didn't make up my mind what the behaviour should be. I expected f to be the model instance, BUT I worked around the problem by passing the record to the HashBlockValidator. It seemed to me that I had to dig deep in rails' callback chain to get the callback called with the model_instance.

In general, to answer the question for myself, first I had to ask: In the future, do I expect the ValidateableHash to become a "full featured class" with some useful methods or will it remain a temporary wrapper? As I can access the hash through the model, I can hardly think of useful methods.

However, not sure if I got your second syntax right because twice there is model. Or did you mean:

validates_hash_keys :data do |model|
  validates :url, presence: true, if: Proc.new{ |validateable_hash| model.required_data_field?(:url) }
end

I like the idea giving access to both. But wouldn't this imply to have to execute the validates_hash_keys&block block every time? This makes me think of undesired side effects - i.e. you cannot statically query for required fields etc.

mjrk avatar Jan 27 '15 18:01 mjrk

Oops, yes, I did mean the syntax that you corrected. Apologies for the confusion!

I'm not sure I understand your hypothetical undesired side effect; can you give me an example of something that you think would break/behave unexpectedly if we supported that new syntax?

brycesenz avatar Jan 27 '15 19:01 brycesenz

I don't know the implementation details. I just thought about validates being a class method that would be called more than once etc., however, maybe it's not like this.

mjrk avatar Jan 27 '15 21:01 mjrk