Copilot commented on code in PR #37557:
URL: https://github.com/apache/superset/pull/37557#discussion_r2791645685
##########
tests/unit_tests/databases/commands/importers/v1/import_test.py:
##########
@@ -120,6 +120,39 @@ def test_import_database_sqlite_invalid(
current_app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True
+def test_import_database_sqlite_allowed_with_ignore_permissions(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Test that SQLite imports succeed when ignore_permissions=True.
+
+ System imports (like examples) use URIs from server config, not user input,
+ so they should bypass the PREVENT_UNSAFE_DB_CONNECTIONS check. This is the
+ key fix from PR #37577 that allows example loading to work in CI/showtime
+ environments where PREVENT_UNSAFE_DB_CONNECTIONS is enabled.
+ """
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import
database_config_sqlite
+
+ current_app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True
+
mocker.patch("superset.commands.database.importers.v1.utils.add_permissions")
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config_sqlite)
+ # With ignore_permissions=True, the security check should be skipped
+ database = import_database(config, ignore_permissions=True)
+
+ assert database.database_name == "imported_database"
+ assert "sqlite" in database.sqlalchemy_uri
+
+ # Cleanup
+ db.session.delete(database)
+ db.session.flush()
Review Comment:
This test mutates the global Flask app config
(`current_app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True`) but doesn’t
restore the previous value at the end. Since the unit test app context is
shared across tests, this can leak into unrelated tests and cause
order-dependent failures. Please capture the original value and restore it in a
`finally` block (or use the existing config/fixture helpers) so the test is
isolated.
```suggestion
original_prevent_unsafe =
current_app.config.get("PREVENT_UNSAFE_DB_CONNECTIONS")
try:
current_app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = True
mocker.patch("superset.commands.database.importers.v1.utils.add_permissions")
engine = db.session.get_bind()
Database.metadata.create_all(engine) # pylint: disable=no-member
config = copy.deepcopy(database_config_sqlite)
# With ignore_permissions=True, the security check should be skipped
database = import_database(config, ignore_permissions=True)
assert database.database_name == "imported_database"
assert "sqlite" in database.sqlalchemy_uri
# Cleanup
db.session.delete(database)
db.session.flush()
finally:
if "PREVENT_UNSAFE_DB_CONNECTIONS" in current_app.config:
# Restore previous value or remove key if it didn't exist
if original_prevent_unsafe is None:
current_app.config.pop("PREVENT_UNSAFE_DB_CONNECTIONS", None)
else:
current_app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = (
original_prevent_unsafe
)
```
--
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]
