hashie/hashie

Co-dependent Dash attributes break requirement validation

demsullivan opened this issue · 2 comments

Given this:

class TestDash < Hashie::Dash
  attribute :a, required: -> { b.nil? }, message: 'is required if b is not set.'
  attribute :b, required: -> { a.nil? }, message: 'is required if a is not set.'
end

I would expect that:

  • if a=nil and b=nil, I would see "a is required if b is not set."
  • if a is not specified and b=1, I would not see an error.
  • if a=nil and b=1, I would not see an error.

However:

>>> TestDash.new()
ArgumentError: The property 'a' is required if b is not set.
>>> TestDash.new(b: 1)
{:b=>1}
>>> TestDash.new(a: nil, b: 1)
ArgumentError: The property 'a' is required if b is not set.

In the last example, since b is set, I wouldn't expect to be getting a validation error on a. It's interesting that omitting a doesn't break it, but specifying a is nil does. Any ideas?

This sounds to me like it's an order-dependence bug. I bet if you re-order the attributes in the Dash so that b is first, you'll see the reverse behavior of what you're currently seeing. Could you try that to see if my hypothesis is correct?

If so, we should probably set all of the attributes before running the validations on the object. I haven't looked at the Dash code for a while, so that might be difficult.

Also, it would be great if you could submit a PR with a failing test. It's an easy way to get an open source contribution. 😄

Thanks so much for the quick response!

That's my thought too - I took a quick look at the Dash source and it looks like the attributes are being set before the validations are run, which I thought was kinda odd. If I have time this week(end) I might dig in and submit a PR to fix the whole issue and include a failing test... we'll see how the rest of the week goes :)

It definitely is an order-dependence thing though! Using the TestDash class above:

>>> TestDash.new(a: nil, b: 1)
ArgumentError: The property 'a' is required if b is not set.
>>> TestDash.new(b: 1, a: nil)
{:b=>1, :a=>nil}

Same applies if you re-order the attribute definitions within the Dash class.