Fix N+1 query on event image attachments in FormResponsePresenter - #11889
Merged
Conversation
Sentry showed an active_storage_attachments N+1 (INTERCODE-18B/18D) appearing after the EventDrop fix in #11883 -- turned out to be a sibling gap in the same "images" association, in a different presenter. FormResponsePresenter#local_images already memoized per instance correctly, but a fresh presenter is constructed per Event when the form_response_attrs_json_with_rendered_markdown GraphQL field (used to render markdown-formatted form fields like description/short_blurb) is requested for multiple events in one query, and each one queried images.includes(:blob) independently. Route it through the same Sources::ActiveStorageAttachment dataloader source EventType#images already uses, when a dataloader is available (GraphQL context) -- falls back to the direct query for the Liquid/service call sites that don't have one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
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.
Summary
After #11883 deployed (fixing the
active_storage_attachmentsN+1 inEventDrop's markdown fields), Sentry surfaced a new occurrence of the same query shape (INTERCODE-18B/18D) underEvent.team_members/Event.registration_policy. Same underlying association (Event#images), different call site.Root cause
FormResponsePresenter#local_images(used to resolve inline image references when rendering markdown-formatted form fields likedescription/short_blurbvia theformResponseAttrsJsonWithRenderedMarkdownGraphQL field) already memoizes correctly within one presenter instance -- but a fresh presenter gets constructed perEventwhen this field is requested for multiple events in one GraphQL query (e.g. a page listing several events each requesting this field), and each one calledresponse.images.includes(:blob)independently with no cross-event batching.Fix
FormResponsePresenteralready receives an optionaldataloader:(only ever passed by the GraphQL-facingFormResponseAttrsFieldsconcern -- Liquid/service callers don't have one). When present, routelocal_imagesthroughSources::ActiveStorageAttachment-- the same dataloader sourceEventType#imagesalready uses -- so all events resolving the field in one request get batched into a single preload. Falls back to the direct query when there's no dataloader (Liquid rendering, mutations).Also investigated, not fixed
INTERCODE-18A(registration_policy_bucketsby id) is a straggler from before the UserConProfileDrop fix deployed (release tag confirms it predates #11882) -- resolving it in Sentry as already fixed, not a new bug.INTERCODE-18C(cms_partialsN+1, same shape as the earlierINTERCODE-182) traces toCadmus::PartialFileSystem#read_template_file(in the third-partycadmusgem) doing an uncachedfind_by!on every{% render "name" %}Liquid tag. The app already builds acached_partialspreload hash for exactly this (CmsRenderingContext#preload_page_content), but Cadmus's file system never consults it -- and the cache stores parsedLiquid::Templateobjects while the file-system contract needs raw content strings, so it's not a drop-in fix. Given the low/ambiguous signal (single occurrences, no "repeated N times" breakdown from Sentry -- could just be two different partials on one page) and the blast radius of changing the global Liquid rendering pipeline used by every convention site, deliberately leaving this one alone rather than rushing a fix to a third-party integration point.Test plan
test/graphql/types/event_type_test.rb) executing a real GraphQL query for 5 events, verified to fail without the fix (5 queries) and pass with it (1 query)test/liquid_drops/,test/services/event_signup_service_test.rb,test/services/execute_ranked_choice_signup_service_test.rb,test/models/run_test.rb,test/presenters/signup_count_presenter_test.rb(98 tests)Metrics/ParameterListsoffense on this file's 7-arg constructor added to.rubocop_todo.yml, matching existing entries for other presenters/services with the same debt -- not something this fix should refactor)🤖 Generated with Claude Code