Remove hierarchy paths and migrate away from parent column

pull/7103/head
Oliver Günther 6 years ago
parent 30d311afcd
commit 622e73664d
No known key found for this signature in database
GPG Key ID: A3A8BDAD7C0C552C
  1. 8
      app/models/queries/work_packages/columns/property_column.rb
  2. 2
      app/models/query.rb
  3. 167
      app/models/relation/hierarchy_paths.rb
  4. 19
      config/initializers/typed_dag.rb
  5. 6
      db/migrate/20180116065518_add_hierarchy_paths.rb
  6. 41
      db/migrate/20190301122554_remove_hierarchy_paths.rb
  7. 6
      spec/features/work_packages/table_sorting_spec.rb
  8. 50
      spec/models/query/results_spec.rb
  9. 2
      spec/models/query_spec.rb
  10. 251
      spec/models/relation/hierarchy_paths_spec.rb

@ -57,13 +57,7 @@ class Queries::WorkPackages::Columns::PropertyColumn < Queries::WorkPackages::Co
parent: {
association: 'ancestors_relations',
default_order: 'asc',
sortable: ["CONCAT_WS(',', hierarchy_paths.path, work_packages.id)"],
sortable_join: <<-SQL
LEFT OUTER JOIN
hierarchy_paths
ON
hierarchy_paths.work_package_id = work_packages.id
SQL
sortable: false
},
status: {
association: 'status',

@ -96,7 +96,7 @@ class Query < ActiveRecord::Base
def set_default_sort
return if sort_criteria.any?
self.sort_criteria = [['parent', 'asc']]
self.sort_criteria = [['id', 'asc']]
end
def context

@ -1,167 +0,0 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2018 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2017 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See docs/COPYRIGHT.rdoc for more details.
#++
module Relation::HierarchyPaths
extend ActiveSupport::Concern
included do
after_create :add_hierarchy_path
after_destroy :remove_hierarchy_path
after_update :update_hierarchy_path
def self.rebuild_hierarchy_paths!
execute_sql remove_hierarchy_path_sql
execute_sql add_hierarchy_path_sql
end
def self.execute_sql(sql)
ActiveRecord::Base.connection.execute sql
end
private
def add_hierarchy_path
return unless hierarchy?
self.class.execute_sql self.class.add_hierarchy_path_sql(to_id)
end
def remove_hierarchy_path
self.class.execute_sql self.class.remove_hierarchy_path_sql(to_id)
self.class.execute_sql self.class.add_hierarchy_path_sql(to_id)
end
def update_hierarchy_path
if was_hierarchy_relation?
remove_hierarchy_path
elsif now_hierarchy_relation_or_former_id_changed?
add_hierarchy_path
elsif hierarchy_relation_and_to_id_changed?
alter_hierarchy_path
end
end
def was_hierarchy_relation?
saved_change_to_relation_type? && relation_type_before_last_save == Relation::TYPE_HIERARCHY
end
def now_hierarchy_relation_or_former_id_changed?
(saved_change_to_relation_type? || saved_change_to_from_id?) && hierarchy?
end
def hierarchy_relation_and_to_id_changed?
hierarchy? && saved_change_to_to_id?
end
def alter_hierarchy_path
self.class.execute_sql self.class.remove_hierarchy_path_sql(to_id_before_last_save)
self.class.execute_sql self.class.add_hierarchy_path_sql(to_id)
end
def self.add_hierarchy_path_sql(id = nil)
<<-SQL
INSERT INTO
#{hierarchy_table_name}
(work_package_id, path)
SELECT
to_id, #{add_hierarchy_agg_function} AS path
FROM
(SELECT to_id, from_id, hierarchy FROM relations #{add_conditions_and_union id}) as rel
GROUP BY to_id
#{add_hierarchy_conflict_statement}
SQL
end
def self.add_conditions_and_union(id)
if id.nil?
<<-SQL
WHERE hierarchy > 0 AND relates = 0 AND blocks = 0 AND duplicates = 0 AND includes = 0 AND requires = 0 AND follows = 0
SQL
else
<<-SQL
WHERE to_id = #{id} AND
hierarchy > 0 AND relates = 0 AND blocks = 0 AND duplicates = 0 AND includes = 0 AND requires = 0 AND follows = 0
UNION SELECT to_id,from_id,hierarchy
FROM relations
WHERE from_id=#{id} AND
hierarchy > 0 AND relates = 0 AND blocks = 0 AND duplicates = 0 AND includes = 0 AND requires = 0 AND follows = 0
UNION SELECT b.to_id, b.from_id, b.hierarchy FROM relations a
JOIN relations b ON b.to_id = a.to_id
WHERE a.from_id = #{id} AND
a.hierarchy > 0 AND a.relates = 0 AND a.blocks = 0 AND a.duplicates = 0 AND a.includes = 0 AND a.requires = 0 AND a.follows = 0 AND
b.hierarchy > 0 AND b.relates = 0 AND b.blocks = 0 AND b.duplicates = 0 AND b.includes = 0 AND b.requires = 0 AND b.follows = 0
SQL
end
end
def self.remove_hierarchy_path_sql(id = nil)
id_constraint = if id
"WHERE work_package_id = #{id}"
end
<<-SQL
DELETE FROM
#{hierarchy_table_name}
#{id_constraint}
SQL
end
def self.add_hierarchy_id_constraint(id)
if id
<<-SQL
AND (to_id = #{id}
OR to_id IN (#{Relation.hierarchy.where(from_id: id).select(:to_id).to_sql}))
SQL
end
end
def self.add_hierarchy_conflict_statement
if ActiveRecord::Base.connection.adapter_name == 'Mysql2'
"ON DUPLICATE KEY
UPDATE #{hierarchy_table_name}.path = VALUES(path)"
else
"ON CONFLICT (work_package_id)
DO UPDATE SET path = EXCLUDED.path"
end
end
def self.add_hierarchy_agg_function
if ActiveRecord::Base.connection.adapter_name == 'Mysql2'
"GROUP_CONCAT(from_id ORDER BY hierarchy DESC SEPARATOR ',')"
else
"string_agg(from_id::TEXT, ',' ORDER BY hierarchy DESC)"
end
end
def self.hierarchy_table_name
'hierarchy_paths'
end
end
end

@ -62,22 +62,3 @@ TypedDag::Configuration.set node_class_name: 'WorkPackage',
all_from: :all_required_by,
all_to: :all_requires }
}
# Hacking the after_* callbacks to be positioned after the callbacks
# for the typed_dag so that the transitive relations are properly adapted before
# the path is build.
module RelationHierarchyIncluder
def self.prepended(base)
included_block = base.instance_variable_get(:@_included_block)
new_includer = Proc.new do
class_eval(&included_block)
include Relation::HierarchyPaths
end
base.instance_variable_set(:@_included_block, new_includer)
end
end
TypedDag::Edge::ClosureMaintenance.prepend(RelationHierarchyIncluder)

@ -39,7 +39,11 @@ class AddHierarchyPaths < ActiveRecord::Migration[5.1]
end
reversible do |dir|
dir.up { Relation.rebuild_hierarchy_paths! }
dir.up do
Relation.rebuild_hierarchy_paths!
rescue StandardError => e
warn "Failed to rebuild hierarchy paths. Call `Relation.rebuild_hierarchy_paths!` manually to correct this: #{e}"
end
end
end
end

@ -0,0 +1,41 @@
require_relative './20180116065518_add_hierarchy_paths'
class RemoveHierarchyPaths < ActiveRecord::Migration[5.2]
def up
AddHierarchyPaths.new.migrate :down
# Set sort to id, asc where parent sort was used
Query
.where("sort_criteria LIKE '%parent%'")
.find_each do |query|
# Use update_column to ensure that value is saved regardless
# of the overall state of the query
query.update_column(:sort_criteria, query.sort_criteria.map { |criteria| map_parent_to_id(criteria) })
rescue StandardError => e
warn "Failed to migrate parent sort_criteria for query #{query.id}: #{e}"
end
end
def down
# Will fail to #rebuild_hierarchy_paths! unless restored to correct version
AddHierarchyPaths.new.migrate :up
# Set sort to parent, asc where query.show_hierarchies is set
# because that is what is implied by the frontend.
Query
.where(show_hierarchies: true)
.update_all(sort_criteria: [%w(parent asc)])
end
private
##
# Map parent sort_criteria to id asc.
def map_parent_to_id(criteria)
if criteria.first.to_s == 'parent'
%w[id asc]
else
criteria
end
end
end

@ -150,9 +150,9 @@ describe 'Select work package row', type: :feature, js: true do
wp_table.visit!
end
it 'default sortation (parent) orders depth first' do
wp_table.expect_work_package_listed parent, child1, grand_child1, grand_child3
wp_table.expect_work_package_order parent.id, child1.id, grand_child1.id, grand_child3.id
it 'default sortation (id) does not order depth first (Reverted in #29122)' do
wp_table.expect_work_package_listed parent, child1, grand_child1, child2
wp_table.expect_work_package_order parent.id, child1.id, grand_child1.id, child2
end
end
end

@ -237,7 +237,7 @@ describe ::Query::Results, type: :model do
end
end
context 'when filtering by precedes and ordering by parent' do
context 'when filtering by precedes and ordering by id' do
let(:query) do
FactoryBot.build :query,
project: project_1
@ -250,7 +250,7 @@ describe ::Query::Results, type: :model do
query.add_filter('precedes', '=', [wp_p1[0].id.to_s])
query.sort_criteria = [['parent', 'asc']]
query.sort_criteria = [['id', 'asc']]
end
it 'returns the work packages preceding the filtered for work package' do
@ -264,7 +264,7 @@ describe ::Query::Results, type: :model do
let(:work_package1) { FactoryBot.create(:work_package, project: project_1, id: 1) }
let(:work_package2) { FactoryBot.create(:work_package, project: project_1, id: 2) }
let(:work_package3) { FactoryBot.create(:work_package, project: project_1, id: 3) }
let(:sort_by) { [['parent', 'asc']] }
let(:sort_by) { [['id', 'asc']] }
let(:columns) { %i(id subject) }
let(:group_by) { '' }
@ -420,50 +420,6 @@ describe ::Query::Results, type: :model do
end
end
context 'sorting by parent' do
let(:work_package1) { FactoryBot.create(:work_package, project: project_1, subject: '1') }
let(:work_package2) { FactoryBot.create(:work_package, project: project_1, parent: work_package1, subject: '2') }
let(:work_package3) { FactoryBot.create(:work_package, project: project_1, parent: work_package2, subject: '3') }
let(:work_package4) { FactoryBot.create(:work_package, project: project_1, parent: work_package1, subject: '4') }
let(:work_package5) { FactoryBot.create(:work_package, project: project_1, parent: work_package4, subject: '5') }
let(:work_package6) { FactoryBot.create(:work_package, project: project_1, parent: work_package4, subject: '6') }
let(:work_package7) { FactoryBot.create(:work_package, project: project_1, subject: '7') }
let(:work_package8) { FactoryBot.create(:work_package, project: project_1, subject: '8') }
let(:work_package9) { FactoryBot.create(:work_package, project: project_1, parent: work_package8, subject: '9') }
let(:work_packages) do
[work_package1, work_package2, work_package3, work_package4, work_package5,
work_package6, work_package7, work_package8, work_package9]
end
# While we set a second sort criteria, it will be ignored as the sorting works solely on the id of the ancestors and
# the work package itself
let(:sort_by) { [['parent', 'asc'], ['subject', 'asc']] }
before do
allow(User).to receive(:current).and_return(user_1)
end
it 'sorts depth first by parent (id) where the second criteria is unfortunately ignored' do
# Reimplementing the algorithm of how the production code sorts lexically on ids (e.g. '15' before '7').
# This is necessary as the ids are not fixed and might span order of magnitude boundaries.
paths = work_packages.map do |wp|
# Only need to include 'relations.hierarchy' in the projection
# to satisfy PG needing to have all ORDER BY columns included on DISTINCT.
[(wp.ancestors.order("relations.hierarchy DESC").pluck(:id, 'relations.hierarchy').map(&:first) << wp.id).join(' '), wp]
end
expected_order = paths.sort_by(&:first).map(&:second).flatten
expect(query_results.sorted_work_packages)
.to match expected_order
query.sort_criteria = [['parent', 'desc'], ['subject', 'asc']]
expect(query_results.sorted_work_packages)
.to match expected_order.reverse
end
end
context 'filtering by bool cf' do
let(:bool_cf) { FactoryBot.create(:bool_wp_custom_field, is_filter: true) }
let(:custom_value) do

@ -51,7 +51,7 @@ describe Query, type: :model do
query = Query.new_default
expect(query.sort_criteria)
.to match_array([['parent', 'asc']])
.to match_array([['id', 'asc']])
end
it 'does not use the default sortation if an order is provided' do

@ -1,251 +0,0 @@
#-- encoding: UTF-8
#-- copyright
# OpenProject is a project management system.
# Copyright (C) 2012-2018 the OpenProject Foundation (OPF)
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License version 3.
#
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
# Copyright (C) 2006-2017 Jean-Philippe Lang
# Copyright (C) 2010-2013 the ChiliProject Team
#
# This program is free software; you can redistribute it and/or
# modify it under the terms of the GNU General Public License
# as published by the Free Software Foundation; either version 2
# of the License, or (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
#
# See docs/COPYRIGHT.rdoc for more details.
#++
require 'spec_helper'
describe Relation, 'hierarchy_paths', type: :model do
let(:parent) { FactoryBot.create(:work_package) }
let(:child) { FactoryBot.create(:work_package) }
let(:grand_parent) do
wp = FactoryBot.create(:work_package)
parent.parent = wp
parent.save!
wp
end
let(:grand_child) { FactoryBot.create(:work_package, parent: child) }
def record_for(id)
ActiveRecord::Base.connection.select_rows <<-SQL
SELECT work_package_id, path from hierarchy_paths WHERE work_package_id = #{id}
SQL
end
def path_for(id)
record_for(id)[0][1]
end
context 'on creation' do
context 'with a relation between two work packages' do
before do
Relation.create relation_type: 'hierarchy', from: parent, to: child
end
it 'adds a hierarchy path for the child' do
expect(record_for(child.id)).not_to be_empty
end
it 'has the parent_id in the path' do
expect(path_for(child.id)).to eql parent.id.to_s
end
it 'has no hierarchy path for the parent' do
expect(record_for(parent.id)).to be_empty
end
end
context 'with a non hierarchy relation between two work packages' do
before do
Relation.create relation_type: Relation::TYPE_BLOCKS, from: parent, to: child
end
it 'has no hierarchy path for the child' do
expect(record_for(child.id)).to be_empty
end
it 'has no hierarchy path for the parent' do
expect(record_for(parent.id)).to be_empty
end
end
context 'with a relation connecting two already existing hierarchies' do
before do
grand_parent
grand_child
Relation.create relation_type: 'hierarchy', from: parent, to: child
end
it 'adds a hierarchy path for the child' do
expect(record_for(child.id)).not_to be_empty
end
it 'has the grand parent and the parent in the path for the child' do
expect(path_for(child.id)).to eql "#{grand_parent.id},#{parent.id}"
end
it 'has grand parent, parent and child in the path for the grand_child' do
expect(path_for(grand_child.id)).to eql "#{grand_parent.id},#{parent.id},#{child.id}"
end
end
end
context 'on deletion' do
context 'with a simple parent-child relationship' do
before do
relation = Relation.create relation_type: 'hierarchy', from: parent, to: child
relation.destroy
end
it 'removes the hierarchy path for the child' do
expect(record_for(child.id)).to be_empty
end
end
context 'with a relation spanning several hops' do
before do
grand_parent
grand_child
relation = Relation.create relation_type: 'hierarchy', from: parent, to: child
relation.destroy
end
it 'removes the hierarchy path for the child' do
expect(record_for(child.id)).to be_empty
end
it 'has the grand parent in the path for the parent' do
expect(path_for(parent.id)).to eql grand_parent.id.to_s
end
it 'child in the path for the grand_child' do
expect(path_for(grand_child.id)).to eql child.id.to_s
end
end
context 'with a non hierarchy relation connecting hierarchies' do
before do
grand_parent
grand_child
relation = Relation.create relation_type: Relation::TYPE_RELATES, from: parent, to: child
relation.destroy
end
it 'removes the hierarchy path for the child' do
expect(record_for(child.id)).to be_empty
end
it 'has the grand parent in the path for the parent' do
expect(path_for(parent.id)).to eql grand_parent.id.to_s
end
it 'child in the path for the grand_child' do
expect(path_for(grand_child.id)).to eql child.id.to_s
end
end
end
context 'on update' do
let(:parent_child_relation) { Relation.create relation_type: 'hierarchy', from: parent, to: child }
let(:follows_relation) { Relation.create relation_type: 'follows', from: parent, to: child }
let(:other_wp) { FactoryBot.create :work_package }
context 'on switching the type to non hierarchy' do
before do
parent_child_relation.relation_type = Relation::TYPE_FOLLOWS
parent_child_relation.save
end
it 'removes the hierarchy_path' do
expect(record_for(child.id)).to be_empty
end
end
context 'on switching from_id' do
before do
parent_child_relation.from_id = other_wp.id
parent_child_relation.save!
end
it 'updates the path' do
expect(path_for(child.id)).to eql other_wp.id.to_s
end
end
context 'on switching to_id' do
before do
grand_child
grand_parent
relation = Relation.create to_id: other_wp.id, from_id: parent.id, relation_type: Relation::TYPE_HIERARCHY
relation.to_id = child.id
relation.save!
end
it 'adds a path for the new child' do
expect(path_for(child.id)).to eql "#{grand_parent.id},#{parent.id}"
end
it 'removes the path for the former child' do
expect(record_for(other_wp.id)).to be_empty
end
it 'updates the path to the children of the new child' do
expect(path_for(grand_child.id)).to eql "#{grand_parent.id},#{parent.id},#{child.id}"
end
end
context 'on switching the type to hierarchy' do
before do
follows_relation.relation_type = Relation::TYPE_HIERARCHY
follows_relation.save
end
it 'adds the hierarchy_path' do
expect(record_for(child.id)).not_to be_empty
end
end
end
describe '#rebuild_hierarchy_paths!' do
let!(:parent_child_relation) { Relation.create relation_type: 'hierarchy', from: parent, to: child }
before do
ActiveRecord::Base.connection.execute <<-SQL
DELETE FROM hierarchy_paths
SQL
ActiveRecord::Base.connection.execute <<-SQL
INSERT INTO hierarchy_paths (work_package_id, path) VALUES (#{parent.id}, '123')
SQL
Relation.rebuild_hierarchy_paths!
end
it 'adds a hierarchy path for the child' do
expect(record_for(child.id)).not_to be_empty
end
it 'has the parent_id in the path' do
expect(path_for(child.id)).to eql parent.id.to_s
end
it 'has no hierarchy path for the parent' do
expect(record_for(parent.id)).to be_empty
end
end
end
Loading…
Cancel
Save