bito-code-review[bot] commented on code in PR #44683:
URL: https://github.com/apache/superset/pull/44683#discussion_r4113535020
##########
superset/commands/database/tables.py:
##########
@@ -177,3 +175,40 @@ def validate(self) -> None:
self._model = cast(Database, DatabaseDAO.find_by_id(self._db_id))
if not self._model:
raise DatabaseNotFoundError()
+
+ self._catalog_name = self._catalog_name or
self._model.get_default_catalog()
+ if not self._model.db_engine_spec.supports_schemas:
+ self._schema_name = None
+
+ if self._schema_name:
+ self._validate_schema(self._schema_name)
+
+ def _validate_schema(self, schema_name: str) -> None:
+ """
+ Accept only a schema that the schemas endpoint would list for this
user.
+
+ The schema has to exist in the database and be accessible to the user,
+ otherwise ``DatabaseSchemaNotFoundError`` is raised before any table or
+ view lookup is run.
+ """
+ try:
+ schemas = self._model.get_all_schema_names(
+ catalog=self._catalog_name,
+ cache=self._model.schema_cache_enabled,
+ cache_timeout=self._model.schema_cache_timeout or None,
+ force=self._force,
+ )
Review Comment:
<!-- Bito Reply -->
The explanation provided clarifies that the schema lookup is necessary to
distinguish between an empty schema and a missing one, which the existing table
lookups cannot do. Since the call uses the same cached mechanism and settings
as the schemas endpoint, it maintains consistency with existing behavior. This
approach is acceptable for ensuring schema existence validation before
proceeding with table lookups.
##########
superset/commands/database/tables.py:
##########
@@ -177,3 +175,40 @@ def validate(self) -> None:
self._model = cast(Database, DatabaseDAO.find_by_id(self._db_id))
if not self._model:
raise DatabaseNotFoundError()
+
+ self._catalog_name = self._catalog_name or
self._model.get_default_catalog()
+ if not self._model.db_engine_spec.supports_schemas:
+ self._schema_name = None
+
+ if self._schema_name:
+ self._validate_schema(self._schema_name)
+
+ def _validate_schema(self, schema_name: str) -> None:
+ """
+ Accept only a schema that the schemas endpoint would list for this
user.
+
+ The schema has to exist in the database and be accessible to the user,
+ otherwise ``DatabaseSchemaNotFoundError`` is raised before any table or
+ view lookup is run.
+ """
+ try:
+ schemas = self._model.get_all_schema_names(
+ catalog=self._catalog_name,
+ cache=self._model.schema_cache_enabled,
+ cache_timeout=self._model.schema_cache_timeout or None,
+ force=self._force,
+ )
+ accessible = schema_name in schemas and bool(
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to use case-insensitive matching is based on the
concern that raw identifiers from the database might not match the unnormalized
input from the API, potentially causing valid schemas to be incorrectly
rejected. However, your assessment that exact matching is required for
consistency with other parts of the system, such as the schemas endpoint and
existing access permissions, is correct. Given that some database engines treat
case-sensitive quoted identifiers as distinct, maintaining strict matching is
the appropriate approach to avoid unintended behavior.
##########
tests/unit_tests/commands/databases/tables_test.py:
##########
@@ -291,3 +309,130 @@ def test_tables_without_catalog(
cache=database_without_catalog.table_cache_enabled,
cache_timeout=database_without_catalog.table_cache_timeout,
)
+
+
+def test_tables_unknown_schema(
+ mocker: MockerFixture,
+ database_without_catalog: MagicMock,
+) -> None:
+ """
+ A schema that does not exist in the database is rejected before any
+ table or view lookup runs.
+ """
+ get_datasources_accessible_by_user = mocker.patch.object(
+ security_manager,
+ "get_datasources_accessible_by_user",
+ )
+
+ with pytest.raises(DatabaseSchemaNotFoundError):
+ TablesDatabaseCommand(1, None, "not_a_schema", False).run()
+
+ database_without_catalog.get_all_table_names_in_schema.assert_not_called()
+ database_without_catalog.get_all_view_names_in_schema.assert_not_called()
+
database_without_catalog.get_all_materialized_view_names_in_schema.assert_not_called()
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to wrap the line is based on the repository's
88-character line length limit, which is enforced by the project's
configuration. While pre-commit may not have flagged this specific line in your
local environment, CI pipelines often use stricter configurations that will
catch this violation. Applying the wrap is recommended to ensure consistency
with the project's style guidelines and to avoid potential CI failures.
**tests/unit_tests/commands/databases/tables_test.py**
```
database_without_catalog.get_all_table_names_in_schema.assert_not_called()
database_without_catalog.get_all_view_names_in_schema.assert_not_called()
database_without_catalog.get_all_materialized_view_names_in_schema.assert_not_called()
```
--
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]
