Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions app/models/work_package/semantic_identifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,46 @@ 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.
#
# 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.
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.
#
# 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
Expand Down
12 changes: 5 additions & 7 deletions app/models/work_package/semantic_identifier/finder_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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>, String, Integer] one or many
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signature is odd and makes it seem like we can combine arrays with scalars. Not really the case.

irb(main):008> Array([["sup", 1], 1])
=> [["sup", 1], 1]
irb(main):009> Array([["sup", 1], 1]).map(&:to_s)
=> ["[\"sup\", 1]", "1"]

Can we keep it simple and make this accept an array of strings and/or integers?

# 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?
Expand Down Expand Up @@ -152,14 +156,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
56 changes: 56 additions & 0 deletions spec/models/work_package/semantic_identifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,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 },
Expand Down
Loading