codeant-ai-for-open-source[bot] commented on code in PR #41968:
URL: https://github.com/apache/superset/pull/41968#discussion_r3564787023
##########
tests/integration_tests/security/row_level_security_tests.py:
##########
@@ -469,6 +469,58 @@ def
test_rls_tables_related_api_with_filter_matching_birth(self):
assert all("birth" in table_name.lower() for table_name in
received_tables)
assert len(result) >= 1 # At least birth_names should be returned
+ @pytest.mark.usefixtures("load_birth_names_data")
+ def test_rls_tables_related_api_returns_all_masked_matches(self):
+ """
+ Regression for #29707: the RLS dataset selector's async search must
+ return *every* dataset whose name matches the typed mask, not a
+ truncated subset. The reporter had two "birth" datasets (birth_names
+ and birth_france_by_region) and searching "birth" showed an
+ incomplete list. This exercises the backend ``related/tables``
+ endpoint that feeds that selector to prove whether the search-by-mask
+ lookup is the culprit.
+ """
+ self.login(ADMIN_USERNAME)
+
+ birth_names = self.get_table(name="birth_names")
+ # Create a sibling dataset whose name also contains the "birth" mask,
+ # mirroring the two-dataset scenario from the issue.
+ sibling = SqlaTable(
+ table_name="birth_france_by_region",
+ database=birth_names.database,
+ schema=birth_names.schema,
+ )
+ db.session.add(sibling)
+ db.session.commit()
+
+ try:
+ # Datasets in the metadata DB that match the "birth" mask.
+ expected = {
+ t.name
+ for t in db.session.query(SqlaTable)
+ .filter(SqlaTable.table_name.ilike("%birth%"))
+ .all()
+ }
+ # Both datasets must be present in the ground truth for the
+ # assertion below to be meaningful.
+ bare_names = {name.split(".")[-1] for name in expected}
+ assert "birth_names" in bare_names
+ assert "birth_france_by_region" in bare_names
+
+ params = rison.dumps({"filter": "birth", "page": 0, "page_size":
100})
+ rv =
self.client.get(f"/api/v1/rowlevelsecurity/related/tables?q={params}")
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ received = {table["text"] for table in data["result"]}
+
+ # The masked search must return *all* matching datasets, and the
+ # advertised count must not under-report them.
+ assert received == expected
+ assert data["count"] == len(expected)
Review Comment:
**Suggestion:** Using sets for `expected` and `received` drops duplicate
display names, but `data["count"]` represents row count from the API. If two
datasets share the same rendered `text` (for example same table/schema name
across databases), this test will undercount expected matches and fail
incorrectly. Preserve multiplicity (e.g., compare sorted lists or IDs) when
validating `count` semantics. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ RLS related-tables test may fail with duplicate dataset names.
⚠️ CI for RLS backend can be flaky on multi-database setups.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a Superset instance backed by this code, create or import two SQL
datasets that
share the same rendered name/text (for example same `schema` and
`table_name` on different
databases), which is allowed because `SqlaTable` only enforces uniqueness on
`(database_id, catalog, schema, table_name)` (see
`superset/connectors/sqla/models.py:64-72`), while the `name` hybrid
property at
`superset/connectors/sqla/models.py:5-7` returns just `schema + "." +
table_name` or
`table_name`.
2. As an ADMIN user, run
`TestRowLevelSecurityWithRelatedAPI.test_rls_tables_related_api_returns_all_masked_matches`
in `tests/integration_tests/security/row_level_security_tests.py:43-94`,
which logs in
(line 54) and builds `expected` by querying all matching `SqlaTable` rows via
`db.session.query(SqlaTable).filter(SqlaTable.table_name.ilike("%birth%")).all()`
and
projecting to a set of `t.name` (lines 68-74).
3. The test then calls `GET /api/v1/rowlevelsecurity/related/tables` with
the same mask
(lines 81-83), which is handled by `BaseSupersetModelRestApi.related()` in
`superset/views/base_api.py:600-679`. That method uses
`datamodel.query(...)` to retrieve
matched rows and returns `count=total_rows` and a `result` list built by
`_get_result_from_rows()` at `superset/views/base_api.py:13-23`, where each
row’s `text`
is derived from `_get_text_for_model()` in
`superset/views/base_api.py:462-467`.
4. Because the test collapses both sides to sets (`expected = {t.name, ...}`
at lines
68-74 and `received = {table["text"] for table in data["result"]}` at line
85), any
duplicate names/texts are deduplicated before the final assertion `assert
data["count"] ==
len(expected)` at line 90. In a scenario with duplicate display names, the
API correctly
reports `count` equal to the number of rows, but `len(expected)` reflects
only the number
of distinct names, causing this regression test to fail even though backend
counting and
pagination are correct.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=65a44aa9dc484bbcbf96a4efa872ce81&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=65a44aa9dc484bbcbf96a4efa872ce81&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/security/row_level_security_tests.py
**Line:** 498:519
**Comment:**
*Logic Error: Using sets for `expected` and `received` drops duplicate
display names, but `data["count"]` represents row count from the API. If two
datasets share the same rendered `text` (for example same table/schema name
across databases), this test will undercount expected matches and fail
incorrectly. Preserve multiplicity (e.g., compare sorted lists or IDs) when
validating `count` semantics.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41968&comment_hash=4b68b5ac48c1bc166e0e15025a88244862bce01530c691a1cb2193d2b943906e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41968&comment_hash=4b68b5ac48c1bc166e0e15025a88244862bce01530c691a1cb2193d2b943906e&reaction=dislike'>👎</a>
--
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]
