Skip to content

G
gitlab-ce
Commit 3baaeb75 authored by Alex Kalderimis's avatar Alex Kalderimis
Browse files
Options

Add stricter tests that we preserve order 

By testing this, we can remove the tests on private members.
parent bca169a3
Hide whitespace changes
Inline Side-by-side
Showing with 118 additions and 334 deletions
app/models/concerns/relative_positioning.rb
View file @ 3baaeb75
......@@ -170,6 +170,30 @@ module RelativePositioning
move_sequence_after(false, next_gap: gap)
end
# Moves the sequence before the current item to the middle of the next gap
# For example, we have
#
# 5 . . . . . 11 12 13 14 [15] 16 . 17
# -----------
#
# This moves the sequence [11 12 13 14] to [8 9 10 11], so we have:
#
# 5 . . 8 9 10 11 . . . [15] 16 . 17
# ---------
#
# Creating a gap to the left of the current item. We can understand this as
# dividing the 5 spaces between 5 and 11 into two smaller gaps of 2 and 3.
#
# If `include_self` is true, the current item will also be moved, creating a
# gap to the right of the current item:
#
# 5 . . 8 9 10 11 [14] . . . 16 . 17
# --------------
#
# As an optimization, the gap can be precalculated and passed to this method.
#
# @api private
# @raises NoSpaceLeft if the sequence cannot be moved
def move_sequence_before(include_self = false, next_gap: find_next_gap_before)
raise NoSpaceLeft unless next_gap.present?
......@@ -178,6 +202,30 @@ module RelativePositioning
move_sequence(next_gap.start_pos, relative_position, -delta, include_self)
end
# Moves the sequence after the current item to the middle of the next gap
# For example, we have:
#
# 8 . 10 [11] 12 13 14 15 . . . . . 21
# -----------
#
# This moves the sequence [12 13 14 15] to [15 16 17 18], so we have:
#
# 8 . 10 [11] . . . 15 16 17 18 . . 21
# -----------
#
# Creating a gap to the right of the current item. We can understand this as
# dividing the 5 spaces between 15 and 21 into two smaller gaps of 3 and 2.
#
# If `include_self` is true, the current item will also be moved, creating a
# gap to the left of the current item:
#
# 8 . 10 . . . [14] 15 16 17 18 . . 21
# ----------------
#
# As an optimization, the gap can be precalculated and passed to this method.
#
# @api private
# @raises NoSpaceLeft if the sequence cannot be moved
def move_sequence_after(include_self = false, next_gap: find_next_gap_after)
raise NoSpaceLeft unless next_gap.present?
......@@ -343,6 +391,24 @@ module RelativePositioning
c
end
# This method takes two integer values (positions) and
# calculates the position between them. The range is huge as
# the maximum integer value is 2147483647.
#
# We avoid open ranges by clamping the range to [MIN_POSITION, MAX_POSITION].
#
# Then we handle one of three cases:
# - If the gap is too small, we raise NoSpaceLeft
# - If the gap is larger than MAX_GAP, we place the new position at most
# IDEAL_DISTANCE from the edge of the gap.
# - otherwise we place the new position at the midpoint.
#
# The new position will always satisfy: pos_before <= midpoint <= pos_after
#
# As a precondition, the gap between pos_before and pos_after MUST be >= 2.
# If the gap is too small, NoSpaceLeft is raised.
#
# @raises NoSpaceLeft
def position_between(pos_before, pos_after)
pos_before ||= range.first
pos_after ||= range.last
......@@ -394,52 +460,6 @@ module RelativePositioning
move_nulls(objects, at_end: false)
end
# This method takes two integer values (positions) and
# calculates the position between them. The range is huge as
# the maximum integer value is 2147483647.
#
# We avoid open ranges by clamping the range to [MIN_POSITION, MAX_POSITION].
#
# Then we handle one of three cases:
# - If the gap is too small, we raise NoSpaceLeft
# - If the gap is larger than MAX_GAP, we place the new position at most
# IDEAL_DISTANCE from the edge of the gap.
# - otherwise we place the new position at the midpoint.
#
# The new position will always satisfy: pos_before <= midpoint <= pos_after
#
# As a precondition, the gap between pos_before and pos_after MUST be >= 2.
# If the gap is too small, NoSpaceLeft is raised.
#
# This class method should only be called by instance methods of this module, which
# include handling for minimum gap size.
#
# @raises NoSpaceLeft
# @api private
def position_between(pos_before, pos_after)
pos_before ||= MIN_POSITION
pos_after ||= MAX_POSITION
pos_before, pos_after = [pos_before, pos_after].sort
gap_width = pos_after - pos_before
midpoint = [pos_after - 1, pos_before + (gap_width / 2)].min
if gap_width < MIN_GAP
raise NoSpaceLeft
elsif gap_width > MAX_GAP
if pos_before <= MIN_POSITION
pos_after - IDEAL_DISTANCE
elsif pos_after >= MAX_POSITION
pos_before + IDEAL_DISTANCE
else
midpoint
end
else
midpoint
end
end
private
# @api private
......@@ -543,310 +563,37 @@ module RelativePositioning
end
end
def min_relative_position(&block)
calculate_relative_position('MIN', &block)
end
def max_relative_position(&block)
calculate_relative_position('MAX', &block)
end
def prev_relative_position(ignoring: nil)
prev_pos = nil
if self.relative_position
prev_pos = max_relative_position do |relation|
relation = relation.id_not_in(ignoring.id) if ignoring.present?
relation.where('relative_position < ?', self.relative_position)
end
end
prev_pos
end
def next_relative_position(ignoring: nil)
next_pos = nil
if self.relative_position
next_pos = min_relative_position do |relation|
relation = relation.id_not_in(ignoring.id) if ignoring.present?
relation.where('relative_position > ?', self.relative_position)
end
end
next_pos
end
def move_between(before, after)
return move_after(before) unless after
return move_before(after) unless before
before, after = after, before if after.relative_position < before.relative_position
pos_left = before.relative_position
pos_right = after.relative_position
mover = Mover.new(START_POSITION, (MIN_POSITION..MAX_POSITION))
if pos_right - pos_left < MIN_GAP
# Not enough room! Make space by shifting all previous elements to the left
# if there is enough space, else to the right
gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend
if gap.present?
after.move_sequence_before(next_gap: gap)
pos_left -= optimum_delta_for_gap(gap)
else
before.move_sequence_after
pos_right = after.reset.relative_position
end
end
new_position = self.class.position_between(pos_left, pos_right)
self.relative_position = new_position
mover.move(self, before, after)
end
def move_after(before = self)
pos_before = before.relative_position
pos_after = before.next_relative_position(ignoring: self)
mover = Mover.new(START_POSITION, (MIN_POSITION..MAX_POSITION))
if pos_before == MAX_POSITION || gap_too_small?(pos_after, pos_before)
gap = before.send(:find_next_gap_after) # rubocop:disable GitlabSecurity/PublicSend
if gap.nil?
before.move_sequence_before(true)
pos_before = before.reset.relative_position
else
before.move_sequence_after(next_gap: gap)
pos_after += optimum_delta_for_gap(gap)
end
end
self.relative_position = self.class.position_between(pos_before, pos_after)
mover.move(self, before, nil)
end
def move_before(after = self)
pos_after = after.relative_position
pos_before = after.prev_relative_position(ignoring: self)
if pos_after == MIN_POSITION || gap_too_small?(pos_before, pos_after)
gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend
if gap.nil?
after.move_sequence_after(true)
pos_after = after.reset.relative_position
else
after.move_sequence_before(next_gap: gap)
pos_before -= optimum_delta_for_gap(gap)
end
end
mover = Mover.new(START_POSITION, (MIN_POSITION..MAX_POSITION))
self.relative_position = self.class.position_between(pos_before, pos_after)
mover.move(self, nil, after)
end
def move_to_end
max_pos = max_relative_position
if max_pos.nil?
self.relative_position = START_POSITION
elsif gap_too_small?(max_pos, MAX_POSITION + 1)
max = relative_siblings.order(Gitlab::Database.nulls_last_order('relative_position', 'DESC')).first
max.move_sequence_before(true)
max.reset
self.relative_position = self.class.position_between(max.relative_position, MAX_POSITION + 1)
else
self.relative_position = self.class.position_between(max_pos, MAX_POSITION + 1)
end
mover = Mover.new(START_POSITION, (MIN_POSITION..MAX_POSITION))
mover.move_to_end(self)
rescue NoSpaceLeft
self.relative_position = MAX_POSITION
end
def move_to_start
min_pos = min_relative_position
if min_pos.nil?
self.relative_position = START_POSITION
elsif gap_too_small?(min_pos, MIN_POSITION - 1)
min = relative_siblings.order(Gitlab::Database.nulls_last_order('relative_position', 'ASC')).first
min.move_sequence_after(true)
min.reset
self.relative_position = self.class.position_between(MIN_POSITION - 1, min.relative_position)
else
self.relative_position = self.class.position_between(MIN_POSITION - 1, min_pos)
end
mover = Mover.new(START_POSITION, (MIN_POSITION..MAX_POSITION))
mover.move_to_start(self)
rescue NoSpaceLeft
self.relative_position = MIN_POSITION
end
# Moves the sequence before the current item to the middle of the next gap
# For example, we have
#
# 5 . . . . . 11 12 13 14 [15] 16 . 17
# -----------
#
# This moves the sequence [11 12 13 14] to [8 9 10 11], so we have:
#
# 5 . . 8 9 10 11 . . . [15] 16 . 17
# ---------
#
# Creating a gap to the left of the current item. We can understand this as
# dividing the 5 spaces between 5 and 11 into two smaller gaps of 2 and 3.
#
# If `include_self` is true, the current item will also be moved, creating a
# gap to the right of the current item:
#
# 5 . . 8 9 10 11 [14] . . . 16 . 17
# --------------
#
# As an optimization, the gap can be precalculated and passed to this method.
#
# @api private
# @raises NoSpaceLeft if the sequence cannot be moved
def move_sequence_before(include_self = false, next_gap: find_next_gap_before)
raise NoSpaceLeft unless next_gap.present?
delta = optimum_delta_for_gap(next_gap)
move_sequence(next_gap[:start], relative_position, -delta, include_self)
end
# Moves the sequence after the current item to the middle of the next gap
# For example, we have:
#
# 8 . 10 [11] 12 13 14 15 . . . . . 21
# -----------
#
# This moves the sequence [12 13 14 15] to [15 16 17 18], so we have:
#
# 8 . 10 [11] . . . 15 16 17 18 . . 21
# -----------
#
# Creating a gap to the right of the current item. We can understand this as
# dividing the 5 spaces between 15 and 21 into two smaller gaps of 3 and 2.
#
# If `include_self` is true, the current item will also be moved, creating a
# gap to the left of the current item:
#
# 8 . 10 . . . [14] 15 16 17 18 . . 21
# ----------------
#
# As an optimization, the gap can be precalculated and passed to this method.
#
# @api private
# @raises NoSpaceLeft if the sequence cannot be moved
def move_sequence_after(include_self = false, next_gap: find_next_gap_after)
raise NoSpaceLeft unless next_gap.present?
delta = optimum_delta_for_gap(next_gap)
move_sequence(relative_position, next_gap[:start], delta, include_self)
end
private
def gap_too_small?(pos_a, pos_b)
return false unless pos_a && pos_b
(pos_a - pos_b).abs < MIN_GAP
end
# Find the first suitable gap to the left of the current position.
#
# Satisfies the relations:
# - gap[:start] <= relative_position
# - abs(gap[:start] - gap[:end]) >= MIN_GAP
# - MIN_POSITION <= gap[:start] <= MAX_POSITION
# - MIN_POSITION <= gap[:end] <= MAX_POSITION
#
# Supposing that the current item is 13, and we have a sequence of items:
#
# 1 . . . 5 . . . . 11 12 [13] 14 . . 17
# ^---------^
#
# Then we return: `{ start: 11, end: 5 }`
#
# Here start refers to the end of the gap closest to the current item.
def find_next_gap_before
items_with_next_pos = scoped_items
.select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position DESC) AS next_pos')
.where('relative_position <= ?', relative_position)
.order(relative_position: :desc)
find_next_gap(items_with_next_pos, MIN_POSITION)
end
# Find the first suitable gap to the right of the current position.
#
# Satisfies the relations:
# - gap[:start] >= relative_position
# - abs(gap[:start] - gap[:end]) >= MIN_GAP
# - MIN_POSITION <= gap[:start] <= MAX_POSITION
# - MIN_POSITION <= gap[:end] <= MAX_POSITION
#
# Supposing the current item is 13, and that we have a sequence of items:
#
# 9 . . . [13] 14 15 . . . . 20 . . . 24
# ^---------^
#
# Then we return: `{ start: 15, end: 20 }`
#
# Here start refers to the end of the gap closest to the current item.
def find_next_gap_after
items_with_next_pos = scoped_items
.select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position ASC) AS next_pos')
.where('relative_position >= ?', relative_position)
.order(:relative_position)
find_next_gap(items_with_next_pos, MAX_POSITION)
end
def find_next_gap(items_with_next_pos, end_is_nil)
gap = self.class
.from(items_with_next_pos, :items)
.where('next_pos IS NULL OR ABS(pos::bigint - next_pos::bigint) >= ?', MIN_GAP)
.limit(1)
.pluck(:pos, :next_pos)
.first
return if gap.nil? || gap.first == end_is_nil
{ start: gap.first, end: gap.second || end_is_nil }
end
def optimum_delta_for_gap(gap)
delta = ((gap[:start] - gap[:end]) / 2.0).abs.ceil
[delta, IDEAL_DISTANCE].min
end
def move_sequence(start_pos, end_pos, delta, include_self = false)
relation = include_self ? scoped_items : relative_siblings
relation
.where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
.update_all("relative_position = relative_position + #{delta}")
end
def calculate_relative_position(calculation)
# When calculating across projects, this is much more efficient than
# MAX(relative_position) without the GROUP BY, due to index usage:
# https://gitlab.com/gitlab-org/gitlab-foss/issues/54276#note_119340977
relation = scoped_items
.order(Gitlab::Database.nulls_last_order('position', 'DESC'))
.group(self.class.relative_positioning_parent_column)
.limit(1)
relation = yield relation if block_given?
relation
.pluck(self.class.relative_positioning_parent_column, Arel.sql("#{calculation}(relative_position) AS position"))
.first&.last
end
def relative_siblings(relation = scoped_items)
relation.id_not_in(id)
end
def scoped_items
self.class.relative_positioning_query_base(self)
end
end
spec/models/concerns/relative_positioning_spec.rb
View file @ 3baaeb75
......@@ -4,7 +4,7 @@ require 'spec_helper'
RSpec.describe RelativePositioning do
let_it_be(:default_user) { create_default(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:project, reload: true) { create(:project) }
def create_issue(pos)
create(:issue, project: project, relative_position: pos)
......@@ -22,24 +22,29 @@ RSpec.describe RelativePositioning do
describe '#move_to_end' do
shared_examples 'able to place a new item at the end' do
it 'can place any new item' do
existing_issues = project.issues.to_a
new_item = create_issue(nil)
subject.move_to_end(new_item)
new_item.save!
expect(new_item.relative_position).to eq(project.issues.maximum(:relative_position))
expect(project.issues.reorder(:relative_position).pluck(:id)).to eq(existing_issues.map(&:id) + [new_item.id])
end
end
shared_examples 'able to move existing items to the end' do
it 'can move any existing item' do
issue = issues[index]
other_issues = issues.reject { |i| i == issue }
subject.move_to_end(issue)
issue.save!
project.reset
expect(project.issues.pluck(:relative_position)).to all(be_between(range.first, range.last))
expect(issue.relative_position).to eq(project.issues.maximum(:relative_position))
expect(project.issues.reorder(:relative_position).pluck(:id)).to eq(other_issues.map(&:id) + [issue.id])
end
end
......@@ -98,24 +103,29 @@ RSpec.describe RelativePositioning do
describe '#move_to_start' do
shared_examples 'able to place a new item at the start' do
it 'can place any new item' do
existing_issues = project.issues.to_a
new_item = create_issue(nil)
subject.move_to_start(new_item)
new_item.save!
expect(new_item.relative_position).to eq(project.issues.minimum(:relative_position))
expect(project.issues.reorder(:relative_position).pluck(:id)).to eq([new_item.id] + existing_issues.map(&:id))
end
end
shared_examples 'able to move existing items to the start' do
it 'can move any existing item' do
issue = issues[index]
other_issues = issues.reject { |i| i == issue }
subject.move_to_start(issue)
issue.save!
project.reset
expect(project.issues.pluck(:relative_position)).to all(be_between(range.first, range.last))
expect(issue.relative_position).to eq(project.issues.minimum(:relative_position))
expect(project.issues.reorder(:relative_position).pluck(:id)).to eq([issue.id] + other_issues.map(&:id))
end
end
......@@ -173,6 +183,9 @@ RSpec.describe RelativePositioning do
describe '#move' do
shared_examples 'able to move a new item' do
let(:other_issues) { project.issues.reorder(relative_position: :asc).to_a }
let!(:previous_order) { other_issues.map(&:id) }
it 'can place any new item betwen two others' do
new_item = create_issue(nil)
......@@ -183,6 +196,9 @@ RSpec.describe RelativePositioning do
expect(new_item.relative_position).to be_between(range.first, range.last)
expect(new_item.relative_position).to be_between(lhs.relative_position, rhs.relative_position)
ids = project.issues.reorder(:relative_position).pluck(:id).reject { |id| id == new_item.id }
expect(ids).to eq(previous_order)
end
it 'can place any new item after another' do
......@@ -194,6 +210,9 @@ RSpec.describe RelativePositioning do
expect(new_item.relative_position).to be_between(range.first, range.last)
expect(new_item.relative_position).to be > lhs.relative_position
ids = project.issues.reorder(:relative_position).pluck(:id).reject { |id| id == new_item.id }
expect(ids).to eq(previous_order)
end
it 'can place any new item before another' do
......@@ -205,12 +224,17 @@ RSpec.describe RelativePositioning do
expect(new_item.relative_position).to be_between(range.first, range.last)
expect(new_item.relative_position).to be < rhs.relative_position
ids = project.issues.reorder(:relative_position).pluck(:id).reject { |id| id == new_item.id }
expect(ids).to eq(previous_order)
end
end
shared_examples 'able to move an existing item' do
let(:item) { issues[index] }
let(:positions) { project.reset.issues.pluck(:relative_position) }
let(:other_issues) { project.issues.reorder(:relative_position).to_a.reject { |i| i == item } }
let!(:previous_order) { other_issues.map(&:id) }
it 'can place any item betwen two others' do
subject.move(item, lhs, rhs)
......@@ -221,6 +245,9 @@ RSpec.describe RelativePositioning do
expect(positions).to all(be_between(range.first, range.last))
expect(positions).to match_array(positions.uniq)
expect(item.relative_position).to be_between(lhs.relative_position, rhs.relative_position)
ids = project.issues.reorder(:relative_position).pluck(:id).reject { |id| id == item.id }
expect(ids).to eq(previous_order)
end
it 'can place any item after another' do
......@@ -238,6 +265,9 @@ RSpec.describe RelativePositioning do
.where(relative_position: (expected_sequence.first.relative_position..expected_sequence.last.relative_position))
expect(sequence).to eq(expected_sequence)
ids = project.issues.reorder(:relative_position).pluck(:id).reject { |id| id == item.id }
expect(ids).to eq(previous_order)
end
it 'can place any item before another' do
......@@ -255,6 +285,9 @@ RSpec.describe RelativePositioning do
.where(relative_position: (expected_sequence.first.relative_position..expected_sequence.last.relative_position))
expect(sequence).to eq(expected_sequence)
ids = project.issues.reorder(:relative_position).pluck(:id).reject { |id| id == item.id }
expect(ids).to eq(previous_order)
end
end
......@@ -303,10 +336,14 @@ RSpec.describe RelativePositioning do
end
context 'there are a couple of siblings' do
where(:pos_a, :pos_b) { range.to_a.product(range.to_a).reject { |x, y| x == y } }
where(:pos_movable, :pos_a, :pos_b) do
xs = range.to_a
xs.product(xs).product(xs).map(&:flatten).select { |vals| vals == vals.uniq }
end
with_them do
let!(:issues) { [range.first, pos_a, pos_b].sort.map { |p| create_issue(p) } }
let!(:issues) { ([pos_movable] + [pos_a, pos_b].sort).map { |p| create_issue(p) } }
let(:index) { 0 }
let(:lhs) { issues[1] }
let(:rhs) { issues[2] }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7