From 525e26417e26086e8d047b8ac5bd644f208ca118 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 6 May 2026 20:27:20 +0300 Subject: [PATCH 1/7] Promote WorkPackage::SemanticIdentifier.semantic_id? to public MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The simple `to_i.to_s` round-trip check was previously a private predicate inside `FinderMethods`. The text-formatting layer needs the same predicate to decide whether a `#PROJ-1`-shaped match should be preloaded as a WP reference, but pulling it through `FinderMethods` would couple the macro parser to finder internals. Move the predicate up one level to the parent module — the same place `ID_ROUTE_CONSTRAINT` already lives — and have `FinderMethods` delegate. Single source of truth, no behaviour change. --- app/models/work_package/semantic_identifier.rb | 15 +++++++++++++++ .../semantic_identifier/finder_methods.rb | 8 +------- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index cfc0e528e73e..8a3a98399fd0 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -84,6 +84,21 @@ 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 simple round-trip check (`value.strip.to_i.to_s != value.strip`) is + # intentional: it's faster than a regex and produces the same answer for + # every shape that can reach a work-package finder. Don't tighten it. + # + # Lives on this module rather than FinderMethods because the matcher / + # text-formatting layer needs the same predicate without pulling in finder + # internals. + def self.semantic_id?(value) + value.is_a?(String) && value.strip.to_i.to_s != value.strip + 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..b7479343525c 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -152,14 +152,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) From c67978a7d1c6a637420b6d69fb2d63deab30e498 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 13 May 2026 17:43:51 +0300 Subject: [PATCH 2/7] Encapsulate canonical-numeric guard as numeric_id? The `value == value.to_i.to_s` round-trip check that filters leading- zero ID forms ("0123") was duplicated across the WP link handler, the PDF export macro, and the cost-query filter. A new `WorkPackage::SemanticIdentifier.numeric_id?(value)` predicate captures the canonical-numeric check at one site. It pairs with `semantic_id?` as the WP-finder shape gate; the two answer different questions (shape vs routing) and so are kept independent rather than expressed as one another's negation. The cost-query filter switches to the predicate in this slice; the text-formatting and PDF callers convert in a follow-up. --- .../work_package/semantic_identifier.rb | 42 +++++++++++++++---- .../cost_query/filter/work_package_id.rb | 2 +- .../work_package/semantic_identifier_spec.rb | 34 +++++++++++++++ 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 8a3a98399fd0..f6c4db80df0c 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -84,21 +84,45 @@ 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. + # Routing predicate for WP finders: returns true when value is a String + # that needs string-based identifier/alias lookup. Three input kinds + # return true: + # - true semantic identifiers ("PROJ-7") + # - non-canonical numerics ("0123", "00") + # - non-numeric strings ("abc") + # Only canonical numeric strings ("123") route to PK lookup and return + # false here. The predicate's name reflects routing intent, not strict + # shape validation — "0123" returns true even though it's clearly not + # a "semantic identifier" in the user-facing sense, because it still + # belongs on the string-lookup path. # - # The simple round-trip check (`value.strip.to_i.to_s != value.strip`) is - # intentional: it's faster than a regex and produces the same answer for - # every shape that can reach a work-package finder. Don't tighten it. + # Non-strings (Integer, Hash, nil) return false — already in PK-lookup + # shape. # - # Lives on this module rather than FinderMethods because the matcher / - # text-formatting layer needs the same predicate without pulling in finder - # internals. + # The round-trip check (rather than a regex) is intentional for + # performance: every value reaching a finder either parses as an + # integer or doesn't, and that's enough to dispatch. Don't tighten it. def self.semantic_id?(value) value.is_a?(String) && value.strip.to_i.to_s != value.strip end + # Shape predicate: 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. + # + # Pairs with `semantic_id?` at call sites that need to gate inputs to + # one of two lookup paths. They answer different questions (shape vs + # routing) and so are kept independent rather than expressing one as + # the negation of the other. + def self.numeric_id?(value) + case value + when Integer then true + when String then value == value.to_i.to_s + 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/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..9fc8ee95798b 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -459,6 +459,40 @@ 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], + [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 }, From 0a5b2b8646fefdb69f8444988bd8b2fd5600a1da Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 8 May 2026 12:20:21 +0300 Subject: [PATCH 3/7] Make numeric_id? the exact complement of semantic_id? for Strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `semantic_id?` already strips surrounding whitespace before its round-trip check, so `" 123 "` was classified as not-semantic. `numeric_id?` did not strip, so the same value was also not-numeric — leaving it "neither" and unreachable through normal routing. Stripping in `numeric_id?` and expressing the String branch as `!semantic_id?(value)` removes the duplicated round-trip logic and gives the routing predicates a single source of truth: any String classified not-semantic is necessarily numeric. Integer/nil/other types are unaffected — they fall on their own branches. Routing-table spec gains a `" 123 "` row to lock the symmetry in. --- app/models/work_package/semantic_identifier.rb | 11 ++++++----- spec/models/work_package/semantic_identifier_spec.rb | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index f6c4db80df0c..794c2f5bbda7 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -111,14 +111,15 @@ def self.semantic_id?(value) # "123"). Rejects leading-zero strings ("0123"), non-numeric strings, # and nil. # - # Pairs with `semantic_id?` at call sites that need to gate inputs to - # one of two lookup paths. They answer different questions (shape vs - # routing) and so are kept independent rather than expressing one as - # the negation of the other. + # 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 value == value.to_i.to_s + when String then !semantic_id?(value) else false end end diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index 9fc8ee95798b..db61d40e68d5 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -467,6 +467,7 @@ [ ["123", :numeric], ["0", :numeric], + [" 123 ", :numeric], [123, :numeric], [0, :numeric], ["0123", :semantic], From 4b164150ac24ae7ffe772fa54b666fcf0a200824 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 8 May 2026 12:20:36 +0300 Subject: [PATCH 4/7] Pin ID_ROUTE_CONSTRAINT against silent widening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The route constraint composes the numeric and semantic shapes from SEMANTIC_ID_PATTERN, which itself composes the project-identifier shape from `Projects::Identifier::SEMANTIC_FORMAT`. A future change to either upstream pattern would shift what URLs Rails accepts without touching routes.rb. Three anchored assertions lock the boundary in place — the constant is used only as a route constraint, so anchored matching mirrors the actual call site. --- .../work_package/semantic_identifier_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index db61d40e68d5..e297008a5d8d 100644 --- a/spec/models/work_package/semantic_identifier_spec.rb +++ b/spec/models/work_package/semantic_identifier_spec.rb @@ -459,6 +459,27 @@ 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 From c3d5a2408f0eda6c6c38b394c423be2d94da25c7 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 8 May 2026 12:20:54 +0300 Subject: [PATCH 5/7] Document singular String input on where_display_id_in The method wraps a non-array via `Array(values)`, so a bare String or Integer is accepted and produces a one-element relation. The parameter name `values` (plural) doesn't make that obvious; a YARD `@param` line spells the contract out for callers. --- app/models/work_package/semantic_identifier/finder_methods.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index b7479343525c..01780022eed3 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -110,6 +110,10 @@ 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. + # + # @param values [Array, String, Integer] one or many + # display ids. A bare String/Integer is wrapped via Array() so callers + # can pass `where_display_id_in("PROJ-1")` and get a one-element relation. def where_display_id_in(values) values = Array(values).map(&:to_s) return none if values.empty? From 41cdfc8b77c5233b7dab9286bc61e5ae5d923414 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 13 May 2026 18:15:28 +0300 Subject: [PATCH 6/7] Trim verbose comments on semantic-id predicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The predicate docblocks above `semantic_id?` and `numeric_id?` leaned exhaustive — three-bullet routing taxonomy, a separate paragraph on non-string behaviour, and a comparative paragraph on the round-trip vs regex tradeoff. A tech lead reading these wants the principle (routing predicate, not strict shape validation) and the surprise (`"0123"` returns true). Each docblock now leads with that, drops the taxonomy bullets, and keeps the "don't tighten it" performance note. --- .../work_package/semantic_identifier.rb | 30 ++++++------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 794c2f5bbda7..621149f51bff 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -84,32 +84,20 @@ def relation end end - # Routing predicate for WP finders: returns true when value is a String - # that needs string-based identifier/alias lookup. Three input kinds - # return true: - # - true semantic identifiers ("PROJ-7") - # - non-canonical numerics ("0123", "00") - # - non-numeric strings ("abc") - # Only canonical numeric strings ("123") route to PK lookup and return - # false here. The predicate's name reflects routing intent, not strict - # shape validation — "0123" returns true even though it's clearly not - # a "semantic identifier" in the user-facing sense, because it still - # belongs on the string-lookup path. + # 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. # - # Non-strings (Integer, Hash, nil) return false — already in PK-lookup - # shape. - # - # The round-trip check (rather than a regex) is intentional for - # performance: every value reaching a finder either parses as an - # integer or doesn't, and that's enough to dispatch. Don't tighten it. + # 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 - # Shape predicate: 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. + # 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) From c24e3cfab0410de6c4a058a32318db2b46b528be Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 13 May 2026 18:26:23 +0300 Subject: [PATCH 7/7] Accept scalars, varargs, and arrays in where_display_id_in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Splat with a depth-1 flatten lets callers pass scalars, varargs, or a pre-built array interchangeably. `Array(values)` would have been misleading here — `Array([[1, "a"], 2])` leaves the inner array intact and `map(&:to_s)` stringifies it as `"[1, \"a\"]"`, which then misclassifies through the semantic-id branch. Bounded `flatten(1)` absorbs the (scalar) and ([scalar, scalar]) shapes that production code already uses while leaving deeper nesting alone, so the same pathology produces no match rather than silently working. Refs https://github.com/opf/openproject/pull/23202#discussion_r3235337043 --- .../semantic_identifier/finder_methods.rb | 11 ++++++----- spec/models/work_package/semantic_identifier_spec.rb | 5 +++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/models/work_package/semantic_identifier/finder_methods.rb b/app/models/work_package/semantic_identifier/finder_methods.rb index 01780022eed3..1cf742151477 100644 --- a/app/models/work_package/semantic_identifier/finder_methods.rb +++ b/app/models/work_package/semantic_identifier/finder_methods.rb @@ -111,11 +111,12 @@ def find_by_display_id!(identifier) # semantic strings may be freely mixed; unknown values produce no match # rather than poisoning the rest of the set. # - # @param values [Array, String, Integer] one or many - # display ids. A bare String/Integer is wrapped via Array() so callers - # can pass `where_display_id_in("PROJ-1")` and get a one-element relation. - 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) } diff --git a/spec/models/work_package/semantic_identifier_spec.rb b/spec/models/work_package/semantic_identifier_spec.rb index e297008a5d8d..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