Skip to content

fix(db): apply engine-spec enforced connection args after encrypted_extra - #45933

Open
sha174n wants to merge 7 commits into
apache:masterfrom
sha174n:fix-enforced-connect-args-ordering
Open

sha174n wants to merge 7 commits into
apache:masterfrom
sha174n:fix-enforced-connect-args-ordering

Conversation

@sha174n

@sha174n sha174n commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

SUMMARY

The engine spec's enforced connection arguments (enforce_uri_query_params, e.g. for MySQL drivers) are applied in adjust_engine_params, while encrypted_extra is merged into the engine kwargs later in Database._get_sqla_engine. This re-applies the enforced arguments after that merge so the engine spec stays the single source of truth for them. Only engine specs that declare enforce_uri_query_params are affected; all other connect_args, including those from encrypted_extra, are preserved.

TESTING INSTRUCTIONS

pytest tests/unit_tests/models/core_test.py -k enforced_connect_args_outlast_encrypted_extra

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration
  • Introduces new feature or API
  • Removes existing feature or API

@netlify

netlify Bot commented Nov 1, 2026

Copy link
Copy Markdown

✅ Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit 53b1b26
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6ae7803900896a7da004f894
😎 Deploy Preview https://deploy-preview-45933--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@codeant-ai-for-open-source

codeant-ai-for-open-source Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

CodeAnt  CodeAnt PR Risk

🟢  Low Risk

  • The PR appears safe to merge; no actionable issues were identified in the review.

Assessed commit: 215e86c82c0d

@codecov

codecov Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.35%. Comparing base (a6f30aa) to head (cf554fd).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #45933   +/-   ##
=======================================
  Coverage   65.35%   65.35%           
=======================================
  Files        3083     3083           
  Lines      211333   211335    +2     
  Branches    49823    49820    -3     
=======================================
+ Hits       138110   138112    +2     
  Misses      70041    70041           
  Partials     3182     3182           
Flag Coverage Δ
hive 34.05% <37.50%> (+<0.01%) ⬆️
mysql 51.81% <100.00%> (+<0.01%) ⬆️
postgres 51.84% <100.00%> (+<0.01%) ⬆️
presto 35.69% <37.50%> (+<0.01%) ⬆️
python 52.13% <100.00%> (+<0.01%) ⬆️
sqlite 51.52% <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 Harness.
📢 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.

@rebenitez1802 rebenitez1802 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.

LGTM, thanks! Verified the new test fails without the fix (assert 1 == 0) and passes with it, and that unrelated connect_args from encrypted_extra (e.g. ssl) are preserved. Since encrypted_extra is only writable with can_write on Database (Admin), this is hardening rather than a vulnerability fix. A few optional nits inline, none blocking.

Comment thread superset/models/core.py Resolved
Comment thread superset/models/core.py Resolved
Comment thread superset/models/core.py Resolved
Comment thread tests/unit_tests/models/core_test.py Resolved
…xtra

The engine spec's enforced connection arguments (enforce_uri_query_params)
are applied in adjust_engine_params, while encrypted_extra is merged into
the engine kwargs later in Database._get_sqla_engine. Re-apply the enforced
arguments after that merge so the engine spec stays the single source of
truth for them. Other connect_args, including those from encrypted_extra,
are preserved.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@sha174n
sha174n force-pushed the fix-enforced-connect-args-ordering branch from 53b1b26 to f8b96f2 Compare November 10, 2026 15:55
sha174n and others added 2 commits November 10, 2026 15:56
Rebuild connect_args into a fresh dict so a non-dict value supplied via
encrypted_extra (null, a list of pairs) is coerced instead of raising a
raw AttributeError, and clarify that specs overriding adjust_engine_params
without super() still get the enforced arguments applied last. Add coverage
for the non-dict case and for per-key merge preserving an unrelated entry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Nov 10, 2026
sha174n and others added 4 commits November 10, 2026 22:09
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bare Mock user's is_guest_user attribute is truthy, so the RLS tool
tests fail whenever EMBEDDED_SUPERSET is left enabled by an earlier test
in the same worker.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants