akashchamp commented on code in PR #44583:
URL: https://github.com/apache/superset/pull/44583#discussion_r4098196923
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1174,6 +1174,73 @@ def test_quoted_name_prevents_double_quoting(mocker:
MockerFixture) -> None:
assert '"MY_DB"."MY_SCHEMA"."MY_TABLE"' in compiled
+def test_get_sqla_table_schema_not_qualified_when_engine_opts_out(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Engines that set ``quote_table_includes_schema = False`` (e.g. MongoDB,
whose
+ PyMongoSQL driver resolves ``schema.collection`` as a literal collection
name
+ instead of parsing it) must get an unqualified FROM-clause identifier from
+ ``get_sqla_table``, built through the engine spec's own ``quote_table``,
the
+ same way ``select_star`` builds it for SQL Lab's Data Preview. Regression
test
+ for datasets on such engines returning no rows once a schema is set.
+ """
+ from sqlalchemy import create_engine, select
+
+ engine = create_engine("sqlite://")
+
+ database = mocker.MagicMock()
+ database.db_engine_spec.supports_cross_catalog_queries = False
+ database.db_engine_spec.quote_table_includes_schema = False
+ database.db_engine_spec.quote_table = (
+ lambda table, dialect: dialect.identifier_preparer.quote(table.table)
+ )
+ database.get_dialect.return_value = engine.dialect
+
+ table = SqlaTable(
+ table_name="orders",
+ database=database,
+ schema="testdb",
+ )
+
+ sqla_table = table.get_sqla_table()
+ compiled = str(
+ select(sqla_table).compile(engine, compile_kwargs={"literal_binds":
True})
+ )
+
+ assert "FROM orders" in compiled
+ assert "testdb" not in compiled
Review Comment:
Applied as suggested — switched `quote_table` to `.side_effect` instead of
replacing the mock outright, and added `assert_called_once()`. Verified the
mutation case you described (collapsing the schema branch back to `return
table(self.table_name)`) now fails this test, where it silently passed before.
Full `models_test.py` + `test_mongodb.py` run: 134 passed. Pushed in e41ad9391.
##########
superset/db_engine_specs/mongodb.py:
##########
@@ -52,6 +52,14 @@ class MongoDBEngineSpec(BaseEngineSpec):
# the default ``authSource``.
supports_dynamic_schema = True
+ # `quote_table` below emits only the bare, quoted collection name --
PyMongoSQL
+ # resolves the whole FROM reference as a literal collection name, so a
+ # schema-qualified identifier would never match. `SqlaTable.get_sqla_table`
+ # (charts) and `select_star` (SQL Lab) both consult this flag to build the
same
+ # unqualified FROM clause and rely on `adjust_engine_params` to select the
+ # schema at the connection level instead.
Review Comment:
Good catch — updated the comment to say `select_star` reaches the
unqualified FROM clause through the `quote_table` override, not by consulting
this flag directly, matching what `base.py`/`models.py` actually do. Pushed in
e41ad9391.
##########
tests/unit_tests/db_engine_specs/test_mongodb.py:
##########
@@ -326,3 +326,40 @@ def test_get_sqla_engine_applies_selected_schema() -> None:
assert credentials.source == "dbone"
finally:
raw_connection.close()
+
+
+def test_get_sqla_table_does_not_qualify_collection() -> None:
+ """
+ Charts built on a MongoDB dataset with a schema hit the same PyMongoSQL
+ ``schema.collection`` resolution bug SQL Lab's Data Preview had before
#44141:
+ ``SqlaTable.get_sqla_table`` must build an unqualified FROM clause too, not
+ just ``select_star``. Regression test for #44576.
+ """
+ from sqlalchemy import create_engine, select
+
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.db_engine_specs.mongodb import MongoDBEngineSpec
+ from superset.models.core import Database
+
+ pytest.importorskip("pymongosql")
+
+ assert MongoDBEngineSpec.quote_table_includes_schema is False
+
+ database = Database(
+ database_name="mongo",
+ sqlalchemy_uri="mongodb://user:pass@host:27017/dbone?mode=superset",
+ )
+ dataset = SqlaTable(table_name="orders", database=database,
schema="testdb")
+
+ sqla_table = dataset.get_sqla_table()
+
+ # PyMongoSQL doesn't have a real SQLAlchemy dialect to compile against in
this
+ # unit test; a generic dialect is enough to verify the FROM-clause
identifier
+ # itself is unqualified, which is what PyMongoSQL parses.
+ engine = create_engine("sqlite://")
Review Comment:
Applied — now compiling against `database.sqlalchemy_uri`'s own dialect via
`create_engine(database.sqlalchemy_uri)` instead of a bare `sqlite://` engine,
so this exercises the same PyMongoSQL dialect `get_sqla_table()` already quotes
with via `database.get_dialect()`. Re-ran the suite: 134 passed. Pushed in
e41ad9391.
--
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]
