From 63e985483605874f9b8b3bf9f3a5408bdf019743 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 17:21:25 +0300 Subject: [PATCH 01/37] Make find/exists? resolve semantic work package identifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract FinderMethods module that transparently resolves both numeric and semantic identifiers (e.g. "PROJ-42") using FriendlyId's Object#friendly_id? for dispatch. The module is included in both the WorkPackage class and extended onto every relation, so scoped queries like WorkPackage.visible(user).find("PROJ-42") work seamlessly. - Override find to resolve semantic IDs via identifier column + alias table - Override exists? with the same resolution chain - Refactor find_by_id_or_identifier to use friendly_id? instead of semantic_id? - Update API route to accept string IDs (type: Integer → type: String) - Update controller and ViewComponent finders to use find_by_id_or_identifier - Pass display_id from Rails views to Angular custom elements --- .../work_packages/full_view/show_component.rb | 2 +- .../work_packages/split_view_component.rb | 2 +- app/controllers/work_packages_controller.rb | 2 +- .../work_package/semantic_identifier.rb | 56 ++++++++--- app/views/work_packages/show.html.erb | 2 +- lib/api/v3/work_packages/work_packages_api.rb | 2 +- .../work_packages_controller_spec.rb | 2 +- .../work_package/semantic_identifier_spec.rb | 94 +++++++++++++++++++ .../form/work_package_form_resource_spec.rb | 5 +- 9 files changed, 145 insertions(+), 22 deletions(-) diff --git a/app/components/work_packages/full_view/show_component.rb b/app/components/work_packages/full_view/show_component.rb index 2547c4a3e68..706daceebee 100644 --- a/app/components/work_packages/full_view/show_component.rb +++ b/app/components/work_packages/full_view/show_component.rb @@ -13,7 +13,7 @@ module WorkPackages @id = id @tab = tab - @work_package = WorkPackage.visible.find_by(id:) + @work_package = WorkPackage.visible.find_by_id_or_identifier(id) end def wrapper_uniq_by diff --git a/app/components/work_packages/split_view_component.rb b/app/components/work_packages/split_view_component.rb index 48d90ebbeb7..0dfa3784fdc 100644 --- a/app/components/work_packages/split_view_component.rb +++ b/app/components/work_packages/split_view_component.rb @@ -39,7 +39,7 @@ class WorkPackages::SplitViewComponent < ApplicationComponent @id = id @tab = tab - @work_package = WorkPackage.visible.find_by(id:) + @work_package = WorkPackage.visible.find_by_id_or_identifier(id) @base_route = base_route end diff --git a/app/controllers/work_packages_controller.rb b/app/controllers/work_packages_controller.rb index 0f2f9b8a8a9..f221f77232e 100644 --- a/app/controllers/work_packages_controller.rb +++ b/app/controllers/work_packages_controller.rb @@ -243,7 +243,7 @@ class WorkPackagesController < ApplicationController def work_package return @work_package if defined?(@work_package) - @work_package = WorkPackage.visible(current_user).find_by(id: params[:id]) + @work_package = WorkPackage.visible(current_user).find_by_id_or_identifier(params[:id]) end def journals diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 0d8861ac95d..3b73d672519 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -31,19 +31,24 @@ module WorkPackage::SemanticIdentifier extend ActiveSupport::Concern - included do - has_many :semantic_aliases, - class_name: "WorkPackageSemanticAlias", - foreign_key: :work_package_id, - inverse_of: :work_package, - dependent: :delete_all + # Finder methods that work on both the WorkPackage class and ActiveRecord::Relation scopes. + # Modeled after FriendlyId::FinderMethods — uses Object#friendly_id? for dispatch: + # "PROJ-42".friendly_id? → true (to_i.to_s != to_s) + # "123".friendly_id? → nil (falsy, delegates to AR) + module FinderMethods + def find(*args) + if args.length == 1 && args.first.friendly_id? + find_by_id_or_identifier!(args.first) + else + super + end + end - after_create :allocate_and_register_semantic_id, if: -> { Setting::WorkPackageIdentifier.semantic? } - end + def exists?(conditions = :none) + return super if conditions.unfriendly_id? + return true if exists_by_semantic_identifier?(conditions) - class_methods do - def semantic_id?(identifier) - identifier.to_s.to_i.to_s != identifier.to_s + super end # Resolves any identifier form to a WorkPackage. @@ -52,7 +57,7 @@ module WorkPackage::SemanticIdentifier # # Returns nil on miss. def find_by_id_or_identifier(identifier) - return find_by(id: identifier) unless semantic_id?(identifier) + return find_by(id: identifier) unless identifier.friendly_id? find_by_semantic_identifier(identifier) end @@ -74,6 +79,33 @@ module WorkPackage::SemanticIdentifier # * Reduce lookup to a single DB round trip joins(:semantic_aliases).find_by(work_package_semantic_aliases: { identifier: }) end + + # rubocop:disable Rails/WhereExists -- intentionally avoid exists?(identifier:) to prevent recursion + def exists_by_semantic_identifier?(identifier) + where(identifier:).exists? || + joins(:semantic_aliases).where(work_package_semantic_aliases: { identifier: }).exists? + end + # rubocop:enable Rails/WhereExists + end + + included do + has_many :semantic_aliases, + class_name: "WorkPackageSemanticAlias", + foreign_key: :work_package_id, + inverse_of: :work_package, + dependent: :delete_all + + after_create :allocate_and_register_semantic_id, if: -> { Setting::WorkPackageIdentifier.semantic? } + end + + class_methods do + include FinderMethods + + # Extend every relation built from this model with semantic finder methods, + # so that WorkPackage.visible(user).find("PROJ-42") works transparently. + def relation + super.extending(FinderMethods) + end end # Returns the user-facing identifier for this work package. diff --git a/app/views/work_packages/show.html.erb b/app/views/work_packages/show.html.erb index ba824c6b45d..b30dcacaeeb 100644 --- a/app/views/work_packages/show.html.erb +++ b/app/views/work_packages/show.html.erb @@ -40,5 +40,5 @@ See COPYRIGHT and LICENSE files for more details. <% end %> <% content_for :content_body do %> - <%= render WorkPackages::FullView::ShowComponent.new(id: work_package.id, tab: params[:tab] || "activity") %> + <%= render WorkPackages::FullView::ShowComponent.new(id: work_package.display_id, tab: params[:tab] || "activity") %> <% end %> diff --git a/lib/api/v3/work_packages/work_packages_api.rb b/lib/api/v3/work_packages/work_packages_api.rb index bfab66edf11..650870f5d72 100644 --- a/lib/api/v3/work_packages/work_packages_api.rb +++ b/lib/api/v3/work_packages/work_packages_api.rb @@ -63,7 +63,7 @@ module API }) .mount) - route_param :id, type: Integer, desc: "Work package ID" do + route_param :id, type: String, desc: "Work package ID" do helpers WorkPackagesSharedHelpers helpers do diff --git a/spec/controllers/work_packages_controller_spec.rb b/spec/controllers/work_packages_controller_spec.rb index 1552aa1e892..96b9877f291 100644 --- a/spec/controllers/work_packages_controller_spec.rb +++ b/spec/controllers/work_packages_controller_spec.rb @@ -64,7 +64,7 @@ RSpec.describe WorkPackagesController do describe "with the permission to see the project " \ "with having the necessary permissions" do before do - expect(WorkPackage).to receive_message_chain("visible.find_by").and_return(stub_work_package) + expect(WorkPackage).to receive_message_chain("visible.find_by_id_or_identifier").and_return(stub_work_package) mock_permissions_for(current_user) do |mock| mock.allow_in_project :view_work_packages, project: stub_work_package.project end diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 6d41e2215a6..856eaff2d2d 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -60,6 +60,100 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end + describe "WorkPackage.find" do + context "with a numeric id" do + it "finds by primary key" do + expect(WorkPackage.find(work_package.id)).to eq(work_package) + end + + it "raises RecordNotFound for unknown numeric id" do + expect { WorkPackage.find(9_999_999) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context "with a numeric string" do + it "falls through to standard AR find" do + expect(WorkPackage.find(work_package.id.to_s)).to eq(work_package) + end + end + + context "with a semantic identifier string" do + it "resolves via the semantic identifier" do + expect(WorkPackage.find("MYPROJ-1")).to eq(work_package) + end + + it "raises RecordNotFound for unknown semantic id" do + expect { WorkPackage.find("MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound) + end + + it "resolves via historic alias" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) + expect(WorkPackage.find("OLDPROJ-1")).to eq(work_package) + end + end + + context "with multiple ids" do + let(:work_package2) { create(:work_package, project:) } + + it "delegates to standard AR find" do + expect(WorkPackage.find([work_package.id, work_package2.id])).to contain_exactly(work_package, work_package2) + end + end + + context "with visibility scoping" do + let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } + let(:non_member_user) { create(:user) } + + it "respects the scope for semantic ids" do + expect(WorkPackage.visible(member_user).find("MYPROJ-1")).to eq(work_package) + end + + it "raises RecordNotFound when the user cannot see it" do + expect { WorkPackage.visible(non_member_user).find("MYPROJ-1") } + .to raise_error(ActiveRecord::RecordNotFound) + end + end + end + + describe "WorkPackage.exists?" do + context "with a numeric id" do + it "returns true for existing record" do + expect(WorkPackage.exists?(work_package.id)).to be true + end + + it "returns false for non-existing record" do + expect(WorkPackage.exists?(9_999_999)).to be false + end + end + + context "with a numeric string" do + it "falls through to standard AR exists?" do + expect(WorkPackage.exists?(work_package.id.to_s)).to be true + end + end + + context "with a semantic identifier string" do + it "checks the identifier column" do + expect(WorkPackage.exists?("MYPROJ-1")).to be true + end + + it "returns false for unknown semantic id" do + expect(WorkPackage.exists?("MYPROJ-999")).to be false + end + + it "checks the alias table for historical identifiers" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) + expect(WorkPackage.exists?("OLDPROJ-1")).to be true + end + end + + context "with hash conditions" do + it "passes through to standard AR exists?" do + expect(WorkPackage.exists?(subject: work_package.subject)).to be true + end + end + end + describe "WorkPackage.find_by_id_or_identifier" do context "with a numeric param" do it "finds by primary key" do diff --git a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb index 96bf850294b..6c660a1aa60 100644 --- a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb @@ -94,10 +94,7 @@ RSpec.describe "API v3 Work package form resource" do include_context "with post request" - it_behaves_like "param validation error" do - let(:id) { "eeek" } - let(:type) { "WorkPackage" } - end + it_behaves_like "not found" end context "with existing work package" do From 75ae132201c7dc23acc3be6205a180941c6fb0b3 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 19:32:17 +0300 Subject: [PATCH 02/37] Replace friendly_id? with semantic_id? for identifier dispatch Replace FriendlyId's Object#friendly_id? monkey-patch with a private semantic_id? method that owns the full dispatch decision: type check, whitespace stripping, and numeric detection in one place. This fixes a bug where numeric strings with whitespace (e.g. " 456 " from comma-split input) were misclassified as semantic identifiers because " 456".to_i.to_s != " 456". --- .../work_package/semantic_identifier.rb | 31 ++++++++++++------- .../work_package/semantic_identifier_spec.rb | 4 +++ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 3b73d672519..a73b86d5ece 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -31,21 +31,20 @@ module WorkPackage::SemanticIdentifier extend ActiveSupport::Concern - # Finder methods that work on both the WorkPackage class and ActiveRecord::Relation scopes. - # Modeled after FriendlyId::FinderMethods — uses Object#friendly_id? for dispatch: - # "PROJ-42".friendly_id? → true (to_i.to_s != to_s) - # "123".friendly_id? → nil (falsy, delegates to AR) + # Finder methods that work on both the WorkPackage class and ActiveRecord::Relation scopes: + # semantic_id?("PROJ-42") → true + # semantic_id?(" 456 ") → false (stripped, then numeric) + # semantic_id?("123") → false + # semantic_id?(123) → false module FinderMethods def find(*args) - if args.length == 1 && args.first.friendly_id? - find_by_id_or_identifier!(args.first) - else - super - end + return find_by_id_or_identifier!(args.first.strip) if args.length == 1 && semantic_id?(args.first) + + super end def exists?(conditions = :none) - return super if conditions.unfriendly_id? + return super unless semantic_id?(conditions) return true if exists_by_semantic_identifier?(conditions) super @@ -57,7 +56,7 @@ module WorkPackage::SemanticIdentifier # # Returns nil on miss. def find_by_id_or_identifier(identifier) - return find_by(id: identifier) unless identifier.friendly_id? + return find_by(id: identifier) unless semantic_id?(identifier) find_by_semantic_identifier(identifier) end @@ -69,6 +68,16 @@ module WorkPackage::SemanticIdentifier private + # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). + # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ") + # return false — these fall through to standard ActiveRecord lookup. + def semantic_id?(value) + return false unless value.is_a?(String) + + stripped = value.strip + stripped.to_i.to_s != stripped + end + def find_by_semantic_identifier(identifier) wp = find_by(identifier:) return wp if wp diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 856eaff2d2d..52d56b09b32 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -75,6 +75,10 @@ RSpec.describe WorkPackage::SemanticIdentifier do it "falls through to standard AR find" do expect(WorkPackage.find(work_package.id.to_s)).to eq(work_package) end + + it "strips whitespace before dispatching" do + expect(WorkPackage.find(" #{work_package.id} ")).to eq(work_package) + end end context "with a semantic identifier string" do From baa1da4a8996cf79cbc5951f23cebe7bc1813989 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 19:32:29 +0300 Subject: [PATCH 03/37] Use real DB objects in work_packages_controller_spec Replace stubbed work package and mock permission chain with real database records and role membership. This eliminates rubocop violations (ExpectInHook, StubbedMock, MessageChain) while also testing the actual finder path through WorkPackage.visible. --- .../work_packages_controller_spec.rb | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/spec/controllers/work_packages_controller_spec.rb b/spec/controllers/work_packages_controller_spec.rb index 96b9877f291..a6671b0e96e 100644 --- a/spec/controllers/work_packages_controller_spec.rb +++ b/spec/controllers/work_packages_controller_spec.rb @@ -37,14 +37,8 @@ RSpec.describe WorkPackagesController do let(:project) { create(:project, identifier: "test_project", public: false) } let(:other_project) { build_stubbed(:project) } - let(:stub_project) { build_stubbed(:project, identifier: "test_project", public: false) } - let(:type) { build_stubbed(:type) } - let(:stub_work_package) do - build_stubbed(:work_package, - id: 1337, - type:, - project: stub_project) - end + let(:type) { create(:type) } + let(:work_package) { create(:work_package, type:, project:) } let(:current_user) { create(:user) } @@ -63,11 +57,10 @@ RSpec.describe WorkPackagesController do describe "with the permission to see the project " \ "with having the necessary permissions" do + let(:role) { create(:project_role, permissions: [:view_work_packages]) } + before do - expect(WorkPackage).to receive_message_chain("visible.find_by_id_or_identifier").and_return(stub_work_package) - mock_permissions_for(current_user) do |mock| - mock.allow_in_project :view_work_packages, project: stub_work_package.project - end + create(:member, project: work_package.project, principal: current_user, roles: [role]) end instance_eval(&) @@ -261,19 +254,19 @@ RSpec.describe WorkPackagesController do end describe "show.html" do - let(:call_action) { get("show", params: { id: "1337" }) } + let(:call_action) { get("show", params: { id: work_package.id.to_s }) } requires_permission_in_project do it "redirects to the full url" do call_action - expect(response).to redirect_to("/projects/test_project/work_packages/1337/activity") + expect(response).to redirect_to("/projects/test_project/work_packages/#{work_package.id}/activity") end end end describe "show.pdf" do - let(:call_action) { get("show", params: { format: "pdf", id: "1337" }) } + let(:call_action) { get("show", params: { format: "pdf", id: work_package.id.to_s }) } let(:exporter) { WorkPackage::PDFExport::WorkPackageToPdf } let(:exporter_instance) { instance_double(exporter) } @@ -286,8 +279,8 @@ RSpec.describe WorkPackagesController do pdf_data = "foobar" time = DateTime.new(2023, 6, 30, 23, 59) allow(DateTime).to receive(:now).and_return(time) - expected_name = [stub_work_package.project.identifier, "##{stub_work_package.id}", - stub_work_package.subject, "2023-06-30_23-59"].join("_").tr(" ", "-") + expected_name = [work_package.project.identifier, "##{work_package.id}", + work_package.subject, "2023-06-30_23-59"].join("_").tr(" ", "-") expected_type = "application/pdf" pdf_result = double("pdf_result", error?: false, @@ -309,7 +302,7 @@ RSpec.describe WorkPackagesController do end describe "show.atom" do - let(:call_action) { get("show", params: { format: "atom", id: "1337" }) } + let(:call_action) { get("show", params: { format: "atom", id: work_package.id.to_s }) } requires_permission_in_project do it "render the journal/index template" do From 84814dbe1f09560392241b94334b8a2ffd7d61f5 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 20:03:52 +0300 Subject: [PATCH 04/37] Widen Rails route constraints to accept semantic work package IDs The route constraint for work_packages#show used /\d+/ which rejected semantic identifiers like "KSTP-7". Widen to also match the PROJECTID-SEQ pattern so both numeric and semantic IDs route correctly. --- config/routes.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 889ba506625..32d617c5063 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -442,7 +442,7 @@ Rails.application.routes.draw do get "/new" => "work_packages#new", on: :collection, as: "new" get "(/:tab)" => "work_packages#show", on: :member, as: "", - constraints: { id: /\d+/, state: /(?!(shares|copy|dialog)).+/ } + constraints: { id: /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/, state: /(?!(shares|copy|dialog)).+/ } # states managed by client-side routing on work_package#index get "(/*state)" => "work_packages#index", on: :collection, as: "", constraints: { state: /(?!(dialog|new)).+/ } @@ -908,7 +908,8 @@ Rails.application.routes.draw do on: :member get "/copy" => "work_packages#copy", on: :member, as: "copy" - get "(/:tab)" => "work_packages#show", on: :member, as: "", constraints: { id: /\d+/, state: /(?!(shares|new|copy)).+/ } + get "(/:tab)" => "work_packages#show", on: :member, as: "", + constraints: { id: /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/, state: /(?!(shares|new|copy)).+/ } # states managed by client-side (angular) routing on work_package#show get "/" => "work_packages#index", on: :collection, as: "index" From 8802ce5871e524b9070d2dd48afbce9225e34ce2 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 20:06:28 +0300 Subject: [PATCH 05/37] Add routing spec for semantic work package identifiers Verifies that GET /work_packages/PROJ-42 routes to work_packages#show, matching the widened route constraint. --- spec/routing/work_packages_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/routing/work_packages_spec.rb b/spec/routing/work_packages_spec.rb index 62452d074da..18646688de4 100644 --- a/spec/routing/work_packages_spec.rb +++ b/spec/routing/work_packages_spec.rb @@ -92,6 +92,12 @@ RSpec.describe WorkPackagesController do id: "1") end + it "connects GET /work_packages/:id to work_packages#show with semantic identifier" do + expect(get("/work_packages/PROJ-42")).to route_to(controller: "work_packages", + action: "show", + id: "PROJ-42") + end + it "connects GET /work_packages/:id/share to work_packages/shares#index" do expect(get("/work_packages/1/shares")).to route_to(controller: "shares", action: "index", From 890786e15679655febcdf5c9bfeb6f61c3797180 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 21:02:45 +0300 Subject: [PATCH 06/37] Extract ID_ROUTE_CONSTRAINT for semantic identifier routing Centralise the regex that matches both numeric IDs and semantic identifiers into a named constant so route definitions reference a single source of truth instead of duplicating the pattern. --- app/models/work_package/semantic_identifier.rb | 5 +++++ config/routes.rb | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index a73b86d5ece..44a1dd918dd 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -31,6 +31,11 @@ module WorkPackage::SemanticIdentifier extend ActiveSupport::Concern + # Matches either a numeric ID ("12345") or a semantic identifier ("PROJ-42"). + # Used in Rails route constraints so both forms are accepted in URLs. + # The frontend equivalent lives in WP_ID_URL_PATTERN (work-package-id-pattern.ts). + ID_ROUTE_CONSTRAINT = /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/ + # Finder methods that work on both the WorkPackage class and ActiveRecord::Relation scopes: # semantic_id?("PROJ-42") → true # semantic_id?(" 456 ") → false (stripped, then numeric) diff --git a/config/routes.rb b/config/routes.rb index 32d617c5063..acfbecf3b10 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -442,7 +442,7 @@ Rails.application.routes.draw do get "/new" => "work_packages#new", on: :collection, as: "new" get "(/:tab)" => "work_packages#show", on: :member, as: "", - constraints: { id: /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/, state: /(?!(shares|copy|dialog)).+/ } + constraints: { id: WorkPackage::SemanticIdentifier::ID_ROUTE_CONSTRAINT, state: /(?!(shares|copy|dialog)).+/ } # states managed by client-side routing on work_package#index get "(/*state)" => "work_packages#index", on: :collection, as: "", constraints: { state: /(?!(dialog|new)).+/ } @@ -909,7 +909,7 @@ Rails.application.routes.draw do get "/copy" => "work_packages#copy", on: :member, as: "copy" get "(/:tab)" => "work_packages#show", on: :member, as: "", - constraints: { id: /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/, state: /(?!(shares|new|copy)).+/ } + constraints: { id: WorkPackage::SemanticIdentifier::ID_ROUTE_CONSTRAINT, state: /(?!(shares|new|copy)).+/ } # states managed by client-side (angular) routing on work_package#show get "/" => "work_packages#index", on: :collection, as: "index" From 7fd6bccba075bb025394dca8afb32959fee3827e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 10 Apr 2026 21:12:59 +0300 Subject: [PATCH 07/37] Use structured RecordNotFound for semantic identifier lookup Pass model, primary_key, and id to ActiveRecord::RecordNotFound so error reporters and rescue handlers get the same structured data that Rails' own find provides. --- app/models/work_package/semantic_identifier.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 44a1dd918dd..bc230890428 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -68,7 +68,10 @@ module WorkPackage::SemanticIdentifier # Same as find_by_id_or_identifier but raises ActiveRecord::RecordNotFound on miss. def find_by_id_or_identifier!(identifier) - find_by_id_or_identifier(identifier) || raise(ActiveRecord::RecordNotFound, "WorkPackage not found: #{identifier}") + find_by_id_or_identifier(identifier) || + raise(ActiveRecord::RecordNotFound.new( + "Couldn't find WorkPackage with identifier=#{identifier}", "WorkPackage", "identifier", identifier + )) end private From 48946fe55835be0718a2882454c77aa416c4b75a Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 13 Apr 2026 13:23:12 +0300 Subject: [PATCH 08/37] Fix form spec to expect WP-specific not-found message "eeek" now matches the semantic identifier pattern, so find_by_id_or_identifier! raises RecordNotFound with model: "WorkPackage", producing the WP-specific error message instead of the generic 404. --- .../v3/work_packages/form/work_package_form_resource_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb index 6c660a1aa60..8c5e5bd97d0 100644 --- a/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/form/work_package_form_resource_spec.rb @@ -94,7 +94,8 @@ RSpec.describe "API v3 Work package form resource" do include_context "with post request" - it_behaves_like "not found" + it_behaves_like "not found", + I18n.t("api_v3.errors.not_found.work_package") end context "with existing work package" do From 7ee8f1b8a1894a085e92594ed4261c52243483f8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 14:37:12 +0300 Subject: [PATCH 09/37] DRY up semantic identifier lookup and fix exists? fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract shared by_semantic_alias method to eliminate duplication of the joins(:semantic_aliases).where(...) pattern between find_by_semantic_identifier and exists_by_semantic_identifier?. Remove the wasteful super fallback in exists? — when a semantic identifier like "PROJ-999" isn't found via the semantic lookup, the old code fell through to super which type-cast the string to 0 and queried WHERE id = 0. Correct by accident but a wasted query. --- .../work_package/semantic_identifier.rb | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index bc230890428..5c01587b89b 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -50,9 +50,8 @@ module WorkPackage::SemanticIdentifier def exists?(conditions = :none) return super unless semantic_id?(conditions) - return true if exists_by_semantic_identifier?(conditions) - super + exists_by_semantic_identifier?(conditions) end # Resolves any identifier form to a WorkPackage. @@ -86,23 +85,22 @@ module WorkPackage::SemanticIdentifier stripped.to_i.to_s != stripped end + # Looks up by current identifier column first, then falls back to + # the alias table for historical identifiers. Two-step because AR's + # .or() requires structurally compatible relations (joins breaks it). def find_by_semantic_identifier(identifier) - wp = find_by(identifier:) - return wp if wp - - # Fallback: alias table lookup. The table holds every identifier a WP has ever been known by: - # Done via a single join to: - # * Respect any parent scoping (e.g. when called as WorkPackage.visible.find_by_semantic_identifier) - # * Reduce lookup to a single DB round trip - joins(:semantic_aliases).find_by(work_package_semantic_aliases: { identifier: }) + find_by(identifier:) || + by_semantic_alias(identifier).first end - # rubocop:disable Rails/WhereExists -- intentionally avoid exists?(identifier:) to prevent recursion def exists_by_semantic_identifier?(identifier) where(identifier:).exists? || - joins(:semantic_aliases).where(work_package_semantic_aliases: { identifier: }).exists? + by_semantic_alias(identifier).exists? + end + + def by_semantic_alias(identifier) + joins(:semantic_aliases).where(work_package_semantic_aliases: { identifier: }) end - # rubocop:enable Rails/WhereExists end included do From a898486c3a9b0e7776fc3641c9f8f0b7798ba273 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 14:42:44 +0300 Subject: [PATCH 10/37] Override find_by/find_by! for semantic ID resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add find_by and find_by! overrides to FinderMethods so that find_by(id: "PROJ-42") transparently resolves semantic identifiers, matching the existing behavior of find and exists?. Only intercepts calls where id: is the sole keyword — all other find_by usage (e.g. find_by(subject: ...)) passes through unchanged. Revert ShowComponent, SplitViewComponent, and WorkPackagesController from the explicit find_by_id_or_identifier back to standard find_by(id:) now that the override handles semantic resolution. Make find_by_id_or_identifier and find_by_id_or_identifier! private since they are now internal implementation details with no external callers. Move their test coverage into the find_by/find_by! specs. --- .../work_packages/full_view/show_component.rb | 2 +- .../work_packages/split_view_component.rb | 2 +- app/controllers/work_packages_controller.rb | 2 +- .../work_package/semantic_identifier.rb | 28 +++++- .../work_package/semantic_identifier_spec.rb | 93 ++++++++++--------- .../semantic_ids/integration_spec.rb | 30 +++--- 6 files changed, 95 insertions(+), 62 deletions(-) diff --git a/app/components/work_packages/full_view/show_component.rb b/app/components/work_packages/full_view/show_component.rb index 706daceebee..2547c4a3e68 100644 --- a/app/components/work_packages/full_view/show_component.rb +++ b/app/components/work_packages/full_view/show_component.rb @@ -13,7 +13,7 @@ module WorkPackages @id = id @tab = tab - @work_package = WorkPackage.visible.find_by_id_or_identifier(id) + @work_package = WorkPackage.visible.find_by(id:) end def wrapper_uniq_by diff --git a/app/components/work_packages/split_view_component.rb b/app/components/work_packages/split_view_component.rb index 0dfa3784fdc..48d90ebbeb7 100644 --- a/app/components/work_packages/split_view_component.rb +++ b/app/components/work_packages/split_view_component.rb @@ -39,7 +39,7 @@ class WorkPackages::SplitViewComponent < ApplicationComponent @id = id @tab = tab - @work_package = WorkPackage.visible.find_by_id_or_identifier(id) + @work_package = WorkPackage.visible.find_by(id:) @base_route = base_route end diff --git a/app/controllers/work_packages_controller.rb b/app/controllers/work_packages_controller.rb index f221f77232e..0f2f9b8a8a9 100644 --- a/app/controllers/work_packages_controller.rb +++ b/app/controllers/work_packages_controller.rb @@ -243,7 +243,7 @@ class WorkPackagesController < ApplicationController def work_package return @work_package if defined?(@work_package) - @work_package = WorkPackage.visible(current_user).find_by_id_or_identifier(params[:id]) + @work_package = WorkPackage.visible(current_user).find_by(id: params[:id]) end def journals diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 5c01587b89b..657a278d260 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -48,12 +48,38 @@ module WorkPackage::SemanticIdentifier super end + # Override find_by to transparently resolve semantic identifiers when called + # with `id:` as the sole keyword (e.g. `find_by(id: "PROJ-42")`). + # All other find_by calls pass through to ActiveRecord unchanged. + # + # AR's find_by signature is find_by(arg, *args) — it doesn't use keyword splat, + # so hash kwargs arrive as the positional `arg`. We match on that. + def find_by(*args) + if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) + find_by_id_or_identifier(args.first[:id]) + else + super + end + end + + # Mirror of find_by — Rails implements find_by! independently (not via find_by), + # so we must override both to keep the pair consistent. + def find_by!(*args) + if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) + find_by_id_or_identifier!(args.first[:id]) + else + super + end + end + def exists?(conditions = :none) return super unless semantic_id?(conditions) exists_by_semantic_identifier?(conditions) end + private + # Resolves any identifier form to a WorkPackage. # - Numeric string ("12345") → find by primary key # - Semantic string ("PROJ-42") → lookup via work_packages table and alias table @@ -73,8 +99,6 @@ module WorkPackage::SemanticIdentifier )) end - private - # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ") # return false — these fall through to standard ActiveRecord lookup. diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 52d56b09b32..add789fcea1 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -158,50 +158,50 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end - describe "WorkPackage.find_by_id_or_identifier" do - context "with a numeric param" do - it "finds by primary key" do - expect(WorkPackage.find_by_id_or_identifier(work_package.id.to_s)).to eq(work_package) + describe "WorkPackage.find_by" do + context "with id: keyword and semantic identifier" do + it "resolves via the semantic identifier" do + expect(WorkPackage.find_by(id: "MYPROJ-1")).to eq(work_package) end - it "returns nil for unknown id" do - expect(WorkPackage.find_by_id_or_identifier("9999999")).to be_nil + it "returns nil for unknown semantic id" do + expect(WorkPackage.find_by(id: "MYPROJ-999")).to be_nil + end + + it "resolves via historic alias" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) + expect(WorkPackage.find_by(id: "OLDPROJ-1")).to eq(work_package) end end - context "with a semantic param" do - context "when the identifier matches work_packages.identifier (fast path)" do - it "finds directly via identifier without hitting the alias table" do - expect(WorkPackage.find_by_id_or_identifier("MYPROJ-1")).to eq(work_package) - end - - it "returns nil when no WP has that identifier and no alias or fallback matches" do - expect(WorkPackage.find_by_id_or_identifier("MYPROJ-999")).to be_nil - end + context "with id: keyword and numeric string" do + it "falls through to standard AR find_by" do + expect(WorkPackage.find_by(id: work_package.id.to_s)).to eq(work_package) end + end - context "when the identifier is a historic alias (alias table path)" do - it "resolves historic entries via the alias registry" do - WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) - expect(WorkPackage.find_by_id_or_identifier("OLDPROJ-1")).to eq(work_package) - end - - it "resolves when identifier differs but an alias row exists" do - work_package.update_columns(identifier: "OTHER-99") - expect(WorkPackage.find_by_id_or_identifier("MYPROJ-1")).to eq(work_package) - end + context "with non-id keywords" do + it "passes through to standard AR find_by" do + expect(WorkPackage.find_by(subject: work_package.subject)).to eq(work_package) end + end - it "returns nil for unknown sequence" do - expect(WorkPackage.find_by_id_or_identifier("MYPROJ-999")).to be_nil + context "with multiple keywords including id:" do + it "passes through to standard AR find_by" do + expect(WorkPackage.find_by(id: work_package.id, project: project)).to eq(work_package) end + end - it "returns nil for unknown project prefix" do - expect(WorkPackage.find_by_id_or_identifier("NOPE-1")).to be_nil + context "with an unparseable semantic string" do + it "returns nil" do + expect(WorkPackage.find_by(id: "not-an-identifier!")).to be_nil end + end - it "returns nil for an unparseable string" do - expect(WorkPackage.find_by_id_or_identifier("not-an-identifier!")).to be_nil + context "when identifier differs but an alias row exists" do + it "resolves via the alias table" do + work_package.update_columns(identifier: "OTHER-99") + expect(WorkPackage.find_by(id: "MYPROJ-1")).to eq(work_package) end end @@ -209,30 +209,39 @@ RSpec.describe WorkPackage::SemanticIdentifier do let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } let(:non_member_user) { create(:user) } - it "returns the WP for a user who can see it" do - expect(WorkPackage.visible(member_user).find_by_id_or_identifier("MYPROJ-1")).to eq(work_package) + it "respects the scope for semantic ids" do + expect(WorkPackage.visible(member_user).find_by(id: "MYPROJ-1")).to eq(work_package) end - it "returns nil for a user who cannot see it" do - expect(WorkPackage.visible(non_member_user).find_by_id_or_identifier("MYPROJ-1")).to be_nil + it "returns nil when the user cannot see it" do + expect(WorkPackage.visible(non_member_user).find_by(id: "MYPROJ-1")).to be_nil end it "also scopes numeric lookup" do - expect(WorkPackage.visible(non_member_user).find_by_id_or_identifier(work_package.id.to_s)).to be_nil + expect(WorkPackage.visible(non_member_user).find_by(id: work_package.id.to_s)).to be_nil end end end - describe "WorkPackage.find_by_id_or_identifier!" do - it "returns the work package when found" do - expect(WorkPackage.find_by_id_or_identifier!(work_package.id.to_s)).to eq(work_package) + # rubocop:disable Rails/FindById -- testing find_by! override specifically, not suggesting find() + describe "WorkPackage.find_by!" do + context "with id: keyword and semantic identifier" do + it "resolves via the semantic identifier" do + expect(WorkPackage.find_by!(id: "MYPROJ-1")).to eq(work_package) + end + + it "raises RecordNotFound for unknown semantic id" do + expect { WorkPackage.find_by!(id: "MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound) + end end - it "raises ActiveRecord::RecordNotFound when not found" do - expect { WorkPackage.find_by_id_or_identifier!("MYPROJ-999") } - .to raise_error(ActiveRecord::RecordNotFound) + context "with non-id keywords" do + it "passes through to standard AR find_by!" do + expect(WorkPackage.find_by!(subject: work_package.subject)).to eq(work_package) + end end end + # rubocop:enable Rails/FindById describe "#display_id" do context "when semantic mode is active", diff --git a/spec/services/work_packages/semantic_ids/integration_spec.rb b/spec/services/work_packages/semantic_ids/integration_spec.rb index 551d28496df..9556f2760e3 100644 --- a/spec/services/work_packages/semantic_ids/integration_spec.rb +++ b/spec/services/work_packages/semantic_ids/integration_spec.rb @@ -97,12 +97,12 @@ RSpec.describe "SemanticIds registry integration", type: :model do it "old identifier still resolves to the WP" do WorkPackages::UpdateService.new(user:, model: work_package).call(project: target_project) - expect(WorkPackage.find_by_id_or_identifier("PROJ-5")).to eq(work_package) + expect(WorkPackage.find_by(id: "PROJ-5")).to eq(work_package) end it "new identifier also resolves to the WP" do WorkPackages::UpdateService.new(user:, model: work_package).call(project: target_project) - expect(WorkPackage.find_by_id_or_identifier(work_package.reload.identifier)).to eq(work_package) + expect(WorkPackage.find_by(id: work_package.reload.identifier)).to eq(work_package) end end @@ -130,15 +130,15 @@ RSpec.describe "SemanticIds registry integration", type: :model do it "old identifiers still resolve to the correct WPs" do Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") - expect(WorkPackage.find_by_id_or_identifier("PROJ-1")).to eq(wp1) - expect(WorkPackage.find_by_id_or_identifier("PROJ-2")).to eq(wp2) + expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1) + expect(WorkPackage.find_by(id: "PROJ-2")).to eq(wp2) end it "new identifiers resolve to the correct WPs" do Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") - expect(WorkPackage.find_by_id_or_identifier("RENAMED-1")).to eq(wp1) - expect(WorkPackage.find_by_id_or_identifier("RENAMED-2")).to eq(wp2) + expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1) + expect(WorkPackage.find_by(id: "RENAMED-2")).to eq(wp2) end it "old prefix resolves for WPs created after the rename" do @@ -147,8 +147,8 @@ RSpec.describe "SemanticIds registry integration", type: :model do Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") wp3 = create(:work_package, project: project.reload) - expect(WorkPackage.find_by_id_or_identifier("RENAMED-3")).to eq(wp3) - expect(WorkPackage.find_by_id_or_identifier("PROJ-3")).to eq(wp3) + expect(WorkPackage.find_by(id: "RENAMED-3")).to eq(wp3) + expect(WorkPackage.find_by(id: "PROJ-3")).to eq(wp3) end end @@ -161,7 +161,7 @@ RSpec.describe "SemanticIds registry integration", type: :model do # PROJ is then renamed to RENAMED (bulk-inserts RENAMED-1 from the retired PROJ-1 row) Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") - expect(WorkPackage.find_by_id_or_identifier("RENAMED-1")).to eq(wp1) + expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1) end it "rename then move: both old identifiers resolve after the WP moves" do @@ -170,8 +170,8 @@ RSpec.describe "SemanticIds registry integration", type: :model do # WP moves to DEST (appends DEST-1 registry row, updates identifier) WorkPackages::UpdateService.new(user:, model: wp1.reload).call(project: target_project) - expect(WorkPackage.find_by_id_or_identifier("PROJ-1")).to eq(wp1) - expect(WorkPackage.find_by_id_or_identifier("RENAMED-1")).to eq(wp1) + expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1) + expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1) end it "rename then new WP then move: pre-rename identifier resolves via alias table" do @@ -183,7 +183,7 @@ RSpec.describe "SemanticIds registry integration", type: :model do # wp2 moves to DEST — old identifier RENAMED-2 kept as alias, gets DEST-1 WorkPackages::UpdateService.new(user:, model: wp2).call(project: target_project) - expect(WorkPackage.find_by_id_or_identifier("PROJ-2")).to eq(wp2) + expect(WorkPackage.find_by(id: "PROJ-2")).to eq(wp2) end end @@ -202,9 +202,9 @@ RSpec.describe "SemanticIds registry integration", type: :model do WorkPackages::UpdateService.new(user:, model: wp1.reload).call(project: project_c) projc_identifier = wp1.reload.identifier - expect(WorkPackage.find_by_id_or_identifier("PROJ-1")).to eq(wp1) - expect(WorkPackage.find_by_id_or_identifier(dest_identifier)).to eq(wp1) - expect(WorkPackage.find_by_id_or_identifier(projc_identifier)).to eq(wp1) + expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1) + expect(WorkPackage.find_by(id: dest_identifier)).to eq(wp1) + expect(WorkPackage.find_by(id: projc_identifier)).to eq(wp1) end end end From 26deb8bc57ca32350515413e6ccdaa02f0375916 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 14:45:04 +0300 Subject: [PATCH 11/37] Extract FinderMethods to its own file and reorder included block Move the growing FinderMethods module (find, find_by, find_by!, exists?, and supporting private methods) into its own file at app/models/work_package/semantic_identifier/finder_methods.rb. Reorder the concern to place the included block at the top, matching the convention used by other WorkPackage concerns (Ancestors, Validations, Hooks, Journalized). --- .../work_package/semantic_identifier.rb | 91 ------------ .../semantic_identifier/finder_methods.rb | 130 ++++++++++++++++++ 2 files changed, 130 insertions(+), 91 deletions(-) create mode 100644 app/models/work_package/semantic_identifier/finder_methods.rb diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 657a278d260..f63383cd403 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -36,97 +36,6 @@ module WorkPackage::SemanticIdentifier # The frontend equivalent lives in WP_ID_URL_PATTERN (work-package-id-pattern.ts). ID_ROUTE_CONSTRAINT = /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/ - # Finder methods that work on both the WorkPackage class and ActiveRecord::Relation scopes: - # semantic_id?("PROJ-42") → true - # semantic_id?(" 456 ") → false (stripped, then numeric) - # semantic_id?("123") → false - # semantic_id?(123) → false - module FinderMethods - def find(*args) - return find_by_id_or_identifier!(args.first.strip) if args.length == 1 && semantic_id?(args.first) - - super - end - - # Override find_by to transparently resolve semantic identifiers when called - # with `id:` as the sole keyword (e.g. `find_by(id: "PROJ-42")`). - # All other find_by calls pass through to ActiveRecord unchanged. - # - # AR's find_by signature is find_by(arg, *args) — it doesn't use keyword splat, - # so hash kwargs arrive as the positional `arg`. We match on that. - def find_by(*args) - if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) - find_by_id_or_identifier(args.first[:id]) - else - super - end - end - - # Mirror of find_by — Rails implements find_by! independently (not via find_by), - # so we must override both to keep the pair consistent. - def find_by!(*args) - if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) - find_by_id_or_identifier!(args.first[:id]) - else - super - end - end - - def exists?(conditions = :none) - return super unless semantic_id?(conditions) - - exists_by_semantic_identifier?(conditions) - end - - private - - # Resolves any identifier form to a WorkPackage. - # - Numeric string ("12345") → find by primary key - # - Semantic string ("PROJ-42") → lookup via work_packages table and alias table - # - # Returns nil on miss. - def find_by_id_or_identifier(identifier) - return find_by(id: identifier) unless semantic_id?(identifier) - - find_by_semantic_identifier(identifier) - end - - # Same as find_by_id_or_identifier but raises ActiveRecord::RecordNotFound on miss. - def find_by_id_or_identifier!(identifier) - find_by_id_or_identifier(identifier) || - raise(ActiveRecord::RecordNotFound.new( - "Couldn't find WorkPackage with identifier=#{identifier}", "WorkPackage", "identifier", identifier - )) - end - - # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). - # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ") - # return false — these fall through to standard ActiveRecord lookup. - def semantic_id?(value) - return false unless value.is_a?(String) - - stripped = value.strip - stripped.to_i.to_s != stripped - end - - # Looks up by current identifier column first, then falls back to - # the alias table for historical identifiers. Two-step because AR's - # .or() requires structurally compatible relations (joins breaks it). - def find_by_semantic_identifier(identifier) - find_by(identifier:) || - by_semantic_alias(identifier).first - end - - def exists_by_semantic_identifier?(identifier) - where(identifier:).exists? || - by_semantic_alias(identifier).exists? - end - - def by_semantic_alias(identifier) - joins(:semantic_aliases).where(work_package_semantic_aliases: { identifier: }) - end - end - included do has_many :semantic_aliases, class_name: "WorkPackageSemanticAlias", diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb new file mode 100644 index 00000000000..e5631c5a092 --- /dev/null +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# 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-2013 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 COPYRIGHT and LICENSE files for more details. +#++ + +# Extends ActiveRecord finder methods (find, find_by, find_by!, exists?) to +# transparently resolve semantic work package identifiers (e.g. "PROJ-42") +# in addition to numeric IDs. +# +# Included into WorkPackage class methods and extended into every +# ActiveRecord::Relation via WorkPackage::SemanticIdentifier. +# +# Examples: +# WorkPackage.find("PROJ-42") +# WorkPackage.visible(user).find_by(id: "PROJ-42") +# WorkPackage.exists?("PROJ-42") +module WorkPackage::SemanticIdentifier::FinderMethods + def find(*args) + return find_by_id_or_identifier!(args.first) if args.length == 1 && semantic_id?(args.first) + + super + end + + # Override find_by to transparently resolve semantic identifiers when called + # with `id:` as the sole keyword (e.g. `find_by(id: "PROJ-42")`). + # All other find_by calls pass through to ActiveRecord unchanged. + # + # AR's find_by signature is find_by(arg, *args) — it doesn't use keyword splat, + # so hash kwargs arrive as the positional `arg`. We match on that. + def find_by(*args) + if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) + find_by_id_or_identifier(args.first[:id]) + else + super + end + end + + # Mirror of find_by — Rails implements find_by! independently (not via find_by), + # so we must override both to keep the pair consistent. + def find_by!(*args) + if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) + find_by_id_or_identifier!(args.first[:id]) + else + super + end + end + + def exists?(conditions = :none) + return super unless semantic_id?(conditions) + + exists_by_semantic_identifier?(conditions) + end + + private + + # Resolves any identifier form to a WorkPackage. + # - Numeric string ("12345") → find by primary key + # - Semantic string ("PROJ-42") → lookup via work_packages table and alias table + # + # Returns nil on miss. + def find_by_id_or_identifier(identifier) + return find_by(id: identifier) unless semantic_id?(identifier) + + find_by_semantic_identifier(identifier) + end + + # Same as find_by_id_or_identifier but raises ActiveRecord::RecordNotFound on miss. + def find_by_id_or_identifier!(identifier) + find_by_id_or_identifier(identifier) || + raise(ActiveRecord::RecordNotFound.new( + "Couldn't find WorkPackage with identifier=#{identifier}", "WorkPackage", "identifier", identifier + )) + end + + # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). + # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ") + # return false — these fall through to standard ActiveRecord lookup. + def semantic_id?(value) + return false unless value.is_a?(String) + + stripped = value.strip + stripped.to_i.to_s != stripped + end + + # Looks up by current identifier column first, then falls back to + # the alias table for historical identifiers. Two-step because AR's + # .or() requires structurally compatible relations (joins breaks it). + def find_by_semantic_identifier(identifier) + find_by(identifier:) || + by_semantic_alias(identifier).first + end + + def exists_by_semantic_identifier?(identifier) + # Use where().exists? instead of exists?(identifier:) to keep the intent + # clear — our exists? override intercepts string conditions, and while + # hash conditions would pass through safely, the explicit form avoids + # any ambiguity about recursion. + where(identifier:).exists? || # rubocop:disable Rails/WhereExists + by_semantic_alias(identifier).exists? + end + + def by_semantic_alias(identifier) + joins(:semantic_aliases).where(work_package_semantic_aliases: { identifier: }) + end +end From 6c253446ef27cd2a812766caaefe3d5073b04495 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 14:52:24 +0300 Subject: [PATCH 12/37] Tighten route constraint to uppercase-only semantic identifiers Project identifiers in semantic mode are validated as [A-Z][A-Z0-9_]* (see Projects::Identifier#identifier_alphanumeric_format). Tighten ID_ROUTE_CONSTRAINT to match only uppercase identifiers, rejecting lowercase patterns at the constrained route level. Note: lowercase identifiers still reach the controller via the unconstrained resource route fallback and will 404 there. --- app/models/work_package/semantic_identifier.rb | 2 +- spec/routing/work_packages_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index f63383cd403..989adbaf249 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -34,7 +34,7 @@ module WorkPackage::SemanticIdentifier # Matches either a numeric ID ("12345") or a semantic identifier ("PROJ-42"). # Used in Rails route constraints so both forms are accepted in URLs. # The frontend equivalent lives in WP_ID_URL_PATTERN (work-package-id-pattern.ts). - ID_ROUTE_CONSTRAINT = /\d+|[A-Za-z][A-Za-z0-9_]*-\d+/ + ID_ROUTE_CONSTRAINT = /\d+|[A-Z][A-Z0-9_]*-\d+/ included do has_many :semantic_aliases, diff --git a/spec/routing/work_packages_spec.rb b/spec/routing/work_packages_spec.rb index 18646688de4..9cf2be23bf1 100644 --- a/spec/routing/work_packages_spec.rb +++ b/spec/routing/work_packages_spec.rb @@ -98,6 +98,12 @@ RSpec.describe WorkPackagesController do id: "PROJ-42") end + it "routes lowercase identifiers to show (caught by resource route, resolved at controller level)" do + expect(get("/work_packages/proj-42")).to route_to(controller: "work_packages", + action: "show", + id: "proj-42") + end + it "connects GET /work_packages/:id/share to work_packages/shares#index" do expect(get("/work_packages/1/shares")).to route_to(controller: "shares", action: "index", From db88a2da7e893622e2aca3b589eb8b4880fb1b3d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 15:07:04 +0300 Subject: [PATCH 13/37] Guard multi-argument find with ArgumentError for semantic IDs Rather than implementing full multi-argument semantic find support (which adds significant complexity for a use case with no current callers), raise ArgumentError with a clear message when semantic identifiers are passed in multi-arg find calls. This makes the limitation explicit and discoverable. Full multi-arg support is being evaluated separately. --- .../semantic_identifier/finder_methods.rb | 12 +++++++++++- spec/models/work_package/semantic_identifier_spec.rb | 12 +++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index e5631c5a092..b4da6eb88ff 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -41,7 +41,17 @@ # WorkPackage.exists?("PROJ-42") module WorkPackage::SemanticIdentifier::FinderMethods def find(*args) - return find_by_id_or_identifier!(args.first) if args.length == 1 && semantic_id?(args.first) + ids = args.length == 1 && args.first.is_a?(Array) ? args.first : args + + if ids.length == 1 && semantic_id?(ids.first) + return find_by_id_or_identifier!(ids.first) + end + + if ids.any? { |id| semantic_id?(id) } + raise ArgumentError, + "Semantic identifiers in multi-argument find are not yet supported. " \ + "Resolve each identifier individually via find_by(id:) instead." + end super end diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index add789fcea1..fb3b0167ddc 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -99,9 +99,19 @@ RSpec.describe WorkPackage::SemanticIdentifier do context "with multiple ids" do let(:work_package2) { create(:work_package, project:) } - it "delegates to standard AR find" do + it "delegates to standard AR find for numeric ids" do expect(WorkPackage.find([work_package.id, work_package2.id])).to contain_exactly(work_package, work_package2) end + + it "raises ArgumentError for multiple semantic ids" do + expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } + .to raise_error(ArgumentError, /multi-argument find are not yet supported/) + end + + it "raises ArgumentError for mixed numeric and semantic ids" do + expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } + .to raise_error(ArgumentError, /multi-argument find are not yet supported/) + end end context "with visibility scoping" do From 81fbb035ccdb69c22095c8db145e5a05ce764cdc Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 15:32:40 +0300 Subject: [PATCH 14/37] Add integration tests for semantic identifier resolution Verify that semantic work package identifiers (e.g. "TESTPROJ-1") are resolved end-to-end through the controller and API layers, using with_settings/with_flag helpers instead of allow mocks. --- .../work_packages_controller_spec.rb | 15 ++++++++++++++ .../v3/work_packages/show_resource_spec.rb | 20 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/spec/controllers/work_packages_controller_spec.rb b/spec/controllers/work_packages_controller_spec.rb index a6671b0e96e..53320ea4f6d 100644 --- a/spec/controllers/work_packages_controller_spec.rb +++ b/spec/controllers/work_packages_controller_spec.rb @@ -265,6 +265,21 @@ RSpec.describe WorkPackagesController do end end + describe "show.html with semantic identifier", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + let(:project) { create(:project, identifier: "TESTPROJ") } + let(:call_action) { get("show", params: { id: work_package.display_id.to_s }) } + + requires_permission_in_project do + it "resolves the semantic identifier and redirects to the full url" do + call_action + + expect(response).to redirect_to("/projects/TESTPROJ/work_packages/#{work_package.display_id}/activity") + end + end + end + describe "show.pdf" do let(:call_action) { get("show", params: { format: "pdf", id: work_package.id.to_s }) } let(:exporter) { WorkPackage::PDFExport::WorkPackageToPdf } diff --git a/spec/requests/api/v3/work_packages/show_resource_spec.rb b/spec/requests/api/v3/work_packages/show_resource_spec.rb index 5eba4712539..63105f96f7b 100644 --- a/spec/requests/api/v3/work_packages/show_resource_spec.rb +++ b/spec/requests/api/v3/work_packages/show_resource_spec.rb @@ -217,6 +217,26 @@ RSpec.describe "API v3 Work package resource", it_behaves_like "not found response based on login_required", I18n.t("api_v3.errors.not_found.work_package") end + + context "with a semantic identifier", + with_flag: { semantic_work_package_ids: true }, + with_settings: { work_packages_identifier: "semantic" } do + let(:get_path) { api_v3_paths.work_package work_package.display_id } + + before do + get get_path + end + + it "resolves the semantic identifier and responds with 200" do + expect(last_response).to have_http_status(:ok) + end + + it "returns the correct work package" do + expect(last_response.body) + .to be_json_eql(work_package.id.to_json) + .at_path("id") + end + end end describe "GET /api/v3/work_packages/:id?timestamps=" do From 3b6ba7dace08374f76f57b67d06828bbb4a8aed6 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 15:38:36 +0300 Subject: [PATCH 15/37] Extract semantic_id_lookup? predicate from find_by/find_by! DRY up the duplicated 4-clause guard condition into a named private predicate, reducing drift risk between the two overrides that must stay in sync. --- .../semantic_identifier/finder_methods.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index b4da6eb88ff..2e7eb431ceb 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -63,7 +63,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods # AR's find_by signature is find_by(arg, *args) — it doesn't use keyword splat, # so hash kwargs arrive as the positional `arg`. We match on that. def find_by(*args) - if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) + if semantic_id_hash_lookup?(args) find_by_id_or_identifier(args.first[:id]) else super @@ -73,7 +73,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods # Mirror of find_by — Rails implements find_by! independently (not via find_by), # so we must override both to keep the pair consistent. def find_by!(*args) - if args.length == 1 && args.first.is_a?(Hash) && args.first.keys == [:id] && semantic_id?(args.first[:id]) + if semantic_id_hash_lookup?(args) find_by_id_or_identifier!(args.first[:id]) else super @@ -88,6 +88,15 @@ module WorkPackage::SemanticIdentifier::FinderMethods private + # Returns true when args represent a single `id:` keyword lookup + # with a semantic identifier value (e.g. `find_by(id: "PROJ-42")`). + def semantic_id_hash_lookup?(args) + args.length == 1 && + args.first.is_a?(Hash) && + args.first.keys == [:id] && + semantic_id?(args.first[:id]) + end + # Resolves any identifier form to a WorkPackage. # - Numeric string ("12345") → find by primary key # - Semantic string ("PROJ-42") → lookup via work_packages table and alias table From accb483a372e8b9806a762eae2be1c46b61beaac Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 16:37:15 +0300 Subject: [PATCH 16/37] Use single OR + EXISTS query for semantic identifier lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the two-step query fallback (identifier column, then JOIN on alias table) with a single query using a correlated EXISTS subquery. AR's .or() requires structurally compatible relations, which broke with joins on one side. EXISTS keeps both sides join-free, letting AR merge them cleanly into one SELECT … WHERE identifier = ? OR EXISTS(…). Halves the query count for alias-based lookups with no behavior change. --- .../semantic_identifier/finder_methods.rb | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 2e7eb431ceb..8ab6c9bf879 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -126,24 +126,38 @@ module WorkPackage::SemanticIdentifier::FinderMethods stripped.to_i.to_s != stripped end - # Looks up by current identifier column first, then falls back to - # the alias table for historical identifiers. Two-step because AR's - # .or() requires structurally compatible relations (joins breaks it). + # Resolves a semantic identifier (e.g. "PROJ-42") to a WorkPackage in + # a single query. Matches against the current identifier column OR a + # correlated EXISTS on the alias table for historical identifiers. + # Returns nil on miss. + # + # Generates: + # + # SELECT "work_packages".* FROM "work_packages" + # WHERE ("work_packages"."identifier" = 'PROJ-42' + # OR EXISTS ( + # SELECT 1 FROM "work_package_semantic_aliases" + # WHERE "work_package_semantic_aliases"."work_package_id" = "work_packages"."id" + # AND "work_package_semantic_aliases"."identifier" = 'PROJ-42' + # )) + # ORDER BY "work_packages"."id" ASC LIMIT 1 def find_by_semantic_identifier(identifier) - find_by(identifier:) || - by_semantic_alias(identifier).first + where(identifier:).or(where(semantic_alias_exists(identifier))).first end def exists_by_semantic_identifier?(identifier) - # Use where().exists? instead of exists?(identifier:) to keep the intent - # clear — our exists? override intercepts string conditions, and while - # hash conditions would pass through safely, the explicit form avoids - # any ambiguity about recursion. - where(identifier:).exists? || # rubocop:disable Rails/WhereExists - by_semantic_alias(identifier).exists? + where(identifier:).or(where(semantic_alias_exists(identifier))).exists? end - def by_semantic_alias(identifier) - joins(:semantic_aliases).where(work_package_semantic_aliases: { identifier: }) + # Correlated EXISTS subquery that matches work packages having a + # semantic alias row with the given identifier. + def semantic_alias_exists(identifier) + alias_table = WorkPackageSemanticAlias.arel_table + + WorkPackageSemanticAlias + .where(alias_table[:work_package_id].eq(arel_table[:id])) + .where(identifier:) + .arel + .exists end end From 8d13c2ac93a7b6416e937c5fd09bb6db3e0370a9 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Apr 2026 16:42:18 +0300 Subject: [PATCH 17/37] Fix grammar in multi-argument error and clarify API route param MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - "find are not yet supported" → "find is not yet supported" - Add semantic identifier example to API route param description --- app/models/work_package/semantic_identifier/finder_methods.rb | 2 +- lib/api/v3/work_packages/work_packages_api.rb | 2 +- spec/models/work_package/semantic_identifier_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 8ab6c9bf879..250b7e57473 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -49,7 +49,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods if ids.any? { |id| semantic_id?(id) } raise ArgumentError, - "Semantic identifiers in multi-argument find are not yet supported. " \ + "Semantic identifiers in multi-argument find is not yet supported. " \ "Resolve each identifier individually via find_by(id:) instead." end diff --git a/lib/api/v3/work_packages/work_packages_api.rb b/lib/api/v3/work_packages/work_packages_api.rb index 650870f5d72..f9a2021bc09 100644 --- a/lib/api/v3/work_packages/work_packages_api.rb +++ b/lib/api/v3/work_packages/work_packages_api.rb @@ -63,7 +63,7 @@ module API }) .mount) - route_param :id, type: String, desc: "Work package ID" do + route_param :id, type: String, desc: "Work package ID or semantic identifier (e.g. PROJ-42)" do helpers WorkPackagesSharedHelpers helpers do diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index fb3b0167ddc..624ea6c13e9 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -105,12 +105,12 @@ RSpec.describe WorkPackage::SemanticIdentifier do it "raises ArgumentError for multiple semantic ids" do expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } - .to raise_error(ArgumentError, /multi-argument find are not yet supported/) + .to raise_error(ArgumentError, /multi-argument find is not yet supported/) end it "raises ArgumentError for mixed numeric and semantic ids" do expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } - .to raise_error(ArgumentError, /multi-argument find are not yet supported/) + .to raise_error(ArgumentError, /multi-argument find is not yet supported/) end end From 462a0f1c78d1b8d1d1804e3262054e3ab305d066 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 17 Apr 2026 18:17:15 +0300 Subject: [PATCH 18/37] Replace transparent find_by resolution with ArgumentError guard find_by(id:) and find_by!(id:) now raise ArgumentError when passed a semantic identifier string, directing developers to use find() or the new find_by_display_id() method instead. This avoids silently altering ActiveRecord's find_by semantics and ensures misuse is caught in development even when semantic mode is not enabled locally. Renames find_by_id_or_identifier to find_by_display_id (public API) and migrates all app callers that receive user-facing strings to use the new method. --- .rubocop.yml | 4 +- .../work_packages/full_view/show_component.rb | 2 +- .../work_packages/hover_card_component.rb | 2 +- .../work_packages/split_view_component.rb | 2 +- app/controllers/work_packages_controller.rb | 2 +- .../work_packages/filter/relatable_filter.rb | 2 +- .../work_package/exports/macros/attributes.rb | 2 +- .../semantic_identifier/finder_methods.rb | 80 +++++------ app/services/mcp_resources/work_package.rb | 2 +- .../work_package/semantic_identifier_spec.rb | 124 ++++++++++++------ .../semantic_ids/integration_spec.rb | 30 ++--- 11 files changed, 141 insertions(+), 111 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index ae173ff1fe2..ed6d645fa8d 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -170,8 +170,8 @@ Rails/ContentTag: Rails/DynamicFindBy: Enabled: true AllowedMethods: - - find_by_id_or_identifier - - find_by_id_or_identifier! + - find_by_display_id + - find_by_display_id! - find_by_semantic_identifier Exclude: - "spec/features/**/*.rb" diff --git a/app/components/work_packages/full_view/show_component.rb b/app/components/work_packages/full_view/show_component.rb index 2547c4a3e68..4b248eacd2b 100644 --- a/app/components/work_packages/full_view/show_component.rb +++ b/app/components/work_packages/full_view/show_component.rb @@ -13,7 +13,7 @@ module WorkPackages @id = id @tab = tab - @work_package = WorkPackage.visible.find_by(id:) + @work_package = WorkPackage.visible.find_by_display_id(id) end def wrapper_uniq_by diff --git a/app/components/work_packages/hover_card_component.rb b/app/components/work_packages/hover_card_component.rb index 8ac0c516543..7e9a1a44c6d 100644 --- a/app/components/work_packages/hover_card_component.rb +++ b/app/components/work_packages/hover_card_component.rb @@ -35,7 +35,7 @@ class WorkPackages::HoverCardComponent < ApplicationComponent super @id = id - @work_package = WorkPackage.visible.find_by(id:) + @work_package = WorkPackage.visible.find_by_display_id(id) @assignee = @work_package.present? ? @work_package.assigned_to : nil end diff --git a/app/components/work_packages/split_view_component.rb b/app/components/work_packages/split_view_component.rb index 48d90ebbeb7..3c7f606d165 100644 --- a/app/components/work_packages/split_view_component.rb +++ b/app/components/work_packages/split_view_component.rb @@ -39,7 +39,7 @@ class WorkPackages::SplitViewComponent < ApplicationComponent @id = id @tab = tab - @work_package = WorkPackage.visible.find_by(id:) + @work_package = WorkPackage.visible.find_by_display_id(id) @base_route = base_route end diff --git a/app/controllers/work_packages_controller.rb b/app/controllers/work_packages_controller.rb index 0f2f9b8a8a9..ad87a9bc100 100644 --- a/app/controllers/work_packages_controller.rb +++ b/app/controllers/work_packages_controller.rb @@ -243,7 +243,7 @@ class WorkPackagesController < ApplicationController def work_package return @work_package if defined?(@work_package) - @work_package = WorkPackage.visible(current_user).find_by(id: params[:id]) + @work_package = WorkPackage.visible(current_user).find_by_display_id(params[:id]) end def journals diff --git a/app/models/queries/work_packages/filter/relatable_filter.rb b/app/models/queries/work_packages/filter/relatable_filter.rb index 988cbb5e5ce..37187bff36b 100644 --- a/app/models/queries/work_packages/filter/relatable_filter.rb +++ b/app/models/queries/work_packages/filter/relatable_filter.rb @@ -49,7 +49,7 @@ class Queries::WorkPackages::Filter::RelatableFilter < Queries::WorkPackages::Fi end def apply_to(query_scope) - query_scope.relatable(WorkPackage.visible.find_by(id: values.first), scope_operator) + query_scope.relatable(WorkPackage.visible.find_by_display_id(values.first), scope_operator) end private diff --git a/app/models/work_package/exports/macros/attributes.rb b/app/models/work_package/exports/macros/attributes.rb index 7a8055e44b7..e61767d4873 100644 --- a/app/models/work_package/exports/macros/attributes.rb +++ b/app/models/work_package/exports/macros/attributes.rb @@ -120,7 +120,7 @@ module WorkPackage::Exports return resolve_label_work_package(attribute) if type == "label" return msg_macro_error(I18n.t("export.macro.model_not_found", model: type)) unless type == "value" - work_package = WorkPackage.visible(user).find_by(id:) + work_package = WorkPackage.visible(user).find_by_display_id(id) if work_package.nil? return msg_macro_error(I18n.t("export.macro.resource_not_found", resource: "#{WorkPackage.name} #{id}")) end diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 250b7e57473..8206c942a95 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -28,56 +28,43 @@ # See COPYRIGHT and LICENSE files for more details. #++ -# Extends ActiveRecord finder methods (find, find_by, find_by!, exists?) to -# transparently resolve semantic work package identifiers (e.g. "PROJ-42") -# in addition to numeric IDs. +# Extends ActiveRecord finder methods to support semantic work package +# identifiers (e.g. "PROJ-42") in addition to numeric IDs. +# +# - find("PROJ-42") resolves transparently +# - find_by(id:)/find_by!(id:) raise ArgumentError for semantic strings +# - find_by_display_id("PROJ-42") is the explicit nil-on-miss resolver +# - exists?("PROJ-42") resolves transparently # # Included into WorkPackage class methods and extended into every # ActiveRecord::Relation via WorkPackage::SemanticIdentifier. -# -# Examples: -# WorkPackage.find("PROJ-42") -# WorkPackage.visible(user).find_by(id: "PROJ-42") -# WorkPackage.exists?("PROJ-42") module WorkPackage::SemanticIdentifier::FinderMethods def find(*args) ids = args.length == 1 && args.first.is_a?(Array) ? args.first : args if ids.length == 1 && semantic_id?(ids.first) - return find_by_id_or_identifier!(ids.first) + return find_by_display_id!(ids.first) end if ids.any? { |id| semantic_id?(id) } raise ArgumentError, "Semantic identifiers in multi-argument find is not yet supported. " \ - "Resolve each identifier individually via find_by(id:) instead." + "Resolve each identifier individually via find_by_display_id instead." end super end - # Override find_by to transparently resolve semantic identifiers when called - # with `id:` as the sole keyword (e.g. `find_by(id: "PROJ-42")`). - # All other find_by calls pass through to ActiveRecord unchanged. - # - # AR's find_by signature is find_by(arg, *args) — it doesn't use keyword splat, - # so hash kwargs arrive as the positional `arg`. We match on that. + # Guard find_by against semantic identifiers passed via `id:` or `identifier:`. + # Developers should use find("PROJ-42") or find_by_display_id("PROJ-42") instead. def find_by(*args) - if semantic_id_hash_lookup?(args) - find_by_id_or_identifier(args.first[:id]) - else - super - end + reject_semantic_id_in_find_by!(args) + super end - # Mirror of find_by — Rails implements find_by! independently (not via find_by), - # so we must override both to keep the pair consistent. def find_by!(*args) - if semantic_id_hash_lookup?(args) - find_by_id_or_identifier!(args.first[:id]) - else - super - end + reject_semantic_id_in_find_by!(args) + super end def exists?(conditions = :none) @@ -86,36 +73,39 @@ module WorkPackage::SemanticIdentifier::FinderMethods exists_by_semantic_identifier?(conditions) end - private - - # Returns true when args represent a single `id:` keyword lookup - # with a semantic identifier value (e.g. `find_by(id: "PROJ-42")`). - def semantic_id_hash_lookup?(args) - args.length == 1 && - args.first.is_a?(Hash) && - args.first.keys == [:id] && - semantic_id?(args.first[:id]) - end - - # Resolves any identifier form to a WorkPackage. + # Resolves any display-facing identifier to a WorkPackage. # - Numeric string ("12345") → find by primary key - # - Semantic string ("PROJ-42") → lookup via work_packages table and alias table + # - Semantic string ("PROJ-42") → lookup via identifier column and alias table # # Returns nil on miss. - def find_by_id_or_identifier(identifier) + def find_by_display_id(identifier) return find_by(id: identifier) unless semantic_id?(identifier) find_by_semantic_identifier(identifier) end - # Same as find_by_id_or_identifier but raises ActiveRecord::RecordNotFound on miss. - def find_by_id_or_identifier!(identifier) - find_by_id_or_identifier(identifier) || + # Same as find_by_display_id but raises ActiveRecord::RecordNotFound on miss. + def find_by_display_id!(identifier) + find_by_display_id(identifier) || raise(ActiveRecord::RecordNotFound.new( "Couldn't find WorkPackage with identifier=#{identifier}", "WorkPackage", "identifier", identifier )) end + private + + def reject_semantic_id_in_find_by!(args) + return unless args.length == 1 && args.first.is_a?(Hash) + + hash = args.first + key, value = hash.assoc(:id) || hash.assoc(:identifier) + return unless key && semantic_id?(value) + + raise ArgumentError, + "Semantic identifier #{value.inspect} cannot be passed to find_by(#{key}:). " \ + "Use find(#{value.inspect}) or find_by_display_id(#{value.inspect}) instead." + end + # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ") # return false — these fall through to standard ActiveRecord lookup. diff --git a/app/services/mcp_resources/work_package.rb b/app/services/mcp_resources/work_package.rb index 0537e6c3a3d..515586ea8d5 100644 --- a/app/services/mcp_resources/work_package.rb +++ b/app/services/mcp_resources/work_package.rb @@ -37,7 +37,7 @@ module McpResources default_description "Access work packages of this OpenProject instance." def read(id:) - work_package = ::WorkPackage.visible.find_by(id:) + work_package = ::WorkPackage.visible.find_by_display_id(id) return nil if work_package.nil? API::V3::WorkPackages::WorkPackageRepresenter.create(work_package, current_user:, embed_links: true) diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 624ea6c13e9..393ce89dc5c 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -170,17 +170,16 @@ RSpec.describe WorkPackage::SemanticIdentifier do describe "WorkPackage.find_by" do context "with id: keyword and semantic identifier" do - it "resolves via the semantic identifier" do - expect(WorkPackage.find_by(id: "MYPROJ-1")).to eq(work_package) + it "raises ArgumentError pointing to find_by_display_id" do + expect { WorkPackage.find_by(id: "MYPROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) end + end - it "returns nil for unknown semantic id" do - expect(WorkPackage.find_by(id: "MYPROJ-999")).to be_nil - end - - it "resolves via historic alias" do - WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) - expect(WorkPackage.find_by(id: "OLDPROJ-1")).to eq(work_package) + context "with identifier: keyword and semantic identifier" do + it "raises ArgumentError pointing to find_by_display_id" do + expect { WorkPackage.find_by(identifier: "MYPROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) end end @@ -198,37 +197,14 @@ RSpec.describe WorkPackage::SemanticIdentifier do context "with multiple keywords including id:" do it "passes through to standard AR find_by" do - expect(WorkPackage.find_by(id: work_package.id, project: project)).to eq(work_package) + expect(WorkPackage.find_by(id: work_package.id, project:)).to eq(work_package) end end context "with an unparseable semantic string" do - it "returns nil" do - expect(WorkPackage.find_by(id: "not-an-identifier!")).to be_nil - end - end - - context "when identifier differs but an alias row exists" do - it "resolves via the alias table" do - work_package.update_columns(identifier: "OTHER-99") - expect(WorkPackage.find_by(id: "MYPROJ-1")).to eq(work_package) - end - end - - context "with visibility scoping" do - let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } - let(:non_member_user) { create(:user) } - - it "respects the scope for semantic ids" do - expect(WorkPackage.visible(member_user).find_by(id: "MYPROJ-1")).to eq(work_package) - end - - it "returns nil when the user cannot see it" do - expect(WorkPackage.visible(non_member_user).find_by(id: "MYPROJ-1")).to be_nil - end - - it "also scopes numeric lookup" do - expect(WorkPackage.visible(non_member_user).find_by(id: work_package.id.to_s)).to be_nil + it "raises ArgumentError" do + expect { WorkPackage.find_by(id: "not-an-identifier!") } + .to raise_error(ArgumentError, /find_by_display_id/) end end end @@ -236,12 +212,9 @@ RSpec.describe WorkPackage::SemanticIdentifier do # rubocop:disable Rails/FindById -- testing find_by! override specifically, not suggesting find() describe "WorkPackage.find_by!" do context "with id: keyword and semantic identifier" do - it "resolves via the semantic identifier" do - expect(WorkPackage.find_by!(id: "MYPROJ-1")).to eq(work_package) - end - - it "raises RecordNotFound for unknown semantic id" do - expect { WorkPackage.find_by!(id: "MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound) + it "raises ArgumentError pointing to find_by_display_id" do + expect { WorkPackage.find_by!(id: "MYPROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) end end @@ -253,6 +226,73 @@ RSpec.describe WorkPackage::SemanticIdentifier do end # rubocop:enable Rails/FindById + describe "WorkPackage.find_by_display_id" do + context "with a semantic identifier" do + it "resolves via the semantic identifier" do + expect(WorkPackage.find_by_display_id("MYPROJ-1")).to eq(work_package) + end + + it "returns nil for unknown semantic id" do + expect(WorkPackage.find_by_display_id("MYPROJ-999")).to be_nil + end + + it "resolves via historic alias" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) + expect(WorkPackage.find_by_display_id("OLDPROJ-1")).to eq(work_package) + end + + it "resolves when identifier column differs but alias row exists" do + work_package.update_columns(identifier: "OTHER-99") + expect(WorkPackage.find_by_display_id("MYPROJ-1")).to eq(work_package) + end + end + + context "with a numeric string" do + it "falls through to standard AR find_by" do + expect(WorkPackage.find_by_display_id(work_package.id.to_s)).to eq(work_package) + end + + it "returns nil for unknown numeric id" do + expect(WorkPackage.find_by_display_id("9999999")).to be_nil + end + end + + context "with visibility scoping" do + let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } + let(:non_member_user) { create(:user) } + + it "respects the scope for semantic ids" do + expect(WorkPackage.visible(member_user).find_by_display_id("MYPROJ-1")).to eq(work_package) + end + + it "returns nil when the user cannot see it" do + expect(WorkPackage.visible(non_member_user).find_by_display_id("MYPROJ-1")).to be_nil + end + + it "also scopes numeric lookup" do + expect(WorkPackage.visible(non_member_user).find_by_display_id(work_package.id.to_s)).to be_nil + end + end + end + + describe "WorkPackage.find_by_display_id!" do + context "with a semantic identifier" do + it "resolves via the semantic identifier" do + expect(WorkPackage.find_by_display_id!("MYPROJ-1")).to eq(work_package) + end + + it "raises RecordNotFound for unknown semantic id" do + expect { WorkPackage.find_by_display_id!("MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context "with a numeric string" do + it "falls through to standard AR find" do + expect(WorkPackage.find_by_display_id!(work_package.id.to_s)).to eq(work_package) + end + end + end + describe "#display_id" do context "when semantic mode is active", with_flag: { semantic_work_package_ids: true }, diff --git a/spec/services/work_packages/semantic_ids/integration_spec.rb b/spec/services/work_packages/semantic_ids/integration_spec.rb index 9556f2760e3..3698c085e7b 100644 --- a/spec/services/work_packages/semantic_ids/integration_spec.rb +++ b/spec/services/work_packages/semantic_ids/integration_spec.rb @@ -97,12 +97,12 @@ RSpec.describe "SemanticIds registry integration", type: :model do it "old identifier still resolves to the WP" do WorkPackages::UpdateService.new(user:, model: work_package).call(project: target_project) - expect(WorkPackage.find_by(id: "PROJ-5")).to eq(work_package) + expect(WorkPackage.find_by_display_id("PROJ-5")).to eq(work_package) end it "new identifier also resolves to the WP" do WorkPackages::UpdateService.new(user:, model: work_package).call(project: target_project) - expect(WorkPackage.find_by(id: work_package.reload.identifier)).to eq(work_package) + expect(WorkPackage.find_by_display_id(work_package.reload.identifier)).to eq(work_package) end end @@ -130,15 +130,15 @@ RSpec.describe "SemanticIds registry integration", type: :model do it "old identifiers still resolve to the correct WPs" do Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") - expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1) - expect(WorkPackage.find_by(id: "PROJ-2")).to eq(wp2) + expect(WorkPackage.find_by_display_id("PROJ-1")).to eq(wp1) + expect(WorkPackage.find_by_display_id("PROJ-2")).to eq(wp2) end it "new identifiers resolve to the correct WPs" do Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") - expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1) - expect(WorkPackage.find_by(id: "RENAMED-2")).to eq(wp2) + expect(WorkPackage.find_by_display_id("RENAMED-1")).to eq(wp1) + expect(WorkPackage.find_by_display_id("RENAMED-2")).to eq(wp2) end it "old prefix resolves for WPs created after the rename" do @@ -147,8 +147,8 @@ RSpec.describe "SemanticIds registry integration", type: :model do Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") wp3 = create(:work_package, project: project.reload) - expect(WorkPackage.find_by(id: "RENAMED-3")).to eq(wp3) - expect(WorkPackage.find_by(id: "PROJ-3")).to eq(wp3) + expect(WorkPackage.find_by_display_id("RENAMED-3")).to eq(wp3) + expect(WorkPackage.find_by_display_id("PROJ-3")).to eq(wp3) end end @@ -161,7 +161,7 @@ RSpec.describe "SemanticIds registry integration", type: :model do # PROJ is then renamed to RENAMED (bulk-inserts RENAMED-1 from the retired PROJ-1 row) Projects::UpdateService.new(user:, model: project).call(identifier: "RENAMED") - expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1) + expect(WorkPackage.find_by_display_id("RENAMED-1")).to eq(wp1) end it "rename then move: both old identifiers resolve after the WP moves" do @@ -170,8 +170,8 @@ RSpec.describe "SemanticIds registry integration", type: :model do # WP moves to DEST (appends DEST-1 registry row, updates identifier) WorkPackages::UpdateService.new(user:, model: wp1.reload).call(project: target_project) - expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1) - expect(WorkPackage.find_by(id: "RENAMED-1")).to eq(wp1) + expect(WorkPackage.find_by_display_id("PROJ-1")).to eq(wp1) + expect(WorkPackage.find_by_display_id("RENAMED-1")).to eq(wp1) end it "rename then new WP then move: pre-rename identifier resolves via alias table" do @@ -183,7 +183,7 @@ RSpec.describe "SemanticIds registry integration", type: :model do # wp2 moves to DEST — old identifier RENAMED-2 kept as alias, gets DEST-1 WorkPackages::UpdateService.new(user:, model: wp2).call(project: target_project) - expect(WorkPackage.find_by(id: "PROJ-2")).to eq(wp2) + expect(WorkPackage.find_by_display_id("PROJ-2")).to eq(wp2) end end @@ -202,9 +202,9 @@ RSpec.describe "SemanticIds registry integration", type: :model do WorkPackages::UpdateService.new(user:, model: wp1.reload).call(project: project_c) projc_identifier = wp1.reload.identifier - expect(WorkPackage.find_by(id: "PROJ-1")).to eq(wp1) - expect(WorkPackage.find_by(id: dest_identifier)).to eq(wp1) - expect(WorkPackage.find_by(id: projc_identifier)).to eq(wp1) + expect(WorkPackage.find_by_display_id("PROJ-1")).to eq(wp1) + expect(WorkPackage.find_by_display_id(dest_identifier)).to eq(wp1) + expect(WorkPackage.find_by_display_id(projc_identifier)).to eq(wp1) end end end From 7ca36b6c016ae3088e42f5a8ba8978ba75239e65 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 17 Apr 2026 18:19:10 +0300 Subject: [PATCH 19/37] Add relation-scoped finder tests for semantic identifiers Test find, exists?, and find_by_display_id through project.work_packages to verify the FinderMethods module works correctly when extended onto ActiveRecord relations. --- .../projects/semantic_identifier_spec.rb | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/spec/models/projects/semantic_identifier_spec.rb b/spec/models/projects/semantic_identifier_spec.rb index a57defc8a9e..5b1be04a792 100644 --- a/spec/models/projects/semantic_identifier_spec.rb +++ b/spec/models/projects/semantic_identifier_spec.rb @@ -137,4 +137,65 @@ RSpec.describe Projects::SemanticIdentifier, with_settings: { work_packages_iden end end end + + describe "relation-scoped finder methods" do + let(:project) { create(:project, identifier: "PROJ", wp_sequence_counter: 0) } + let(:other_project) { create(:project, identifier: "OTHER", wp_sequence_counter: 0) } + let!(:wp1) { create(:work_package, project:) } + let!(:wp2) { create(:work_package, project: other_project) } + + describe "project.work_packages.find" do + it "resolves a semantic identifier scoped to the project" do + expect(project.work_packages.find("PROJ-1")).to eq(wp1) + end + + it "raises RecordNotFound for a WP belonging to another project" do + expect { project.work_packages.find("OTHER-1") } + .to raise_error(ActiveRecord::RecordNotFound) + end + + it "raises RecordNotFound for unknown semantic id" do + expect { project.work_packages.find("PROJ-999") } + .to raise_error(ActiveRecord::RecordNotFound) + end + + it "resolves via historic alias" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package: wp1) + expect(project.work_packages.find("OLDPROJ-1")).to eq(wp1) + end + end + + describe "project.work_packages.exists?" do + it "returns true for a semantic identifier within the project" do + expect(project.work_packages.exists?("PROJ-1")).to be true + end + + it "returns false for a WP belonging to another project" do + expect(project.work_packages.exists?("OTHER-1")).to be false + end + + it "returns false for unknown semantic id" do + expect(project.work_packages.exists?("PROJ-999")).to be false + end + + it "checks the alias table for historical identifiers" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package: wp1) + expect(project.work_packages.exists?("OLDPROJ-1")).to be true + end + end + + describe "project.work_packages.find_by_display_id" do + it "resolves a semantic identifier scoped to the project" do + expect(project.work_packages.find_by_display_id("PROJ-1")).to eq(wp1) + end + + it "returns nil for a WP belonging to another project" do + expect(project.work_packages.find_by_display_id("OTHER-1")).to be_nil + end + + it "returns nil for unknown semantic id" do + expect(project.work_packages.find_by_display_id("PROJ-999")).to be_nil + end + end + end end From 7824f5908c586c9558ac617effe66192d2b00ca6 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 17 Apr 2026 18:42:35 +0300 Subject: [PATCH 20/37] Handle string-keyed hashes in find_by semantic ID guard AR sometimes passes string keys ("id") instead of symbol keys (:id) to find_by. Check both forms when detecting semantic identifiers. --- .../semantic_identifier/finder_methods.rb | 3 ++- .../work_package/semantic_identifier_spec.rb | 12 ++++++++++++ .../work_packages/semantic_ids/integration_spec.rb | 14 ++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 8206c942a95..8d81d1ba1db 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -98,7 +98,8 @@ module WorkPackage::SemanticIdentifier::FinderMethods return unless args.length == 1 && args.first.is_a?(Hash) hash = args.first - key, value = hash.assoc(:id) || hash.assoc(:identifier) + key, value = (hash.assoc(:id) || hash.assoc("id")) || + (hash.assoc(:identifier) || hash.assoc("identifier")) return unless key && semantic_id?(value) raise ArgumentError, diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 393ce89dc5c..9326cd99bb7 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -183,6 +183,18 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end + context "with string-keyed hash (AR internal representation)" do + it "raises ArgumentError for string 'id' key with semantic value" do + expect { WorkPackage.find_by("id" => "MYPROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) + end + + it "raises ArgumentError for string 'identifier' key with semantic value" do + expect { WorkPackage.find_by("identifier" => "MYPROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) + end + end + context "with id: keyword and numeric string" do it "falls through to standard AR find_by" do expect(WorkPackage.find_by(id: work_package.id.to_s)).to eq(work_package) diff --git a/spec/services/work_packages/semantic_ids/integration_spec.rb b/spec/services/work_packages/semantic_ids/integration_spec.rb index 3698c085e7b..32ed2a8ec8a 100644 --- a/spec/services/work_packages/semantic_ids/integration_spec.rb +++ b/spec/services/work_packages/semantic_ids/integration_spec.rb @@ -50,6 +50,20 @@ RSpec.describe "SemanticIds registry integration", type: :model do login_as(user) end + describe "find_by guard rejects semantic identifiers" do + let!(:work_package) { create(:work_package, project:) } + + it "raises ArgumentError for find_by(id:) with a semantic string" do + expect { WorkPackage.find_by(id: "PROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) + end + + it "raises ArgumentError for find_by(id:) with a semantic string on a relation" do + expect { WorkPackage.where(project:).find_by(id: "PROJ-1") } + .to raise_error(ArgumentError, /find_by_display_id/) + end + end + describe "WP creation via CreateService" do let(:attributes) do { From 02f58dd3bb1368e42487e77f2dbec412e27e3f2e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 17 Apr 2026 19:11:16 +0300 Subject: [PATCH 21/37] Clarify why find_by rejects semantic identifiers in error message --- app/models/work_package/semantic_identifier/finder_methods.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 8d81d1ba1db..c7ee220c294 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -103,7 +103,8 @@ module WorkPackage::SemanticIdentifier::FinderMethods return unless key && semantic_id?(value) raise ArgumentError, - "Semantic identifier #{value.inspect} cannot be passed to find_by(#{key}:). " \ + "find_by(#{key}: #{value.inspect}) does not support semantic identifiers " \ + "because it cannot resolve aliases or match across identifier history. " \ "Use find(#{value.inspect}) or find_by_display_id(#{value.inspect}) instead." end From 8d1c6656526f5dc44221b8b196aba33bf32ad117 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 17 Apr 2026 19:23:17 +0300 Subject: [PATCH 22/37] Use where().take in find_by_display_id to skip redundant guard The numeric-string path was calling find_by(id:) which re-entered our override and ran semantic_id? a second time. Bypass with where(id:).take, matching AR's own find_by implementation. --- .../work_package/semantic_identifier/finder_methods.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index c7ee220c294..558b7ad6294 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -79,9 +79,11 @@ module WorkPackage::SemanticIdentifier::FinderMethods # # Returns nil on miss. def find_by_display_id(identifier) - return find_by(id: identifier) unless semantic_id?(identifier) - - find_by_semantic_identifier(identifier) + if semantic_id?(identifier) + find_by_semantic_identifier(identifier) + else + where(id: identifier).take # rubocop:disable Rails/FindBy -- avoid find_by, it would rerun semantic_id? + end end # Same as find_by_display_id but raises ActiveRecord::RecordNotFound on miss. From df9535bdcf4fcf6bfdb56a40f32ea008af59d63f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 17 Apr 2026 19:41:29 +0300 Subject: [PATCH 23/37] Disallow multi-argument find and improve find_by guard readability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkPackage.find(['DP-1']) returned a single record instead of an array, breaking Rails' convention that array-in means array-out. Rather than fixing that edge case, simplify by disallowing all multi-argument and array-argument find calls — nobody uses them and the complexity isn't worth maintaining. Also extract the hash.assoc chain in reject_semantic_id_in_find_by! into an explicit pair variable for clearer two-step logic. --- .../semantic_identifier/finder_methods.rb | 25 ++++++++----------- .../app/controllers/costlog_controller.rb | 2 +- .../work_package/semantic_identifier_spec.rb | 14 ++++++++--- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 558b7ad6294..f1c94708a5d 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -40,19 +40,13 @@ # ActiveRecord::Relation via WorkPackage::SemanticIdentifier. module WorkPackage::SemanticIdentifier::FinderMethods def find(*args) - ids = args.length == 1 && args.first.is_a?(Array) ? args.first : args - - if ids.length == 1 && semantic_id?(ids.first) - return find_by_display_id!(ids.first) + if args.length == 1 && !args.first.is_a?(Array) + return semantic_id?(args.first) ? find_by_display_id!(args.first) : super end - if ids.any? { |id| semantic_id?(id) } - raise ArgumentError, - "Semantic identifiers in multi-argument find is not yet supported. " \ - "Resolve each identifier individually via find_by_display_id instead." - end - - super + raise ArgumentError, + "WorkPackage.find does not support multiple arguments or array arguments. " \ + "Use find_by_display_id for semantic identifiers or find with a single ID." end # Guard find_by against semantic identifiers passed via `id:` or `identifier:`. @@ -100,9 +94,12 @@ module WorkPackage::SemanticIdentifier::FinderMethods return unless args.length == 1 && args.first.is_a?(Hash) hash = args.first - key, value = (hash.assoc(:id) || hash.assoc("id")) || - (hash.assoc(:identifier) || hash.assoc("identifier")) - return unless key && semantic_id?(value) + pair = (hash.assoc(:id) || hash.assoc("id")) || + (hash.assoc(:identifier) || hash.assoc("identifier")) + return unless pair + + key, value = pair + return unless semantic_id?(value) raise ArgumentError, "find_by(#{key}: #{value.inspect}) does not support semantic identifiers " \ diff --git a/modules/costs/app/controllers/costlog_controller.rb b/modules/costs/app/controllers/costlog_controller.rb index 611720e2336..9faed75ae12 100644 --- a/modules/costs/app/controllers/costlog_controller.rb +++ b/modules/costs/app/controllers/costlog_controller.rb @@ -123,7 +123,7 @@ class CostlogController < ApplicationController @work_package = if @cost_entry.present? && @cost_entry.entity_type == "WorkPackage" && @cost_entry.entity_id == entity_id @cost_entry.entity elsif entity_type == "WorkPackage" - WorkPackage.visible.find_by(id: entity_id) + WorkPackage.visible.find_by_display_id(entity_id) end cost_type_id = cost_entry_params.delete(:cost_type_id) diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 9326cd99bb7..7b65f72f8a8 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -99,18 +99,24 @@ RSpec.describe WorkPackage::SemanticIdentifier do context "with multiple ids" do let(:work_package2) { create(:work_package, project:) } - it "delegates to standard AR find for numeric ids" do - expect(WorkPackage.find([work_package.id, work_package2.id])).to contain_exactly(work_package, work_package2) + it "raises ArgumentError for multiple numeric ids" do + expect { WorkPackage.find([work_package.id, work_package2.id]) } + .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) + end + + it "raises ArgumentError for a single-element array with a semantic id" do + expect { WorkPackage.find(["MYPROJ-1"]) } + .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) end it "raises ArgumentError for multiple semantic ids" do expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } - .to raise_error(ArgumentError, /multi-argument find is not yet supported/) + .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) end it "raises ArgumentError for mixed numeric and semantic ids" do expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } - .to raise_error(ArgumentError, /multi-argument find is not yet supported/) + .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) end end From 08fe973a24ce3bccec50bb24ad3992e997c610b1 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 13:27:12 +0300 Subject: [PATCH 24/37] Allow multi-argument find for numeric IDs, reject for semantic IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Internal callers like FilterForWpMixin#value_objects pass arrays of numeric IDs to find. Blanket-rejecting array/multi-arg calls broke those paths. Fall through to standard AR find for all-numeric arrays and multi-arg calls, but continue raising ArgumentError if any argument is a semantic identifier — batch resolution across the alias table would be ambiguous and is out of scope. --- .../semantic_identifier/finder_methods.rb | 11 ++++++++--- .../work_package/semantic_identifier_spec.rb | 15 +++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index f1c94708a5d..53c5deeb6ce 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -44,9 +44,14 @@ module WorkPackage::SemanticIdentifier::FinderMethods return semantic_id?(args.first) ? find_by_display_id!(args.first) : super end - raise ArgumentError, - "WorkPackage.find does not support multiple arguments or array arguments. " \ - "Use find_by_display_id for semantic identifiers or find with a single ID." + ids = args.first.is_a?(Array) ? args.first : args + if ids.any? { |id| semantic_id?(id) } + raise ArgumentError, + "Semantic identifiers in multi-argument find are not supported. " \ + "Resolve each identifier individually via find_by_display_id instead." + end + + super end # Guard find_by against semantic identifiers passed via `id:` or `identifier:`. diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 7b65f72f8a8..5d5238e82de 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -99,24 +99,27 @@ RSpec.describe WorkPackage::SemanticIdentifier do context "with multiple ids" do let(:work_package2) { create(:work_package, project:) } - it "raises ArgumentError for multiple numeric ids" do - expect { WorkPackage.find([work_package.id, work_package2.id]) } - .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) + it "delegates to standard AR find for an array of numeric ids" do + expect(WorkPackage.find([work_package.id, work_package2.id])).to contain_exactly(work_package, work_package2) + end + + it "delegates to standard AR find for multiple numeric id arguments" do + expect(WorkPackage.find(work_package.id, work_package2.id)).to contain_exactly(work_package, work_package2) end it "raises ArgumentError for a single-element array with a semantic id" do expect { WorkPackage.find(["MYPROJ-1"]) } - .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) + .to raise_error(ArgumentError, /multi-argument find/) end it "raises ArgumentError for multiple semantic ids" do expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } - .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) + .to raise_error(ArgumentError, /multi-argument find/) end it "raises ArgumentError for mixed numeric and semantic ids" do expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } - .to raise_error(ArgumentError, /does not support multiple arguments or array arguments/) + .to raise_error(ArgumentError, /multi-argument find/) end end From 0fc7cf28bbd54342eaed02cf65d29c4013dfb166 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:10:26 +0300 Subject: [PATCH 25/37] Resolve relatable filter values via primary key The filter's values come from the available relation candidates API which hardcodes work package IDs. Never semantic strings, so using the display-id resolver is overkill and leaks it into query code where PK semantics are the contract. --- app/models/queries/work_packages/filter/relatable_filter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/queries/work_packages/filter/relatable_filter.rb b/app/models/queries/work_packages/filter/relatable_filter.rb index 37187bff36b..988cbb5e5ce 100644 --- a/app/models/queries/work_packages/filter/relatable_filter.rb +++ b/app/models/queries/work_packages/filter/relatable_filter.rb @@ -49,7 +49,7 @@ class Queries::WorkPackages::Filter::RelatableFilter < Queries::WorkPackages::Fi end def apply_to(query_scope) - query_scope.relatable(WorkPackage.visible.find_by_display_id(values.first), scope_operator) + query_scope.relatable(WorkPackage.visible.find_by(id: values.first), scope_operator) end private From e032c683d3264e4ef671522aa8abacc8f96512d8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:12:34 +0300 Subject: [PATCH 26/37] Resolve cost entry work package via primary key The entity_id is a polymorphic FK populated from the autocompleter's hidden form field (which writes the WP primary key) or an existing cost_entry's entity_id column. Never a semantic string, so the display-id resolver is overkill. Updated the matching spec fixture to use a realistic non-existent numeric ID instead of an arbitrary string that happened to trigger the find_by semantic guard. --- modules/costs/app/controllers/costlog_controller.rb | 2 +- modules/costs/spec/controllers/costlog_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/costs/app/controllers/costlog_controller.rb b/modules/costs/app/controllers/costlog_controller.rb index 9faed75ae12..611720e2336 100644 --- a/modules/costs/app/controllers/costlog_controller.rb +++ b/modules/costs/app/controllers/costlog_controller.rb @@ -123,7 +123,7 @@ class CostlogController < ApplicationController @work_package = if @cost_entry.present? && @cost_entry.entity_type == "WorkPackage" && @cost_entry.entity_id == entity_id @cost_entry.entity elsif entity_type == "WorkPackage" - WorkPackage.visible.find_by_display_id(entity_id) + WorkPackage.visible.find_by(id: entity_id) end cost_type_id = cost_entry_params.delete(:cost_type_id) diff --git a/modules/costs/spec/controllers/costlog_controller_spec.rb b/modules/costs/spec/controllers/costlog_controller_spec.rb index cb62c508057..3fbc1f98462 100644 --- a/modules/costs/spec/controllers/costlog_controller_spec.rb +++ b/modules/costs/spec/controllers/costlog_controller_spec.rb @@ -691,7 +691,7 @@ RSpec.describe CostlogController do before do grant_current_user_permissions user, %i[view_project view_work_packages view_cost_entries edit_cost_entries] - params["cost_entry"]["entity_id"] = "this-id-does-not-exist" + params["cost_entry"]["entity_id"] = "999999999" end it_behaves_like "invalid update" From 01fd71b1f0282a0e2c61a847e44ef69b437d9409 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:13:23 +0300 Subject: [PATCH 27/37] Point multi-arg find error to primary-key lookup The convention is that low-level code (queries, filters, services) uses primary keys and only frontend-facing code uses display-id resolution. The previous message only pointed at find_by_display_id, which encouraged the wrong tool for batch lookups. Also mention both the raising and nil-returning variants so callers can pick the right semantics. --- .../work_package/semantic_identifier/finder_methods.rb | 3 ++- spec/models/work_package/semantic_identifier_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 53c5deeb6ce..fffb357791f 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -48,7 +48,8 @@ module WorkPackage::SemanticIdentifier::FinderMethods if ids.any? { |id| semantic_id?(id) } raise ArgumentError, "Semantic identifiers in multi-argument find are not supported. " \ - "Resolve each identifier individually via find_by_display_id instead." + "Use primary keys for multi-argument lookup, or resolve each identifier " \ + "individually via find_by_display_id! (raises) or find_by_display_id (nil on miss)." end super diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 5d5238e82de..35df86c58a3 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -109,17 +109,17 @@ RSpec.describe WorkPackage::SemanticIdentifier do it "raises ArgumentError for a single-element array with a semantic id" do expect { WorkPackage.find(["MYPROJ-1"]) } - .to raise_error(ArgumentError, /multi-argument find/) + .to raise_error(ArgumentError, /primary keys for multi-argument/) end it "raises ArgumentError for multiple semantic ids" do expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } - .to raise_error(ArgumentError, /multi-argument find/) + .to raise_error(ArgumentError, /primary keys for multi-argument/) end it "raises ArgumentError for mixed numeric and semantic ids" do expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } - .to raise_error(ArgumentError, /multi-argument find/) + .to raise_error(ArgumentError, /primary keys for multi-argument/) end end From 5aa3a70ddc7bcf6b37e271a214ad880026aa20c0 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:14:24 +0300 Subject: [PATCH 28/37] Introduce UnsupportedLookup for find_by semantic-id guard Use a dedicated error class so callers that need to rescue this specifically can do so without catching unrelated ArgumentErrors. It still inherits from ArgumentError, so existing rescue clauses continue to work. --- app/models/work_package/semantic_identifier.rb | 7 +++++++ .../work_package/semantic_identifier/finder_methods.rb | 2 +- spec/models/work_package/semantic_identifier_spec.rb | 8 ++++++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 989adbaf249..22aac781e7f 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -36,6 +36,13 @@ module WorkPackage::SemanticIdentifier # The frontend equivalent lives in WP_ID_URL_PATTERN (work-package-id-pattern.ts). ID_ROUTE_CONSTRAINT = /\d+|[A-Z][A-Z0-9_]*-\d+/ + # Raised when a finder is invoked in a way that cannot resolve a semantic + # identifier — e.g. find_by(id: "PROJ-42") which reduces to a raw SQL + # WHERE clause that cannot consult the alias table. Subclasses ArgumentError + # so callers that rescue ArgumentError still catch it, but it can be rescued + # specifically when needed. + class UnsupportedLookup < ArgumentError; end + included do has_many :semantic_aliases, class_name: "WorkPackageSemanticAlias", diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index fffb357791f..d5ee3611eeb 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -107,7 +107,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods key, value = pair return unless semantic_id?(value) - raise ArgumentError, + raise WorkPackage::SemanticIdentifier::UnsupportedLookup, "find_by(#{key}: #{value.inspect}) does not support semantic identifiers " \ "because it cannot resolve aliases or match across identifier history. " \ "Use find(#{value.inspect}) or find_by_display_id(#{value.inspect}) instead." diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 35df86c58a3..74664cd3831 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -179,9 +179,13 @@ RSpec.describe WorkPackage::SemanticIdentifier do describe "WorkPackage.find_by" do context "with id: keyword and semantic identifier" do - it "raises ArgumentError pointing to find_by_display_id" do + it "raises UnsupportedLookup (an ArgumentError subclass) pointing to find_by_display_id" do expect { WorkPackage.find_by(id: "MYPROJ-1") } - .to raise_error(ArgumentError, /find_by_display_id/) + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /find_by_display_id/) + end + + it "is rescuable as ArgumentError for backwards compatibility" do + expect { WorkPackage.find_by(id: "MYPROJ-1") }.to raise_error(ArgumentError) end end From a53ca0b9de9fcca77966b93078f25f9be5e36bfb Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:14:56 +0300 Subject: [PATCH 29/37] Extract shared scope for semantic identifier lookups find_by_semantic_identifier and exists_by_semantic_identifier? were building the same WHERE condition twice. Extract scope_for_semantic_identifier as the single source of truth. --- .../semantic_identifier/finder_methods.rb | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index d5ee3611eeb..cdc4c3a73ff 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -123,10 +123,16 @@ module WorkPackage::SemanticIdentifier::FinderMethods stripped.to_i.to_s != stripped end - # Resolves a semantic identifier (e.g. "PROJ-42") to a WorkPackage in - # a single query. Matches against the current identifier column OR a - # correlated EXISTS on the alias table for historical identifiers. - # Returns nil on miss. + def find_by_semantic_identifier(identifier) + scope_for_semantic_identifier(identifier).first + end + + def exists_by_semantic_identifier?(identifier) + scope_for_semantic_identifier(identifier).exists? + end + + # Builds a scope that matches work packages by semantic identifier, considering + # both the current identifier column and the alias table for historical identifiers. # # Generates: # @@ -137,13 +143,8 @@ module WorkPackage::SemanticIdentifier::FinderMethods # WHERE "work_package_semantic_aliases"."work_package_id" = "work_packages"."id" # AND "work_package_semantic_aliases"."identifier" = 'PROJ-42' # )) - # ORDER BY "work_packages"."id" ASC LIMIT 1 - def find_by_semantic_identifier(identifier) - where(identifier:).or(where(semantic_alias_exists(identifier))).first - end - - def exists_by_semantic_identifier?(identifier) - where(identifier:).or(where(semantic_alias_exists(identifier))).exists? + def scope_for_semantic_identifier(identifier) + where(identifier:).or(where(semantic_alias_exists(identifier))) end # Correlated EXISTS subquery that matches work packages having a From c64254f3167fa39e7adfec0fffdbeca39776244f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:15:55 +0300 Subject: [PATCH 30/37] Guard find_by array values against semantic identifiers find_by(id: [...]) is a legitimate AR pattern for matching the first record in a set. Previously we only guarded scalar values, so find_by(id: ["PROJ-1", 1]) silently fell through to a SQL WHERE id IN ('PROJ-1', 1) that would error or coerce unexpectedly on Postgres. Extend the guard to scan array elements and raise UnsupportedLookup on the first semantic one found, keeping the error message focused on the offending value. --- .../semantic_identifier/finder_methods.rb | 5 +++-- .../models/work_package/semantic_identifier_spec.rb | 13 +++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index cdc4c3a73ff..6acdcef5eee 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -105,12 +105,13 @@ module WorkPackage::SemanticIdentifier::FinderMethods return unless pair key, value = pair - return unless semantic_id?(value) + offending = value.is_a?(Array) ? value.find { |v| semantic_id?(v) } : (value if semantic_id?(value)) + return unless offending raise WorkPackage::SemanticIdentifier::UnsupportedLookup, "find_by(#{key}: #{value.inspect}) does not support semantic identifiers " \ "because it cannot resolve aliases or match across identifier history. " \ - "Use find(#{value.inspect}) or find_by_display_id(#{value.inspect}) instead." + "Use find(#{offending.inspect}) or find_by_display_id(#{offending.inspect}) instead." end # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 74664cd3831..4f8b2ab2bc9 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -214,6 +214,19 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end + context "with id: keyword and an array" do + let(:work_package2) { create(:work_package, project:) } + + it "falls through to standard AR find_by for an all-numeric array" do + expect(WorkPackage.find_by(id: [work_package.id, work_package2.id])).to eq(work_package) + end + + it "raises UnsupportedLookup when the array contains a semantic identifier" do + expect { WorkPackage.find_by(id: [work_package.id, "MYPROJ-2"]) } + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /does not support semantic identifiers/) + end + end + context "with non-id keywords" do it "passes through to standard AR find_by" do expect(WorkPackage.find_by(subject: work_package.subject)).to eq(work_package) From 79d9ff78d3deba7b17a145254864255672a5706a Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:16:24 +0300 Subject: [PATCH 31/37] Add exists? visibility scoping tests The shared scope for semantic identifier lookups needs to compose cleanly with the visible scope (uses join+allowed_to) for both current identifiers and historical aliases. Pin that behavior so structural regressions in the OR construction surface early. --- .../work_package/semantic_identifier_spec.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 4f8b2ab2bc9..e1ece5c59dd 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -175,6 +175,25 @@ RSpec.describe WorkPackage::SemanticIdentifier do expect(WorkPackage.exists?(subject: work_package.subject)).to be true end end + + context "with visibility scoping" do + let(:member_user) { create(:user, member_with_permissions: { project => [:view_work_packages] }) } + let(:non_member_user) { create(:user) } + + it "respects the scope for semantic ids" do + expect(WorkPackage.visible(member_user).exists?("MYPROJ-1")).to be true + end + + it "returns false when the user cannot see it" do + expect(WorkPackage.visible(non_member_user).exists?("MYPROJ-1")).to be false + end + + it "respects the scope for historical aliases" do + WorkPackageSemanticAlias.create!(identifier: "OLDPROJ-1", work_package:) + expect(WorkPackage.visible(member_user).exists?("OLDPROJ-1")).to be true + expect(WorkPackage.visible(non_member_user).exists?("OLDPROJ-1")).to be false + end + end end describe "WorkPackage.find_by" do From 335faedfc098c77fc934c223e474a8e65d90d932 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:17:13 +0300 Subject: [PATCH 32/37] Document deliberate asymmetry and relation override seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transparent find vs. guarded find_by isn't an oversight — the rationale belongs in the module docstring so the next maintainer doesn't 'fix for consistency' and break controllers. Add a matching comment on the relation override explaining why it's the seam that reaches scopes and association proxies. --- app/models/work_package/semantic_identifier.rb | 6 +++++- .../semantic_identifier/finder_methods.rb | 13 ++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 22aac781e7f..44252c328c9 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -57,7 +57,11 @@ module WorkPackage::SemanticIdentifier include FinderMethods # Extend every relation built from this model with semantic finder methods, - # so that WorkPackage.visible(user).find("PROJ-42") works transparently. + # so that WorkPackage.visible(user).find("PROJ-42") and + # project.work_packages.find_by_display_id("PROJ-42") both work. Overriding + # `relation` is the seam that reaches every scope and association proxy; + # including FinderMethods into class_methods alone only covers class-level + # calls like WorkPackage.find. def relation super.extending(FinderMethods) end diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 6acdcef5eee..2bd04dbd1dc 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -32,10 +32,21 @@ # identifiers (e.g. "PROJ-42") in addition to numeric IDs. # # - find("PROJ-42") resolves transparently -# - find_by(id:)/find_by!(id:) raise ArgumentError for semantic strings +# - find_by(id:)/find_by!(id:) raise UnsupportedLookup for semantic strings # - find_by_display_id("PROJ-42") is the explicit nil-on-miss resolver # - exists?("PROJ-42") resolves transparently # +# The asymmetry between find (transparent) and find_by (guarded) is deliberate: +# controllers and URL-driven callers already pass user input into find, and +# losing semantic resolution there would break the feature. find_by on the other +# hand reduces to a raw SQL WHERE id = ? that cannot consult the alias table, +# so silently matching nothing would be a worse bug than raising. +# +# Convention: use find_by_display_id only when the input could legitimately be +# either numeric or semantic (controllers, view components fed from URL params, +# macro resolvers). Low-level code (queries, filters, services) should stick to +# find_by(id:) with primary keys. +# # Included into WorkPackage class methods and extended into every # ActiveRecord::Relation via WorkPackage::SemanticIdentifier. module WorkPackage::SemanticIdentifier::FinderMethods From f90ba91ae4cd333a8a207473e18ede3eb40435a9 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:17:39 +0300 Subject: [PATCH 33/37] Use Enumerable#detect in find_by array guard for clarity Enumerable#find is aliased to #detect but reads confusingly in a module that overrides ActiveRecord's find. Prefer the alias so the array scan doesn't look like a recursive call. --- app/models/work_package/semantic_identifier/finder_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 2bd04dbd1dc..24878fa71a2 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -116,7 +116,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods return unless pair key, value = pair - offending = value.is_a?(Array) ? value.find { |v| semantic_id?(v) } : (value if semantic_id?(value)) + offending = value.is_a?(Array) ? value.detect { |v| semantic_id?(v) } : (value if semantic_id?(value)) return unless offending raise WorkPackage::SemanticIdentifier::UnsupportedLookup, From 50c00590af316c10e0c4fc75e698d641846aaba8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:18:22 +0300 Subject: [PATCH 34/37] Document WorkPackage identifier finder convention in AGENTS.md Point agents at the module docstring so the frontend-vs-primary-key split is discoverable without reading through past PRs. --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index dac00fe3410..a7cce259da1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,6 +142,7 @@ cd frontend && npm test && cd .. - Keep controllers thin, models focused - Document with [YARD](https://yardoc.org/) - Write RSpec tests for all new features +- **Work package identifiers**: `WorkPackage.find("PROJ-42")` resolves semantic identifiers transparently. Use `find_by_display_id` only when input could legitimately be numeric OR semantic (controllers, URL-driven components, macro resolvers). Low-level code (queries, filters, services) should stick to `find_by(id:)` with primary keys. See `app/models/work_package/semantic_identifier/finder_methods.rb`. ### JavaScript/TypeScript - **New development**: Use Hotwire (Turbo + Stimulus) with server-rendered HTML From 911baebfdb561f9d656d5c777e994214b2275631 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 14:25:48 +0300 Subject: [PATCH 35/37] Extract helpers to tame reject_semantic_id_in_find_by! complexity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The array guard pushed AbcSize to 20/17 and PerceivedComplexity to 11/8. Split the pair lookup and offending-value detection into named helpers — the shape of the check is now readable top-to-bottom. --- .../semantic_identifier/finder_methods.rb | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 24878fa71a2..4f6b9c98bec 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -110,13 +110,11 @@ module WorkPackage::SemanticIdentifier::FinderMethods def reject_semantic_id_in_find_by!(args) return unless args.length == 1 && args.first.is_a?(Hash) - hash = args.first - pair = (hash.assoc(:id) || hash.assoc("id")) || - (hash.assoc(:identifier) || hash.assoc("identifier")) + pair = id_or_identifier_pair(args.first) return unless pair key, value = pair - offending = value.is_a?(Array) ? value.detect { |v| semantic_id?(v) } : (value if semantic_id?(value)) + offending = first_semantic_value(value) return unless offending raise WorkPackage::SemanticIdentifier::UnsupportedLookup, @@ -125,6 +123,19 @@ module WorkPackage::SemanticIdentifier::FinderMethods "Use find(#{offending.inspect}) or find_by_display_id(#{offending.inspect}) instead." end + def id_or_identifier_pair(hash) + (hash.assoc(:id) || hash.assoc("id")) || + (hash.assoc(:identifier) || hash.assoc("identifier")) + end + + def first_semantic_value(value) + if value.is_a?(Array) + value.detect { |v| semantic_id?(v) } + elsif semantic_id?(value) + value + end + end + # Returns true when value looks like a semantic work package identifier (e.g. "PROJ-42"). # Non-string values (Integer, Hash, nil, Array) and numeric strings ("123", " 456 ") # return false — these fall through to standard ActiveRecord lookup. From e545bd4b790c1c8822c1d466c252aa5eab0c94fb Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 20 Apr 2026 17:24:08 +0300 Subject: [PATCH 36/37] Pin find_by multi-keyword guard with explicit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document that find_by(subject: x, id: "PROJ-1") raises UnsupportedLookup — the args.length == 1 check in the guard distinguishes hash-conditions form from the SQL-string form, not hash arity. --- spec/models/work_package/semantic_identifier_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index e1ece5c59dd..ea72684ff2a 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -256,6 +256,11 @@ RSpec.describe WorkPackage::SemanticIdentifier do it "passes through to standard AR find_by" do expect(WorkPackage.find_by(id: work_package.id, project:)).to eq(work_package) end + + it "raises UnsupportedLookup when id: is semantic even among other keywords" do + expect { WorkPackage.find_by(subject: "anything", id: "MYPROJ-1") } + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup) + end end context "with an unparseable semantic string" do From 270da251e05790347063546ba206535b276c10ac Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 21 Apr 2026 12:39:04 +0300 Subject: [PATCH 37/37] Raise UnsupportedLookup from multi-arg find for consistency find_by already raises WorkPackage::SemanticIdentifier::UnsupportedLookup when a semantic identifier is passed through id:/identifier:. Align the multi-argument find guard so callers can rescue a single specific error across both entry points, while the ArgumentError parent class keeps legacy rescuers working. --- .../semantic_identifier/finder_methods.rb | 2 +- .../work_package/semantic_identifier_spec.rb | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 4f6b9c98bec..debd2a238b1 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -57,7 +57,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods ids = args.first.is_a?(Array) ? args.first : args if ids.any? { |id| semantic_id?(id) } - raise ArgumentError, + raise WorkPackage::SemanticIdentifier::UnsupportedLookup, "Semantic identifiers in multi-argument find are not supported. " \ "Use primary keys for multi-argument lookup, or resolve each identifier " \ "individually via find_by_display_id! (raises) or find_by_display_id (nil on miss)." diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index ea72684ff2a..efab06eb7c9 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -107,19 +107,23 @@ RSpec.describe WorkPackage::SemanticIdentifier do expect(WorkPackage.find(work_package.id, work_package2.id)).to contain_exactly(work_package, work_package2) end - it "raises ArgumentError for a single-element array with a semantic id" do + it "raises UnsupportedLookup for a single-element array with a semantic id" do expect { WorkPackage.find(["MYPROJ-1"]) } - .to raise_error(ArgumentError, /primary keys for multi-argument/) + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /primary keys for multi-argument/) end - it "raises ArgumentError for multiple semantic ids" do + it "raises UnsupportedLookup for multiple semantic ids" do expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } - .to raise_error(ArgumentError, /primary keys for multi-argument/) + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /primary keys for multi-argument/) end - it "raises ArgumentError for mixed numeric and semantic ids" do + it "raises UnsupportedLookup for mixed numeric and semantic ids" do expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } - .to raise_error(ArgumentError, /primary keys for multi-argument/) + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /primary keys for multi-argument/) + end + + it "is rescuable as ArgumentError for backwards compatibility" do + expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") }.to raise_error(ArgumentError) end end