feat: Labels for encrypted fields#38075
Conversation
dosubot
bot
added
the
data:connect
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
superset/db_engine_specs/bigquery.py
Outdated
| encrypted_extra_sensitive_field_labels = { | ||
| "$.credentials_info.private_key": "Service Account Private Key", | ||
| } |
There was a problem hiding this comment.
I think it would be better to just make encrypted_extra_sensitive_fields: set[str] | dict[str, str] for now, and where we consume it we do:
fields = (
{field: field for field in encrypted_extra_sensitive_fields}
if isinstance(encrypted_extra_sensitive_fields, set)
else encrypted_extra_sensitive_fields
)Then in 7.0 we make encrypted_extra_sensitive_fields: dict[str, str] only.
Otherwise we need to keep encrypted_extra_sensitive_fields and encrypted_extra_sensitive_field_labels in sync, ensuring they have the same keys.
We could also be fancy and try to autogenerate a label when encrypted_extra_sensitive_fields is a set, like:
def generate_field_label(key: str) -> str:
return " ".join(part.title() for part in key[2:].replace("_", " ").split("."))But it might not be worth the trouble.
There was a problem hiding this comment.
Yeah, I actually thought at this at first, but thought that perhaps changing an existing property from the DB Engine Spec could be bad. Let me work on this
|
Your suggestion to change superset/db_engine_specs/base.py |
Code Review Agent Run #7163c6Actionable 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 |
It would generate the label |
Sequence DiagramShows how encrypted_extra fields are masked/unmasked using configurable JSONPath paths (now supporting human-readable labels). The BaseEngineSpec centralizes path extraction and calls redaction/reveal utilities during edit and update flows. sequenceDiagram
participant Client
participant Backend
participant BaseEngineSpec
participant RedactionUtil
Client->>Backend: Submit database edit (encrypted_extra)
Backend->>BaseEngineSpec: mask_encrypted_extra(encrypted_extra)
BaseEngineSpec->>RedactionUtil: redact_sensitive(config, paths=encrypted_extra_sensitive_field_paths())
RedactionUtil-->>BaseEngineSpec: masked_config
BaseEngineSpec-->>Backend: masked_encrypted_extra (returned to client)
Backend->>BaseEngineSpec: unmask_encrypted_extra(old_encrypted_extra, new_encrypted_extra)
BaseEngineSpec->>RedactionUtil: reveal_sensitive(old_config, new_config, paths=encrypted_extra_sensitive_field_paths())
RedactionUtil-->>BaseEngineSpec: merged_config
BaseEngineSpec-->>Backend: new_encrypted_extra (with reused sensitive values)
Generated by CodeAnt AI |
Vitor-Avila
force-pushed
the
feat/label-encrypted-fields
branch
from
f384f67 to
f0ccf8b
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #824288
Actionable Suggestions - 1
-
tests/unit_tests/db_engine_specs/test_base.py - 1
- Parametrize decorator expects tuple not string · Line 363-364
Additional Suggestions - 1
-
superset/db_engine_specs/base.py - 1
-
Incomplete docstring in new method · Line 588-594The docstring for the new `encrypted_extra_sensitive_field_paths` method contains placeholder text ('Description') for the parameter and return value descriptions. According to repository guidelines requiring complete docstrings for new functions, these should be replaced with meaningful explanations.
Code suggestion
@@ -589,3 +589,3 @@ - :param cls: Description - - :return: Description + :param cls: The database engine spec class. + + :return: A set of field paths that should be masked.
-
Review Details
-
Files reviewed - 9 · Commit Range:
7a78893..f0ccf8b- superset/db_engine_specs/base.py
- superset/db_engine_specs/bigquery.py
- superset/db_engine_specs/gsheets.py
- superset/db_engine_specs/mysql.py
- superset/db_engine_specs/postgres.py
- superset/db_engine_specs/redshift.py
- superset/db_engine_specs/snowflake.py
- superset/db_engine_specs/ydb.py
- tests/unit_tests/db_engine_specs/test_base.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| @pytest.mark.parametrize( | ||
| "masked_encrypted_extra,expected_result", |
There was a problem hiding this comment.
The @pytest.mark.parametrize decorator at line 364 uses a string for the first argument instead of a tuple. Change "masked_encrypted_extra,expected_result" to a tuple ("masked_encrypted_extra", "expected_result") to comply with pytest requirements.
Code suggestion
Check the AI-generated fix before applying
| @pytest.mark.parametrize( | |
| "masked_encrypted_extra,expected_result", | |
| @pytest.mark.parametrize( | |
| ("masked_encrypted_extra", "expected_result"), |
Code Review Run #824288
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2 participants
SUMMARY
This PR adds labels to
encrypted_extra_sensitive_fields. This is needed to implement support for export/import includingmasked_encrypted_extra(#38077 and #38078).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
TESTING INSTRUCTIONS
N/A
ADDITIONAL INFORMATION