Skip to content

Remove nano_view_queue from unscoped MDM commands list query#45674

Open
raju249 wants to merge 1 commit into
fleetdm:mainfrom
raju249:44509-nano-view-queue-perf
Open

Remove nano_view_queue from unscoped MDM commands list query#45674
raju249 wants to merge 1 commit into
fleetdm:mainfrom
raju249:44509-nano-view-queue-perf

Conversation

@raju249
Copy link
Copy Markdown
Contributor

@raju249 raju249 commented May 16, 2026

Drops nano_view_queue from the Apple branch of the unscoped commands list and joins the underlying nano_* tables directly. The view's definition bakes in ORDER BY q.priority DESC, q.created_at, which MySQL re-materializes on every query — the outer LIMIT can't push past it, so each unscoped list call pays the full sort cost over the post-join row set regardless of page size.

This is the same join shape the host-scoped path already uses (see listMDMCommandsByHostIdentifier), so I followed that pattern. Column output is identical to what the view was producing, which is why no test updates were needed — all the existing TestListMDMCommands* cases pass without modification.

Scope of this PR is just the hot caller. The view itself isn't touched. The issue notes other call sites (vpp.go, apple_mdm.go) still go through it, and that dropping the ORDER BY from the view's definition would be the durable fix. Both feel like separate PRs — the audit work for other call sites is non-trivial, and modifying the view risks silently breaking any consumer that relied on its implicit ordering. Happy to follow up on either.

Refs #44509.

Checklist for submitter

  • Input is properly validated (no new user input paths; same parameterized query shape)
  • Tests: existing TestListMDMCommands* coverage exercises this path and passes unchanged. No new tests added — see rationale above.
  • Changes file: not applicable, internal query refactor with no user-visible behavior change.

Summary by CodeRabbit

Bug Fixes

  • Updated Mobile Device Management command status reporting to ensure accurate status and timestamp information.

Review Change Stack

The view's baked-in ORDER BY forces MySQL to materialize the full three-way join and sort it before any outer LIMIT applies, defeating pagination. The Apple branch in getMDMCommandsSubqueries now joins the underlying nano_* tables directly, mirroring the host-scoped path that already bypasses the view. Column shape and filter semantics are unchanged. Refs fleetdm#44509.
@raju249 raju249 requested a review from a team as a code owner May 16, 2026 14:10
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d8b5d620-45f3-40bd-93ef-3ca2d0b933b9

📥 Commits

Reviewing files that changed from the base of the PR and between ad5bb8a and 1d677ed.

📒 Files selected for processing (1)
  • server/datastore/mysql/mdm.go

Walkthrough

This PR refactors the Apple MDM commands query in getMDMCommandsSubqueries to bypass the nano_view_queue view and instead directly join underlying tables: nano_enrollment_queue, nano_commands, hosts, and nano_command_results. Field projections were updated to source host UUID from enrollment queue, status from command results with a pending fallback, updated timestamps from command results, and command metadata from the commands table. The filtering condition remains unchanged.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description explains the problem, solution, and rationale well. However, it does not follow the provided template structure (missing explicit checklist boxes, related issue formatting, and other template sections). Reformat the description to match the repository template: add the 'Related issue' section, use checkbox syntax for checklist items, and clearly mark which items apply.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing nano_view_queue from the unscoped MDM commands list query, which is the primary focus of this refactoring PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@raju249
Copy link
Copy Markdown
Contributor Author

raju249 commented May 16, 2026

Hello @getvictor - Please take a look. 🙇

Thanks! 🙏 🙇

@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.75%. Comparing base (ad5bb8a) to head (1d677ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #45674      +/-   ##
==========================================
- Coverage   66.75%   66.75%   -0.01%     
==========================================
  Files        2745     2745              
  Lines      219361   219359       -2     
  Branches    10840    10840              
==========================================
- Hits       146438   146424      -14     
- Misses      59695    59704       +9     
- Partials    13228    13231       +3     
Flag Coverage Δ
backend 68.57% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@getvictor
Copy link
Copy Markdown
Member

assigning to @JordanMontgomery for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants