From b03f5803071f5c205516b5a5c61adaad01047054 Mon Sep 17 00:00:00 2001 From: Gregor Schmidt Date: Tue, 28 Nov 2017 10:18:03 +0100 Subject: [PATCH] Disallow creating inverse relates issue relations Previously, it was possible to create a relates relations from issue1 to issue2 and another one from issue2 to issue1. Since the relates relation is undirected, this is effectively a duplicate relation. The DB uniqueness index would not be triggered, since the two relations are inverse. This commit adds an additional validation (to get proper error messages) and sorts the IDs before saving them to the DB (to get DB level uniqueness). --- app/models/issue_relation.rb | 10 +++++++++- test/unit/issue_relation_test.rb | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/app/models/issue_relation.rb b/app/models/issue_relation.rb index 44c7c8dba..0a47b1341 100644 --- a/app/models/issue_relation.rb +++ b/app/models/issue_relation.rb @@ -207,13 +207,19 @@ class IssueRelation < ActiveRecord::Base # Reverses the relation if needed so that it gets stored in the proper way # Should not be reversed before validation so that it can be displayed back - # as entered on new relation form + # as entered on new relation form. + # + # Orders relates relations by ID, so that uniqueness index in DB is triggered + # on concurrent access. def reverse_if_needed if TYPES.has_key?(relation_type) && TYPES[relation_type][:reverse] issue_tmp = issue_to self.issue_to = issue_from self.issue_from = issue_tmp self.relation_type = TYPES[relation_type][:reverse] + + elsif relation_type == TYPE_RELATES && issue_from_id > issue_to_id + self.issue_to, self.issue_from = issue_from, issue_to end end @@ -228,6 +234,8 @@ class IssueRelation < ActiveRecord::Base issue_from.blocks? issue_to when 'blocks' issue_to.blocks? issue_from + when 'relates' + self.class.where(issue_from_id: issue_to, issue_to_id: issue_from).present? else false end diff --git a/test/unit/issue_relation_test.rb b/test/unit/issue_relation_test.rb index e6d126dcb..585a8293e 100644 --- a/test/unit/issue_relation_test.rb +++ b/test/unit/issue_relation_test.rb @@ -65,6 +65,20 @@ class IssueRelationTest < ActiveSupport::TestCase assert_equal from, relation.issue_to end + def test_cannot_create_inverse_relates_relations + from = Issue.find(1) + to = Issue.find(2) + + relation1 = IssueRelation.new :issue_from => from, :issue_to => to, + :relation_type => IssueRelation::TYPE_RELATES + assert relation1.save + + relation2 = IssueRelation.new :issue_from => to, :issue_to => from, + :relation_type => IssueRelation::TYPE_RELATES + assert !relation2.save + assert_not_equal [], relation2.errors[:base] + end + def test_follows_relation_should_not_be_reversed_if_validation_fails from = Issue.find(1) to = Issue.find(2) -- 2.14.1