refactor: do not cascade to TableColumn#42222
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #fd5a32Actionable Suggestions - 0Additional Suggestions - 1
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42222 +/- ##
==========================================
- Coverage 65.22% 65.22% -0.01%
==========================================
Files 2768 2768
Lines 156280 156285 +5
Branches 35774 35774
==========================================
+ Hits 101933 101936 +3
- Misses 52382 52384 +2
Partials 1965 1965
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| table: Mapped["SqlaTable"] = relationship( | ||
| "SqlaTable", | ||
| back_populates="columns", | ||
| cascade_backrefs=False, |
There was a problem hiding this comment.
Suggestion: Setting this relationship to disable backref cascading can cause newly created child objects to be silently left out of the session when they are attached via reverse assignment, so expected column inserts may never be flushed. If any code path creates a column with column.table = table and only persists the table graph, you need to explicitly add each column to the session or keep backref save-update cascading enabled. [logic error]
Severity Level: Major ⚠️
- ❌ Dataset column creation via ORM fails to persist rows.
- ⚠️ Metadata refresh may silently skip saving new columns.
- ⚠️ Administrators see datasets without expected column definitions.Steps of Reproduction ✅
1. Start Superset with the PR code, so `TableColumn.table` relationship at
`superset/connectors/sqla/models.py:961-965` uses `cascade_backrefs=False`.
2. Trigger a dataset metadata refresh or creation that constructs columns via
`TableColumn(table=sqla_table, ...)` (same module `superset/connectors/sqla/models.py`, in
the `SqlaTable` helper that builds `TableColumn` instances from inspector metadata).
3. In this flow, the `SqlaTable` instance is added to the SQLAlchemy session, but the new
`TableColumn` instances rely on the `table` assignment (`column.table = sqla_table`) and
backref cascade to be pulled into the session; they are not explicitly `session.add()`-ed
nor appended through `sqla_table.columns.append(column)`.
4. On flush, because `cascade_backrefs=False` at `superset/connectors/sqla/models.py:964`,
the ORM does not cascade `save-update` from `SqlaTable.columns` to these newly-associated
`TableColumn` objects, so expected column rows are never inserted into the `columns`
table; the dataset is created/refreshed without persisted column records.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 964:964
**Comment:**
*Logic Error: Setting this relationship to disable backref cascading can cause newly created child objects to be silently left out of the session when they are attached via reverse assignment, so expected column inserts may never be flushed. If any code path creates a column with `column.table = table` and only persists the table graph, you need to explicitly add each column to the session or keep backref save-update cascading enabled.
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
|
The flagged issue is correct. Disabling superset/connectors/sqla/models.py |
7050c28 to
171e144
Compare
Code Review Agent Run #9ba9f8Actionable Suggestions - 0Additional Suggestions - 1
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 |
|
Rebased, resolved the usual conflict with these, and CI is running :) |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| columns = [ | ||
| TableColumn(column_name="user", type="VARCHAR(255)"), | ||
| TableColumn( | ||
| column_name="username", | ||
| type="VARCHAR(255)", | ||
| expression="{{ current_username() }}", | ||
| ), | ||
| ] | ||
| db.session.add_all(columns) | ||
| table = SqlaTable( | ||
| table_name="test_has_no_extra_cache_keys_table", | ||
| sql="SELECT 'abc' as user", | ||
| database=get_example_database(), | ||
| columns=[ | ||
| TableColumn(column_name="user", type="VARCHAR(255)"), | ||
| TableColumn( | ||
| column_name="username", | ||
| type="VARCHAR(255)", | ||
| expression="{{ current_username() }}", | ||
| ), | ||
| ], | ||
| columns=columns, |
There was a problem hiding this comment.
Suggestion: TableColumn objects are added to the session before they are attached to a table, and then get_example_database() is called while building the dataset. That call is likely to execute a query, which triggers SQLAlchemy autoflush and can try to insert these columns with a null table_id, causing an integrity error. Attach the columns to the table before adding them to the session (or defer add_all until after table construction / use no_autoflush). [logic error]
Severity Level: Major ⚠️
- ❌ `test_extra_cache_keys_in_dataset_metrics_and_columns` may raise IntegrityError.
- ❌ Integration test suite / CI run can be blocked.
- ⚠️ Flaky behavior if database constraints differ between environments.Steps of Reproduction ✅
1. Run the integration test `test_extra_cache_keys_in_dataset_metrics_and_columns` in
`tests/integration_tests/sqla_models_tests.py:1008`.
2. Inside this test, at lines 1012–1020, two `TableColumn` instances are created **without
a `table`/`table_id` assigned** and immediately added to the session via
`db.session.add_all(columns)` at line 1020.
3. Still in the same test, at line 1021, `SqlaTable` is instantiated with
`database=get_example_database()` (imported from `superset.utils.database`), which
internally performs a `db.session.query(...)` to fetch the example `Database` record,
causing SQLAlchemy to autoflush the pending `TableColumn` objects.
4. During this autoflush, SQLAlchemy attempts to INSERT the two `TableColumn` rows while
their `table_id` is still `NULL` (they are not yet linked to `table` because
`SqlaTable(..., columns=columns)` hasn’t run), which can raise an `IntegrityError` if
`TableColumn.table_id` is non-nullable, causing this test (and therefore the integration
test suite/CI run) to fail.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/integration_tests/sqla_models_tests.py
**Line:** 1012:1025
**Comment:**
*Logic Error: `TableColumn` objects are added to the session before they are attached to a table, and then `get_example_database()` is called while building the dataset. That call is likely to execute a query, which triggers SQLAlchemy autoflush and can try to insert these columns with a null `table_id`, causing an integrity error. Attach the columns to the table before adding them to the session (or defer `add_all` until after table construction / use `no_autoflush`).
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
|
@hy144328 thanks for the PR, mind adding more details to your PR description? |
c9d8724 to
718a10d
Compare
718a10d to
6f9e4f0
Compare
6f9e4f0 to
575416a
Compare
Code Review Agent Run #c2b2f8Actionable Suggestions - 0Additional Suggestions - 3
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 |
3 participants
See #40273 .
SUMMARY
Disable
cascade_backrefsonTableColumnbecause it is deprecated in Airflow 2.Manually add objects to session that were previously added automatically.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Unit tests:
Integration tests via CI.
ADDITIONAL INFORMATION