Skip to content

fix(views): emit deprecated-endpoint log warning once per endpoint per process #41286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eschutho wants to merge 2 commits into
base: master
Choose a base branch
from
+36 −13
Open

fix(views): emit deprecated-endpoint log warning once per endpoint per process #41286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2e6877
fix(views): suppress repeated deprecated-endpoint log noise
eschutho Jun 22, 2026
5e06591
test(views): add test for deprecated decorator once-per-endpoint beha…
eschutho Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions superset/views/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,20 +171,25 @@ def deprecated(
"""

def _deprecated(f: Callable[..., FlaskResponse]) -> Callable[..., FlaskResponse]:
_warned = False

def wraps(self: BaseSupersetView, *args: Any, **kwargs: Any) -> FlaskResponse:
message = (
"%s.%s "
"This API endpoint is deprecated and will be removed in version %s"
)
logger_args = [
self.__class__.__name__,
f.__name__,
eol_version,
]
if new_target:
message += " . Use the following API endpoint instead: %s"
logger_args.append(new_target)
logger.warning(message, *logger_args)
nonlocal _warned
if not _warned:
_warned = True
message = (
"%s.%s "
"This API endpoint is deprecated and will be removed in version %s"
)
logger_args = [
self.__class__.__name__,
f.__name__,
eol_version,
]
if new_target:
message += " . Use the following API endpoint instead: %s"
logger_args.append(new_target)
logger.warning(message, *logger_args)
Comment on lines +177 to +192

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The new one-time warning guard is not thread-safe: concurrent first requests can both pass the if not _warned check before either write is visible to the other, so the warning can still be emitted multiple times in a worker. This violates the intended "once per endpoint per process" behavior; guard the check/update with a lock (or another atomic one-time primitive) around the warning block. [race condition]

Severity Level: Minor 🧹 
- ⚠️ Deprecated explore_json logs may duplicate under concurrent load.
- ⚠️ Deprecation logging not strictly once-per-process as intended.
Steps of Reproduction ✅ 
1. Observe the deprecated decorator implementation in `superset/views/base.py` at lines
15–48 (Read output), specifically the inner `wraps` function where the guard is
implemented (diff hunk lines 176–193): it uses `nonlocal _warned`, checks `if not
_warned:`, then sets `_warned = True` and emits `logger.warning(...)` without any
synchronization.

2. Confirm that a real HTTP endpoint uses this decorator: in `superset/views/core.py`
lines 280–319, the `explore_json` view is exposed via `@expose("/explore_json/",
methods=("GET","POST"))` and
`@expose("/explore_json/<datasource_type>/<int:datasource_id>/", ...)`, and is decorated
with `@deprecated(eol_version="5.0.0", new_target="/api/v1/chart/data")` at line 18 (Read
output), so every call to `/explore_json` passes through the `_warned` guard in
`superset/views/base.py`.

3. Run Superset with this PR code under a multi-threaded WSGI server (e.g., gunicorn
worker configured with `--threads > 1`), so that multiple requests to `/explore_json` can
be processed concurrently by the same process and thus share the same `_warned` closure
defined in `deprecated` at `superset/views/base.py:24–46`.

4. Immediately after starting a worker, send two or more concurrent HTTP requests to
`/explore_json` (or `/explore_json/<datasource_type>/<datasource_id>/`) so they reach the
same process at nearly the same time; due to the unsynchronized `if not _warned: _warned =
True` sequence in `superset/views/base.py` lines 28–31, two threads can both see `_warned`
as `False` before either write is visible, causing the warning block to execute in both
threads and resulting in multiple `"This API endpoint is deprecated and will be removed in
version %s"` log entries instead of a single once-per-process warning.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset/views/base.py
**Line:** 177:192
**Comment:**
	*Race Condition: The new one-time warning guard is not thread-safe: concurrent first requests can both pass the `if not _warned` check before either write is visible to the other, so the warning can still be emitted multiple times in a worker. This violates the intended "once per endpoint per process" behavior; guard the check/update with a lock (or another atomic one-time primitive) around the warning block.

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
👍 | 👎

@betodealmeida betodealmeida Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Ok, it would result in multiple warnings but not that many compared to today.

return f(self, *args, **kwargs)

return functools.update_wrapper(wraps, f)
Expand Down
18 changes: 18 additions & 0 deletions tests/unit_tests/views/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,3 +209,21 @@ def test_csv_response_leaves_bytes_untouched() -> None:

payload = "Ürün\n".encode("utf-8-sig")
assert CsvResponse(payload).get_data() == payload


def test_deprecated_logs_warning_exactly_once() -> None:
from superset.views.base import BaseSupersetView, deprecated

@deprecated(eol_version="5.0.0", new_target="/api/v1/chart/data")
def endpoint(self: BaseSupersetView) -> None:
return None

view = MagicMock(spec=BaseSupersetView)
view.__class__.__name__ = "Superset"

with patch("superset.views.base.logger") as mock_logger:
endpoint(view)
endpoint(view)

assert mock_logger.warning.call_count == 1
assert "5.0.0" in mock_logger.warning.call_args[0][0]
Loading