aminghadersohi commented on code in PR #44583:
URL: https://github.com/apache/superset/pull/44583#discussion_r4096788295
##########
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:
Dropping this whole branch for `return table(self.table_name)` keeps 1565
tests in `connectors/`+`db_engine_specs/` green — nothing pins that the
identifier routes through the spec. Setting `side_effect` rather than replacing
the mock makes `assert_called_once` usable; verified it reds that mutant.
```suggestion
database.db_engine_spec.quote_table.side_effect = (
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
database.db_engine_spec.quote_table.assert_called_once()
```
##########
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:
`select_star` does not consult this flag — `base.py:2321` calls
`quote_table` unconditionally, and grep shows one production reader,
`models.py:1967`. SQL Lab works because of the `quote_table` override above,
not because of the flag.
```suggestion
# schema-qualified identifier would never match.
`SqlaTable.get_sqla_table`
# (charts) consults this flag; `select_star` (SQL Lab) reaches the same
# unqualified FROM clause through the `quote_table` override above. Both
# rely on `adjust_engine_params` to select the schema at the connection
# level instead.
```
##########
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:
PyMongoSQL does ship a dialect, and `get_sqla_table` quoted with it two
lines up via `database.get_dialect()`. `create_engine(database.sqlalchemy_uri)`
needs no server, so the test can compile on the same dialect production uses
(134 passed with this applied).
```suggestion
# Compile with the engine's own dialect -- the one `get_sqla_table`
already
# quoted the identifier with -- so the assertion covers the real path.
engine = create_engine(database.sqlalchemy_uri)
```
--
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]
