fix(migration): strip metricSqlExpressions from ag_grid_table params#41591
Conversation
Code Review Agent Run #145917Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41591 +/- ##
==========================================
+ Coverage 64.26% 65.05% +0.79%
==========================================
Files 2747 2747
Lines 153782 153782
Branches 35254 35254
==========================================
+ Hits 98822 100044 +1222
+ Misses 53060 51828 -1232
- Partials 1900 1910 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #a28f5cActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| try: | ||
| params = json.loads(slc.params) | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Suggestion: Use the shared JSON-loading helper from migration utilities instead of manual JSON parsing with broad exception handling. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The shared migration utilities include try_load_json, which is designed for this exact pattern. Since the migration manually parses JSON with broad exception handling instead of using the helper, this is a real shared-utils violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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:66
**Comment:**
*Custom Rule: Use the shared JSON-loading helper from migration utilities instead of manual JSON parsing with broad exception handling.
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| try: | ||
| qc = json.loads(slc.query_context) | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Suggestion: Use the shared migration JSON-loading helper for query_context parsing to keep migration behavior consistent and compatibility-focused. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The migration utilities provide try_load_json for safe JSON loading in migrations, but this code performs manual parsing with a broad exception handler instead. That is the kind of ad hoc logic the shared-utils rule is meant to avoid.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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:85
**Comment:**
*Custom Rule: Use the shared migration JSON-loading helper for `query_context` parsing to keep migration behavior consistent and compatibility-focused.
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| 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) |
There was a problem hiding this comment.
Suggestion: Use the shared migration pagination helper instead of iterating directly over a raw ORM query, so the migration follows the project’s compatibility-safe migration utilities pattern. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The migration iterates a raw ORM query directly, while the project has a shared compatibility-safe batching helper (paginated_update) used in other migrations. This matches the migration-use-shared-utils rule, so the warning is valid.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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:**
*Custom Rule: Use the shared migration pagination helper instead of iterating directly over a raw ORM query, so the migration follows the project’s compatibility-safe migration utilities pattern.
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
Code Review Agent Run #eb11bdActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
20ffbea to
8134c08
Compare
| try: | ||
| params = json.loads(slc.params) | ||
| except Exception: | ||
| return False | ||
|
|
||
| extra = params.get("extra_form_data", {}) |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Not a realistic concern for this codebase. Superset always serialises params as a JSON object (the chart form produces a dict); a scalar or array value would indicate a pathologically corrupt row, not something a migration author should paper over. _strip_params already skips empty/null params and any row that fails json.loads; if a valid-but-non-dict JSON value somehow appeared, the correct outcome is to let the error surface so it can be investigated, not to silently skip it. Treating this as critical is a false positive.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
In this migration, do not add extra defensive type checks for
paramsbeyond JSON parsing; if a valid but non-dict JSON value appears, let the error surface rather than silently skipping it.
Applied to:
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| 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: |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Same reasoning as the params thread above: Superset always serialises query_context as a JSON object with a form_data dict. A non-dict value at the top level would be corrupt data, not an expected edge case. The existing try/except around json.loads handles parse failures; silently skipping structurally wrong but parseable rows would mask data corruption that should be investigated. False positive.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
In migration scripts, do not add defensive structure/type checks to silently skip parseable-but-malformed
query_contextpayloads; treat structurally invalid but parseable rows as data corruption that should be surfaced instead of masked.
Applied to:
**/migration*/****/*migration*.<ext>
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| 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) |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ AG Grid table slices keep oversized metricSqlExpressions blobs.
- ❌ Dashboard load still reads inflated slice params JSON.
- ⚠️ Migration appears successful but leaves data unchanged.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fixThere was a problem hiding this comment.
Stale — the current code uses session.flush() (not session.commit()) at the end of upgrade(), added in response to Joe's review. Calling session.commit() inside a migration would break Alembic's transaction management and prevent rollback; flush() is the correct pattern here — it sends the pending SQL UPDATEs within the outer transaction that Alembic owns.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
In Alembic migrations, use
session.flush()within the migration transaction instead ofsession.commit(); do not flag missing commits when the migration is flushed under Alembic-managed transaction control.
Applied to:
**/migration*/****/*migration*.<ext>
💡 To manage or update this instruction, visit: CodeAnt AI Settings
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #b27f64
Actionable Suggestions - 1
-
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py - 1
- Missing database commit · Line 96-102
Review Details
-
Files reviewed - 1 · Commit Range:
c9592d1..c882d5b- superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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) |
There was a problem hiding this comment.
The upgrade() function modifies slc.params and slc.query_context but never calls session.commit(). All 20+ comparable migrations in this codebase that modify records call session.commit() after processing (e.g., 2023-07-19_17-54_ee179a490af9_deckgl_path_width_units.py:66). Without commit, all changes are rolled back when the migration context closes.
Code Review Run #b27f64
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Stale — the bot scanned an earlier snapshot. The current code has session.flush() at the end of upgrade(), added in response to Joe's review. Using session.commit() here would be incorrect: it would commit mid-migration and break Alembic's ability to roll back if a later step fails. session.flush() is the right pattern — it sends the pending SQL UPDATEs inside the outer transaction that Alembic owns and commits.
There was a problem hiding this comment.
The reviewer's concern regarding the missing commit has been addressed in the current version of the migration. The upgrade() function now includes a session.commit() call that executes only if changes were made to the records, which is the correct pattern for Alembic migrations to ensure atomicity and proper rollback capabilities.
171edcf to
5a9e24f
Compare
|
Heads up for maintainers: this PR keeps hitting the "Multiple head revisions" CI failure because new migrations are landing on master faster than I can rechain. Each time a migration merges with the same I've rechained If another migration lands before this merges, the |
…t tests Addresses two review comments on PR apache#41591: 1. upgrade() mutated ORM objects without calling session.flush(), so no SQL UPDATEs were ever sent within Alembic's outer transaction. Added session.flush() at the end of upgrade() to emit the pending UPDATEs before Alembic commits. 2. Added tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py covering _strip_params/_strip_query_context helper functions and a full upgrade() integration test that seeds an ag_grid_table slice with metricSqlExpressions in both params and query_context, runs upgrade(), and asserts both are stripped. A non-ag_grid_table slice is also seeded to verify upgrade() leaves other viz types untouched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t tests Addresses two review comments on PR apache#41591: 1. upgrade() mutated ORM objects without calling session.flush(), so no SQL UPDATEs were ever sent within Alembic's outer transaction. Added session.flush() at the end of upgrade() to emit the pending UPDATEs before Alembic commits. 2. Added tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py covering _strip_params/_strip_query_context helper functions and a full upgrade() integration test that seeds an ag_grid_table slice with metricSqlExpressions in both params and query_context, runs upgrade(), and asserts both are stripped. A non-ag_grid_table slice is also seeded to verify upgrade() leaves other viz types untouched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
567210b to
30aee1e
Compare
There was a problem hiding this comment.
Code Review Agent Run #004b81
Actionable Suggestions - 1
-
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py - 1
- Replace blind except with specific exception · Line 82-85
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py - 1
- Missing pytest.fixture decorator and return type · Line 74-74
-
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py - 1
- Missing session commit in migration · Line 96-106
Review Details
-
Files reviewed - 2 · Commit Range:
7111e7c..5a9d636- superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
- tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| try: | ||
| qc = json.loads(slc.query_context) | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Replace bare Exception catch with json.JSONDecodeError since only JSON parsing exceptions are expected. (Also applies at line 84)
Code suggestion
Check the AI-generated fix before applying
| try: | |
| qc = json.loads(slc.query_context) | |
| except Exception: | |
| return False | |
| try: | |
| qc = json.loads(slc.query_context) | |
| except (json.JSONDecodeError, ValueError): | |
| return False |
Code Review Run #004b81
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @pytest.fixture | ||
| def engine(): | ||
| engine = create_engine("sqlite:///:memory:") | ||
| migration.Base.metadata.create_all(engine) | ||
| return engine |
There was a problem hiding this 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 🧹
Why it matters? ⭐
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.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def test_upgrade_strips_both_fields(engine) -> None: |
There was a problem hiding this comment.
Suggestion: Add a type annotation for the function parameter to satisfy the rule requiring typed function signatures. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
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.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
Code Review Agent Run #e17bd6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
PR apache#41555 stopped new writes of metricSqlExpressions into extra_form_data, but existing slices saved before that fix still carry the bloated data in their params and query_context columns. This Alembic migration strips metricSqlExpressions from extra_form_data in both columns for all ag_grid_table slices, so existing charts no longer load hundreds of MB of runtime-derived SQL expressions on every dashboard request. The field is regenerated automatically by transformProps on the next chart render and does not need to be restored on downgrade. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Per review: session.commit() inside a migration breaks rollback if anything fails after the commit. session.close() is unnecessary because Alembic owns the session lifecycle. Let Alembic manage both. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t tests Addresses two review comments on PR apache#41591: 1. upgrade() mutated ORM objects without calling session.flush(), so no SQL UPDATEs were ever sent within Alembic's outer transaction. Added session.flush() at the end of upgrade() to emit the pending UPDATEs before Alembic commits. 2. Added tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py covering _strip_params/_strip_query_context helper functions and a full upgrade() integration test that seeds an ag_grid_table slice with metricSqlExpressions in both params and query_context, runs upgrade(), and asserts both are stripped. A non-ag_grid_table slice is also seeded to verify upgrade() leaves other viz types untouched. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…iple heads b4a3f2e1d0c9 (fix_security_view_menu_case) landed on master with down_revision=9e1f3b8c4d2a, creating a second head alongside d24e6b0a9c7f. Rechain d24e6b0a9c7f to follow b4a3f2e1d0c9 instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on test Two pre-commit/CI failures in the unit test: 1. TID251: stdlib `json` is banned — replaced `import json as stdlib_json` with `from superset.utils import json` throughout the test file. 2. AttributeError on `conn.commit()`: SQLAlchemy 1.4 Connection objects don't have a `.commit()` method in legacy mode. Replaced `engine.connect()` + manual `conn.commit()` with `engine.begin()`, which auto-commits the transaction on context exit — correctly simulating how Alembic commits its outer transaction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ruff-format with target-version=py310 reformats multi-context-manager `with` statements into the parenthesized form. Update the `patch.object` block in the upgrade() test to use the parenthesized syntax so ruff-format no longer modifies the file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Four migrations landed on master since the last rechain (charts/datasets soft-delete, Subject model, entity-version infrastructure), each chaining off the previous. New HEAD is 8f3a1b2c4d5e. Rechain d24e6b0a9c7f to follow it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cbb467e to
6cd2fb3
Compare
Code Review Agent Run #b514dbActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@fitzee there are still some bot review comments marked as critical... can you assess these and either apply or refute the suggestions on those threads? |
6 participants
SUMMARY
Follow-up to #41555, which stopped new writes of
metricSqlExpressionsintoextra_form_data. However, charts saved before that fix still carry the bloated data in theirparamsandquery_contextcolumns — for datasources with many metrics this can be hundreds of MB per chart, loaded eagerly on every dashboard request.This migration strips
metricSqlExpressionsfromextra_form_datain both columns for allag_grid_tableslices.Root cause recap: The AG Grid table plugin puts
metricSqlExpressions(aRecord<string, string>mapping every datasource metric/column to its SQL expression) intoownStateon filter interactions. Before #41555,ExploreViewContainerspread all ofownStateintoextra_form_data, which was then serialised intoparamson save.What this migration does:
viz_type = 'ag_grid_table'metricSqlExpressionsfromparams.extra_form_datametricSqlExpressionsfromquery_context.form_data.extra_form_dataDowngrade: no-op — the field is runtime-derived by
transformPropson each render and does not need to be restored.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — data migration only.
TESTING INSTRUCTIONS
ag_grid_tableslice that was saved with filters applied before fix(table): exclude metricSqlExpressions from ownState→extra_form_data spread #41555 (or manually insertmetricSqlExpressionsintoparams.extra_form_datato simulate)superset db upgrademetricSqlExpressionsis absent fromparamsfor allag_grid_tableslicessuperset db downgrade— migration is a no-op and succeeds cleanlyADDITIONAL INFORMATION
Runtime estimate: O(n) over
ag_grid_tableslices. Reads and rewritesparams/query_contextin Python — for a typical installation with tens to hundreds of such slices this completes in seconds. For very large installations (thousands of slices) consider running during a maintenance window.🤖 Generated with Claude Code