fix(database connection) Database connection export fix #36958
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #483bc7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
bot
added
the
data:connect
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
superset/databases/schemas.py
Outdated
| external_url = fields.String(allow_none=True) | ||
| ssh_tunnel = fields.Nested(DatabaseSSHTunnel, allow_none=True) | ||
| configuration_method = fields.Enum( | ||
| ConfigurationMethod, by_value=True, required=False, allow_none=True |
There was a problem hiding this comment.
Suggestion: Logic bug: the new Enum field has no load_default, so when configuration_method is missing during import it will deserialize to None instead of the default enum value expected elsewhere; this can cause downstream logic that compares to enum members to behave incorrectly or miss the intended default behavior. Add a load_default to ensure a sensible default enum is set during deserialization. [logic error]
Severity Level: Minor
| ConfigurationMethod, by_value=True, required=False, allow_none=True | |
| ConfigurationMethod, | |
| by_value=True, | |
| required=False, | |
| allow_none=True, | |
| load_default=ConfigurationMethod.SQLALCHEMY_FORM, |
Why it matters? ⭐
Adding a load_default here is a harmless, sensible improvement: other parts of the file (DatabaseParametersSchemaMixin) already set a load_default for the same concept, so making the ImportV1 schema consistent avoids subtle downstream logic relying on an enum member vs None when the field is absent in older exports.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/databases/schemas.py
**Line:** 892:892
**Comment:**
*Logic Error: Logic bug: the new Enum field has no load_default, so when `configuration_method` is missing during import it will deserialize to None instead of the default enum value expected elsewhere; this can cause downstream logic that compares to enum members to behave incorrectly or miss the intended default behavior. Add a load_default to ensure a sensible default enum is set during deserialization.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
superset/models/core.py
Outdated
| "encrypted_extra", | ||
| "impersonate_user", | ||
| "ssh_tunnel", | ||
| "configuration_method", |
There was a problem hiding this comment.
Suggestion: Allowing configuration_method in extra_import_fields lets import payloads set arbitrary strings into the column, which can create invalid enum-like values in the DB; remove it from extra_import_fields so imports can't inject unvalidated values (or ensure import pipeline validates it explicitly). [security]
Severity Level: Critical 🚨
| "configuration_method", |
Why it matters? ⭐
Allowing unvalidated values to be imported into a field that is effectively an enum can lead to invalid DB state and surprising behavior. Removing it from extra_import_fields or otherwise enforcing validation in the import pipeline prevents uncontrolled values from being written. This suggestion addresses a real data-integrity/security concern visible in the diff.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/core.py
**Line:** 209:209
**Comment:**
*Security: Allowing `configuration_method` in `extra_import_fields` lets import payloads set arbitrary strings into the column, which can create invalid enum-like values in the DB; remove it from `extra_import_fields` so imports can't inject unvalidated values (or ensure import pipeline validates it explicitly).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #f97ffcActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
isaac-jaynes-imperva
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36958 +/- ##
===========================================
+ Coverage 0 68.19% +68.19%
===========================================
Files 0 639 +639
Lines 0 47702 +47702
Branches 0 5210 +5210
===========================================
+ Hits 0 32528 +32528
- Misses 0 13893 +13893
- Partials 0 1281 +1281
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rusackas
requested review from
Antonio-RiveroMartnez,
Vitor-Avila and
Copilot
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue with database connection exports by including the configuration_method field in the exported YAML. This field is essential for properly restoring the database connection configuration UI, particularly for Google BigQuery connections where users need to access the Service Account input field.
Changes:
- Added
configuration_methodto the database export fields list in the Database model - Added
configuration_methodfield definition to the ImportV1DatabaseSchema with appropriate defaults - Updated integration and unit tests to reflect the new field in exported database configurations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset/models/core.py | Added configuration_method to the export_fields list in the Database model |
| superset/databases/schemas.py | Added configuration_method field to ImportV1DatabaseSchema with enum type, allowing None, and defaulting to SQLALCHEMY_FORM |
| tests/integration_tests/databases/commands_tests.py | Updated the key order test to include configuration_method in the expected export field order |
| tests/unit_tests/datasets/commands/export_test.py | Updated the expected database export output to include configuration_method: sqlalchemy_form |
1 participant
Exporting a database connection will now include the 'configuration_method' in the exported yaml. This is used when importing a database connection (primarily Google BigQuery) to show the correct page when editing it, allowing the user to be shown the 'Service Account' input where they can add their credentials info.
Fixes #36956
After footage:
Screen.Recording.2026-01-07.at.3.03.46.PM.mov
Before footage:
Screen.Recording.2026-01-07.at.3.09.52.PM.mov