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/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 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/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.rb b/app/models/work_package/semantic_identifier.rb index 0d8861ac95d..44252c328c9 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -31,6 +31,18 @@ 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-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", @@ -42,37 +54,16 @@ module WorkPackage::SemanticIdentifier end class_methods do - def semantic_id?(identifier) - identifier.to_s.to_i.to_s != identifier.to_s - end + include FinderMethods - # 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, "WorkPackage not found: #{identifier}") - end - - private - - 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: }) + # Extend every relation built from this model with semantic finder methods, + # 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 end 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..debd2a238b1 --- /dev/null +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -0,0 +1,184 @@ +# 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 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 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 + def find(*args) + if args.length == 1 && !args.first.is_a?(Array) + return semantic_id?(args.first) ? find_by_display_id!(args.first) : super + end + + ids = args.first.is_a?(Array) ? args.first : args + if ids.any? { |id| semantic_id?(id) } + 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)." + end + + super + end + + # 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) + reject_semantic_id_in_find_by!(args) + super + end + + def find_by!(*args) + reject_semantic_id_in_find_by!(args) + super + end + + def exists?(conditions = :none) + return super unless semantic_id?(conditions) + + exists_by_semantic_identifier?(conditions) + end + + # Resolves any display-facing identifier to a WorkPackage. + # - Numeric string ("12345") → find by primary key + # - Semantic string ("PROJ-42") → lookup via identifier column and alias table + # + # Returns nil on miss. + def find_by_display_id(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. + 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) + + pair = id_or_identifier_pair(args.first) + return unless pair + + key, value = pair + offending = first_semantic_value(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(#{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. + 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) + 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: + # + # 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' + # )) + def scope_for_semantic_identifier(identifier) + where(identifier:).or(where(semantic_alias_exists(identifier))) + end + + # 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 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/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/config/routes.rb b/config/routes.rb index cdb2dd4b32f..f80a43b24dd 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: 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)).+/ } @@ -935,7 +935,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: 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" diff --git a/lib/api/v3/work_packages/work_packages_api.rb b/lib/api/v3/work_packages/work_packages_api.rb index bfab66edf11..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: Integer, 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/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" diff --git a/spec/controllers/work_packages_controller_spec.rb b/spec/controllers/work_packages_controller_spec.rb index 1552aa1e892..53320ea4f6d 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").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,34 @@ 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.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: "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 +294,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 +317,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 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 diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 6d41e2215a6..efab06eb7c9 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -60,50 +60,70 @@ RSpec.describe WorkPackage::SemanticIdentifier do end end - describe "WorkPackage.find_by_id_or_identifier" do - context "with a numeric param" do + describe "WorkPackage.find" do + context "with a numeric id" do it "finds by primary key" do - expect(WorkPackage.find_by_id_or_identifier(work_package.id.to_s)).to eq(work_package) + expect(WorkPackage.find(work_package.id)).to eq(work_package) end - it "returns nil for unknown id" do - expect(WorkPackage.find_by_id_or_identifier("9999999")).to be_nil + it "raises RecordNotFound for unknown numeric id" do + expect { WorkPackage.find(9_999_999) }.to raise_error(ActiveRecord::RecordNotFound) 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 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 - 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 "strips whitespace before dispatching" do + expect(WorkPackage.find(" #{work_package.id} ")).to eq(work_package) + end + 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 a semantic identifier string" do + it "resolves via the semantic identifier" do + expect(WorkPackage.find("MYPROJ-1")).to eq(work_package) end - it "returns nil for unknown sequence" do - expect(WorkPackage.find_by_id_or_identifier("MYPROJ-999")).to be_nil + it "raises RecordNotFound for unknown semantic id" do + expect { WorkPackage.find("MYPROJ-999") }.to raise_error(ActiveRecord::RecordNotFound) end - it "returns nil for unknown project prefix" do - expect(WorkPackage.find_by_id_or_identifier("NOPE-1")).to be_nil + 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 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 "returns nil for an unparseable string" do - expect(WorkPackage.find_by_id_or_identifier("not-an-identifier!")).to be_nil + 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 UnsupportedLookup for a single-element array with a semantic id" do + expect { WorkPackage.find(["MYPROJ-1"]) } + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /primary keys for multi-argument/) + end + + it "raises UnsupportedLookup for multiple semantic ids" do + expect { WorkPackage.find("MYPROJ-1", "MYPROJ-2") } + .to raise_error(WorkPackage::SemanticIdentifier::UnsupportedLookup, /primary keys for multi-argument/) + end + + it "raises UnsupportedLookup for mixed numeric and semantic ids" do + expect { WorkPackage.find([work_package.id, "MYPROJ-2"]) } + .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 @@ -111,28 +131,231 @@ 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("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 - 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 + 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.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) + 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 - it "raises ActiveRecord::RecordNotFound when not found" do - expect { WorkPackage.find_by_id_or_identifier!("MYPROJ-999") } - .to raise_error(ActiveRecord::RecordNotFound) + 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 + + 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 + context "with id: keyword and semantic identifier" do + it "raises UnsupportedLookup (an ArgumentError subclass) pointing to find_by_display_id" do + expect { WorkPackage.find_by(id: "MYPROJ-1") } + .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 + + 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 + + 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) + 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) + end + end + + 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:)).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 + it "raises ArgumentError" do + expect { WorkPackage.find_by(id: "not-an-identifier!") } + .to raise_error(ArgumentError, /find_by_display_id/) + end + end + end + + # 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 "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 + + 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 "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 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..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,10 +94,8 @@ 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", + I18n.t("api_v3.errors.not_found.work_package") end context "with existing work package" do 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 diff --git a/spec/routing/work_packages_spec.rb b/spec/routing/work_packages_spec.rb index 62452d074da..9cf2be23bf1 100644 --- a/spec/routing/work_packages_spec.rb +++ b/spec/routing/work_packages_spec.rb @@ -92,6 +92,18 @@ 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 "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", diff --git a/spec/services/work_packages/semantic_ids/integration_spec.rb b/spec/services/work_packages/semantic_ids/integration_spec.rb index 551d28496df..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 { @@ -97,12 +111,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_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_or_identifier(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 +144,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_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_or_identifier("RENAMED-1")).to eq(wp1) - expect(WorkPackage.find_by_id_or_identifier("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 +161,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_display_id("RENAMED-3")).to eq(wp3) + expect(WorkPackage.find_by_display_id("PROJ-3")).to eq(wp3) end end @@ -161,7 +175,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_display_id("RENAMED-1")).to eq(wp1) end it "rename then move: both old identifiers resolve after the WP moves" do @@ -170,8 +184,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_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 +197,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_display_id("PROJ-2")).to eq(wp2) end end @@ -202,9 +216,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_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