Skip to content

The fix skips nodes where type === "column_ref", since those aren't table references - #42821

Merged
getvictor merged 2 commits into
mainfrom
victor/40117-fix-sql-table-alias
Apr 1, 2026
Merged

The fix skips nodes where type === "column_ref", since those aren't table references#42821
getvictor merged 2 commits into
mainfrom
victor/40117-fix-sql-table-alias

Conversation

@getvictor

@getvictor getvictor commented Apr 1, 2026

Copy link
Copy Markdown
Member

Related issue: Resolves #40117

Fix:
image

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.

Testing

  • QA'd all new/changed functionality manually

Summary by CodeRabbit

  • Bug Fixes
    • Fixed incorrect platform detection for SQL queries that use table aliases (e.g., FROM mounts m).

@getvictor

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes platform compatibility detection for SQL policies by preventing table-alias column references (AST column_ref nodes) from being misidentified as table names during table extraction.

Changes:

  • Update SQL AST traversal to skip nodes where type === "column_ref" when collecting node.table.
  • Add a changelog entry describing the user-visible fix.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.

File Description
frontend/utilities/sql_tools.ts Avoids treating column_ref.table (often a table alias like m/d) as an actual table reference when parsing tables for compatibility checks.
changes/40117-fix-sql-table-alias-platform-detection Documents the bugfix for release notes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread frontend/utilities/sql_tools.ts
@coderabbitai

coderabbitai Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This change fixes SQL query analysis to correctly identify table platforms when table aliases are used. Previously, the parseSqlTables function in frontend/utilities/sql_tools.ts was treating all node.table entries as table references during FROM and JOIN clause parsing. The fix adds a condition to skip node.table entries when the AST node represents a column reference, preventing column identifiers from being misclassified as tables. This resolves issues where queries using aliases like FROM mounts m were incorrectly marked as having no compatible platforms.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete, missing critical sections from the template such as security validation, testing details, and database/configuration checks. Complete the PR description by filling in all applicable checklist items, including security validation, automated testing verification, and any relevant migration or configuration checks.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change—skipping column_ref nodes in SQL table extraction—and is specific and clear.
Linked Issues check ✅ Passed The code changes directly address the root cause: filtering out column_ref AST nodes prevents table aliases from being misclassified, fixing platform compatibility detection for #40117.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to fix SQL table alias platform detection—the changelog entry and SQL parsing logic update are directly aligned with #40117 objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch victor/40117-fix-sql-table-alias

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.

@codecov

codecov Bot commented Apr 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.81%. Comparing base (f8d73cf) to head (c2fbc13).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #42821   +/-   ##
=======================================
  Coverage   66.81%   66.81%           
=======================================
  Files        2541     2541           
  Lines      203971   203971           
  Branches     9164     9164           
=======================================
  Hits       136282   136282           
  Misses      55344    55344           
  Partials    12345    12345           
Flag Coverage Δ
frontend 54.80% <100.00%> (ø)

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
getvictor marked this pull request as ready for review April 1, 2026 18:20
@getvictor
getvictor requested a review from a team as a code owner April 1, 2026 18:20

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@getvictor

Copy link
Copy Markdown
Member Author

@sgress454 Can you review since you're familiar with the SQL parser.

@sgress454

Copy link
Copy Markdown
Contributor

@sgress454 Can you review since you're familiar with the SQL parser.

sure but what's with this PR title 😆

@sgress454 sgress454 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

glad this didn't require an actual parser fix! tested 👍

@getvictor
getvictor merged commit 945b7e6 into main Apr 1, 2026
19 checks passed
@getvictor
getvictor deleted the victor/40117-fix-sql-table-alias branch April 1, 2026 19:22
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.

Linux disk encryption policy should be "Compatible with" Linux

3 participants