sadpandajoe opened a new pull request, #34769:
URL: https://github.com/apache/superset/pull/34769
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
#### Problem
The Slice.get() method introduced in PR #29573 was causing a TypeError that
broke chart thumbnail generation.
```
TypeError: Query.filter_by() takes 1 positional argument but 2 were given
```
#### Root Cause Analysis
**The Bug**
In superset/models/slice.py line 366, the code incorrectly used filter_by()
with a BinaryExpression:
```
# BROKEN
qry = db.session.query(Slice).filter_by(id_or_uuid_filter(id_or_uuid))
```
#### Why It Failed
- filter_by() expects keyword arguments: filter_by(id=123, name="test")
- id_or_uuid_filter() returns a BinaryExpression object like Slice.id == 123
- SQLAlchemy requirement: BinaryExpression objects must use filter(), not
filter_by()
#### Pattern Comparison
The bug was introduced when copying the UUID support pattern from
Dashboard.get(), but using the wrong filter method:
```
# CORRECT (Dashboard.get() - already working)
qry = db.session.query(Dashboard).filter(id_or_slug_filter(id_or_slug))
# INCORRECT (Slice.get() - what was broken)
qry = db.session.query(Slice).filter_by(id_or_uuid_filter(id_or_uuid))
```
#### Solution
One-line fix: Change filter_by() to filter() in Slice.get()
```
# FIXED
qry = db.session.query(Slice).filter(id_or_uuid_filter(id_or_uuid))
```
#### Impact & Testing
#### What This Fixes
- ✅ Chart thumbnail generation - cache_chart_thumbnail() in
superset/tasks/thumbnails.py
- ✅ All Slice.get() calls - Both ID and UUID string lookups now work
- ✅ API endpoints - Any chart API calls that trigger Slice.get()
#### Comprehensive Test Coverage Added
Unit Tests (tests/unit_tests/models/slice_test.py - 5 tests):
- Mock verification that filter() is called instead of filter_by()
- Integration tests with real database calls for ID and UUID scenarios
- Helper function tests for id_or_uuid_filter()
#### Integration Tests (tests/integration_tests/charts/api_tests.py - 4
tests):
- test_slice_get_by_id() - Test with numeric ID strings
- test_slice_get_by_uuid() - Test with UUID strings
- test_slice_get_nonexistent_id() - Test error handling for invalid IDs
- test_slice_get_nonexistent_uuid() - Test error handling for invalid UUIDs
### Investigation Depth
- ✅ Confirmed isolation: Only Slice.get() was broken, Dashboard.get() was
already correct
- ✅ No other models affected: Only charts/slices use this pattern
- ✅ Screenshots analysis: Dashboard screenshots work (correct
implementation), only chart screenshots were broken
- ✅ Downloads unaffected: Export/download functionality uses different
code paths
🤖 Generated with [Claude Code](https://claude.ai/code)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
