Prevent WorkPackage::SemanticIdentifier::UnsupportedLookup with empty ID (#23213)

---------

Co-authored-by: Kabiru Mwenja <k.mwenja@openproject.com>
This commit is contained in:
Alexander
2026-05-15 08:36:10 +03:00
committed by GitHub
parent 11fbfc3880
commit 6350700c99
3 changed files with 49 additions and 12 deletions
+17 -10
View File
@@ -85,29 +85,36 @@ module WorkPackage::SemanticIdentifier
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.
# ("PROJ-42"). Non-strings (Integer, Hash, nil, Array), empty 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
return false unless value.is_a?(String)
stripped = value.strip
return false if stripped.empty?
stripped.to_i.to_s != stripped
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.
# Rejects leading-zero strings ("0123"), non-numeric strings, empty 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.
# A numeric ID is always a routable primary key; a semantic ID is always
# routed through the identifier/alias path. Anything else — nil, blank
# strings, Hashes, Arrays — is neither, and both predicates return false
# so the caller short-circuits before any lookup.
def self.numeric_id?(value)
case value
when Integer then true
when String then !semantic_id?(value)
when String
return false if value.strip.blank?
!semantic_id?(value)
else false
end
end
@@ -116,7 +116,7 @@ module WorkPackage::SemanticIdentifier::FinderMethods
# (`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)
values = values.flatten(1).compact_blank.map(&:to_s)
return none if values.empty?
semantic, numeric = values.partition { semantic_id?(it) }
@@ -266,6 +266,18 @@ RSpec.describe WorkPackage::SemanticIdentifier do
end
end
context "with id: keyword and blank string" do
it "does not treat an empty id as a semantic identifier" do
expect { WorkPackage.find_by(id: "") }.not_to raise_error
expect(WorkPackage.find_by(id: "")).to be_nil
end
it "does not treat whitespace-only id as semantic" do
expect { WorkPackage.find_by(id: " \t ") }.not_to raise_error
expect(WorkPackage.find_by(id: " \t ")).to be_nil
end
end
context "with id: keyword and an array" do
let(:work_package2) { create(:work_package, project:) }
@@ -390,6 +402,24 @@ RSpec.describe WorkPackage::SemanticIdentifier do
expect(WorkPackage.where_display_id_in([])).to be_empty
end
it "treats a blank string as no input" do
relation = WorkPackage.where_display_id_in("")
expect(relation).to be_empty
# Without filtering, "".to_i would build WHERE id = 0 — semantically wrong
# even though no row matches in practice.
expect(relation.to_sql).not_to match(/"id"\s*=\s*0\b/)
expect(relation.to_sql).not_to match(/"id"\s+IN\s*\(0\)/)
end
it "treats whitespace-only strings as no input" do
expect(WorkPackage.where_display_id_in(" ")).to be_empty
end
it "drops blank entries from arrays without poisoning the rest of the set" do
expect(WorkPackage.where_display_id_in(["MYPROJ-1", "", " ", nil]))
.to contain_exactly(work_package)
end
it "wraps a single non-array value" do
expect(WorkPackage.where_display_id_in("MYPROJ-1")).to contain_exactly(work_package)
end
@@ -500,7 +530,7 @@ RSpec.describe WorkPackage::SemanticIdentifier do
["00", :semantic],
["PROJ-1", :semantic],
["abc", :semantic],
["", :semantic],
["", :neither],
[nil, :neither],
[{}, :neither]
].each do |value, classification|