Fixes #2073 -- Added DatabaseStore for persistent debug data storage. #2121
Conversation
- Introduced `DatabaseStore` to store debug toolbar data in the database. - Added `DebugToolbarEntry` model and migrations for persistent storage. - Updated documentation to include configuration for `DatabaseStore`. - Added tests for `DatabaseStore` functionality, including CRUD operations and cache size enforcement. Fixes django-commons#2073
|
Tests are all failing, you'd probably have to do a merge of |
|
The only failing tests are unrelated to this PR. Both tests have already been fixed, but the fixes have not yet been merged into the serializable branch. |
| def save_panel(cls, request_id: str, panel_id: str, data: Any = None): | ||
| """Save the panel data for the given request_id""" | ||
| # First ensure older entries are cleared if we exceed cache size | ||
| cls.set(request_id) |
There was a problem hiding this comment.
I wonder what the set method is for. @tim-schilling , do you remember what the thinking behind it was when we have save_panel to really do saving?
The call can probably be removed here because we're immediately doing get_or_create afterwards, so if the entry doesn't exist already saving the panel data works anyway.
There was a problem hiding this comment.
I'll push the rest of changes for now, while this is being discussed.
For reference, here's our the save_panel method was implemented in MemoryStore, including the set method call.
@classmethod
def save_panel(cls, request_id: str, panel_id: str, data: Any = None):
"""Save the panel data for the given request_id"""
cls.set(request_id)
cls._request_store[request_id][panel_id] = serialize(data)We can also call _cleanup_old_entries directly rather than set to call _cleanup_old_entries, if I understand the purpose correctly.
There was a problem hiding this comment.
cls.set() is meant to create the record for the request in the toolbar's store. For the MemoryStore, there's a separate collection of ids from the actual store. I suspect it's for performance more than anything.
- Updated model name from `DebugToolbarEntry` to `HistoryEntry` to make string representations of the app_model less redundant. - Adjusted verbose names to use translations with `gettext_lazy`. - Updated all references in `store.py` to use the new model name. - Modified tests to reflect the model name change. - Added a test to check the default ordering of the model and make it the defaul ordering in methods reliable.
Fix verbose names and capitalization.
Fix verbose name
|
By the way, you could try pinning django-csp<4 in this branch again, this would probably allow the tests to pass. And you don't have to worry about merging main for now. |
|
Good. I'll try pinning CSP. That way, I can also rest assured that's the only cause of the failing tests. |
|
I'll look into the failing (MySQL) tests later today. Good that we pinned django-csp. Otherwise, I would have assumed that was the only reason for the failing tests. |
|
The MariaDB failure has to do with using |
| HistoryEntry.objects.filter(request_id=request_id).delete() | ||
|
|
||
| @classmethod | ||
| def save_panel(cls, request_id: str, panel_id: str, data: Any = None): |
There was a problem hiding this comment.
Similar to set(), do we want to use transaction.atomic() here as well?
There was a problem hiding this comment.
This comment is showing under panel()... I wonder if somehow the comment shifted and it was meant for save_panel().
There was a problem hiding this comment.
I meant this comment for both methods.
There was a problem hiding this comment.
As per our call, there's a concern about panels updating this concurrently and needing to avoid clearing another panel's data when saving.
|
I applied some of the changes suggested by @tim-schilling without attempting to simplify tests yet to see if all tests pass in CI. |
|
@tim-schilling I'm good with this, and I hope we can get |
Description
DatabaseStoreto store debug toolbar data in the database.DebugToolbarEntrymodel and migrations for persistent storage.DatabaseStore.DatabaseStorefunctionality, including CRUD operations and cache size enforcement.Fixes #2073
Checklist:
docs/changes.rst.