bito-code-review[bot] commented on code in PR #36958:
URL: https://github.com/apache/superset/pull/36958#discussion_r2710683582
##########
tests/unit_tests/databases/api_test.py:
##########
@@ -2274,3 +2274,148 @@ def test_schemas_with_oauth2(
}
]
}
+
+
+def test_export_includes_configuration_method(
+ mocker: MockerFixture, client: Any, full_api_access: None
+) -> None:
+ """
+ Test that exporting a database
+ includes the 'configuration_method' field in the YAML.
+ """
+ import zipfile
+
+ import prison
+
+ from superset.models.core import Database
+
+ # Create a database with a non-default configuration_method
+ db_obj = Database(
+ database_name="export_test_db",
+ sqlalchemy_uri="bigquery://gcp-project-id/",
+ configuration_method="dynamic_form",
+ uuid=UUID("12345678-1234-5678-1234-567812345678"),
+ )
+ db.session.add(db_obj)
+ db.session.commit()
+
+ rison_ids = prison.dumps([db_obj.id])
+ response = client.get(f"/api/v1/database/export/?q={rison_ids}")
+ assert response.status_code == 200
+
+ # Read the zip file from the response
+ buf = BytesIO(response.data)
+ with zipfile.ZipFile(buf) as zf:
+ # Find the database yaml file
+ db_yaml_path = None
+ for name in zf.namelist():
+ if (
+ name.endswith(".yaml")
+ and name.startswith("database_export_")
+ and "/databases/" in name
+ ):
+ db_yaml_path = name
+ break
+ assert db_yaml_path, "Database YAML not found in export zip"
+ with zf.open(db_yaml_path) as f:
+ db_yaml = yaml.safe_load(f.read())
+ # Assert configuration_method is present and correct
+ assert "configuration_method" in db_yaml
+ assert db_yaml["configuration_method"] == "dynamic_form"
+
+
+def test_import_includes_configuration_method(
+ session: Session, client: Any, full_api_access: None
+) -> None:
+ """
+ Test that importing a database YAML with configuration_method
+ sets the value on the imported DB connection.
+ """
+ from io import BytesIO
+ from unittest.mock import patch
+
+ import yaml
+
+ from superset import db
+ from superset.models.core import Database
+
+ Database.metadata.create_all(session.get_bind())
+
+ # Prepare the contents for the import zip
+ metadata = {
+ "version": "1.0.0",
+ "type": "Database",
+ "timestamp": "2025-12-08T18:06:31.356738+00:00",
+ }
+ db_yaml = {
+ "database_name": "Test_Import_Configuration_Method",
+ "sqlalchemy_uri": "bigquery://gcp-project-id/",
+ "cache_timeout": 0,
+ "expose_in_sqllab": True,
+ "allow_run_async": False,
+ "allow_ctas": False,
+ "allow_cvas": False,
+ "allow_dml": False,
+ "allow_csv_upload": False,
+ "extra": {"allows_virtual_table_explore": True},
+ "impersonate_user": False,
+ "uuid": "87654321-4321-8765-4321-876543218765",
+ "configuration_method": "dynamic_form",
+ "version": "1.0.0",
+ }
+ contents = {
+ "metadata.yaml": yaml.safe_dump(metadata),
+ "databases/test.yaml": yaml.safe_dump(db_yaml),
+ }
+
+ # Patch the import logic to use our in-memory contents and provide a mock
user context
+ from types import SimpleNamespace
+
+ from flask import g
+
+ mock_user = SimpleNamespace(
+ id=1, username="admin", is_active=True, is_authenticated=True
+ )
+ with (
+ patch("superset.databases.api.is_zipfile", return_value=True),
+ patch("superset.databases.api.ZipFile"),
+ patch("superset.databases.api.get_contents_from_bundle",
return_value=contents),
+ patch.dict(g._get_current_object().__dict__, {"user": mock_user}),
+ ):
+ form_data = {"formData": (BytesIO(b"test"), "test.zip")}
+ response = client.post(
+ "/api/v1/database/import/",
+ data=form_data,
+ content_type="multipart/form-data",
+ )
+ session.commit()
+ from superset import db
+
+ db.session.remove()
+ assert response.status_code == 200, response.data
+
+ from superset.models.core import Database as DBModel
+
+ db_obj = (
+ session.query(DBModel)
+ .filter_by(database_name="Test_Import_Configuration_Method")
+ .first()
+ )
+ assert db_obj is not None, "Database not found in SQLAlchemy session after
import"
+ assert hasattr(db_obj, "configuration_method"), (
+ "'configuration_method' not found on model"
+ )
+ assert db_obj.configuration_method == "dynamic_form", (
+ f"Expected configuration_method 'dynamic_form', got
{db_obj.configuration_method}"
+ )
+
+ get_resp = client.get(
+
"/api/v1/database/?q=(filters:!((col:database_name,opr:eq,value:'Test_Import_Configuration_Method')))"
+ )
+ assert get_resp.status_code == 200, get_resp.data
+ if result := get_resp.json["result"]:
+ db_obj_api = result[0]
+ assert "configuration_method" in db_obj_api, (
+ f"'configuration_method' not found in database list response:
{db_obj_api}"
+ )
+ assert db_obj_api["configuration_method"] == "dynamic_form"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test assertion weakened</b></div>
<div id="fix">
The conditional 'if result := get_resp.json["result"]' weakens the test by
skipping assertions when no database is returned in the API, unlike the
original code which asserted 'result' exists. This could allow the test to pass
incorrectly if the import succeeds but the API listing fails.
</div>
</div>
<small><i>Code Review Run #c584cd</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]
