From 8d9aa18ad3ff769a2da49130c2c4d8932852db92 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 May 2026 16:31:56 +0300 Subject: [PATCH] Collapse work-package preload state into one cache value object Pairs unscoped label resolution and viewer-scoped link gating in a WorkPackagePreloadCache instead of two RequestStore keys with a five-method save/restore protocol. Exposes one `current_cache` reader; consumers ask the cache directly via `fetch` and `visible?`. Extracts a `text_only?` predicate in the WP link handler so the `context[:plain_text]` and invisible-WP guards collapse into a single call site. `SemanticIdentifier.format` renames its parameter to reflect that the input may or may not be semantic. --- .../work_package/semantic_identifier.rb | 10 ++- .../matchers/link_handlers/work_packages.rb | 23 +++--- .../matchers/resource_links_matcher.rb | 78 +++++++++---------- .../link_handlers/work_packages_spec.rb | 8 +- 4 files changed, 62 insertions(+), 57 deletions(-) diff --git a/app/models/work_package/semantic_identifier.rb b/app/models/work_package/semantic_identifier.rb index 4dbd5dc76f2..aeb1025bf66 100644 --- a/app/models/work_package/semantic_identifier.rb +++ b/app/models/work_package/semantic_identifier.rb @@ -137,10 +137,12 @@ module WorkPackage::SemanticIdentifier WorkPackage::SemanticIdentifier.format(display_id) end - # Module-level variant for callers that already hold a display id and - # don't need the WorkPackage record. - def self.format(display_id) - display_id.is_a?(String) && display_id.match?(/[A-Za-z]/) ? display_id : "##{display_id}" + # Module-level variant of `#formatted_id` for callers that already hold + # an identifier value (which may or may not be semantic) and don't need + # the WorkPackage record. Semantic shapes pass through unchanged; + # numeric shapes get the classic `#N` prefix. + def self.format(value) + value.is_a?(String) && value.match?(/[A-Za-z]/) ? value : "##{value}" end # Override ActiveRecord's default `to_param` so Rails URL helpers diff --git a/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb b/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb index 755e67c1dd1..5926c297e41 100644 --- a/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb +++ b/lib/open_project/text_formatting/matchers/link_handlers/work_packages.rb @@ -78,7 +78,7 @@ module OpenProject::TextFormatting::Matchers # Both quickinfo and plain link need the WP record so the rendered # HTML can carry the record id in `data-id`. Unresolved WP → # literal text rather than a broken reference. - wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(display_id) + wp = preload_cache.fetch(display_id) return nil unless wp if quickinfo? @@ -89,7 +89,7 @@ module OpenProject::TextFormatting::Matchers end def render_for_numeric(wp_id) - wp = OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.work_package_for(wp_id) + wp = preload_cache.fetch(wp_id) if quickinfo? render_work_package_macro(work_package: wp, fallback_id: wp_id, detailed: detailed?) @@ -103,8 +103,7 @@ module OpenProject::TextFormatting::Matchers display_id = work_package&.display_id || fallback_id label = WorkPackage::SemanticIdentifier.format(display_id) - return label if context[:plain_text] - return label if work_package && !visible_to_current_user?(work_package.id) + return label if text_only?(work_package) ApplicationController.helpers.content_tag "opce-macro-wp-quickinfo", "", @@ -116,8 +115,7 @@ module OpenProject::TextFormatting::Matchers # render path bypassing `PatternMatcherFilter`) rather than running a # per-link query inside the renderer. label = work_package&.formatted_id || "##{fallback_id}" - return label if context[:plain_text] - return label if work_package && !visible_to_current_user?(work_package.id) + return label if text_only?(work_package) href_id = work_package&.display_id || fallback_id @@ -130,9 +128,16 @@ module OpenProject::TextFormatting::Matchers }) end - def visible_to_current_user?(work_package_id) - OpenProject::TextFormatting::Matchers::ResourceLinksMatcher - .visible_to_current_user?(work_package_id) + # Plain-text channels and inaccessible WPs both render the label + # without an anchor or quickinfo. Visibility is checked only when a + # WP was preloaded — a nil work_package means a classic-mode render + # or an unresolved reference, neither of which needs gating. + def text_only?(work_package) + context[:plain_text] || (work_package && !preload_cache.visible?(work_package.id)) + end + + def preload_cache + OpenProject::TextFormatting::Matchers::ResourceLinksMatcher.current_cache end end end diff --git a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb index e558eb677b9..e5574ea8ef0 100644 --- a/lib/open_project/text_formatting/matchers/resource_links_matcher.rb +++ b/lib/open_project/text_formatting/matchers/resource_links_matcher.rb @@ -69,9 +69,32 @@ module OpenProject::TextFormatting # identifier:version:1.0.0 # identifier:source:some/file class ResourceLinksMatcher < RegexMatcher - WORK_PACKAGES_LOOKUP_KEY = :text_formatting_work_packages_lookup - VISIBLE_WORK_PACKAGE_IDS_KEY = :text_formatting_visible_work_package_ids - private_constant :WORK_PACKAGES_LOOKUP_KEY, :VISIBLE_WORK_PACKAGE_IDS_KEY + # Per-render preload state shared between the matcher and the link + # handler. Pairs unscoped label resolution (`lookup`) with viewer-scoped + # link gating (`visible_ids`) so a single render computes both with a + # bounded number of round-trips and renders consistent labels for + # invisible WPs. + class WorkPackagePreloadCache + attr_reader :lookup, :visible_ids + + def initialize(lookup:, visible_ids:) + @lookup = lookup + @visible_ids = visible_ids + end + + def fetch(identifier) + lookup[identifier.to_s] + end + + def visible?(work_package_id) + visible_ids.include?(work_package_id) + end + + EMPTY = new(lookup: {}.freeze, visible_ids: Set.new.freeze).freeze + end + + CACHE_KEY = :text_formatting_work_package_preload_cache + private_constant :CACHE_KEY include ::OpenProject::TextFormatting::Truncation # used for the work package quick links @@ -131,57 +154,31 @@ module OpenProject::TextFormatting ] end - # Returns the preloaded WorkPackage for the given identifier (numeric - # or semantic), or nil if no preload is active (classic mode, no `#N` - # references) or the WP couldn't be resolved. Lookup keys are always - # strings — see `index_by_id_and_identifier`. - def self.work_package_for(identifier) - RequestStore.store.dig(WORK_PACKAGES_LOOKUP_KEY, identifier.to_s) - end - - # The main lookup is unscoped so labels resolve regardless of - # current-user permissions. This predicate gates whether the - # link handler emits a navigable anchor or a plain-text label. - def self.visible_to_current_user?(work_package_id) - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY]&.include?(work_package_id) || false + # The active preload cache for the current render, or an empty + # singleton when no preload is in scope (classic mode, no `#N` + # references, or rendering outside `with_preloaded_resources`). + def self.current_cache + RequestStore.store[CACHE_KEY] || WorkPackagePreloadCache::EMPTY end # Doc-level preload called by `PatternMatcherFilter`. Save/restores - # the lookup so a nested `format_text` (e.g. custom-field formatter + # the cache so a nested `format_text` (e.g. custom-field formatter # re-entering the pipeline) doesn't clobber the outer render. Classic # mode skips the load — `display_id` collapses to numeric, so the # link handler can render from the matched id alone. def self.with_preloaded_resources(doc, _context) - previous = stash_preload_state + previous = RequestStore.store[CACHE_KEY] return yield unless Setting::WorkPackageIdentifier.semantic? identifiers = collect_work_package_identifiers(doc) return yield if identifiers.empty? - install_preload_state(*build_lookup(identifiers)) + RequestStore.store[CACHE_KEY] = build_cache(identifiers) yield ensure - restore_preload_state(previous) + RequestStore.store[CACHE_KEY] = previous end - def self.stash_preload_state - [RequestStore.store[WORK_PACKAGES_LOOKUP_KEY], - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY]] - end - private_class_method :stash_preload_state - - def self.install_preload_state(lookup, visible_ids) - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = lookup - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY] = visible_ids - end - private_class_method :install_preload_state - - def self.restore_preload_state(previous) - RequestStore.store[WORK_PACKAGES_LOOKUP_KEY] = previous[0] - RequestStore.store[VISIBLE_WORK_PACKAGE_IDS_KEY] = previous[1] - end - private_class_method :restore_preload_state - def self.collect_work_package_identifiers(doc) identifiers = Set.new doc.search(".//text()").each do |node| @@ -218,14 +215,15 @@ module OpenProject::TextFormatting # for historical aliases — the loaded row carries only the current # identifier, so unmapped inputs are filled in from # `WorkPackageSemanticAlias`. - def self.build_lookup(identifiers) + def self.build_cache(identifiers) work_packages = WorkPackage.where_display_id_in(*identifiers).select(:id, :identifier).to_a all_wp_ids = work_packages.map(&:id) visible_ids = WorkPackage.visible.where(id: all_wp_ids).pluck(:id).to_set lookup = index_by_id_and_identifier(work_packages) fold_in_alias_keys(lookup, identifiers, all_wp_ids:) - [lookup, visible_ids] + WorkPackagePreloadCache.new(lookup:, visible_ids:) end + private_class_method :build_cache # Keys are stringified at write time so callers can read with a single # `identifier.to_s` regardless of whether the input is a numeric id or diff --git a/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb b/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb index 14a2a1cbbc4..a819e110a82 100644 --- a/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb +++ b/spec/lib/open_project/text_formatting/matchers/link_handlers/work_packages_spec.rb @@ -94,17 +94,17 @@ RSpec.describe OpenProject::TextFormatting::Matchers::LinkHandlers::WorkPackages inner_doc = Nokogiri::HTML.fragment("##{inner.id}") matcher.with_preloaded_resources(outer_doc, {}) do - expect(matcher.work_package_for(outer.id)).to eq(outer) + expect(matcher.current_cache.fetch(outer.id)).to eq(outer) matcher.with_preloaded_resources(inner_doc, {}) do - expect(matcher.work_package_for(inner.id)).to eq(inner) + expect(matcher.current_cache.fetch(inner.id)).to eq(inner) end - expect(matcher.work_package_for(outer.id)) + expect(matcher.current_cache.fetch(outer.id)) .to eq(outer), "outer lookup should be restored after nested call" end - expect(matcher.work_package_for(outer.id)).to be_nil + expect(matcher.current_cache.fetch(outer.id)).to be_nil end end