Share semantic-id predicates and pin the route constraint#23202
Merged
Conversation
Base automatically changed from
refactor/extract-semantic-project-identifier-format
to
dev
May 13, 2026 14:50
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.
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.
`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.
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.
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.
akabiru
force-pushed
the
refactor/semantic-id-predicates
branch
from
e002771 to
c3d5a24
Compare
4 tasks
thykel
reviewed
May 13, 2026
| # 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 |
Contributor
There was a problem hiding this comment.
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?
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.
akabiru
added a commit
that referenced
this pull request
May 13, 2026
The `Array(values)` wrapper was a convenience for callers passing a single scalar, but it made the signature look like the method handles arbitrary mixed inputs (`Array([['sup', 1], 1])` produces `[['sup', 1], 1]` — the inner array gets `to_s`-stringified into `"['sup', 1]"`). Drop the wrap, require an Array, and move the scalar-to-Array shim to the one caller that needed it: ApplicationController#find_work_packages turns a possibly-scalar `params[:work_package_id]` into an Array before calling. The conversion intent now lives at the call site, where ambiguous inputs become explicit. Refs #23202 (comment)
akabiru
added a commit
that referenced
this pull request
May 13, 2026
The previous tightening to `Array<String, Integer>` only (see commit bab0a45) addressed the reviewer's concern that bare `Array(...)` made the signature look like it handled nested-array inputs cleanly when it doesn't (`Array([[1, "a"], 2])` produces `[[1, "a"], 2]` — the nested array survives and stringifies oddly). But forcing arrays meant every caller — controller, wiki parser, text-formatting preload — that holds a value that might be a scalar or might be an array had to re-derive `Array(...)`. That's shotgun surgery across slices, and `ApplicationController#find_work_packages` was the first casualty. Splat fixes both: the signature reads as "one or more ids", and `flatten(1)` absorbs the ("a", "b") and (["a", "b"]) shapes that production code already uses, while still leaving deeper nesting alone (so `where_display_id_in([["a", "b"], "c"])` no longer collapses silently — `["a", "b"]` would partition into the semantic branch and produce no match). Refs #23202 (comment)
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 #23202 (comment)
akabiru
force-pushed
the
refactor/semantic-id-predicates
branch
from
2417304 to
c24e3cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/74946 (Split out of #22976)
What are you trying to accomplish?
Three call sites duplicated the
value == value.to_i.to_sround-trip that filters leading-zero ID forms ("0123"and friends) — the text-formatting matcher, the PDF export macro, and the cost-query filter. Each spelled it out independently.WorkPackage::SemanticIdentifiernow exposes two predicates that capture the question once:.numeric_id?for the canonical-numeric guard, and.semantic_id?(promoted fromFinderMethods) for the project-prefixed shape. They share a single whitespace-stripping path so" 123 "is consistently classified as numeric, not "neither".The cost-query filter switches to
numeric_id?here. Text-formatting and PDF callers convert in the follow-up slice.Three anchored assertions on
ID_ROUTE_CONSTRAINTlock the routing boundary in place — the constant composes fromSEMANTIC_ID_PATTERN, which composes fromProjects::Identifier::SEMANTIC_FORMAT, so a future change to either upstream pattern would silently widen what URLs Rails accepts without touching routes.rb. The assertions catch that.Merge checklist