Quickfix: errata admin 500s from shared ModelAdmin instance race#1735
Open
mwvolo wants to merge 1 commit into
Open
Quickfix: errata admin 500s from shared ModelAdmin instance race#1735mwvolo wants to merge 1 commit into
mwvolo wants to merge 1 commit into
Conversation
ErrataAdmin is registered once and reused as a singleton across every request. get_form()/changelist_view() mutated self.fields, self.readonly_fields, self.list_display, etc. per-request based on request.user's role. Under concurrent requests from different-role users, one user's field list could leak into another's readonly_fields check between Django's get_fieldsets() and get_form() calls, causing fields_for_model to raise "Unknown field(s) specified for Errata" (Sentry OPENSTAX-CMS-VJ/VH, v13.0.0). Replace the self.* mutations with Django's per-request hooks (get_fields, get_readonly_fields, get_list_display, get_list_filter, render_change_form), which return computed values instead of writing shared state. No behavior change per role. Add a regression test that replays the exact request interleaving Django performs internally and confirms it no longer raises.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| return ['id', 'book_title', 'created', 'modified', 'short_detail', 'number_of_errors', 'status', 'error_type', 'resource', 'location', 'additional_location_information', 'resolution', 'archived', 'junk'] | ||
| return ['id', 'book_title', 'created', 'short_detail', 'status', 'error_type', 'resource', 'location', 'created', 'archived'] | ||
|
|
||
| def get_list_filter(self, request): |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes intermittent Django admin 500s in the Errata admin caused by mutating shared ModelAdmin instance state (self.fields, self.readonly_fields, etc.) across concurrent requests. It refactors ErrataAdmin to compute role-specific admin configuration via per-request hooks and adds a regression test that reproduces the problematic request interleaving.
Changes:
- Refactored
ErrataAdminto useget_fields,get_readonly_fields,get_list_display, andget_list_filterrather than mutatingself.*per request. - Added a test that simulates cross-role request interleaving to ensure
get_form()remains stable. - Adjusted admin list configuration defaults (e.g.,
list_display_links) to avoid per-request mutation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| errata/admin.py | Replaces shared-instance mutations with per-request ModelAdmin hooks to prevent cross-request role leakage and resulting FieldError 500s. |
| errata/tests.py | Adds a regression test that reproduces the interleaving that previously triggered “Unknown field(s) specified for Errata” errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+201
to
+204
| def render_change_form(self, request, context, add=False, change=False, form_url='', obj=None): | ||
| response = super().render_change_form(request, context, add, change, form_url, obj) | ||
| response.context_data['save_as'] = self._is_content_manager(request) or self._is_editorial_vendor(request) | ||
| return response |
Comment on lines
+165
to
+168
| def get_list_display(self, request): | ||
| if self._is_content_manager(request): | ||
| return ['id', 'book_title', 'created', 'modified', 'short_detail', 'number_of_errors', 'status', 'error_type', 'resource', 'location', 'additional_location_information', 'resolution', 'archived', 'junk'] | ||
| return ['id', 'book_title', 'created', 'short_detail', 'status', 'error_type', 'resource', 'location', 'created', 'archived'] |
Comment on lines
+170
to
+173
| def get_list_filter(self, request): | ||
| if self._is_content_manager(request): | ||
| return (('created', DateRangeFilter), ('modified', DateRangeFilter), ('book', ActiveBookListFilter), 'status', 'created', 'modified', 'is_assessment_errata', 'modified', 'error_type', 'resolution', 'archived', 'junk', 'resource') | ||
| return (('created', DateRangeFilter), ('modified', DateRangeFilter), ('book', ActiveBookListFilter), 'status', 'created', 'modified', 'is_assessment_errata', 'error_type', 'resolution', 'archived', 'resource') |
Comment on lines
+159
to
+163
| # Mirrors Django's own _changeform_view: get_fieldsets() runs for one | ||
| # user, then get_form() runs for a *different* user before the first | ||
| # request finishes - the interleaving that produced the "Unknown | ||
| # field(s) specified for Errata" 500s in production. | ||
| errata_admin.get_form(plain_request, None) |
6 tasks
Member
Author
|
Superseded by #1736, which is based off this branch and includes all of these commits plus the permission-model redesign. Closing this one to avoid keeping two open PRs for the same file. |
mwvolo
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FieldError500s on/django-admin/errata/errata/(add and change views) right after the last release:Unknown field(s) (accounts_link)andUnknown field(s) (accounts_user_faculty_status)specified for Errata.ErrataAdminis registered once and reused as a singleton across every request.get_form()/changelist_view()mutatedself.fields,self.readonly_fields,self.list_display, etc. based onrequest.user's role. Under concurrent requests from different-role users (matches the two different users in the Sentry issues), one user's field list could leak into another user'sreadonly_fieldscheck between Django's internalget_fieldsets()call and itsget_form()call, sofields_for_modelsaw a field name that wasn't excluded and wasn't a real model field, and raised.self.*mutations with Django's proper per-request hooks (get_fields,get_readonly_fields,get_list_display,get_list_filter,render_change_form), which return computed values instead of writing shared instance state. No behavior change per role — same fields/permissions as before, just computed safely.Test plan
ErrataAdminSharedInstanceTestwhich replays the exact request interleaving Django performs internally (get_fieldsets for user A, then get_form for user B) — confirmed it reproduces the originalFieldErroragainst the pre-fix code and passes against the fix.python manage.py test errata --settings=openstax.settings.test(7 tests) passes.