diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index cfc0e528e73e..621149f51bff 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -84,6 +84,34 @@ def relation end end + # Returns true when value looks like a semantic work package identifier + # ("PROJ-42"). Non-strings (Integer, Hash, nil, Array) and numeric strings + # ("123", " 456 ") return false — these fall through to standard PK lookup. + # + # The round-trip check (rather than a regex) is intentional for performance. + # Every value that reaches a work-package finder either parses as an integer + # or doesn't, and that's enough to dispatch correctly. Don't tighten it. + def self.semantic_id?(value) + value.is_a?(String) && value.strip.to_i.to_s != value.strip + end + + # Returns true when value is a canonical numeric ID — + # an Integer, or a String that round-trips through `to_i.to_s` ("0", "123"). + # Rejects leading-zero strings ("0123"), non-numeric strings, and nil. + # + # For Strings the predicate is the exact complement of `semantic_id?`, + # so the routing question (lookup by primary key vs by identifier/alias) + # has a single source of truth. For non-String inputs the two diverge: + # Integers are numeric-only (no string-lookup routing applies); nil and + # other types are neither and both return false. + def self.numeric_id?(value) + case value + when Integer then true + when String then !semantic_id?(value) + else false + end + end + # Returns the user-facing identifier for this work package. # In semantic mode: the project-based identifier (e.g. "PROJ-42") # In classic mode: the numeric database ID diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index d03c7904b8ee..1cf742151477 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -110,8 +110,13 @@ def find_by_display_id!(identifier) # historical alias matches one of the supplied display ids. Numeric and # semantic strings may be freely mixed; unknown values produce no match # rather than poisoning the rest of the set. - def where_display_id_in(values) - values = Array(values).map(&:to_s) + # + # @param values [String, Integer, Array] one or more + # display ids. Pass scalars (`where_display_id_in("PROJ-1")`), varargs + # (`where_display_id_in("PROJ-1", "PROJ-2")`), or a pre-built array + # (`where_display_id_in(ids)`) interchangeably. + def where_display_id_in(*values) + values = values.flatten(1).compact.map(&:to_s) return none if values.empty? semantic, numeric = values.partition { semantic_id?(it) } @@ -152,14 +157,8 @@ def first_semantic_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 + WorkPackage::SemanticIdentifier.semantic_id?(value) end def find_by_semantic_identifier(identifier) diff --git a/modules/reporting/app/models/cost_query/filter/work_package_id.rb b/modules/reporting/app/models/cost_query/filter/work_package_id.rb index 2a899f9a4ddd..e4104f0f1fb5 100644 --- a/modules/reporting/app/models/cost_query/filter/work_package_id.rb +++ b/modules/reporting/app/models/cost_query/filter/work_package_id.rb @@ -47,7 +47,7 @@ def self.available_values(*) # Overwrites Report::Filter::Base self.label_for_value method # to achieve a more performant implementation def self.label_for_value(value) - return nil unless value.to_i.to_s == value.to_s # we expect an work_package-id + return nil unless WorkPackage::SemanticIdentifier.numeric_id?(value.to_s) work_package = WorkPackage.visible.find(value.to_i) [text_for_work_package(work_package), work_package.id] if work_package&.visible?(User.current) diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index d6de9c83cb0e..9113ac5e50bf 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -394,6 +394,11 @@ expect(WorkPackage.where_display_id_in("MYPROJ-1")).to contain_exactly(work_package) end + it "accepts identifiers as varargs" do + expect(WorkPackage.where_display_id_in("MYPROJ-1", "MYPROJ-2")) + .to contain_exactly(work_package, work_package2) + end + it "resolves a single numeric string" do expect(WorkPackage.where_display_id_in([work_package.id.to_s])).to contain_exactly(work_package) end @@ -459,6 +464,62 @@ end end + describe "ID_ROUTE_CONSTRAINT" do + # Rails wraps route-constraint regexps with `\A…\z` when matching a path + # segment, so the spec uses an anchored regex to model the way the + # constant is actually used. This pins the composition with + # SEMANTIC_ID_PATTERN so a future change to the upstream prefix or + # sequence shape can't silently widen what the routes accept. + let(:anchored) { /\A(?:#{described_class::ID_ROUTE_CONSTRAINT.source})\z/ } + + it "matches numeric work package ids" do + expect(anchored.match?("123")).to be true + end + + it "matches semantic work package identifiers" do + expect(anchored.match?("PROJ-7")).to be true + end + + it "rejects lowercased semantic shapes" do + expect(anchored.match?("proj-7")).to be false + end + end + + describe ".numeric_id? and .semantic_id?" do + # `numeric_id?` answers a shape question (canonical numeric ID), + # `semantic_id?` answers a routing question (needs identifier/alias + # lookup). For Strings the two are mutually exclusive; Integers are + # numeric-only (no string-lookup routing applies). + [ + ["123", :numeric], + ["0", :numeric], + [" 123 ", :numeric], + [123, :numeric], + [0, :numeric], + ["0123", :semantic], + ["00", :semantic], + ["PROJ-1", :semantic], + ["abc", :semantic], + ["", :semantic], + [nil, :neither], + [{}, :neither] + ].each do |value, classification| + it "routes #{value.inspect} to #{classification}" do + case classification + when :numeric + expect(described_class.numeric_id?(value)).to be true + expect(described_class.semantic_id?(value)).to be false + when :semantic + expect(described_class.semantic_id?(value)).to be true + expect(described_class.numeric_id?(value)).to be false + when :neither + expect(described_class.numeric_id?(value)).to be false + expect(described_class.semantic_id?(value)).to be false + end + end + end + end + describe "#display_id" do context "when semantic mode is active", with_flag: { semantic_work_package_ids: true },