bito-code-review[bot] commented on code in PR #34875:
URL: https://github.com/apache/superset/pull/34875#discussion_r2326061845
##########
tests/unit_tests/migrations/shared/catalogs_test.py:
##########
@@ -510,13 +698,63 @@ def test_upgrade_catalog_perms_simplified_migration(
)
mocker.patch("superset.migrations.shared.catalogs.op", session)
+ # Mock the db_engine_spec for BigQuery with catalog support
+ mock_db_engine_spec = mocker.MagicMock()
+ mock_db_engine_spec.supports_catalog = True
+ mock_db_engine_spec.get_default_catalog.return_value = "my-test-project"
+
+ mocker.patch.object(
+ Database,
+ "db_engine_spec",
+ new_callable=mocker.PropertyMock,
+ return_value=mock_db_engine_spec,
+ )
+ mocker.patch.object(
+ Database,
+ "get_default_catalog",
+ return_value="my-test-project",
+ )
+
database = Database(
- database_name="my_db",
+ id=1,
sqlalchemy_uri="bigquery://my-test-project",
)
+ database.database_name = "my_db"
+ session.add(database)
+ session.commit()
+
+ # Create initial permissions for testing
+ db_perm = ViewMenu(name="[my_db].(id:1)")
+ table_perm = ViewMenu(name="[my_db].[my_table](id:1)")
+ schema_perm = ViewMenu(name="[my_db].[public]")
+
+ database_access = Permission(name="database_access")
+ datasource_access = Permission(name="datasource_access")
+ schema_access = Permission(name="schema_access")
+
+ session.add_all(
+ [
+ db_perm,
+ table_perm,
+ schema_perm,
+ database_access,
+ datasource_access,
+ schema_access,
+ ]
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing required catalog_access permission</b></div>
<div id="fix">
The test setup is missing the `catalog_access` permission that will be
created by the `upgrade_catalog_perms()` function. This permission needs to be
added to the test setup and included in the `session.add_all()` call to prevent
test failures when verifying migration results.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
database_access = Permission(name="database_access")
datasource_access = Permission(name="datasource_access")
schema_access = Permission(name="schema_access")
catalog_access = Permission(name="catalog_access")
session.add_all(
[
db_perm,
table_perm,
schema_perm,
database_access,
datasource_access,
schema_access,
catalog_access,
]
)
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34875#issuecomment-3259748393>#273f77</a></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
##########
tests/unit_tests/migrations/shared/catalogs_test.py:
##########
@@ -494,13 +674,21 @@
This should only update existing permissions + create a new permission
for the default catalog.
"""
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from superset.models.sql_lab import Query, SavedQuery, TableSchema,
TabState
+ from superset.migrations.shared.catalogs import (
+ Database,
+ Query,
+ SavedQuery,
+ Slice,
+ SqlaTable,
+ TableSchema,
+ TabState,
+ )
engine = session.get_bind()
Database.metadata.create_all(engine)
+ Permission.metadata.create_all(engine)
+ PermissionView.metadata.create_all(engine)
+ ViewMenu.metadata.create_all(engine)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>SQLAlchemy metadata registry inconsistency</b></div>
<div id="fix">
Metadata inconsistency issue: Permission, PermissionView, and ViewMenu
models use a different SQLAlchemy Base metadata registry than the Database
model. This will cause table creation failures and foreign key constraint
issues. Replace separate metadata.create_all() calls with `from
superset.migrations.shared.security_converge import Base as SecurityBase;
SecurityBase.metadata.create_all(engine)` to use the correct metadata registry.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
from superset.migrations.shared.security_converge import Base as SecurityBase
SecurityBase.metadata.create_all(engine)
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34875#issuecomment-3259748393>#273f77</a></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
##########
tests/unit_tests/migrations/shared/catalogs_test.py:
##########
@@ -245,32 +321,88 @@
catalog browsing on the database (permissions are always synced on a DB
update, see
`UpdateDatabaseCommand`).
"""
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import Database
- from superset.models.slice import Slice
- from superset.models.sql_lab import Query, SavedQuery, TableSchema,
TabState
+ from superset.migrations.shared.catalogs import (
+ Database,
+ Query,
+ SavedQuery,
+ Slice,
+ SqlaTable,
+ TableSchema,
+ TabState,
+ )
engine = session.get_bind()
Database.metadata.create_all(engine)
+ Permission.metadata.create_all(engine)
+ PermissionView.metadata.create_all(engine)
+ ViewMenu.metadata.create_all(engine)
mocker.patch("superset.migrations.shared.catalogs.op")
db = mocker.patch("superset.migrations.shared.catalogs.db")
db.Session.return_value = session
+ # Mock the db_engine_spec to support catalogs but fail on
get_all_schema_names
+ mock_db_engine_spec = mocker.MagicMock()
+ mock_db_engine_spec.supports_catalog = True
+ mock_db_engine_spec.get_default_catalog.return_value = "db"
+
+ mocker.patch.object(
+ Database,
+ "db_engine_spec",
+ new_callable=mocker.PropertyMock,
+ return_value=mock_db_engine_spec,
+ )
mocker.patch.object(
Database,
- "get_all_schema_names",
- side_effect=Exception("Failed to connect to the database"),
+ "get_default_catalog",
+ return_value="db",
)
+
+ def get_all_schema_names_mock(catalog=None):
+ raise Exception("Failed to connect to the database")
+
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unused mock function breaks test logic</b></div>
<div id="fix">
The `get_all_schema_names_mock` function is defined but never patched to the
database object. This breaks the test's intended behavior of simulating
database connection failures. Add `mocker.patch.object(database,
"get_all_schema_names", get_all_schema_names_mock)` after the database object
is created to properly mock the failure scenario.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
def get_all_schema_names_mock(catalog=None):
raise Exception("Failed to connect to the database")
mocker.patch.object(
database,
"get_all_schema_names",
side_effect=get_all_schema_names_mock,
)
```
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/34875#issuecomment-3259748393>#273f77</a></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]
