Reinstate doc-level preload cache for WP text macros#23221
Open
akabiru wants to merge 1 commit into
Open
Conversation
Deploying openproject with ⚡ PullPreview
|
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reinstates a document-level preload cache for Work Package (#N / #PROJ-N) text macros so that, in semantic mode, numeric references can reliably render with the canonical semantic label (e.g., #1234 → PROJ-7) without falling back to the input label.
Changes:
- Adds a per-document WorkPackage lookup in
ResourceLinksMatcher(stored inRequestStore) and updates the WP link handler to use it instead of per-reference DB lookups. - Wraps
PatternMatcherFilterprocessing in matcher-level preload hooks, with save/restore semantics to support nestedformat_textcalls. - Introduces a shared
PreformattedBlockshelper for consistent<pre>/<code>ancestry skipping, and updates/extends specs to cover query bounds and nested preload restoration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb | Updates query-cost expectations (1 WP SELECT per render) and adds a regression test for nested preload save/restore behavior. |
| lib/open_project/text_formatting/preformatted_blocks.rb | Adds a reusable helper to detect <pre>/<code> ancestry without relying on HTML::Pipeline filter helpers. |
| lib/open_project/text_formatting/matchers/resource_links_matcher.rb | Implements RequestStore-backed doc-level preload for WP identifiers and alias folding to avoid N+1 lookups. |
| lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb | Switches WP resolution to use the preloaded cache and keeps a non-querying fallback label in classic/no-preload paths. |
| lib/open_project/text_formatting/filters/pattern_matcher_filter.rb | Wraps node processing with matcher preload hooks and reuses the shared preformatted-block skipping set. |
akabiru
force-pushed
the
feature/text-macro-preload-cache
branch
from
4d3b379 to
c406fc6
Compare
akabiru
force-pushed
the
feature/text-macro-semantic-id-rendering
branch
from
a70f389 to
c9d3b81
Compare
In semantic mode, every `#N` reference in legacy content needs a WP record to render the consistent `formatted_id` label users opted into. The per-match `find_by_display_id` fallback that ships in #23203 scales linearly with reference count: a wiki page with 50 refs runs 50 indexed PK lookups per render, and API collection endpoints / mailer fan-out / journal feeds compound the multiplier across records. `PatternMatcherFilter` now primes a `RequestStore`-backed lookup once per document via `ResourceLinksMatcher.with_preloaded_resources`. The link handler reads from `work_package_for(identifier)` so the same path serves numeric and semantic input. Cost is one batched `WorkPackage` SELECT per render, plus an alias-table SELECT only when historical identifiers are referenced. Classic mode short-circuits before any preload — `formatted_id` collapses to the numeric form, so the matched id alone is enough. The save/restore around nested `format_text` is retained: custom-field formatters re-enter the pipeline mid-render and must not clobber the outer document's lookup. The earlier `MAX_PRELOAD_IDENTIFIERS` cap is intentionally omitted. Silent truncation past position N would render the (N+1)th reference as literal text — a regression of the feature itself. Postgres handles several-thousand-bind `IN` clauses comfortably; the right safety net, if one is needed later, is log-and-continue, not truncate. `PreformattedBlocks` is restored so the preload visitor and the matcher's text-node walk share one `<pre>` / `<code>` skip. Follow-up to #23203
akabiru
force-pushed
the
feature/text-macro-preload-cache
branch
from
c406fc6 to
ce1681e
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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/74948
Stacked on #23203 (Please review that first)
What are you trying to accomplish?
Stored markdown runs through two filters in this order (formatter.rb):
MentionFilter—<mention data-id="1234" data-display-id="PROJ-7">envelopes (autocomplete-picked).PatternMatcherFilter— bare text like#1234,#PROJ-7,##1234.In classic mode the matched id is the label, so link building skips the database entirely. Semantic mode always translates to the canonical
formatted_id, regardless of the form stored in the body:#1234(primary key — the form most legacy content carries, since bulk content is not rewritten when an instance switches modes) →PROJ-7.#OLD-7(historical alias, left behind by a work package move or project rename) →PROJ-7.#PROJ-7(current semantic id) →PROJ-7.All three reach the same renderer and all three need a lookup.
What the preload changes for the user
One thing:
#1234in semantic mode, with preload →<a>PROJ-7</a>(translated label).#1234in semantic mode, without preload →<a>#1234</a>(label falls back to input).Both link to the right work package. Only the visible label regresses, which is the bug this fixes.
What approach did you choose and why?
PatternMatcherFilterprimes a per-documentRequestStorelookup before walking matches.build_lookupissuesWHERE id IN (…) OR EXISTS (…aliases…)againstwork_packages, plus a targetedWorkPackageSemanticAliasquery when historical identifiers appear — two round-trips, bounded, regardless of match count. Classic mode short-circuits before any preload.No truncation cap on the identifier set: silently dropping past the Nth match would render the (N+1)th reference as literal text, which is the regression this fixes. Postgres handles several-thousand-bind
INclauses without complaint.The save/restore around nested
format_textis there because custom-field formatters re-enter the pipeline mid-render; the outer document's lookup must not be clobbered by an inner render's preload.Merge checklist
work_packages_spec.rb,in_tool_links_spec.rb,mentions_spec.rbgreen#Nbody, mixed#N/#PROJ-Nbody)