Merge pull request #10349 from opf/feature/closure_tree_hierarchy

move wp hierarchy to closure_tree

Gemfile

@@ -57,8 +57,11 @@ gem 'friendly_id', '~> 5.4.0'
 gem 'acts_as_list', '~> 1.0.1'
 gem 'acts_as_tree', '~> 2.9.0'
 gem 'awesome_nested_set', '~> 3.5.0'
+gem 'closure_tree', '~> 7.4.0'
 gem 'rubytree', '~> 1.0.0'
-gem 'typed_dag', '~> 2.0.2'
+# Only used in down migrations now.
+# Is to be removed once the referencing migrations have been squashed.
+gem 'typed_dag', '~> 2.0.2', require: false
 
 gem 'addressable', '~> 2.8.0'
 

Gemfile.lock

@@ -348,6 +348,9 @@ GEM
       cork
       nap
       open4 (~> 1.3)
+    closure_tree (7.4.0)
+      activerecord (>= 4.2.10)
+      with_advisory_lock (>= 4.0.0)
     coderay (1.1.3)
     colored2 (3.1.2)
     commonmarker (0.23.4)
@@ -1019,6 +1022,7 @@ DEPENDENCIES
   carrierwave_direct (~> 2.1.0)
   cells-erb (~> 0.1.0)
   cells-rails (~> 0.1.4)
+  closure_tree (~> 7.4.0)
   commonmarker (~> 0.23.0)
   compare-xml (~> 0.66)
   costs!

app/contracts/relations/base_contract.rb

@@ -39,7 +39,7 @@ class BaseContract < ::ModelContract
     validate :manage_relations_permission?
     validate :validate_from_exists
     validate :validate_to_exists
-    validate :validate_only_one_follow_direction_between_hierarchies
+    validate :validate_nodes_relatable
     validate :validate_accepted_type
 
     def self.model
@@ -63,22 +63,21 @@ def validate_to_exists
       errors.add :to, :error_not_found unless visible_work_packages.exists? model.to_id
     end
 
-    def validate_only_one_follow_direction_between_hierarchies
-      return unless [Relation::TYPE_HIERARCHY, Relation::TYPE_FOLLOWS].include? model.relation_type
-
-      if follow_relations_in_opposite_direction.exists?
+    def validate_nodes_relatable
+      if (model.from_id_changed? || model.to_id_changed?) &&
+         WorkPackage.relatable(model.from, model.relation_type).where(id: model.to).empty?
         errors.add :base, I18n.t(:'activerecord.errors.messages.circular_dependency')
       end
     end
 
     def validate_accepted_type
-      return if (Relation::TYPES.keys + [Relation::TYPE_HIERARCHY]).include?(model.relation_type)
+      return if (Relation::TYPES.keys + [Relation::TYPE_PARENT]).include?(model.relation_type)
 
       errors.add :relation_type, :inclusion
     end
 
     def manage_relations_permission?
-      if !manage_relations?
+      unless manage_relations?
         errors.add :base, :error_unauthorized
       end
     end
@@ -90,66 +89,5 @@ def visible_work_packages
     def manage_relations?
       user.allowed_to? :manage_work_package_relations, model.from.project
     end
-
-    # Go up to's hierarchy to the highest ancestor not shared with from.
-    # Fetch all endpoints of relations that are reachable by following at least one follows
-    # and zero or more hierarchy relations.
-    # We now need to check whether those endpoints include any that
-    #
-    # * are an ancestor of from
-    # * are a descendant of from
-    # * are from itself
-    #
-    # Siblings and sibling subtrees of ancestors are ok to have relations
-    def follow_relations_in_opposite_direction
-      to_set = hierarchy_or_follows_of
-
-      follows_relations_to_ancestors(to_set)
-        .or(follows_relations_to_descendants(to_set))
-        .or(follows_relations_to_from(to_set))
-    end
-
-    def hierarchy_or_follows_of
-      to_root_ancestor = tos_highest_ancestor_not_shared_by_from
-
-      Relation
-        .hierarchy_or_follows
-        .where(from_id: to_root_ancestor)
-        .where('follows > 0')
-    end
-
-    def tos_highest_ancestor_not_shared_by_from
-      # mysql does not support a limit inside a subquery.
-      # we thus join/subselect the query for ancestors of to not shared by from
-      # with itself and exclude all that have a hierarchy value smaller than hierarchy - 1
-      unshared_ancestors = tos_ancestors_not_shared_by_from
-
-      unshared_ancestors
-        .where.not(hierarchy: unshared_ancestors.select('hierarchy - 1'))
-        .select(:from_id)
-    end
-
-    def tos_ancestors_not_shared_by_from
-      Relation
-        .hierarchy_or_reflexive
-        .where(to_id: model.to_id)
-        .where.not(from_id: Relation.hierarchy_or_reflexive
-                              .where(to_id: model.from_id)
-                              .select(:from_id))
-    end
-
-    def follows_relations_to_ancestors(to_set)
-      ancestors = Relation.hierarchy.where(to_id: model.from)
-      to_set.where(to_id: ancestors.select(:from_id))
-    end
-
-    def follows_relations_to_descendants(to_set)
-      descendants = Relation.hierarchy.where(from_id: model.from)
-      to_set.where(to_id: descendants.select(:to_id))
-    end
-
-    def follows_relations_to_from(to_set)
-      to_set.where(to_id: model.from_id)
-    end
   end
 end

app/contracts/work_packages/base_contract.rb

@@ -114,8 +114,8 @@ class BaseContract < ::ModelContract
 
     validate :validate_parent_exists
     validate :validate_parent_in_same_project
-    validate :validate_parent_not_subtask
     validate :validate_parent_not_self
+    validate :validate_parent_not_subtask
 
     validate :validate_status_exists
     validate :validate_status_transition
@@ -245,8 +245,11 @@ def validate_parent_in_same_project
 
     # have to validate ourself as the parent relation is created after saving
     def validate_parent_not_subtask
-      if model.parent_id_changed? && model.parent && invalid_relations_with_new_hierarchy.exists?
-        errors.add :base, :cant_link_a_work_package_with_a_descendant
+      if model.parent_id_changed? &&
+         model.parent_id &&
+         errors.exclude?(:parent) &&
+         WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty?
+        errors.add :parent, :cant_link_a_work_package_with_a_descendant
       end
     end
 
@@ -357,25 +360,6 @@ def status_transition_exists?
       assignable_statuses.exists?(model.status_id)
     end
 
-    def invalid_relations_with_new_hierarchy
-      query = Relation.from_parent_to_self_and_descendants(model)
-                      .or(Relation.from_self_and_descendants_to_ancestors(model))
-                      .direct
-
-      # Ignore the immediate relation from the old parent to the model
-      # since that will still exist before saving.
-      old_parent_id = model.parent_id_was
-
-      if old_parent_id.present?
-        query
-          .where.not(hierarchy: 1)
-          .where.not(from_id: old_parent_id)
-          .where.not(to_id: model.id)
-      else
-        query
-      end
-    end
-
     def type_context_changed?
       model.project && !type_inexistent? && (model.type_id_changed? || model.project_id_changed?)
     end
@@ -386,7 +370,7 @@ def type_inexistent?
 
     # Returns a scope of status the user is able to apply
     def new_statuses_allowed_from(status)
-      return Status.where('1=0') if status.nil?
+      return Status.none if status.nil?
 
       current_status = Status.where(id: status.id)
 

app/controllers/work_packages/bulk_controller.rb

@@ -114,7 +114,7 @@ def attributes_for_update
     permitted_params
       .update_work_package
       .tap { |attributes| attributes[:custom_field_values]&.reject! { |_k, v| v.blank? } }
-      .reject { |_k, v| v.blank? }
+      .compact_blank
       .transform_values { |v| v == 'none' ? '' : v }
       .to_h
   end

app/controllers/work_packages/moves_controller.rb

@@ -96,16 +96,14 @@ def check_project_uniqueness
   end
 
   def prepare_for_work_package_move
-    @work_packages = @work_packages.includes(:ancestors)
     @copy = params.has_key? :copy
     @allowed_projects = WorkPackage.allowed_target_projects_on_move(current_user)
     @target_project = @allowed_projects.detect { |p| p.id.to_s == params[:new_project_id].to_s } if params[:new_project_id]
     @target_project ||= @project
     @types = @target_project.types
     @available_versions = @target_project.assignable_versions
     @available_statuses = Workflow.available_statuses(@project)
-    @notes = params[:notes]
-    @notes ||= ''
+    @notes = params[:notes] || ''
   end
 
   def permitted_create_params

app/models/queries/relations/filters/type_filter.rb

@@ -45,13 +45,9 @@ def self.key
         end
 
         def where
-          Array(values).map do |value|
-            column = Relation.relation_column(value)
-
-            operator_strategy.sql_for_field(['1'],
-                                            self.class.model.table_name,
-                                            column)
-          end.join(' OR ')
+          operator_strategy.sql_for_field(values.map { |value| Relation.canonical_type(value) },
+                                          self.class.model.table_name,
+                                          :relation_type)
         end
       end
     end

app/models/queries/relations/relation_query.rb

@@ -33,11 +33,6 @@ def self.model
         Relation
       end
 
-      def default_scope
-        Relation
-          .direct
-      end
-
       def results
         # Filters marked to already check visibility free us from the need
         # to check it here.

app/models/queries/work_packages/filter/filter_on_directed_relations_mixin.rb

@@ -30,13 +30,9 @@ module Queries::WorkPackages::Filter::FilterOnDirectedRelationsMixin
   include ::Queries::WorkPackages::Filter::FilterForWpMixin
 
   def where
-    # The order in which we call the methods on `Relation` matters, as
-    # the `Relation`'s association `includes` is overwritten with the method `includes`
-    # otherwise.
     relations_subselect = Relation
-                          .send(normalized_relation_type)
-                          .direct
                           .where(relation_filter)
+                          .where(relation_type: normalized_relation_type)
                           .select(relation_select)
 
     operator = if operator_class == Queries::Operators::Equals

app/models/queries/work_packages/filter/filter_on_undirected_relations_mixin.rb

@@ -54,18 +54,19 @@ def operator_and_junction
   end
 
   def relations_subselect_to_from
-    Relation
-      .direct
-      .send(relation_type)
+    relation_subselect
       .where(to_id: values)
       .select(:from_id)
   end
 
   def relations_subselect_from_to
-    Relation
-      .direct
-      .send(relation_type)
+    relation_subselect
       .where(from_id: values)
       .select(:to_id)
   end
+
+  def relation_subselect
+    Relation
+      .where(relation_type: relation_type)
+  end
 end

app/models/queries/work_packages/filter/parent_filter.rb

@@ -28,19 +28,17 @@
 
 class Queries::WorkPackages::Filter::ParentFilter <
   Queries::WorkPackages::Filter::WorkPackageFilter
-  include ::Queries::WorkPackages::Filter::FilterOnDirectedRelationsMixin
+  include ::Queries::WorkPackages::Filter::FilterForWpMixin
 
   def relation_type
-    ::Relation::TYPE_HIERARCHY
+    # While this is not a relation (in the sense of it being stored in a different database table) we still
+    # want it to be used same as every other relation filter.
+    Relation::TYPE_PARENT
   end
 
-  private
-
-  def relation_filter
-    { from_id: values }
-  end
-
-  def relation_select
-    :to_id
+  def where
+    # The filter had been called parent before and it is stored in the database like that.
+    # The other association filters all have _id in their self.key.
+    operator_strategy.sql_for_field(no_templated_values, self.class.model.table_name, :parent_id)
   end
 end

app/models/queries/work_packages/filter/relatable_filter.rb

@@ -47,33 +47,11 @@ def where
   end
 
   def scope
-    if operator == Relation::TYPE_RELATES
-      relateable_from_or_to
-    elsif operator != 'parent' && canonical_operator == operator
-      relateable_to
-    else
-      relateable_from
-    end
+    WorkPackage.relatable(WorkPackage.find_by(id: values.first), Relation.canonical_type(operator))
   end
 
   private
 
-  def relateable_from_or_to
-    relateable_to.or(relateable_from)
-  end
-
-  def relateable_from
-    WorkPackage.relateable_from(from)
-  end
-
-  def relateable_to
-    WorkPackage.relateable_to(from)
-  end
-
-  def from
-    WorkPackage.find(values.first)
-  end
-
   def canonical_operator
     Relation.canonical_type(operator)
   end