You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mathiasi (via GitHub)" <gi...@apache.org> on 2026/06/12 09:26:56 UTC
[PR] fix(cli): add --username option to import-directory command [superset]
mathiasi opened a new pull request, #40994:
URL: https://github.com/apache/superset/pull/40994
### SUMMARY
The \import-directory\ CLI command imports assets (charts, datasets, dashboards, databases) without setting an active user (\g.user\). As a result, all imported assets are created with an empty \owners\ list — making them uneditable in the UI. Attempting to save an imported chart fails because the ownership check finds no owner to match against the logged-in user.
All sibling import commands already handle this correctly:
| Command | Has \--username\ | How \g.user\ is set |
|---|---|---|
| \import-dashboards\ | Yes (required) | \g.user = security_manager.find_user(username=username)\ |
| \import-datasources\ | Yes (optional, default \dmin\) | \override_user(security_manager.find_user(username=username))\ |
| \legacy-import-dashboards\ | Yes (optional) | \g.user = security_manager.find_user(username=username)\ |
| \import-directory\ | **No** | **Nothing — g.user is None** |
This PR brings \import-directory\ into alignment by adding an optional \--username\/\-u\ option (default: \dmin\) using the \override_user\ context manager, identical to \import-datasources\. No new imports are required as both \override_user\ and \security_manager\ are already imported in the module.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — CLI change only.
### TESTING INSTRUCTIONS
1. Run \superset import-directory /path/to/assets\ — assets import as before, owned by \dmin\ (default)
2. Run \superset import-directory --username someuser /path/to/assets\ — assets are owned by \someuser\
3. Log in to the UI and verify that imported charts and dashboards are editable and saveable
4. Run \pytest tests/integration_tests/cli_tests.py::test_import_directory tests/integration_tests/cli_tests.py::test_failing_import_directory\
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-4691510132
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-4689601867
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40994:
URL: https://github.com/apache/superset/pull/40994#discussion_r3433913966
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ 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)
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+ security_manager_mock.find_user.return_value = fake_user
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ captured_user.append(g.user)
+
+ load_configs_mock.side_effect = capture_g_user
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ security_manager_mock.find_user.assert_called_once_with(username="admin")
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
+ assert str(call_kwargs.kwargs["root"]) == "/assets"
+ assert call_kwargs.kwargs["overwrite"] is False
+ assert call_kwargs.kwargs["force_data"] is False
+ assert captured_user[0] is fake_user
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+def test_import_directory_unknown_user(
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory fails fast with a clear error when the
+ specified user does not exist.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ security_manager_mock.find_user.return_value = None
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "nonexistent"),
+ )
+
+ assert response.exit_code != 0
+ assert "nonexistent" in response.output
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+@mock.patch(
+ "superset.examples.utils.load_configs_from_directory",
+ side_effect=Exception("import failed"),
+)
+def test_failing_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+ caplog: Any,
+) -> None:
+ """
+ Test that a failure in import-directory is handled gracefully.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
Review Comment:
**Suggestion:** The patched `security_manager` is invalidated by reloading `superset.cli.importexport`, so this failure-path test is not actually controlling user resolution through `security_manager_mock`. This can make the test flaky across environments and weakens what the test is verifying. Reload before patching (or remove reload) to keep the mock active. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Failure-path test hits real security manager unexpectedly.
- ⚠️ Test determinism depends on external security configuration.
- ⚠️ Mocked user resolution is not actually exercised by test.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest tests/integration_tests/cli_tests.py::test_failing_import_directory` to
execute the failure-path test defined at `tests/integration_tests/cli_tests.py:452-479`.
2. When the test file is imported, the decorator
`@mock.patch("superset.cli.importexport.security_manager")` at line `447` replaces
`superset.cli.importexport.security_manager` with a `MagicMock`, and passes that mock as
`security_manager_mock` into `test_failing_import_directory(...)`.
3. Inside the test, lines `463-465` re-import and reload the module: `import
superset.cli.importexport` then `importlib.reload(superset.cli.importexport)`. This
re-executes `superset/cli/importexport.py`, including `from superset import
security_manager` at line `29`, which rebinds `superset.cli.importexport.security_manager`
to the real security manager object and discards the patch, while `security_manager_mock`
remains an un-used `MagicMock`.
4. The click runner at `tests/integration_tests/cli_tests.py:473-477` invokes
`superset.cli.importexport.import_directory`, whose implementation at
`superset/cli/importexport.py:58-76` now resolves the user with the real
`security_manager.find_user` rather than `security_manager_mock`. Although the failure
condition is driven by the patched `load_configs_from_directory` (decorator at lines
`448-451`), the user resolution path is no longer controlled by the test, making the
failure-path verification partially dependent on external security configuration instead
of a fully mocked environment.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6508575f84ae4cdc92fa612358c1d9f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6508575f84ae4cdc92fa612358c1d9f3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 447:465
**Comment:**
*Logic Error: The patched `security_manager` is invalidated by reloading `superset.cli.importexport`, so this failure-path test is not actually controlling user resolution through `security_manager_mock`. This can make the test flaky across environments and weakens what the test is verifying. Reload before patching (or remove reload) to keep the mock active.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=30e28810b4b2cf25538db9c2b7c276222c1a3a0dc81e09f640784d2f406bbd6c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=30e28810b4b2cf25538db9c2b7c276222c1a3a0dc81e09f640784d2f406bbd6c&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ 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)
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
Review Comment:
**Suggestion:** Reloading the module after applying `@mock.patch("superset.cli.importexport.security_manager")` overwrites the patched attribute with the real object, so this test no longer uses `security_manager_mock`. That makes `assert_called_once_with` unreliable and can fail by calling real user lookup instead of the mock. Remove the reload (or patch after reloading) so the mocked dependency is actually exercised. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ `test_import_directory` exercises real security manager dependency.
- ⚠️ Mock-based assertion on `find_user` may silently misrepresent behavior.
- ⚠️ Test reliability depends on real users in the test database.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest tests/integration_tests/cli_tests.py::test_import_directory` to execute the
test defined at `tests/integration_tests/cli_tests.py:372-415`.
2. During test module import, the decorator
`@mock.patch("superset.cli.importexport.security_manager")` at
`tests/integration_tests/cli_tests.py:370` replaces
`superset.cli.importexport.security_manager` with a `MagicMock`, and passes that mock as
`security_manager_mock` into `test_import_directory(...)`.
3. Inside the test body, lines `383-385` re-import and then reload the module: `import
superset.cli.importexport` followed by `importlib.reload(superset.cli.importexport)`. This
re-executes `superset/cli/importexport.py`, including `from superset import
security_manager` at `superset/cli/importexport.py:29`, which rebinds
`superset.cli.importexport.security_manager` back to the real security manager object,
discarding the earlier mock.
4. The click runner at `tests/integration_tests/cli_tests.py:401-405` invokes
`superset.cli.importexport.import_directory`, which now uses the real `security_manager`
in `import_directory` (lines `superset/cli/importexport.py:58-76`) to call
`security_manager.find_user`. As a result, `security_manager_mock.find_user` at
`tests/integration_tests/cli_tests.py:388-389` is never called, making the assertion
`security_manager_mock.find_user.assert_called_once_with(username="admin")` at line `408`
unreliable and causing the test to exercise the real dependency instead of the intended
mock.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a873fe07b9d4dda9ce4b2bf6c8e899b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1a873fe07b9d4dda9ce4b2bf6c8e899b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 370:385
**Comment:**
*Logic Error: Reloading the module after applying `@mock.patch("superset.cli.importexport.security_manager")` overwrites the patched attribute with the real object, so this test no longer uses `security_manager_mock`. That makes `assert_called_once_with` unreliable and can fail by calling real user lookup instead of the mock. Remove the reload (or patch after reloading) so the mocked dependency is actually exercised.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=817d18bb0b66797cc77c92b854e6cb7de897191979d5a8f36caed3b180284d90&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=817d18bb0b66797cc77c92b854e6cb7de897191979d5a8f36caed3b180284d90&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ 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)
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+ security_manager_mock.find_user.return_value = fake_user
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ captured_user.append(g.user)
+
+ load_configs_mock.side_effect = capture_g_user
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ security_manager_mock.find_user.assert_called_once_with(username="admin")
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
+ assert str(call_kwargs.kwargs["root"]) == "/assets"
+ assert call_kwargs.kwargs["overwrite"] is False
+ assert call_kwargs.kwargs["force_data"] is False
+ assert captured_user[0] is fake_user
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+def test_import_directory_unknown_user(
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory fails fast with a clear error when the
+ specified user does not exist.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
Review Comment:
**Suggestion:** This test has the same patching-order bug: `security_manager` is patched and then immediately replaced by `importlib.reload`, so the command runs against the real security manager instead of the mock. That makes the test non-isolated and environment-dependent for unknown-user behavior. Keep a single import without reload, or reload first and then patch. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unknown-user path test depends on real security manager.
- ⚠️ Behavior may vary with actual users in database.
- ⚠️ Test no longer isolates failure mode of `import_directory`.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `pytest tests/integration_tests/cli_tests.py::test_import_directory_unknown_user`
to execute the test defined at `tests/integration_tests/cli_tests.py:418-445`.
2. When the test module is imported, the decorator
`@mock.patch("superset.cli.importexport.security_manager")` at line `417` patches
`superset.cli.importexport.security_manager` with a `MagicMock`, and provides it as the
`security_manager_mock` argument to the test.
3. At the start of the test body, lines `428-430` execute `import
superset.cli.importexport` followed by `importlib.reload(superset.cli.importexport)`,
which re-runs `superset/cli/importexport.py`. This re-imports the real `security_manager`
via `from superset import security_manager` at `superset/cli/importexport.py:29`,
overwriting the patched `superset.cli.importexport.security_manager` with the real
implementation.
4. The CLI runner at `tests/integration_tests/cli_tests.py:437-441` invokes
`superset.cli.importexport.import_directory`, whose implementation at
`superset/cli/importexport.py:58-76` now uses the real
`security_manager.find_user(username="nonexistent")` instead of `security_manager_mock`.
The test's expectations (`response.exit_code != 0` and `"nonexistent" in response.output`
at lines `443-444`) are satisfied or not based on real environment state (whether such a
user exists and how click formats the error), making the test behavior
environment-dependent and bypassing the intended mock isolation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31753eb213bf4f98a4c9999b10602f8a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=31753eb213bf4f98a4c9999b10602f8a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 417:430
**Comment:**
*Logic Error: This test has the same patching-order bug: `security_manager` is patched and then immediately replaced by `importlib.reload`, so the command runs against the real security manager instead of the mock. That makes the test non-isolated and environment-dependent for unknown-user behavior. Keep a single import without reload, or reload first and then patch.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=dc090c87eb67039eecea45edffeeca3215d40618236ca0cd4f6a5846135b7cd3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=dc090c87eb67039eecea45edffeeca3215d40618236ca0cd4f6a5846135b7cd3&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-4739085498
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 99d0c8b2f987eac1cb7dabc3890b454fc7339a8c |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a339b4de95125000861d012 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40994--superset-docs-preview.netlify.app](https://deploy-preview-40994--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8y6yE9GdYOOYc5EQqZMBkl099y9xXshcQwpwwzQwMr2dHv15p_OhxQ5t-iZXI5c2vsiPm2gGpHSEcuCtQ5W62ntkSYjlRFCjINEIRV8Pw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40994--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-4741228569
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40994:
URL: https://github.com/apache/superset/pull/40994
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-4733510258
<!-- translation-regression-bot -->
## ⚠️ Translation Regression Detected
A source change in this PR renamed or reworded strings, invalidating existing translations (they are now `#, fuzzy`) in `de`. Please resolve the affected `.po` files before merging.
_Note: intentionally **deleting** a translatable string is not a regression and is not flagged here — only translations invalidated by a renamed/reworded source string are._
| Language | Fuzzy before | Fuzzy after | New |
|----------|-------------:|------------:|----:|
| `de` | 0 | 997 | +997 |
### How to fix
**1. Install dependencies** (if not already set up):
```bash
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext
```
**2. Re-extract strings and sync `.po` files:**
```bash
./scripts/translations/babel_update.sh
```
This rewrites `superset/translations/messages.pot` from the current source files and merges the changes into every `.po` file. Strings whose `msgid` changed will be marked `#, fuzzy`.
**3. Resolve the fuzzy entries** in the affected language files (`de`):
```bash
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po
```
For each fuzzy entry, either rewrite the `msgstr` to match the new string and remove the `#, fuzzy` line, or clear the `msgstr` to `""` if you cannot provide a translation.
**4. Commit your changes to the `.po` files.**
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40994:
URL: https://github.com/apache/superset/pull/40994#discussion_r3402185484
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +364,56 @@ 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)
+
+
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(load_configs_mock, app_context, fs):
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
+ assert str(call_kwargs.kwargs["root"]) == "/assets"
+ assert call_kwargs.kwargs["overwrite"] is False
+ assert call_kwargs.kwargs["force_data"] is False
+
+
+@mock.patch(
+ "superset.examples.utils.load_configs_from_directory",
+ side_effect=Exception(),
+)
+def test_failing_import_directory(load_configs_mock, app_context, fs, caplog):
Review Comment:
**Suggestion:** Add explicit type hints for each function argument and the return type in this new test function to satisfy the typing rule for newly introduced Python code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced Python test function without any parameter annotations or return type annotation. That directly violates the rule that new Python code should be fully typed.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=19c4ecbbeca943078870f814872b1162&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=19c4ecbbeca943078870f814872b1162&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 401:401
**Comment:**
*Custom Rule: Add explicit type hints for each function argument and the return type in this new test function to satisfy the typing rule for newly introduced Python code.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=4483e6a5b4ae24289038138e30f85eca2309d91b70777934505fc5115e89f943&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=4483e6a5b4ae24289038138e30f85eca2309d91b70777934505fc5115e89f943&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +364,56 @@ 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)
+
+
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(load_configs_mock, app_context, fs):
Review Comment:
**Suggestion:** Add explicit type hints for all parameters and the return value in this newly added test function so it complies with the requirement that new Python code is fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test function, and its parameters and return type are unannotated. The rule requires new Python code to be fully typed, so this is a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6947d1ebc43c49a9a97c7f81ac6ef4b5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6947d1ebc43c49a9a97c7f81ac6ef4b5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 370:370
**Comment:**
*Custom Rule: Add explicit type hints for all parameters and the return value in this newly added test function so it complies with the requirement that new Python code is fully typed.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=6c7fcb7acfbaa4f4ad8b6d4c74f804d8a0470383994186ec67fb432c982d2d84&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=6c7fcb7acfbaa4f4ad8b6d4c74f804d8a0470383994186ec67fb432c982d2d84&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-4733515773
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40994?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.59%. Comparing base ([`814b72c`](https://app.codecov.io/gh/apache/superset/commit/814b72c6f91b2f917c79bc020c4279cc9531107b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`cafa3a1`](https://app.codecov.io/gh/apache/superset/commit/cafa3a1583d7d872ffa53ea029e095920b3e0abf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 114 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40994?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/cli/importexport.py](https://app.codecov.io/gh/apache/superset/pull/40994?src=pr&el=tree&filepath=superset%2Fcli%2Fimportexport.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2xpL2ltcG9ydGV4cG9ydC5weQ==) | 50.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40994?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (814b72c) and HEAD (cafa3a1). Click for more details.
>
> <details><summary>HEAD has 135 uploads less than BASE</summary>
>
>| Flag | BASE (814b72c) | HEAD (cafa3a1) |
>|------|------|------|
>|python|118|1|
>|presto|19|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40994 +/- ##
==========================================
- Coverage 64.29% 55.59% -8.71%
==========================================
Files 2659 2659
Lines 144266 144268 +2
Branches 33247 33247
==========================================
- Hits 92754 80199 -12555
- Misses 49881 63346 +13465
+ Partials 1631 723 -908
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.03% <50.00%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.03% <50.00%> (-18.69%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40994/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/40994?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40994:
URL: https://github.com/apache/superset/pull/40994#issuecomment-5007650254
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40994:
URL: https://github.com/apache/superset/pull/40994#discussion_r3402221201
##########
superset/cli/importexport.py:
##########
@@ -48,16 +48,26 @@
is_flag=True,
help="Force load data even if table already exists",
)
-def import_directory(directory: str, overwrite: bool, force: bool) -> None:
+@click.option(
+ "--username",
+ "-u",
+ required=False,
+ default="admin",
+ help="Specify the user name to assign imported assets to",
+)
+def import_directory(
+ directory: str, overwrite: bool, force: bool, username: Optional[str] = "admin"
+) -> None:
"""Imports configs from a given directory"""
# pylint: disable=import-outside-toplevel
from superset.examples.utils import load_configs_from_directory
- load_configs_from_directory(
- root=Path(directory),
- overwrite=overwrite,
- force_data=force,
- )
+ with override_user(user=security_manager.find_user(username=username)):
Review Comment:
**Suggestion:** `security_manager.find_user(username=username)` can return `None` for an unknown username, and the command currently proceeds anyway. In that case `override_user` sets `g.user` to `None`, so imports will again create ownerless assets (the exact issue this PR is trying to fix). Fail fast with a clear CLI error when the user does not exist before calling `override_user`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Imported dashboards/charts become uneditable due to missing owners.
- ⚠️ CLI hides username typos; admins see confusing ownership results.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/cli/importexport.py:36-60`, the `import_directory` Click command is
defined with a `--username/-u` option and calls
`override_user(user=security_manager.find_user(username=username))` at line 65.
2. `security_manager.find_user` delegates to FAB's `find_user`, which returns `None` when
the username does not exist (see `superset/security/manager.py:5-15` for the similar
`get_user_by_username` behavior and FAB's documented `find_user` semantics).
3. When a user runs `superset import-directory -u nonexisting_user /assets`,
`security_manager.find_user(username="nonexisting_user")` returns `None`, so
`override_user` is entered with `user=None`. In `superset/utils/core.py:1507-1515`,
`override_user` assigns `g.user = user`, so `g.user` becomes `None` for the duration of
the context.
4. Inside this `override_user` context, `load_configs_from_directory` is invoked
(`superset/cli/importexport.py:63-70`), which builds and runs an `ImportExamplesCommand`
(`superset/examples/utils.py:243-259`,
`superset/commands/importers/v1/examples.py:14-33`). Ownership for imported assets is
computed from `g.user`; because `g.user` is `None`, imported charts/dashboards/datasets
end up ownerless, reproducing the "no owners, uneditable assets" behavior the PR
description is trying to eliminate — but now triggered whenever an invalid username is
passed.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b6772eeef864ae68f9c0e3e2e9165eb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5b6772eeef864ae68f9c0e3e2e9165eb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/cli/importexport.py
**Line:** 65:65
**Comment:**
*Logic Error: `security_manager.find_user(username=username)` can return `None` for an unknown username, and the command currently proceeds anyway. In that case `override_user` sets `g.user` to `None`, so imports will again create ownerless assets (the exact issue this PR is trying to fix). Fail fast with a clear CLI error when the user does not exist before calling `override_user`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=82bbdf2e17bf3b361de35de378ef566e8da2067bdfe37b69b28ff82ed36d87dd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=82bbdf2e17bf3b361de35de378ef566e8da2067bdfe37b69b28ff82ed36d87dd&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40994:
URL: https://github.com/apache/superset/pull/40994#discussion_r3433881724
##########
tests/integration_tests/cli_tests.py:
##########
@@ -364,3 +365,115 @@ 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)
+
+
+@mock.patch("superset.cli.importexport.security_manager")
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ security_manager_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+ security_manager_mock.find_user.return_value = fake_user
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ captured_user.append(g.user)
Review Comment:
**Suggestion:** Add an inline docstring to the newly introduced helper function so it complies with the rule requiring documentation for new functions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The helper `capture_g_user` is a newly introduced Python function in the changed code and it does not include a docstring. This matches the custom rule requiring new functions and classes to be documented inline.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b4ed5de7c31c4a6a8d29ad398d5d28ee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b4ed5de7c31c4a6a8d29ad398d5d28ee&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 393:394
**Comment:**
*Custom Rule: Add an inline docstring to the newly introduced helper function so it complies with the rule requiring documentation for new functions.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=7f119f0c28c7e57c4bd602dcf360e20f8b4ffa134471aa91252153ff5938ecc5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=7f119f0c28c7e57c4bd602dcf360e20f8b4ffa134471aa91252153ff5938ecc5&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(cli): add --username option to import-directory command [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40994:
URL: https://github.com/apache/superset/pull/40994#discussion_r3600399076
##########
superset/cli/importexport.py:
##########
@@ -48,16 +48,38 @@
is_flag=True,
help="Force load data even if table already exists",
)
-def import_directory(directory: str, overwrite: bool, force: bool) -> None:
+@click.option(
+ "--username",
+ "-u",
+ required=False,
+ default="admin",
+ help="Specify the user name to assign imported assets to",
+)
+def import_directory(
+ directory: str, overwrite: bool, force: bool, username: Optional[str]
+) -> None:
"""Imports configs from a given directory"""
# pylint: disable=import-outside-toplevel
from superset.examples.utils import load_configs_from_directory
- load_configs_from_directory(
- root=Path(directory),
- overwrite=overwrite,
- force_data=force,
- )
+ user = security_manager.find_user(username=username)
Review Comment:
**Suggestion:** Add an explicit type annotation for the `user` local variable to satisfy the type-hint requirement for relevant newly introduced variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new local variable `user` is introduced without a type annotation, and it is a relevant variable that can be annotated. This matches the Python type-hint rule for modified code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f211d3b755c94a048511800200b56f31&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f211d3b755c94a048511800200b56f31&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/cli/importexport.py
**Line:** 65:65
**Comment:**
*Custom Rule: Add an explicit type annotation for the `user` local variable to satisfy the type-hint requirement for relevant newly introduced variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=f63e665b0c0f7f27cedece0ddefb93cbbb2008f1bcf7a12a17dfce9ff04ee088&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=f63e665b0c0f7f27cedece0ddefb93cbbb2008f1bcf7a12a17dfce9ff04ee088&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -466,3 +467,118 @@ def test_re_encrypt_secrets_engine_option_invalid_raises_usage(
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
+
+
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ """Capture g.user at call time to verify override_user is active."""
+ captured_user.append(g.user)
+
+ load_configs_mock.side_effect = capture_g_user
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.importexport, "security_manager"
+ ) as security_manager_mock:
+ security_manager_mock.find_user.return_value = fake_user
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ security_manager_mock.find_user.assert_called_once_with(username="admin")
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
Review Comment:
**Suggestion:** Add a concrete type annotation for `call_kwargs` so this newly added variable is explicitly typed. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The newly added `call_kwargs` variable is unannotated even though it can be typed. This matches the stated type-hint requirement for new Python code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f115af0985a4f29b0eae7ea6bf125c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3f115af0985a4f29b0eae7ea6bf125c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 514:514
**Comment:**
*Custom Rule: Add a concrete type annotation for `call_kwargs` so this newly added variable is explicitly typed.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=eb370e1f139237bb99b5ff608a75ee0d257e0c7011ab4d8bf3e037b19db0adeb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=eb370e1f139237bb99b5ff608a75ee0d257e0c7011ab4d8bf3e037b19db0adeb&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -466,3 +467,118 @@ def test_re_encrypt_secrets_engine_option_invalid_raises_usage(
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
+
+
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
Review Comment:
**Suggestion:** Add an explicit type annotation for `fake_user` to satisfy the rule requiring type-hinted relevant variables in new code. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The new variable `fake_user` is introduced without a type hint, and the rule explicitly requires type hints for new or modified Python variables that can be annotated. This is a real violation.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07a387fa80a2465b8cf4ea07362208d3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=07a387fa80a2465b8cf4ea07362208d3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 487:487
**Comment:**
*Custom Rule: Add an explicit type annotation for `fake_user` to satisfy the rule requiring type-hinted relevant variables in new code.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=1882de083986cf13e7aebadb4ae43779d0158d0480c9c769c66b35fcbebf130c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=1882de083986cf13e7aebadb4ae43779d0158d0480c9c769c66b35fcbebf130c&reaction=dislike'>👎</a>
##########
tests/integration_tests/cli_tests.py:
##########
@@ -466,3 +467,118 @@ def test_re_encrypt_secrets_engine_option_invalid_raises_usage(
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
+
+
+@mock.patch("superset.examples.utils.load_configs_from_directory")
+def test_import_directory(
+ load_configs_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory calls load_configs_from_directory with the
+ correct arguments and assigns assets to the specified user.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
+ fake_user.username = "admin"
+
+ captured_user: list[Any] = []
+
+ def capture_g_user(**kwargs: Any) -> None:
+ """Capture g.user at call time to verify override_user is active."""
+ captured_user.append(g.user)
+
+ load_configs_mock.side_effect = capture_g_user
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.importexport, "security_manager"
+ ) as security_manager_mock:
+ security_manager_mock.find_user.return_value = fake_user
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "admin"),
+ )
+
+ assert response.exit_code == 0
+ security_manager_mock.find_user.assert_called_once_with(username="admin")
+ load_configs_mock.assert_called_once()
+ call_kwargs = load_configs_mock.call_args
+ assert str(call_kwargs.kwargs["root"]) == "/assets"
+ assert call_kwargs.kwargs["overwrite"] is False
+ assert call_kwargs.kwargs["force_data"] is False
+ assert captured_user[0] is fake_user
+
+
+def test_import_directory_unknown_user(
+ app_context: Any,
+ fs: Any,
+) -> None:
+ """
+ Test that import-directory fails fast with a clear error when the
+ specified user does not exist.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fs.create_dir("/assets")
+ fs.create_file("/assets/metadata.yaml", contents="version: 1.0.0\n")
+
+ runner = current_app.test_cli_runner()
+ with mock.patch.object(
+ superset.cli.importexport, "security_manager"
+ ) as security_manager_mock:
+ security_manager_mock.find_user.return_value = None
+ response = runner.invoke(
+ superset.cli.importexport.import_directory,
+ ("/assets", "-u", "nonexistent"),
+ )
+
+ assert response.exit_code != 0
+ assert "nonexistent" in response.output
+
+
+@mock.patch(
+ "superset.examples.utils.load_configs_from_directory",
+ side_effect=Exception("import failed"),
+)
+def test_failing_import_directory(
+ load_configs_mock: mock.MagicMock,
+ app_context: Any,
+ fs: Any,
+ caplog: Any,
+) -> None:
+ """
+ Test that a failure in import-directory is handled gracefully.
+ """
+ # pylint: disable=reimported, redefined-outer-name
+ import superset.cli.importexport # noqa: F811
+
+ importlib.reload(superset.cli.importexport)
+
+ fake_user = mock.MagicMock()
Review Comment:
**Suggestion:** Annotate `fake_user` with an explicit type in this test as well, matching the mandatory type-hint policy for new variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is the same issue as the earlier `fake_user` assignment: a newly introduced variable lacks a type hint, which violates the Python type-hint rule.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=58275f9f4fec48f2850112a8d174a83e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=58275f9f4fec48f2850112a8d174a83e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/cli_tests.py
**Line:** 569:569
**Comment:**
*Custom Rule: Annotate `fake_user` with an explicit type in this test as well, matching the mandatory type-hint policy for new variables.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=b066816aa34b432650fe4869bf2c493ca186cfb96a3717ff246aa8f0349ab249&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40994&comment_hash=b066816aa34b432650fe4869bf2c493ca186cfb96a3717ff246aa8f0349ab249&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org