fix: Blueprinter adoption + query performance hardening#714
Conversation
* upating package.json to 2.3.3 for clean release Signed-off-by: Will <will@dower.dev> * fix: remove release workflow specs after workflow deletion The release_infrastructure_spec tested the deleted release.yml workflow file, causing CI to fail on the release-triggered run. Remove the release workflow describe block; keep changelog, version consistency, Docker trigger, and API version specs. Signed-off-by: Will Dower <will@dower.dev> * Fix/OIDC provider conflict (#711) * fix: resolve OIDC provider conflict and add auto-link OmniAuth returns auth.provider as symbol (:oidc) but DB stores string ("oidc"). Comparison always triggered ProviderConflictError. rescue_from ordering hid the actionable message behind generic "unexpected error." - Provider+uid-first lookup in User.from_omniauth - .to_s coercion for type-safe comparison - Fix rescue_from ordering (StandardError first) - Add VULCAN_AUTO_LINK_USER global setting - email_verified check for auto-link security - just_auto_linked? flag (no duplicate lookup) - Genericize oauth_error flash message - Replace gitlab_omniauth-ldap with omniauth-ldap - Remove nkf gem, lazy-load LDAP - 75 backend auth specs Authored by: Aaron Lippold<lippold@gmail.com> * feat: session auth tracking, profile UX, unlink - session[:auth_method] tracks HOW user signed in - Profile shows "Signed in via X" + "Linked to Y" - POST /users/unlink_identity with password check - Unlink button with confirmation modal - 15 backend + 29 frontend tests Authored by: Aaron Lippold<lippold@gmail.com> * fix: pre-existing bugs + infrastructure hardening Bug fixes found during auth code review: - vulcan_audit.rb: bitwise & vs && crash on nil rule - users_controller: Slack fires on every update (now gated on saved_change_to_admin?) - History.vue: suppress raw field changes when audit has a comment (link/unlink UX) - registrations: polymorphic audit + user_type Infrastructure: - Ruby 3.4.8 -> 3.4.9 - parallel_sync.rake: recursion guard - docker-compose.dev.yml: trust auth for VPN Authored by: Aaron Lippold<lippold@gmail.com> * docs: add what's-left tracking and beads export - WHATS-LEFT.md: pending bug fixes, future features - beads-export.jsonl: full task board for handoff Authored by: Aaron Lippold<lippold@gmail.com> * chore: fix Gemfile extra blank line (rubocop) Authored by: Aaron Lippold<lippold@gmail.com> * test: add TDD tests for Slack notification gate and polymorphic audit filter 71q.1: Verify Slack notification only fires on admin flag changes, not on name/email updates. Tests promotion, demotion, and no-op cases. 71q.2: Verify registrations#edit filters audits by user_type: 'User', excluding rogue audits with matching user_id but different user_type. Signed-off-by: Will Dower <will@dower.dev> * fix: restore prev_unconfirmed_email for reconfirmation flash (71q.3) The update action read resource.unconfirmed_email but discarded the value (no assignment). After email change, users got generic "Profile updated" instead of "confirmation link sent to new address". Fix: capture prev_unconfirmed_email, compare after update, show appropriate flash message per Devise stock behavior. TDD: red (email-change test failed with generic flash) → green. Signed-off-by: Will Dower <will@dower.dev> * fix: use update_columns for reset token to bypass validations (71q.4) generate_reset_url used update! which runs full validations. If a user has pre-existing validation failures (e.g., name exceeds a tightened limit), the admin's reset link generation fails with RecordInvalid — blocking an unrelated recovery flow. Fix: use update_columns (matches Devise's save(validate: false) pattern). Token writes carry no business logic. TDD: red (user with 500-char name → validation error) → green. Signed-off-by: Will Dower <will@dower.dev> * fix: stop leaking exception.message in users_controller rescues (71q.5) Both send_password_reset and set_password rescue blocks echoed e.message directly to the client, leaking SMTP hostnames, DB connection strings, or other internal details. Fix: log full exception with backtrace server-side via Rails.logger.error, return generic message to client. TDD: - send_password_reset: red (response contained 'smtp.internal.corp') → green (generic message returned) - set_password: source inspection test verifies rescue block does not interpolate e.message (Rails test mode re-raises before controller rescue runs, preventing request-level testing) Signed-off-by: Will Dower <will@dower.dev> * fix: remove dead authProvider computed and add visibility guard (71q.6, 71q.7) 71q.6: Add source-inspection test ensuring no bare 'public' keyword exists between private and protected sections in registrations controller (issue was already fixed, test prevents regression). 71q.7: Remove dead authProvider computed property from UserProfile.vue. Nothing in the template or script references it — the refactored linkedProvider + currentSessionMethod properties cover all use cases. TDD: red (authProvider still in computed options) → green (removed). Signed-off-by: Will Dower <will@dower.dev> * fix: P3/P4 hardening — backtraces, email_verified, null guard, constants, docs 71q.8: Log OmniAuth exception backtraces in all environments (was dev-only). Use error level with first 10 frames. 71q.9: Cast email_verified OIDC claim via ActiveModel::Type::Boolean to catch providers sending "false" (string) instead of false. 71q.10: Use falsy check in UsersTable typeColumn to handle both null and undefined provider gracefully. 71q.11: Normalize PROJECT_MEMBER_ADMINS to array for consistency with VIEWERS/AUTHORS/REVIEWERS. Add ROLE_ADMIN scalar constant for attribute assignment. Update call sites. 71q.12: Document valid_password? hidden bcrypt→PBKDF2 rehash side-effect at the unlink call site. Signed-off-by: Will Dower <will@dower.dev> * fix: use fake ID in vulcan_audit destroy action test Test referenced undefined `rule` variable. Use a fake ID like the adjacent nil-rule test — the destroy action guard skips before any DB lookup so the ID value doesn't matter. Signed-off-by: Will Dower <will@dower.dev> * fix: address Copilot review — v-for key, remove dev artifacts - Fix History.vue v-for key: changes.id is undefined, use changes.field with index fallback for stable Vue diffing - Remove WHATS-LEFT.md and beads-export.jsonl (branch-specific tracking docs, not for the repo) Signed-off-by: Will Dower <will@dower.dev> * fix: properly exercise RecordNotUnique retry path in race condition test Test claimed to verify retry-on-RecordNotUnique but never stubbed save! to raise. Now stubs save! to raise once, pre-creates the user with matching provider+uid so retry lookup succeeds, and asserts save! was called exactly once before the retry found the existing user. Signed-off-by: Will Dower <will@dower.dev> --------- Signed-off-by: Will Dower <will@dower.dev> Co-authored-by: Aaron Lippold <lippold@gmail.com> * fix: exclude xml/binary blobs from with_severity_counts (prod crash) (#713) * fix: exclude xml/binary blobs from with_severity_counts The with_severity_counts scope used select("table.*") which loaded ALL columns including multi-MB xml blobs on Stig and SRG models. On index pages this blew Heroku dyno memory (R14/R15) and caused 30s timeouts (H12), crashing the app. Fix: auto-detect columns of type xml/binary and exclude them from the SELECT in with_severity_counts. This is a DRY fix at the concern level — all models (Stig, SRG, Component) benefit without per-controller workarounds. Models without heavy columns are unaffected (all columns loaded as before). The xml column is still loaded via Stig.find(id) for export/download endpoints that need it. Authored by: Aaron Lippold<lippold@gmail.com> * fix: address Copilot review — use abstract column type, update docs, dynamic test assertions - Use c.type (ActiveRecord abstract) instead of c.sql_type (adapter- specific) for heavy column detection. Catches Postgres bytea, MySQL blob, etc. - Update concern header docs to reflect auto-generated scope (was stale "define your own" instruction) - Spec column assertions now dynamically check all non-blob columns instead of hardcoded subset Signed-off-by: Will Dower <will@dower.dev> --------- Signed-off-by: Will Dower <will@dower.dev> Co-authored-by: Will Dower <will@dower.dev> * bumping versionfile Signed-off-by: Will Dower <will@dower.dev> * fix: fixes to development composefile - Bind Puma to 0.0.0.0 in dev compose (was 127.0.0.1, unreachable from Docker network) - Clear stale server.pid on startup (volume mount persists it) - Run yarn build:watch in background instead of foreman (avoids missing bundle exec and bind address issues) - Align Dockerfile Ruby version to 3.4.9 Signed-off-by: Will Dower <will@dower.dev> --------- Signed-off-by: Will <will@dower.dev> Signed-off-by: Will Dower <will@dower.dev> Co-authored-by: Aaron Lippold <lippold@gmail.com>
Rule#as_json called SecurityRequirementsGuide.find_by() per rule, generating 200+ identical queries per component page — each loading the full SRG including multi-MB xml. Replace with srg_rule&.security_requirements_guide&.version which uses the already-loaded association chain. Zero additional queries when eager_loaded. 4 TDD specs verifying zero SRG queries during as_json. Authored by: Aaron Lippold<lippold@gmail.com>
Global search loaded stigs.* and srgs.* without .select(), pulling multi-MB xml blobs into memory on every search keystroke. Added explicit .select() with only the columns used in the .map block. 2 TDD specs verify xml is not loaded during search. Authored by: Aaron Lippold<lippold@gmail.com>
The HTML show path called .to_json(methods: [:stig_rules]) which serialized the full multi-MB xml column into the page body as a Vue v-bind attribute. The JSON path already used jbuilder which correctly excluded xml. Add except: [:xml] to both STIG and SRG show HTML paths. Export/download endpoints are unaffected (use separate find). 2 TDD specs verify xml is not in the HTML response. Authored by: Aaron Lippold<lippold@gmail.com>
Adopt Blueprinter for JSON serialization, replacing model as_json overrides. Follows GitLab/Discourse pattern of dedicated serializer classes per context. Setup: blueprinter 1.2.1, blueprinter-activerecord 1.3.0, oj for 2x faster JSON generation, auto N+1 preloader. Blueprints: Check, DisaRuleDescription, RuleDescription, AdditionalAnswer, Review, Membership, User (compact). 16 TDD specs verifying output shape compatibility. Authored by: Aaron Lippold<lippold@gmail.com>
Three context-specific serialization views: - :navigator — sidebar list (id, rule_id, status, severity) - :viewer — read-only detail (adds text fields, checks, DRDs) - :editor — full form (adds reviews, SRG data, satisfactions) Also includes SrgRuleBlueprint, SatisfactionBlueprint, SatisfiedByBlueprint for nested associations. Zero N+1 queries when rules are properly eager-loaded. 11 TDD specs including N+1 verification. Authored by: Aaron Lippold<lippold@gmail.com>
XML column is structurally excluded from all views — never declared in the blueprint, so it can never be serialized. StigBlueprint :index (listing), :show (detail + stig_rules) SrgBlueprint :index (listing), :show (detail + srg_rules) StigRuleBlueprint for nested STIG rule associations. 13 TDD specs verifying xml exclusion and field presence. Authored by: Aaron Lippold<lippold@gmail.com>
Replaces to_json(methods: [...]) pattern on component show. Views: - :index — listing (name, prefix, version, severity_counts) - :show — read-only (adds rules via RuleBlueprint :viewer) - :editor — full form (histories, memberships, metadata, inherited_memberships, available_members, all_users, rules via RuleBlueprint :editor, reviews, status_counts) Excludes dead admins field (no Vue consumer on component pages per analysis). Uses UserBlueprint for available_members and all_users (only id, name, email — no sensitive fields). 13 TDD specs. Total blueprint suite: 53 specs passing. Authored by: Aaron Lippold<lippold@gmail.com>
Replace to_json(methods: [...]) with Blueprint.render in: - StigsController index + show - SecurityRequirementsGuidesController index + show - ComponentsController index, show, find HAML templates updated to use pre-rendered JSON from controller instead of calling .to_json in the view. Component show now uses ComponentBlueprint :editor/:show based on effective_permissions, eliminating the 9-method to_json call that triggered N+1 chains. 55 existing request specs pass without modification. Authored by: Aaron Lippold<lippold@gmail.com>
Migrate RulesController (show, create, section_locks, bulk_section_locks, related_rules) and rules/index HAML to use RuleBlueprint/ComponentBlueprint. Add DEPRECATED comments to as_json overrides on BaseRule, Rule, Review, Membership — directing to blueprints. Overrides remain for non-controller callers (rake tasks, projects controller, application controller) until those are migrated in follow-up work. 129 specs passing. Authored by: Aaron Lippold<lippold@gmail.com>
Migrate ALL remaining controllers to blueprints: - ProjectsController index (ProjectIndexBlueprint with batched access_request lookup, no more N+1) - ProjectsController show (ProjectBlueprint :show) - RulesController show, create, section_locks, related - ApplicationController check_access_request_notifications rewritten as single query (was N+1 per project) - Component jbuilder uses RuleBlueprint for rules Remove as_json overrides from: BaseRule, Rule, Review, Membership. Rake task migrated from as_json to .attributes. Update 9 test files to use RuleBlueprint instead of removed as_json. All 2018 frontend + 1772 backend tests. Authored by: Aaron Lippold<lippold@gmail.com>
Authored by: Aaron Lippold<lippold@gmail.com>
Previously "Generate Reset Link" and "Set password manually" were hidden behind v-else when SMTP was enabled. Admins should always have access to direct password management regardless of SMTP configuration. Now all three options are visible: - Send Password Reset Email (when SMTP enabled) - Generate Reset Link (always) - Set password manually (always, collapsed) 32 EditUserModal specs passing. Authored by: Aaron Lippold<lippold@gmail.com>
9502600 to
9d3e8e6
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Migrates JSON serialization from model as_json overrides to Blueprinter while hardening key endpoints against memory/timeouts caused by loading multi‑MB XML columns and N+1 queries.
Changes:
- Introduces Blueprinter + Oj + auto-preloading, adding model-specific blueprints with context views.
- Updates controllers/views to render blueprint JSON (and excludes XML from search/show paths).
- Adds request/model/blueprint specs to lock in performance and output shape.
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/requests/stigs_spec.rb | Adds regression coverage to ensure STIG HTML doesn’t embed XML blob |
| spec/requests/api/search_spec.rb | Adds query-level checks to ensure search excludes XML columns |
| spec/models/rules_spec.rb | Updates rule serialization expectations to use RuleBlueprint |
| spec/models/rule_as_json_performance_spec.rb | Adds performance spec around SRG query behavior during serialization |
| spec/blueprints/stig_srg_blueprints_spec.rb | Adds blueprint contract tests for STIG/SRG output + XML exclusion |
| spec/blueprints/rule_blueprint_spec.rb | Adds RuleBlueprint contract + N+1 assertions |
| spec/blueprints/review_membership_blueprints_spec.rb | Adds tests for review/membership/user blueprint shapes |
| spec/blueprints/leaf_blueprints_spec.rb | Adds tests for leaf blueprints and nested-attrs compatibility |
| spec/blueprints/component_blueprint_spec.rb | Adds tests for component blueprint views and nested serialization |
| lib/tasks/stig_and_srg_puller.rake | Stops relying on as_json for DB updates; uses .attributes |
| config/initializers/blueprinter.rb | Configures Blueprinter generator (Oj), sorting, and AR preloader |
| app/views/stigs/index.html.haml | Switches STIG list prop from to_json to pre-rendered blueprint JSON |
| app/views/security_requirements_guides/index.html.haml | Switches SRG list prop from to_json to pre-rendered blueprint JSON |
| app/views/rules/index.html.haml | Switches component/rules props to pre-rendered blueprint JSON |
| app/views/components/show.json.jbuilder | Replaces rule as_json merge with RuleBlueprint rendering |
| app/models/rule.rb | Removes Rule as_json override; documents blueprint ownership |
| app/models/review.rb | Removes Review as_json override; documents blueprint ownership |
| app/models/membership.rb | Removes Membership as_json override; documents blueprint ownership |
| app/models/base_rule.rb | Removes BaseRule as_json override; documents blueprint ownership |
| app/javascript/components/users/EditUserModal.vue | Adjusts password management section rendering |
| app/controllers/stigs_controller.rb | Uses StigBlueprint for HTML JSON payloads |
| app/controllers/security_requirements_guides_controller.rb | Uses SrgBlueprint for HTML JSON payloads |
| app/controllers/rules_controller.rb | Uses Rule/Component blueprints; updates related-rules endpoint |
| app/controllers/projects_controller.rb | Batches access-request lookup; uses Project blueprints |
| app/controllers/components_controller.rb | Uses Component/Rule blueprints for HTML and JSON rendering |
| app/controllers/application_controller.rb | Rewrites access-request notifications to a single query |
| app/controllers/api/search_controller.rb | Adds .select to SRG/STIG search to exclude XML columns |
| app/blueprints/user_blueprint.rb | Adds compact user blueprint |
| app/blueprints/stig_rule_blueprint.rb | Adds STIG rule blueprint for show views |
| app/blueprints/stig_blueprint.rb | Adds STIG blueprint with XML excluded by design |
| app/blueprints/srg_rule_blueprint.rb | Adds SRG rule blueprint for nested SRG data |
| app/blueprints/srg_blueprint.rb | Adds SRG blueprint with XML excluded by design |
| app/blueprints/satisfied_by_blueprint.rb | Adds satisfied-by specialization blueprint |
| app/blueprints/satisfaction_blueprint.rb | Adds base satisfaction relationship blueprint |
| app/blueprints/rule_description_blueprint.rb | Adds rule description leaf blueprint |
| app/blueprints/rule_blueprint.rb | Adds RuleBlueprint with :navigator/:viewer/:editor views |
| app/blueprints/review_blueprint.rb | Adds ReviewBlueprint to control review payload shape |
| app/blueprints/project_index_blueprint.rb | Adds ProjectIndexBlueprint with per-user computed fields |
| app/blueprints/project_blueprint.rb | Adds ProjectBlueprint with :show view + nested associations |
| app/blueprints/membership_blueprint.rb | Adds MembershipBlueprint including delegated user fields |
| app/blueprints/disa_rule_description_blueprint.rb | Adds DISA rule description leaf blueprint |
| app/blueprints/component_blueprint.rb | Adds ComponentBlueprint with :index/:show/:editor views |
| app/blueprints/check_blueprint.rb | Adds check leaf blueprint with _destroy support |
| app/blueprints/additional_answer_blueprint.rb | Adds additional answer leaf blueprint |
| Gemfile.lock | Adds blueprinter, blueprinter-activerecord, and oj dependencies |
| Gemfile | Adds blueprinter, blueprinter-activerecord, and oj gems |
Comments suppressed due to low confidence (8)
app/views/components/show.json.jbuilder:1
- This does a JSON encode + JSON.parse round-trip inside the Jbuilder template, adding avoidable CPU and memory overhead. Prefer
RuleBlueprint.render_as_hash(@component.rules, view: :editor)(or equivalent) so Jbuilder receives Ruby hashes/arrays directly and you avoid parsing potentially large payloads.
spec/requests/api/search_spec.rb:1 - The SQL detection is overly broad:
sql.include?('.*')can match unrelatedSELECT \"users\".*queries, andsql.include?('stigs')/security_requirements_guideswill also match JOINs where the selected columns are not from those tables. This can cause false positives/flaky failures. Tighten the matcher to specifically detect selecting\"stigs\".*/\"security_requirements_guides\".*or selecting thexmlcolumn for those tables (ideally via a regex), and consider excluding schema/cached queries similar to other specs.
spec/requests/api/search_spec.rb:1 - The SQL detection is overly broad:
sql.include?('.*')can match unrelatedSELECT \"users\".*queries, andsql.include?('stigs')/security_requirements_guideswill also match JOINs where the selected columns are not from those tables. This can cause false positives/flaky failures. Tighten the matcher to specifically detect selecting\"stigs\".*/\"security_requirements_guides\".*or selecting thexmlcolumn for those tables (ideally via a regex), and consider excluding schema/cached queries similar to other specs.
spec/requests/stigs_spec.rb:1 - This failure message references
except: [:xml] in to_json, but the implementation is now blueprint-based (StigBlueprint). Update the message to reflect the current mechanism (e.g., 'ensure StigBlueprint does not declare :xml / ensure controller uses StigBlueprint :show') so future failures point to the right fix.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is still framed around
Rule#as_json, but the PR removes the customas_jsonoverrides and shifts the performance-critical serialization toRuleBlueprint. As written, it primarily asserts default ActiveRecordas_jsonbehavior (which is less relevant) and may give a false sense of coverage for blueprint serialization. Consider renaming/re-scoping the spec toRuleBlueprintand measuringRuleBlueprint.render_as_hash(..., view: :editor)instead ofrule.as_json.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is still framed around
Rule#as_json, but the PR removes the customas_jsonoverrides and shifts the performance-critical serialization toRuleBlueprint. As written, it primarily asserts default ActiveRecordas_jsonbehavior (which is less relevant) and may give a false sense of coverage for blueprint serialization. Consider renaming/re-scoping the spec toRuleBlueprintand measuringRuleBlueprint.render_as_hash(..., view: :editor)instead ofrule.as_json.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is still framed around
Rule#as_json, but the PR removes the customas_jsonoverrides and shifts the performance-critical serialization toRuleBlueprint. As written, it primarily asserts default ActiveRecordas_jsonbehavior (which is less relevant) and may give a false sense of coverage for blueprint serialization. Consider renaming/re-scoping the spec toRuleBlueprintand measuringRuleBlueprint.render_as_hash(..., view: :editor)instead ofrule.as_json.
config/initializers/blueprinter.rb:1 - The comment says fields are sorted alphabetically by name, but
config.sort_fields_by = :definitionreads as sorting by definition order (not name). Align the configuration with the stated intent (or update the comment to match the actual behavior) so readers don’t misinterpret output ordering guarantees.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| admin_project_ids = if current_user.admin? | ||
| Project.pluck(:id) | ||
| else | ||
| Membership.where(user_id: current_user.id, role: 'admin') |
There was a problem hiding this comment.
For non-super-admins, this plucks membership_id from a polymorphic Membership without constraining membership_type. If the user is an admin of any non-Project membership (e.g., Component), those IDs can be incorrectly treated as project_ids when querying ProjectAccessRequest. Filter the membership query to membership_type: 'Project' (and ideally pluck project IDs via an explicit join/scope) so only project IDs are used.
| Membership.where(user_id: current_user.id, role: 'admin') | |
| Membership.where(user_id: current_user.id, role: 'admin', membership_type: 'Project') |
There was a problem hiding this comment.
Fixed in commit b24bb7c — added membership_type: 'Project' filter. Already addressed.
|
|
||
| <!-- SMTP available: send reset email --> | ||
| <div v-if="smtpEnabled"> | ||
| <div v-if="smtpEnabled" class="mb-3"> |
There was a problem hiding this comment.
The second block used to be v-else but is now unconditional, so when smtpEnabled is true both the 'send reset email' UI and the 'generate link or set password' UI will render together. If the intent is to show one or the other, restore mutually-exclusive rendering (or add an explicit condition matching the comment, e.g., admin-only).
There was a problem hiding this comment.
Intentional — this is a deliberate UX change. Admins should always see all password management options (send email + generate link + set directly) regardless of SMTP. Previously, admins with no SMTP had no way to help users reset passwords. Both blocks rendering together is correct.
- ApplicationController: add membership_type: 'Project' filter to prevent Component admin memberships from being treated as project IDs in access request query - Jbuilder: use render_as_hash instead of JSON.parse(render) to avoid unnecessary encode+parse round-trip - Fix misleading sort_fields_by comment in initializer Authored by: Aaron Lippold<lippold@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 50 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (7)
spec/models/rule_as_json_performance_spec.rb:1
- This spec is now validating
as_json, but the PR explicitly moves serialization toRuleBlueprint. As written, it may pass even if the blueprint regresses into N+1s (since default ActiveRecordas_jsonlikely won’t traverse SRG associations). Update the spec description and assertions to measureRuleBlueprint.render_as_hash(..., view: :editor)instead ofas_json, and consider renaming the file to reflect the blueprint.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is now validating
as_json, but the PR explicitly moves serialization toRuleBlueprint. As written, it may pass even if the blueprint regresses into N+1s (since default ActiveRecordas_jsonlikely won’t traverse SRG associations). Update the spec description and assertions to measureRuleBlueprint.render_as_hash(..., view: :editor)instead ofas_json, and consider renaming the file to reflect the blueprint.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is now validating
as_json, but the PR explicitly moves serialization toRuleBlueprint. As written, it may pass even if the blueprint regresses into N+1s (since default ActiveRecordas_jsonlikely won’t traverse SRG associations). Update the spec description and assertions to measureRuleBlueprint.render_as_hash(..., view: :editor)instead ofas_json, and consider renaming the file to reflect the blueprint.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is now validating
as_json, but the PR explicitly moves serialization toRuleBlueprint. As written, it may pass even if the blueprint regresses into N+1s (since default ActiveRecordas_jsonlikely won’t traverse SRG associations). Update the spec description and assertions to measureRuleBlueprint.render_as_hash(..., view: :editor)instead ofas_json, and consider renaming the file to reflect the blueprint.
spec/models/rule_as_json_performance_spec.rb:1 - This spec is now validating
as_json, but the PR explicitly moves serialization toRuleBlueprint. As written, it may pass even if the blueprint regresses into N+1s (since default ActiveRecordas_jsonlikely won’t traverse SRG associations). Update the spec description and assertions to measureRuleBlueprint.render_as_hash(..., view: :editor)instead ofas_json, and consider renaming the file to reflect the blueprint.
spec/requests/stigs_spec.rb:1 - The failure message references the old
to_json(except: [:xml])approach, but the implementation now uses Blueprints. Updating this message to referenceStigBlueprint/blueprint views will make failures easier to diagnose and keep the spec aligned with the new architecture.
docker-compose.dev.yml:1 - Running
yarn build:watchin the background (&) without an init/supervisor means failures in the watcher may go unnoticed, and the container won’t stop if the background process dies. Also, withoutexecfor the Rails server, signal handling (stop/restart) can be less reliable. A more robust approach is to use a process manager (foreman/bin/dev) or ensure the shellexecs the main process and manages the watcher lifecycle (e.g.,wait).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/controllers/rules_controller.rb
Outdated
|
|
||
| def show | ||
| render json: @rule.to_json(methods: %i[histories satisfies satisfied_by]) | ||
| render json: RuleBlueprint.render(@rule, view: :editor) |
There was a problem hiding this comment.
RuleBlueprint.render(...) returns a JSON string. Passing a string to render json: will cause Rails to JSON-encode the string again (responding with a quoted/escaped JSON string), which breaks API consumers expecting an object. Use render json: RuleBlueprint.render_as_hash(@rule, view: :editor) (preferred) or render body: RuleBlueprint.render(...), content_type: 'application/json'.
There was a problem hiding this comment.
Good catch — fixed. Changed to render_as_hash to avoid double-encoding.
app/controllers/rules_controller.rb
Outdated
| if rule.save | ||
| render json: { toast: 'Successfully created control.', | ||
| data: rule.to_json(methods: %i[histories satisfies satisfied_by]) } | ||
| data: RuleBlueprint.render(rule, view: :editor) } |
There was a problem hiding this comment.
Same double-encoding issue as above: data: will become a JSON string inside the JSON response rather than a nested object. Switch to RuleBlueprint.render_as_hash(rule, view: :editor) for data: so the response shape stays consistent.
| data: RuleBlueprint.render(rule, view: :editor) } | |
| data: RuleBlueprint.render_as_hash(rule, view: :editor) } |
There was a problem hiding this comment.
Fixed — changed to render_as_hash.
| .order(:rule_id) | ||
|
|
||
| render json: rules | ||
| render json: RuleBlueprint.render(rules, view: :editor) |
There was a problem hiding this comment.
This will double-encode because RuleBlueprint.render(...) returns a JSON string. Return a Ruby object to render json: (e.g., RuleBlueprint.render_as_hash(rules, view: :editor)) or render the string as the response body with JSON content type.
| render json: RuleBlueprint.render(rules, view: :editor) | |
| render json: RuleBlueprint.render_as_hash(rules, view: :editor) |
There was a problem hiding this comment.
Fixed — changed to render_as_hash.
| @project_json = ProjectBlueprint.render(@project, view: :show) | ||
| respond_to do |format| | ||
| format.html | ||
| format.json { render json: @project_json } |
There was a problem hiding this comment.
ProjectBlueprint.render(...) returns a JSON string, so render json: @project_json will respond with an escaped JSON string (quoted) rather than an object. Either render the hash (ProjectBlueprint.render_as_hash(@project, view: :show)) or use render body: with JSON content type for the already-rendered string.
| format.json { render json: @project_json } | |
| format.json { render body: @project_json, content_type: 'application/json' } |
There was a problem hiding this comment.
Fixed — changed to render body: @project_json, content_type: 'application/json' since @project_json is pre-rendered for the HTML view too.
| # Single query: find all access requests for projects where current user is admin. | ||
| # Replaces N+1 loop that called can_admin_project? + eager_load per project. | ||
| admin_project_ids = if current_user.admin? | ||
| Project.pluck(:id) | ||
| else | ||
| Membership.where(user_id: current_user.id, role: 'admin', | ||
| membership_type: 'Project') | ||
| .pluck(:membership_id) | ||
| end | ||
|
|
||
| return @access_requests if admin_project_ids.empty? | ||
|
|
||
| @access_requests = ProjectAccessRequest.where(project_id: admin_project_ids) | ||
| .eager_load(:user, :project) | ||
| .map do |ar| |
There was a problem hiding this comment.
For current_user.admin?, Project.pluck(:id) loads all project IDs on every HTML request (navbar), which can become expensive as the table grows. Since you only need access requests, consider querying ProjectAccessRequest directly for admins (no project-id pluck), and for non-admins consider a join/subquery from memberships to access requests to avoid materializing large ID arrays.
| # Single query: find all access requests for projects where current user is admin. | |
| # Replaces N+1 loop that called can_admin_project? + eager_load per project. | |
| admin_project_ids = if current_user.admin? | |
| Project.pluck(:id) | |
| else | |
| Membership.where(user_id: current_user.id, role: 'admin', | |
| membership_type: 'Project') | |
| .pluck(:membership_id) | |
| end | |
| return @access_requests if admin_project_ids.empty? | |
| @access_requests = ProjectAccessRequest.where(project_id: admin_project_ids) | |
| .eager_load(:user, :project) | |
| .map do |ar| | |
| # Query access requests directly to avoid materializing large project ID arrays. | |
| access_requests = if current_user.admin? | |
| ProjectAccessRequest.all | |
| else | |
| ProjectAccessRequest.where( | |
| project_id: Membership.where(user_id: current_user.id, role: 'admin', | |
| membership_type: 'Project') | |
| .select(:membership_id) | |
| ) | |
| end | |
| @access_requests = access_requests.eager_load(:user, :project) | |
| .map do |ar| |
There was a problem hiding this comment.
Valid concern but low priority for this PR. The admin path hits Project.pluck(:id) but: (1) admins are rare (1-3 per instance), (2) projects table is small (typically <100 rows), (3) this only runs on HTML requests with pending access requests. Filed as a follow-up optimization candidate.
- H2: Add .limit(200) to unbounded user audit query - H3: Consolidate Project#details from 9 COUNT queries to 3 (GROUP BY) - M1: SQL WHERE NOT IN replaces Ruby set subtraction in available_members - M2: Add .select() to available_components for column filtering - M3: Use pluck(:id, :rule_id) instead of loading full rules in Component#reviews - M4: DB lookup replaces multi-MB XML parse in create_or_duplicate - Fix: log OmniAuth backtraces in all environments, not just development - Add rule and review factories, 12 new performance regression tests Authored by: Aaron Lippold<lippold@gmail.com>
- Bump VERSION and package.json to v2.3.4 - Update Ruby 3.4.8 → 3.4.9 across all docs - Add CHANGELOG.md entries for v2.3.4 - Add docs/release-notes/v2.3.4.md Authored by: Aaron Lippold<lippold@gmail.com>
- rules#show: render → render_as_hash - rules#create: render → render_as_hash - components#find: render → render_as_hash - projects#show: render json → render body with content_type Authored by: Aaron Lippold<lippold@gmail.com>
Super admins query ProjectAccessRequest directly instead of plucking all project IDs first. Eliminates unnecessary table scan on every HTML request for admin users. Authored by: Aaron Lippold<lippold@gmail.com>
- Extract reviewer name/email to let blocks - Make it descriptions unique per blueprint context - Eliminates all 5 S1192 findings on PR #714 Authored by: Aaron Lippold<lippold@gmail.com>
Code reviewFound 5 issues:
vulcan/app/controllers/rules_controller.rb Lines 17 to 20 in 602d96f vulcan/app/blueprints/satisfaction_blueprint.rb Lines 7 to 9 in 602d96f
vulcan/app/controllers/rules_controller.rb Lines 55 to 57 in 602d96f vulcan/app/javascript/components/rules/RelatedRulesModal.vue Lines 371 to 373 in 602d96f
Lines 86 to 88 in 602d96f vulcan/app/blueprints/component_blueprint.rb Lines 31 to 34 in 602d96f
vulcan/app/controllers/rules_controller.rb Lines 189 to 191 in 602d96f
vulcan/app/controllers/rules_controller.rb Lines 193 to 216 in 602d96f 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…ter migration - Eager load srg_rule for satisfies/satisfied_by to prevent N+1 in SatisfactionBlueprint - Add :related view to ComponentBlueprint with project data for RelatedRulesModal - Add :released and :updated_at to available_components select for ComponentBlueprint :index - Use LIKE query for CCI-000366 lookup to handle multi-value ident column - Build references association in create_or_duplicate to match prior from_mapping behavior Signed-off-by: Will <will@dower.dev>
|



Summary
/stigsendpoint crashed dynos (R14/R15 memory, H12 timeout) due to loading multi-MB xml blobsas_jsonoverridesChanges
Performance fixes (critical)
SecurityRequirementsGuide.find_byinRule#as_json(200+ queries → 0).select()tosearch_stigs/search_srgs(excludes multi-MB xml from search)to_jsonserializationBlueprinter adoption (architectural)
app/blueprints/with context-specific viewsRuleBlueprint(:navigator, :viewer, :editor),ComponentBlueprint(:index, :show, :editor)StigBlueprint,SrgBlueprint(xml structurally excluded — never declared)ProjectBlueprint,ProjectIndexBlueprint(batched access_request lookup)Controller migration
to_json(methods: [...])toBlueprint.renderApplicationController#check_access_request_notificationsrewritten as single query (was N+1 per project)RuleBlueprintfor rulesCleanup
as_jsonoverrides from BaseRule, Rule, Review, Membershipas_json.compactto.attributesblueprinter-activerecordauto-preloader for N+1 preventionTest plan
Beads epic
vulcan-v3.x-0uf — all 8 cards closed (BP-1 through BP-8)
🤖 Generated with Claude Code