From 72d44b8ad50529c8918cc00608a120d669335227 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Tue, 24 Jun 2025 11:11:08 +0200 Subject: [PATCH] [65062] Fix infinite loop when getting automatically scheduled ancestors https://community.openproject.org/wp/65062 When there is a cycle in the hierarchy, like trying to set the root parent as a child of its own child, the scheduling dependency would loop when trying to find all the automatically scheduled ancestors of the work package. This is now fixed by marking the visited work packages. --- app/contracts/work_packages/base_contract.rb | 15 ++- .../work_packages/schedule_dependency.rb | 18 ++- .../work_packages/schedule_dependency_spec.rb | 113 ++++++++++++++++++ .../update_service_integration_spec.rb | 30 ++++- 4 files changed, 167 insertions(+), 9 deletions(-) diff --git a/app/contracts/work_packages/base_contract.rb b/app/contracts/work_packages/base_contract.rb index 65edad4d4a9..22689ac7108 100644 --- a/app/contracts/work_packages/base_contract.rb +++ b/app/contracts/work_packages/base_contract.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + #-- copyright # OpenProject is an open source project management software. # Copyright (C) the OpenProject GmbH @@ -304,11 +306,22 @@ module WorkPackages if model.parent_id_changed? && model.parent_id && errors.exclude?(:parent) && - WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty? + current_parent_unrelatable? + # closure_tree adds an error on :parent_id because of the cycle + # detection, and active_record sees the error when saving the children + # association and adds an error on :children as well. We need to remove + # them. + errors.delete(:parent_id) # remove the error added by closure_tree + errors.delete(:children) # remove the error added by active_record + # add our own error errors.add :parent, :cant_link_a_work_package_with_a_descendant end end + def current_parent_unrelatable? + WorkPackage.relatable(model, Relation::TYPE_PARENT).where(id: model.parent_id).empty? + end + def validate_status_exists errors.add :status, :does_not_exist if model.status && !status_exists? end diff --git a/app/services/work_packages/schedule_dependency.rb b/app/services/work_packages/schedule_dependency.rb index 76f291a83a0..bac24732f6c 100644 --- a/app/services/work_packages/schedule_dependency.rb +++ b/app/services/work_packages/schedule_dependency.rb @@ -102,13 +102,21 @@ class WorkPackages::ScheduleDependency def automatically_scheduled_ancestors(work_package) @automatically_scheduled_ancestors ||= {} @automatically_scheduled_ancestors[work_package] ||= begin - parent = parent_of(work_package) + work_packages_to_process = [work_package] + result = [] + processed_ids = Set.new - if parent&.schedule_automatically? - [parent, *automatically_scheduled_ancestors(parent)] - else - [] + while current = work_packages_to_process.shift + processed_ids.add(current.id) + + parent = parent_of(current) + + if parent&.schedule_automatically? + result << parent unless parent.id == work_package.id + work_packages_to_process << parent unless processed_ids.include?(parent.id) + end end + result end end diff --git a/spec/services/work_packages/schedule_dependency_spec.rb b/spec/services/work_packages/schedule_dependency_spec.rb index 47dbb8ae8e8..c9fb9f3522d 100644 --- a/spec/services/work_packages/schedule_dependency_spec.rb +++ b/spec/services/work_packages/schedule_dependency_spec.rb @@ -75,4 +75,117 @@ RSpec.describe WorkPackages::ScheduleDependency do end end end + + describe "#automatically_scheduled_ancestors" do + shared_let(:work_package) { create(:work_package, subject: "work_package") } + let(:schedule_dependency) { described_class.new(work_package) } + + context "with no parent" do + it "returns an empty array" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty + end + end + + context "with a single automatically scheduled parent" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + parent | automatic + TABLE + + before do + work_package.update(parent:) + end + + it "returns the parent" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent) + end + end + + context "with a manually scheduled parent" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + parent | manual + TABLE + + before do + work_package.update(parent:) + end + + it "returns an empty array" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty + end + end + + context "with multiple levels of automatically scheduled ancestors" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + grandparent | automatic + parent | automatic + TABLE + + before do + work_package.update(parent:) + end + + it "returns all automatically scheduled ancestors" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent, grandparent) + end + end + + context "with mixed scheduling modes in the hierarchy" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + grandparent | automatic + parent | manual + TABLE + + before do + work_package.update(parent:) + end + + it "returns only automatically scheduled ancestors" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to be_empty + end + end + + context "with a cycle in the hierarchy" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + child | automatic + grandchild | manual + TABLE + + before do + child.update(parent: work_package) + work_package.update(schedule_manually: false) + + # Create the cycle: set the parent with a current child, but do not + # save, like when set by an update during a set attributes service call + work_package.parent = child + end + + it "handles the cycle gracefully and does not cause an infinite loop" do + expect { schedule_dependency.automatically_scheduled_ancestors(work_package) }.not_to raise_error + # Should return the parent but not get stuck in an infinite loop + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(child) + end + end + + context "with a complex hierarchy" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + great_grandparent | automatic + grandparent | manual + parent | automatic + TABLE + + before do + work_package.update(parent:) + end + + it "returns only automatically scheduled ancestors" do + expect(schedule_dependency.automatically_scheduled_ancestors(work_package)).to contain_exactly(parent) + end + end + end end diff --git a/spec/services/work_packages/update_service_integration_spec.rb b/spec/services/work_packages/update_service_integration_spec.rb index b86c09d06e1..7e53bd4277e 100644 --- a/spec/services/work_packages/update_service_integration_spec.rb +++ b/spec/services/work_packages/update_service_integration_spec.rb @@ -64,7 +64,7 @@ RSpec.describe WorkPackages::UpdateService, "integration", type: :model do set_factory_default(:user, user) end - shared_let(:work_package, refind: true, reload: false) do + let(:work_package) do create(:work_package, subject: "work_package") end @@ -1892,9 +1892,33 @@ RSpec.describe WorkPackages::UpdateService, "integration", type: :model do } end - # Bug #63499: this was causing an infinite loop when computing the future + # Bug #64973: this was causing an infinite loop when computing the future # dates of the predecessor. - it "displays an error about the inability to have multiple relations between the same work packages (Bug #63499)" do + it "displays an error about the inability to have multiple relations between the same work packages (Bug #64973)" do + expect(subject).to be_failure + + expect(subject.errors.attribute_names).to contain_exactly(:parent) + # the error message in this case is far from ideal + expect(subject.errors.details).to include(parent: [{ error: :cant_link_a_work_package_with_a_descendant }]) + end + end + + context "when a work package with a child and a grandchild is made a child of its child" do + let_work_packages(<<~TABLE) + hierarchy | scheduling mode + work_package | automatic + child | automatic + grandchild | manual + TABLE + let(:attributes) do + { + parent: child + } + end + + # Bug #65062: this was causing an infinite loop when computing automatically + # scheduled ancestors of the updated work package. + it "displays an error about the inability to have multiple relations between the same work packages (Bug #65062)" do expect(subject).to be_failure expect(subject.errors.attribute_names).to contain_exactly(:parent)