codeant-ai-for-open-source[bot] commented on code in PR #41591: URL: https://github.com/apache/superset/pull/41591#discussion_r3509946291
########## superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py: ########## @@ -0,0 +1,108 @@ +# 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. +"""strip metricSqlExpressions from ag_grid_table params + +Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions`` +(a mapping of every datasource metric/column name to its SQL expression) into +``extra_form_data`` on every filter interaction, which was then serialised into +the ``params`` and ``query_context`` columns on save. For datasources with +many metrics this bloated each chart record by hundreds of MB. + +This migration strips the field from existing rows so those records are no +longer loaded eagerly on every dashboard request. + +Revision ID: d24e6b0a9c7f +Revises: 2bee73611e32 +Create Date: 2026-06-30 00:00:00.000000 + +""" + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.utils import json + +# revision identifiers, used by Alembic. +revision = "d24e6b0a9c7f" +down_revision = "2bee73611e32" + +Base = declarative_base() + +_FIELD = "metricSqlExpressions" +_VIZ_TYPE = "ag_grid_table" + + +class Slice(Base): + __tablename__ = "slices" + id = Column(Integer, primary_key=True) + viz_type = Column(String(250)) + params = Column(Text) + query_context = Column(Text) + + +def _strip_params(slc: Slice) -> bool: + """Remove _FIELD from extra_form_data in params. Returns True if changed.""" + if not slc.params: + return False + try: + params = json.loads(slc.params) + except Exception: + return False + + extra = params.get("extra_form_data", {}) Review Comment: **Suggestion:** This code assumes decoded `params` is always a dict; if any legacy row contains valid JSON that is not an object (e.g. list/string), `.get(...)` raises and aborts the whole migration. Guard with a dict type check before accessing keys so malformed-but-parseable rows are safely skipped. [type error] <details> <summary><b>Severity Level:</b> Critical ๐จ</summary> ```mdx - โ superset db upgrade can fail on malformed params. - โ ๏ธ One bad slice blocks cleaning remaining ag_grid_table charts. - โ ๏ธ Migration robustness reduced for historical params JSON payloads. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. `upgrade()` (lines 96โ102) in `superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py` iterates over all ag_grid_table slices and calls `_strip_params(slc)` (line 101). 2. `_strip_params` (lines 59โ75) loads JSON from `slc.params` using `params = json.loads(slc.params)` inside a `try` block; any parseable JSON value, including non-dict structures, will be stored in `params` without error. 3. The function immediately does `extra = params.get("extra_form_data", {})` at line 68; if `params` is not a dict (e.g. a list or string from legacy or corrupted data), this `.get` call raises `AttributeError` because those types do not implement `get`. 4. This exception is not caught (only `json.loads` is wrapped in `try/except`), so the error propagates out of `_strip_params`, aborting the `for` loop in `upgrade()` and causing `superset db upgrade` to fail whenever a sliceโs `params` column contains valid JSON that is not a dict. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=702cd8d3ffd74d77b92de27bfbc24f6e&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=702cd8d3ffd74d77b92de27bfbc24f6e&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:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py **Line:** 63:68 **Comment:** *Type Error: This code assumes decoded `params` is always a dict; if any legacy row contains valid JSON that is not an object (e.g. list/string), `.get(...)` raises and aborts the whole migration. Guard with a dict type check before accessing keys so malformed-but-parseable rows are safely skipped. 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=33aa1c260b2f0f82545fe08fb4e97e97903d612b1ae441859f3621f8d9eaea16&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=33aa1c260b2f0f82545fe08fb4e97e97903d612b1ae441859f3621f8d9eaea16&reaction=dislike'>๐</a> ########## superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py: ########## @@ -0,0 +1,108 @@ +# 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. +"""strip metricSqlExpressions from ag_grid_table params + +Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions`` +(a mapping of every datasource metric/column name to its SQL expression) into +``extra_form_data`` on every filter interaction, which was then serialised into +the ``params`` and ``query_context`` columns on save. For datasources with +many metrics this bloated each chart record by hundreds of MB. + +This migration strips the field from existing rows so those records are no +longer loaded eagerly on every dashboard request. + +Revision ID: d24e6b0a9c7f +Revises: 2bee73611e32 +Create Date: 2026-06-30 00:00:00.000000 + +""" + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.utils import json + +# revision identifiers, used by Alembic. +revision = "d24e6b0a9c7f" +down_revision = "2bee73611e32" + +Base = declarative_base() + +_FIELD = "metricSqlExpressions" +_VIZ_TYPE = "ag_grid_table" + + +class Slice(Base): + __tablename__ = "slices" + id = Column(Integer, primary_key=True) + viz_type = Column(String(250)) + params = Column(Text) + query_context = Column(Text) + + +def _strip_params(slc: Slice) -> bool: + """Remove _FIELD from extra_form_data in params. Returns True if changed.""" + if not slc.params: + return False + try: + params = json.loads(slc.params) + except Exception: + return False + + extra = params.get("extra_form_data", {}) + if _FIELD not in extra: + return False + + del extra[_FIELD] + params["extra_form_data"] = extra + slc.params = json.dumps(params) + return True + + +def _strip_query_context(slc: Slice) -> bool: + """Remove _FIELD from query_context.form_data.extra_form_data.""" + if not slc.query_context: + return False + try: + qc = json.loads(slc.query_context) + except Exception: + return False + + extra = qc.get("form_data", {}).get("extra_form_data", {}) + if _FIELD not in extra: Review Comment: **Suggestion:** This logic assumes decoded `query_context` is a dict with nested dicts; if a row contains a different JSON shape, `.get(...)` may raise and stop the migration. Add structure/type checks before nested access so unexpected historical payloads do not fail the entire upgrade. [type error] <details> <summary><b>Severity Level:</b> Critical ๐จ</summary> ```mdx - โ superset db upgrade may crash on malformed query_context. - โ ๏ธ Single corrupted slice prevents cleaning other ag_grid_table slices. - โ ๏ธ Less robust than prior query_context repair migration. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. `upgrade()` (lines 96โ102) iterates over ag_grid_table slices and calls `_strip_query_context(slc)` for each row (line 102) in `superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py`. 2. `_strip_query_context` (lines 78โ93) loads JSON from `slc.query_context` with `qc = json.loads(slc.query_context)` inside a `try` block; any parseable JSON value, including non-dict shapes, will be assigned to `qc`. 3. The function then performs nested dictionary access via `extra = qc.get("form_data", {}).get("extra_form_data", {})` at line 87; if `qc` is not a dict or `qc["form_data"]` is not a dict (for example, a string or list), this chained `.get` call raises `AttributeError` or `TypeError`, which are not caught. 4. Supersetโs existing migration `fix_form_data_string_in_query_context` at `superset/migrations/versions/2025-12-16_12-00_f5b5f88d8526_fix_form_data_string_in_query_context.py:29โ63` demonstrates that `query_context` and its `form_data` field have historically been stored with unexpected shapes (e.g. `form_data` as a JSON string) and guards nested access with broad exception handling, so this migrationโs unvalidated nested `.get` chain can realistically terminate `superset db upgrade` when it encounters similarly irregular `query_context` payloads. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bc96aa8d5f7e436fb4bf981d1b65c3d5&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=bc96aa8d5f7e436fb4bf981d1b65c3d5&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:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py **Line:** 82:88 **Comment:** *Type Error: This logic assumes decoded `query_context` is a dict with nested dicts; if a row contains a different JSON shape, `.get(...)` may raise and stop the migration. Add structure/type checks before nested access so unexpected historical payloads do not fail the entire upgrade. 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=3bd6072c5f0169fb57f2b16413885003bed089a56434b684837b7b3b6cb7aa86&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=3bd6072c5f0169fb57f2b16413885003bed089a56434b684837b7b3b6cb7aa86&reaction=dislike'>๐</a> ########## superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py: ########## @@ -0,0 +1,108 @@ +# 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. +"""strip metricSqlExpressions from ag_grid_table params + +Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions`` +(a mapping of every datasource metric/column name to its SQL expression) into +``extra_form_data`` on every filter interaction, which was then serialised into +the ``params`` and ``query_context`` columns on save. For datasources with +many metrics this bloated each chart record by hundreds of MB. + +This migration strips the field from existing rows so those records are no +longer loaded eagerly on every dashboard request. + +Revision ID: d24e6b0a9c7f +Revises: 2bee73611e32 +Create Date: 2026-06-30 00:00:00.000000 + +""" + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.ext.declarative import declarative_base + +from superset import db +from superset.utils import json + +# revision identifiers, used by Alembic. +revision = "d24e6b0a9c7f" +down_revision = "2bee73611e32" + +Base = declarative_base() + +_FIELD = "metricSqlExpressions" +_VIZ_TYPE = "ag_grid_table" + + +class Slice(Base): + __tablename__ = "slices" + id = Column(Integer, primary_key=True) + viz_type = Column(String(250)) + params = Column(Text) + query_context = Column(Text) + + +def _strip_params(slc: Slice) -> bool: + """Remove _FIELD from extra_form_data in params. Returns True if changed.""" + if not slc.params: + return False + try: + params = json.loads(slc.params) + except Exception: + return False + + extra = params.get("extra_form_data", {}) + if _FIELD not in extra: + return False + + del extra[_FIELD] + params["extra_form_data"] = extra + slc.params = json.dumps(params) + return True + + +def _strip_query_context(slc: Slice) -> bool: + """Remove _FIELD from query_context.form_data.extra_form_data.""" + if not slc.query_context: + return False + try: + qc = json.loads(slc.query_context) + except Exception: + return False + + extra = qc.get("form_data", {}).get("extra_form_data", {}) + if _FIELD not in extra: + return False + + del extra[_FIELD] + slc.query_context = json.dumps(qc) + return True + + +def upgrade() -> None: + bind = op.get_bind() + session = db.Session(bind=bind) + + for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE): + _strip_params(slc) + _strip_query_context(slc) Review Comment: **Suggestion:** The migration mutates ORM objects but never commits the session, so the stripped fields may never be persisted to the database. Add an explicit `session.commit()` (and ideally close the session) after processing rows to ensure updates are written. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical ๐จ</summary> ```mdx - โ AG Grid table slices keep oversized metricSqlExpressions blobs. - โ Dashboard load still reads inflated slice params JSON. - โ ๏ธ Migration appears successful but leaves data unchanged. ``` </details> <details> <summary><b>Steps of Reproduction โ </b></summary> ```mdx 1. Run `superset db upgrade`, which executes migration `d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params` in `superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py`. 2. In `upgrade()` (lines 96โ102), a SQLAlchemy session is created via `session = db.Session(bind=bind)` and all `Slice` rows with `viz_type == "ag_grid_table"` are iterated, mutating `slc.params` and `slc.query_context` through `_strip_params` and `_strip_query_context`. 3. Unlike other Superset data migrations that use ORM sessions and explicitly commit (for example `country_map_use_lowercase_country_name.upgrade()` at `versions/2021-04-09_16-14_085f06488938_country_map_use_lowercase_country_name.py:48โ57` which calls `session.commit()` and `session.close()` at lines 16โ17), this migration returns from `upgrade()` without any `session.commit()` or `session.close()` call. 4. Because the sessionโs dirty objects are never flushed or committed, the in-memory changes to `params` and `query_context` are discarded when the migration finishes, leaving `metricSqlExpressions` present in the database for all ag_grid_table slices despite the migration having run. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=686d537c4a7f4b34a2e494175b4b30c0&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=686d537c4a7f4b34a2e494175b4b30c0&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:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py **Line:** 96:102 **Comment:** *Incomplete Implementation: The migration mutates ORM objects but never commits the session, so the stripped fields may never be persisted to the database. Add an explicit `session.commit()` (and ideally close the session) after processing rows to ensure updates are written. 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=59c21c470289e14e91d21c84b2b08aec1261e214289f8272ccfb0dd795049208&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=59c21c470289e14e91d21c84b2b08aec1261e214289f8272ccfb0dd795049208&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]
