-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix KeyError in DatabaseStore when dynamically adding panels and ensure from_store flag is set correctly #2184
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.
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
base: main
Are you sure you want to change the base?
Fix KeyError in DatabaseStore when dynamically adding panels and ensure from_store flag is set correctly #2184
Conversation
…re from_store flag is set correctly
The DatabaseStore fix caused all panels to be loaded, including disabled ones like ProfilingPanel. The history sidebar was including all panels in its JSON response rather than filtering by enabled status. This change adds a check for panel.enabled in the history sidebar view to ensure only enabled panels are included in the response, matching the behavior expected by tests and consistent with the main toolbar display logic. Fixes failing tests: - test_history_sidebar - test_history_sidebar_expired_request_id - test_history_sidebar_includes_history
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Makes sense to me. I think the settings handling should be cleaned up if possible.
@@ -223,7 +223,9 @@ def from_store(cls, request_id, panel_id=None): | |||
data = toolbar.store.panel(toolbar.request_id, panel.panel_id) | |||
if data: | |||
panel.load_stats_from_store(data) | |||
toolbar._panels[panel.panel_id] = panel | |||
else: | |||
panel.from_store = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only setting from_store
in debug_toolbar/panels/__init__.py
right now.
I think it would be better to call load_stats_from_store
unconditionally if we actually depend on this attribute.
delattr(settings, "DEBUG_TOOLBAR_PANELS") | ||
DebugToolbar._panel_classes = None | ||
StoredDebugToolbar._panel_classes = None | ||
get_panels.cache_clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder a bit if all that is really necessary?
If you're using the @override_settings
decorator, a signal is emitted which also calls cache_clear
.
I'd prefer this to use @override_settings(DEBUG_TOOLBAR_PANELS=...)
or something similar instead.
Description
This commit addresses the KeyError issue that occurs when using DatabaseStore with dynamically added panels to DEBUG_TOOLBAR_PANELS. The problem occurred in
StoredDebugToolbar.from_store()
where the method would only load panels that had stored data, causing a KeyError when accessing panels that were added after the initial request.The fix modifies the panel loading logic to:
from_store
flag for panels that have stored dataThis ensures that the toolbar works correctly when panels are dynamically added to the configuration, particularly when using DatabaseStore for persistent storage.
Fixes #2166
Checklist:
docs/changes.rst
.