codeant-ai-for-open-source[bot] commented on code in PR #39509: URL: https://github.com/apache/superset/pull/39509#discussion_r3559552449
########## tests/unit_tests/models/test_helpers_offset.py: ########## @@ -0,0 +1,93 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import ast +from pathlib import Path + +HELPERS_PATH = ( + Path(__file__).resolve().parents[3] / "superset" / "models" / "helpers.py" +) + + +def _uses_supports_offset(node: ast.AST) -> bool: + """True if any attribute access on `node` references 'supports_offset'.""" + return any( + isinstance(child, ast.Attribute) and child.attr == "supports_offset" + for child in ast.walk(node) + ) + + +def _is_qry_offset_assignment(stmt: ast.AST) -> bool: + """True if stmt is `qry = qry.offset(...)` (any LHS, call to `.offset`).""" + if not isinstance(stmt, ast.Assign): + return False + call = stmt.value + if not isinstance(call, ast.Call): + return False + func = call.func + return isinstance(func, ast.Attribute) and func.attr == "offset" + + +def test_helpers_guards_offset_with_supports_offset_flag() -> None: + """ + Regression guard: the `.offset()` call in get_sqla_query must be wrapped + in an `if` that checks `supports_offset`. Without this guard, + engines that do not support OFFSET (Elasticsearch SQL) crash drill- + to-detail on page 2+. + + We parse the AST rather than grep the source so the test survives + Black-style reformatting and trivial refactors. + """ + source = HELPERS_PATH.read_text() + assert "supports_offset" in source, ( + "helpers.py no longer references supports_offset; the OFFSET " + "guard is gone — Elasticsearch drill-to-detail will crash on page 2+." + ) + + tree = ast.parse(source) + unguarded: list[int] = [] + + class Visitor(ast.NodeVisitor): + """Flag `.offset()` assignments not guarded by a `supports_offset` check.""" + + def __init__(self) -> None: + """Track nesting depth inside `supports_offset`-guarded `if` blocks.""" + self._in_guarded_if = 0 + + def visit_If(self, node: ast.If) -> None: # noqa: N802 + """Descend into the body under a `supports_offset` guard when present.""" + if _uses_supports_offset(node.test): + self._in_guarded_if += 1 + for child in node.body: + self.visit(child) + self._in_guarded_if -= 1 + for child in node.orelse: + self.visit(child) + else: + self.generic_visit(node) Review Comment: **Suggestion:** The AST guard logic only checks whether `supports_offset` appears anywhere in the `if` condition, but it does not verify that the condition is actually the positive guard (eg `...supports_offset` rather than `not ...supports_offset`). Because of that, this test would still pass if `.offset()` were placed under an inverted condition and emitted on engines that do not support offsets. Tighten the AST check to validate the condition polarity before marking a block as guarded. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ❌ Future inverted OFFSET guard not caught by regression test. ⚠️ Drill-detail and samples may emit OFFSET on unsupported engines. ⚠️ Test suite gives false assurance about offset gating. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `superset/models/helpers.py` and locate `ExploreMixin.get_sqla_query` near lines 3900–4019; observe the OFFSET guard: `if row_offset and self.database.db_engine_spec.supports_offset: qry = qry.offset(row_offset)` at lines 3934–3936. 2. Open `tests/unit_tests/models/test_helpers_offset.py` and inspect `_uses_supports_offset` (lines 25–30) and `Visitor.visit_If` (lines 70–80); note that `_uses_supports_offset` returns True for any `ast.If.test` containing an attribute named `supports_offset`, without checking polarity (e.g. positive vs `not supports_offset`), and `visit_If` treats only the `if` body as guarded when `_uses_supports_offset(node.test)` is True. 3. Consider a regression where `helpers.py` is modified so the guard is inverted, e.g. `if row_offset and not self.database.db_engine_spec.supports_offset: qry = qry.offset(row_offset)` at the same location (lines 3934–3936). In the AST, the `If.test` still contains a `supports_offset` attribute access, so `_uses_supports_offset(node.test)` returns True, `_in_guarded_if` is incremented while visiting `node.body`, and the `qry = qry.offset(...)` assignment is seen with `_in_guarded_if == 1`, causing `visit_Assign` (lines 82–86) not to record it in `unguarded`. The Visitor finishes with `unguarded` empty, and the regression test still passes even though `.offset()` is now emitted precisely when `supports_offset` is False. 4. Follow the runtime path for samples/drill-to-detail pagination: `get_samples` in `superset/views/datasource/utils.py` (lines 98–215) uses `get_limit_clause` to compute `row_offset`/`row_limit` and builds a `QueryContext` that ultimately calls `datasource.get_query_str(...)`, which in turn uses `ExploreMixin.get_query_str_extended` and `get_sqla_query` (helpers.py lines 1443–1455 and 3900–4019). On an engine where `engine_spec.supports_offset` is False (e.g. Elasticsearch SQL), the inverted guard would still attach `.offset(row_offset)` to the compiled SQL, causing `parsing_exception` errors on page > 1 while the regression test in `tests/unit_tests/models/test_helpers_offset.py` continues to pass, because it only checks for the presence of `supports_offset` in the condition, not that the condition is a positive guard. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c21f900496f0477a8d0785ef329940b8&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=c21f900496f0477a8d0785ef329940b8&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/unit_tests/models/test_helpers_offset.py **Line:** 70:80 **Comment:** *Incorrect Condition Logic: The AST guard logic only checks whether `supports_offset` appears anywhere in the `if` condition, but it does not verify that the condition is actually the positive guard (eg `...supports_offset` rather than `not ...supports_offset`). Because of that, this test would still pass if `.offset()` were placed under an inverted condition and emitted on engines that do not support offsets. Tighten the AST check to validate the condition polarity before marking a block as guarded. 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%2F39509&comment_hash=3c798d625855e75a305261e86532cb34fb14a246bb0a77f92cf1f7e07adbfcbc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39509&comment_hash=3c798d625855e75a305261e86532cb34fb14a246bb0a77f92cf1f7e07adbfcbc&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]
