fix(datasets): isolate filter state to fix concurrent /dataset race#39685
Conversation
Override _handle_filters_args on DatasetRestApi to build a fresh Filters instance per request. The FAB default mutates a single self._filters shared across requests, which under concurrent traffic leaks filters from one request into another (#33828). Adopts #33895 with review-comment cleanups (drop unused imports, fix docstring) and adds a regression test. Co-Authored-By: Michelle <MIKEX818>
Code Review Agent Run #afaa7bActionable 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 |
dosubot
Bot
added
api
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39685 +/- ##
==========================================
- Coverage 64.48% 63.83% -0.65%
==========================================
Files 2566 2585 +19
Lines 133926 136973 +3047
Branches 31096 31519 +423
==========================================
+ Hits 86357 87434 +1077
- Misses 46074 48007 +1933
- Partials 1495 1532 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@betodealmeida @michael-s-molina @sadpandajoe — gentle ping on this one: CI is green and there are no open review threads. Let me know if any of you happen to have bandwidth to take a look :) |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Clean fix for the FAB shared-filter race condition. Implementation is minimal and correct — three lines that construct a fresh Filters per call, add request-scoped filters, and return the joined result without touching self._filters. Docstring is exemplary: names the overridden method, explains the shared-state mechanism, gives a concrete failure example, and states the fix invariant. Regression test directly verifies the fresh-instance-per-call contract.
One medium-priority observation worth a follow-up: the same FAB _handle_filters_args race exists in all other BaseSupersetModelRestApi subclasses (ChartRestApi, DashboardRestApi, SavedQueryRestApi, QueryRestApi, etc.). Scoping the override to DatasetRestApi fixes the reported issue but leaves the others unprotected. Worth either moving the fix to BaseSupersetModelRestApi in base_api.py or opening a tracking issue for the remaining subclasses.
Not a blocker — the targeted fix is valid and the PR ships cleanly as-is.
Per reviewer feedback on #39685: the FAB `_handle_filters_args` race that this PR addressed for `DatasetRestApi` exists in every other subclass too (ChartRestApi, DashboardRestApi, SavedQueryRestApi, QueryRestApi, DatabaseRestApi, etc.) — they all inherit from FAB's `ModelRestApi`, which mutates a single shared `self._filters` instance across requests. Move the override from `DatasetRestApi` to `BaseSupersetModelRestApi` so all subclasses inherit the request-scoped behavior. The override is purely generic — no dataset-specific logic — and no other superset REST API overrides `_handle_filters_args` (checked), so there's zero collision risk. `BaseSupersetModelRestApi` is documented as the place to "implement specific superset generic functionality", so wrapping FAB behavior we want to differ from upstream is exactly its purpose. Test stays in `tests/unit_tests/datasets/api_tests.py` using `DatasetRestApi` as a concrete subclass to exercise the inherited behavior; docstring updated to note the broader scope.
|
Done in 862b193. Lifted The override is purely generic (no dataset-specific logic), and a grep confirmed no other superset REST API overrides Thanks for the catch — this would've been a follow-up that nobody got to. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Bito Automatic Review Skipped – PR Already Merged |
…pache#39685) Co-authored-by: Claude Code <noreply@anthropic.com>
3 participants
Summary
Adopts #33895 (thanks @MIKEX818!) with the requested cleanups and a regression test.
DatasetRestApinow overrides_handle_filters_argsso each request builds a freshFiltersinstance. The Flask-AppBuilder default mutates a singleself._filtersshared across requests, which under concurrent traffic leaks filter state from one request into another — observed in #33828 asGET /api/v1/dataset/?q=(filters:!((col:table_name,opr:eq,value:...)))returning the wrong dataset roughly 5% of the time when ~12 parallel calls fan out.Returning a request-scoped
Filterskeeps each call isolated.Changes vs. #33895
Addresses review feedback from @hainenber and @sadpandajoe:
import copy.from typing import Any, Callable, Dictline; use moderndict[str, Any]annotation instead.API_FILTERS_RIS_KEYinto the existing FAB-const import block.# self._filters.clear_filters()comment) and explicitly note this overrides the FAB method (clarifying the question @sadpandajoe raised).test_handle_filters_args_returns_request_scoped_filters) that asserts a freshFiltersinstance is constructed per call — the property that prevents the race.Re: @Vitor-Avila's question about whether this should live in FAB instead — that's a reasonable position, but FAB hasn't responded since June 2025 and Superset users are hitting this in production. Patching here unblocks 5.x; FAB can absorb it later.
Before / After
Before: parallel
GET /api/v1/dataset/filtered by differenttable_namevalues intermittently returns mismatched datasets.After: each request's filter state is isolated; the race window is closed.
Testing instructions
pytest tests/unit_tests/datasets/api_tests.py::test_handle_filters_args_returns_request_scoped_filterspasses.GET /api/v1/dataset/?q=(filters:!((col:table_name,opr:eq,value:<unique>)))requests with distincttable_namevalues; each response should match its query'stable_name100% of the time.Additional information
🤖 Generated with Claude Code via Custodial Engineer task #10