bito-code-review[bot] commented on code in PR #40654:
URL: https://github.com/apache/superset/pull/40654#discussion_r3365740810
##########
superset/cli/update.py:
##########
@@ -110,17 +110,37 @@ def update_api_docs() -> None:
help="An optional previous secret key, if PREVIOUS_SECRET_KEY "
"is not set on the config",
)
-def re_encrypt_secrets(previous_secret_key: Optional[str] = None) -> None:
[email protected](
+ "--engine",
+ "-e",
+ "target_engine_name",
+ required=False,
+ type=click.Choice(sorted(ENCRYPTION_ENGINES), case_sensitive=False),
+ help="Re-encrypt all app-encrypted fields with this encryption engine "
+ "(e.g. 'aes-gcm' for authenticated encryption). The SECRET_KEY is "
+ "unchanged. Take a metadata-DB backup first, then set "
+ "SQLALCHEMY_ENCRYPTED_FIELD_ENGINE to the same value and restart.",
+)
+def re_encrypt_secrets(
+ previous_secret_key: Optional[str] = None,
+ target_engine_name: Optional[str] = None,
+) -> None:
previous_secret_key = previous_secret_key or current_app.config.get(
"PREVIOUS_SECRET_KEY"
)
- if previous_secret_key is None:
+ target_engine = (
+ ENCRYPTION_ENGINES[target_engine_name] if target_engine_name else None
+ )
+ if previous_secret_key is None and target_engine is None:
click.secho(
- "No previous secret key provided; nothing to re-encrypt.",
+ "No previous secret key or target engine provided; nothing to
re-encrypt.",
fg="yellow",
)
return
- secrets_migrator = SecretsMigrator(previous_secret_key=previous_secret_key)
+ secrets_migrator = SecretsMigrator(
+ previous_secret_key=previous_secret_key,
+ target_engine=target_engine,
+ )
try:
stats = secrets_migrator.run()
except Exception as exc: # pylint: disable=broad-except
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing CLI integration test</b></div>
<div id="fix">
The new `--engine` CLI option lacks integration test coverage. The existing
test at line 328 only verifies the no-op path when neither key nor engine is
provided; it mocks `SecretsMigrator.run` and passes no arguments. There are no
tests that invoke the CLI with `--engine aes-gcm` or validate that the engine
is correctly passed through to `SecretsMigrator`. This gap means a future
regression (e.g., wrong argument name or type mismatch) would go undetected.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/tests/integration_tests/cli_tests.py
+++ b/tests/integration_tests/cli_tests.py
@@ -1,5 +1,7 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
+from unittest import mock
+
import pytest
from sqlalchemy.engine import make_url
@@ -360,3 +362,47 @@ def
test_re_encrypt_secrets_failure_exits_nonzero(app_context):
# The failure path must be handled by the CLI, not leaked as an
# uncaught exception.
assert response.exception is None or isinstance(response.exception,
SystemExit)
+
+
+def test_re_encrypt_secrets_engine_option_invokes_migrator(app_context):
+ """
+ When --engine is provided, the CLI must resolve the engine name to the
+ correct engine class and pass it to SecretsMigrator as target_engine.
+ """
+ from sqlalchemy_utils.types.encrypted.encrypted_type import
AesGcmEngine
+
+ current_app.config.pop("PREVIOUS_SECRET_KEY", None)
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update.SecretsMigrator,
+ "run",
+ return_value=superset.utils.encrypt.ReEncryptStats(),
+ ) as run_mock:
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "aes-gcm"],
+ )
+
+ assert response.exit_code == 0
+ call_kwargs = run_mock.call_args.kwargs
+ assert call_kwargs.get("target_engine") is AesGcmEngine
+ assert call_kwargs.get("previous_secret_key") is None
+
+
+def test_re_encrypt_secrets_engine_option_case_insensitive(app_context):
+ """
+ The --engine option must be case-insensitive per click.Choice(...
+ case_sensitive=False).
+ """
+ from sqlalchemy_utils.types.encrypted.encrypted_type import
AesGcmEngine
+
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.update.SecretsMigrator,
+ "run",
+ return_value=superset.utils.encrypt.ReEncryptStats(),
+ ) as run_mock:
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "AES-GCM"],
+ )
+
+ assert response.exit_code == 0
+ assert run_mock.call_args.kwargs.get("target_engine") is AesGcmEngine
+
+
+def
test_re_encrypt_secrets_engine_option_invalid_raises_usage(app_context):
+ """
+ An unrecognized engine name must produce a click usage error, not a
+ traceback or silent failure.
+ """
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.update.re_encrypt_secrets,
+ ["--engine", "nonexistent-engine"],
+ )
+
+ assert response.exit_code != 0
+ assert "Invalid value" in response.output or "Usage:" in
response.output
+ assert "aes" in response.output or "aes-gcm" in response.output
```
</div>
</details>
</div>
<small><i>Code Review Run #64dc65</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
