Skip to content
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

Wangjohn validates associated uniqueness #8996

Conversation

wangjohn
Copy link
Contributor

Pull request in response to #4568. Uses jeyb's pull request and cleans up some code.

)

distinct_validations = collection.map{ |record|
record.attributes.select { |k,v| attributes.include?(k.to_sym) }.values.join.strip()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check to see if the strip is actually necessary?

…ject. Cleaned up the code from jeyb's pull request.
@wangjohn
Copy link
Contributor Author

Ok, I'm now using grep and have fixed the indent.

@schneems
Copy link
Member

You could clean up the protected method unique_for_nested_attributes? by pulling out some of the logic into other methods something like this:

def unique_for_nested_attributes?(collection)
  return true if collection.empty?
  uniq_validators = uniq_validators_for(collection)
  return true if uniq_validators.empty?

  validations = nested_validations(collection, uniq_validators)

  collection.length == validations.length
end

def uniq_validators_for(collection)
  validator = ActiveRecord::Validations::UniquenessValidator
  collection.first.class.validators.grep(validator)
end

def uniq_attributes_for(validators)
  validators.flat_map do |validator|
    [validator.attributes, validator.options[:scope]]
  end.compact.to_set
end

def distinct_validations_on_attributes(collection, attributes)
  collection.map do |record|
    record.attributes.select { |k,v| attributes.include?(k.to_sym) }.values.join("")
  end.uniq
end

def nested_validations(collection, validations)
  attributes = uniq_attributes_for(validators)
  distinct_validations_on_attributes(collection, attributes)
end

Though this is just an off-the cuff refactoring, you'll want to pick some better names for methods, and make sure none of the logic was lost in the translations (just run the tests). I also changed Set.new to call .to_set on the array, and changed the multi line block syntax from { to do/end and moved from map.flatten to a flat_map. Even after refactoring this, i'm not 100% sure how this logic works, commenting on individual methods could help.

Thanks a ton for submitting this, the more people with more knowledge of Active Record code, the better ❤️

@wangjohn
Copy link
Contributor Author

Actually, I realized the code I wrote will have a bug. Consider the following test case:

  def test_validates_associated_nested_attributes_uniequeness_with_scoping
    Topic.validates_associated(:replies)
    Topic.accepts_nested_attributes_for(:replies)
    Reply.validates_uniqueness_of(:title, :scope => :content)

    t = Topic.create("title" => "Programming", "content" => "For the masses")
    t.replies_attributes = [{"title" => "aa", "content" => "aa"}, {"title" => "a", :content => "aaa"}]

    assert t.valid?
    t.save!
    assert_equal 2, t.replies.size
  end

Because of the way I'm joining the values together, this test will fail. I'm going to close this PR and make a new one when I've got time to work on this more.

@wangjohn wangjohn closed this Feb 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants