codeant-ai-for-open-source[bot] commented on code in PR #41591: URL: https://github.com/apache/superset/pull/41591#discussion_r3576892635
########## tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py: ########## @@ -0,0 +1,200 @@ +# 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. +"""Tests for migration ``d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params``. + +Covers the two helper functions (_strip_params, _strip_query_context) and the +full upgrade() path that seeds an ag_grid_table slice with metricSqlExpressions +in both params and query_context, then asserts both fields are stripped. +""" + +from __future__ import annotations + +import json as stdlib_json +from importlib import import_module +from unittest.mock import patch + +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import Session + +migration = import_module( + "superset.migrations.versions." + "2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params" +) + +Slice = migration.Slice +_strip_params = migration._strip_params +_strip_query_context = migration._strip_query_context +_FIELD = migration._FIELD +_VIZ_TYPE = migration._VIZ_TYPE + +_SQL_EXPR = {"col1": "SELECT col1 FROM t", "col2": "SUM(revenue)"} + + +def _contaminated_params() -> str: + return stdlib_json.dumps( + { + "viz_type": _VIZ_TYPE, + "extra_form_data": { + _FIELD: _SQL_EXPR, + "filters": [{"col": "col1", "op": "==", "val": "a"}], + }, + } + ) + + +def _contaminated_query_context() -> str: + return stdlib_json.dumps( + { + "form_data": { + "extra_form_data": { + _FIELD: _SQL_EXPR, + }, + }, + "queries": [], + } + ) + + [email protected] +def engine(): + engine = create_engine("sqlite:///:memory:") + migration.Base.metadata.create_all(engine) + return engine + + +# --------------------------------------------------------------------------- +# Helper-function unit tests +# --------------------------------------------------------------------------- + + +def test_strip_params_removes_field() -> None: + slc = Slice(params=_contaminated_params()) + changed = _strip_params(slc) + assert changed + result = stdlib_json.loads(slc.params) + assert _FIELD not in result["extra_form_data"] + assert result["extra_form_data"]["filters"] == [{"col": "col1", "op": "==", "val": "a"}] + + +def test_strip_params_noop_when_field_absent() -> None: + params = stdlib_json.dumps({"extra_form_data": {"filters": []}}) + slc = Slice(params=params) + changed = _strip_params(slc) + assert not changed + assert stdlib_json.loads(slc.params) == {"extra_form_data": {"filters": []}} + + +def test_strip_params_noop_when_params_empty() -> None: + assert not _strip_params(Slice(params=None)) + + +def test_strip_params_noop_on_invalid_json() -> None: + slc = Slice(params="not-json") + changed = _strip_params(slc) + assert not changed + assert slc.params == "not-json" + + +def test_strip_query_context_removes_field() -> None: + slc = Slice(query_context=_contaminated_query_context()) + changed = _strip_query_context(slc) + assert changed + result = stdlib_json.loads(slc.query_context) + assert _FIELD not in result["form_data"]["extra_form_data"] + + +def test_strip_query_context_noop_when_field_absent() -> None: + qc = stdlib_json.dumps({"form_data": {"extra_form_data": {}}}) + slc = Slice(query_context=qc) + assert not _strip_query_context(slc) + + +def test_strip_query_context_noop_when_query_context_empty() -> None: + assert not _strip_query_context(Slice(query_context=None)) + + +def test_strip_query_context_noop_on_invalid_json() -> None: + slc = Slice(query_context="not-json") + changed = _strip_query_context(slc) + assert not changed + assert slc.query_context == "not-json" + + +# --------------------------------------------------------------------------- +# Full upgrade() integration test +# --------------------------------------------------------------------------- + + +def test_upgrade_strips_both_fields(engine) -> None: Review Comment: **Suggestion:** Add a type annotation for the function parameter to satisfy the rule requiring typed function signatures. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The test function takes the `engine` fixture parameter without a type annotation, and the rule requires type hints on function parameters when they can be annotated. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c4b0d328f02e4da8b29aa5d60c34b878&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=c4b0d328f02e4da8b29aa5d60c34b878&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/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py **Line:** 143:143 **Comment:** *Custom Rule: Add a type annotation for the function parameter to satisfy the rule requiring typed function signatures. 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%2F41591&comment_hash=c708c9211a7bd07373aa2ec2cfbb77a2c6f54520ddebd57f9e0c6f849b342672&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=c708c9211a7bd07373aa2ec2cfbb77a2c6f54520ddebd57f9e0c6f849b342672&reaction=dislike'>๐</a> ########## tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py: ########## @@ -0,0 +1,200 @@ +# 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. +"""Tests for migration ``d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params``. + +Covers the two helper functions (_strip_params, _strip_query_context) and the +full upgrade() path that seeds an ag_grid_table slice with metricSqlExpressions +in both params and query_context, then asserts both fields are stripped. +""" + +from __future__ import annotations + +import json as stdlib_json +from importlib import import_module +from unittest.mock import patch + +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import Session + +migration = import_module( + "superset.migrations.versions." + "2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params" +) + +Slice = migration.Slice +_strip_params = migration._strip_params +_strip_query_context = migration._strip_query_context +_FIELD = migration._FIELD +_VIZ_TYPE = migration._VIZ_TYPE + +_SQL_EXPR = {"col1": "SELECT col1 FROM t", "col2": "SUM(revenue)"} + + +def _contaminated_params() -> str: + return stdlib_json.dumps( + { + "viz_type": _VIZ_TYPE, + "extra_form_data": { + _FIELD: _SQL_EXPR, + "filters": [{"col": "col1", "op": "==", "val": "a"}], + }, + } + ) + + +def _contaminated_query_context() -> str: + return stdlib_json.dumps( + { + "form_data": { + "extra_form_data": { + _FIELD: _SQL_EXPR, + }, + }, + "queries": [], + } + ) + + [email protected] +def engine(): + engine = create_engine("sqlite:///:memory:") + migration.Base.metadata.create_all(engine) + return engine Review Comment: **Suggestion:** Add a return type annotation to this fixture function so it complies with the required type-hint rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This fixture function is newly added and omits a return type annotation, which violates the rule requiring type hints on functions that can be annotated. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1eb9f681e35e40cc935fd7c3a6f815b3&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=1eb9f681e35e40cc935fd7c3a6f815b3&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/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py **Line:** 73:77 **Comment:** *Custom Rule: Add a return type annotation to this fixture function so it complies with the required type-hint rule. 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%2F41591&comment_hash=adf21d7edfae04a4f7fe182bbc342da86d612cdfa5ea002cd2fdd308dd2e5f20&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=adf21d7edfae04a4f7fe182bbc342da86d612cdfa5ea002cd2fdd308dd2e5f20&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]
