Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
93 changes: 93 additions & 0 deletions change_implementation/concept_location.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# DRF Issue #6855 Concept Location

## Problem Summary

The browsable API can crash while rendering an extra action page when the response data comes from a serializer whose `instance` is not a normal model object for the current view's permission class.

The failing path is in the browsable API renderer, not in view dispatch:

1. The extra action returns `Response(serializer.data)`.
2. `serializer.data` is a `ReturnDict` or `ReturnList` that keeps a backlink to the serializer.
3. `BrowsableAPIRenderer` inspects that serializer during HTML rendering.
4. While building placeholder UI for other HTTP methods, it calls `view.check_object_permissions(request, instance)`.
5. `instance` may be a non-model object from the serializer, which can be incompatible with the permission class and crash with `AttributeError`.

## Primary Source Locations

### `rest_framework/renderers.py`

This is the main issue location.

- `BrowsableAPIRenderer.get_context()`
- Always asks for `put_form`, `post_form`, `delete_form`, and `options_form`.
- This means a normal `GET` render also evaluates synthetic UI for other methods.

- `BrowsableAPIRenderer.get_rendered_html_form()`
- Pulls `serializer = getattr(data, 'serializer', None)`.
- Extracts `instance = getattr(serializer, 'instance', None)` when `many=False`.
- Calls `show_form_for_method()` before the current `DELETE` / `OPTIONS` bailout.
- This ordering is what exposes the bug.

- `BrowsableAPIRenderer.show_form_for_method()`
- Calls `view.check_permissions(request)`.
- If `obj is not None`, also calls `view.check_object_permissions(request, obj)`.
- Only catches `APIException`, so an unexpected `AttributeError` from a permission class bubbles up and crashes rendering.

## Supporting Locations

### `rest_framework/utils/serializer_helpers.py`

- `ReturnDict`
- `ReturnList`

These wrappers preserve `data.serializer`, which is why renderers can see the original serializer and its `.instance`.

### `rest_framework/views.py`

- `APIView.check_object_permissions()`

This loops through permission classes and directly passes the provided object to `has_object_permission()`. It assumes the caller supplied the correct domain object.

### `rest_framework/request.py`

- `override_method`

`get_rendered_html_form()` uses this context manager while probing alternate HTTP methods such as `OPTIONS` and `DELETE`.

## UI/Template Locations

### `rest_framework/templates/rest_framework/base.html`

- `options_form`
- `delete_form`

These booleans control whether the browsable API shows the `OPTIONS` button and `DELETE` button/modal.

### `rest_framework/templates/rest_framework/admin.html`

- `delete_form`

The admin renderer also consumes the truthiness of `delete_form`.

## Existing Test Locations To Extend

### `tests/test_renderers.py`

Contains `BrowsableAPIRendererTests`, which is the closest existing unit/integration coverage for renderer behavior and extra actions.

### `tests/browsable_api/test_form_rendering.py`

Already has regression coverage around browsable API form generation and serializer shapes, including `many=True`.

### `tests/browsable_api/views.py`

Contains an example object-permission class that accesses nested attributes on the object and is useful as a pattern for reproducing this class of failure.

## Key Conceptual Insight

The renderer is mixing two different concepts:

- the object that was serialized for display
- the object that should be used for permission checks when deciding whether to show action UI

For issue #6855, the immediate crash is triggered by the synthetic `OPTIONS` UI path, where no object-level edit form is actually rendered, so reusing `serializer.instance` is not appropriate.
102 changes: 102 additions & 0 deletions change_implementation/impact_analysis.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
# DRF Issue #6855 Impact Analysis

## Scope Of The Bug

The bug affects HTML rendering through `BrowsableAPIRenderer`. It does not affect normal JSON responses or the primary view execution path.

The issue appears when all of the following are true:

1. A browsable API page is rendered.
2. The response data came from `serializer.data`, so the renderer can access `data.serializer`.
3. The serializer has `many=False`, so `serializer.instance` is used.
4. The serializer instance is not a safe object for the view's `has_object_permission()` logic.
5. The renderer probes another method, especially `OPTIONS`, during page construction.

## Observable User Impact

- The browsable API page crashes instead of rendering successfully.
- The failure happens after the view has already produced a valid response payload.
- API clients using non-HTML renderers are unaffected, so the bug can be invisible in automated API tests unless browsable rendering is exercised.

## Why Extra Actions Are A Common Trigger

Extra actions are more likely to serialize data that is not the same object type returned by `get_object()`. Examples include:

- projection objects
- service-layer result objects
- denormalized DTO-like objects
- serializer-backed views that do not call `get_object()`

That makes `serializer.instance` an unreliable proxy for object-permission checks during browsable API rendering.

## Method-Level Impact

### `OPTIONS`

This is the most important path for issue #6855.

- `OPTIONS` is typically present in `view.allowed_methods`.
- `get_context()` asks for `options_form` on normal page render.
- `get_rendered_html_form()` currently runs permission checks before its `OPTIONS` early return.
- No real serializer-bound form is produced for `OPTIONS`, so the object-level permission check is not necessary for rendering the placeholder button.

### `DELETE`

This method shares the same code shape, but the impact is different.

- `DELETE` may or may not be allowed on the route.
- `delete_form` is used to decide whether to show destructive UI.
- Unlike `OPTIONS`, object-level permission checks may still be meaningful here because the button reflects an action against a resource.

This means a broad "skip object checks for both `DELETE` and `OPTIONS`" change would be riskier than a focused `OPTIONS` fix.

### `PUT` / `PATCH`

These are not the direct cause of the reported crash for a GET-only extra action because they usually fail the `allowed_methods` check earlier. They remain important regression surface because they rely on `instance` to prepopulate edit forms.

## Compatibility Risks

### Low-risk changes

- Preventing object-permission checks for the synthetic `OPTIONS` path.
- Keeping existing permission behavior for edit and delete affordances.

### Medium-risk changes

- Replacing `serializer.instance` with `view.get_object()` inside the renderer.

This could:

- trigger additional database queries
- fail on views that do not implement `get_object()`
- introduce side effects in renderer code
- blur the boundary between response rendering and view object retrieval

### Higher-risk changes

- Skipping object-permission checks for `DELETE`
- Removing `delete_form` behavior entirely

Either could change which users see destructive UI and would need stronger review.

## Testing Impact

Regression coverage should include:

1. A browsable API request to an extra action that returns `serializer.data` for a non-model object.
2. A permission class whose `has_object_permission()` would fail if handed that serializer instance.
3. Confirmation that the HTML response renders successfully instead of crashing.
4. Confirmation that existing delete/edit form behavior is not unintentionally broadened.

Useful secondary coverage:

- a direct renderer test for the `OPTIONS` path
- confirmation that `many=True` behavior remains unchanged

## Documentation Impact

No public API documentation change is likely required if the fix is internal and behavior-preserving. A release note entry may still be appropriate because the change resolves a user-visible browsable API crash.

## Recommended Impact Conclusion

The smallest safe fix is to narrow the change to the synthetic `OPTIONS` form-generation path. That addresses the reported crash while minimizing risk to object-permission-sensitive UI such as delete and edit actions.
71 changes: 71 additions & 0 deletions change_implementation/other_useful_info.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
## Quick Overview of Important Classes

Inside `rest_framework/`, common areas are:

`serializers.py`: converts Python/Django objects to API data and validates input.
`views.py`, `generics.py`, `viewsets.py`: request handling abstractions.
`routers.py`: URL routing for viewsets.
`permissions.py`, `authentication.py`, `throttling.py`: access control and API policies.
`renderers.py`, `parsers.py`: output/input formats like JSON and the browsable API.
`response.py`, `request.py`: DRF’s request/response wrappers.

## Issue in Plain English

Issue #6855 is a Browsable API bug. The actual API endpoint works, but the HTML page crashes while DRF is trying to decide which buttons/forms to show.

The key flow is:

- A view action returns `Response(serializer.data)`.
- In browsable mode, DRF looks at `data.serializer`.
- It pulls `serializer.instance`.
- It uses that instance for permission checks while deciding whether to show forms/buttons.
- For some extra actions, that serializer.instance is not the real model object the permission logic expects.
- Result: DRF calls object-permission logic on the wrong kind of object and crashes.

This is the critical code path:

```python
# renderers.py
def show_form_for_method(self, view, method, request, obj):
"""
Returns True if a form should be shown for this method.
"""
if method not in view.allowed_methods:
return # Not a valid method

try:
view.check_permissions(request)
if obj is not None:
view.check_object_permissions(request, obj)
```

And this is where the renderer derives the object from the serializer and then asks for forms for several methods:

```python
# renderers.py
# See issue #2089 for refactoring this.
serializer = getattr(data, 'serializer', None)
if serializer and not getattr(serializer, 'many', False):
instance = getattr(serializer, 'instance', None)
if isinstance(instance, Page):
instance = None
else:
instance = None

with override_method(view, request, method) as request:
if not self.show_form_for_method(view, method, request, instance):
return

if method in ('DELETE', 'OPTIONS'):
return True # Don't actually need to return a form
```

And the browsable page always probes multiple methods, even on a normal GET render:

```python
# renderers.py
'put_form': self.get_rendered_html_form(data, view, 'PUT', request),
'post_form': self.get_rendered_html_form(data, view, 'POST', request),
'delete_form': self.get_rendered_html_form(data, view, 'DELETE', request),
'options_form': self.get_rendered_html_form(data, view, 'OPTIONS', request),
```
93 changes: 93 additions & 0 deletions change_implementation/source_code_strategy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# DRF Issue #6855 Source Code Strategy

## Goal

Fix the browsable API crash for extra actions that return serializer-backed data whose `serializer.instance` is not a valid object for the view's object-permission logic.

## Recommended Strategy

Implement a focused change in `rest_framework/renderers.py` so that the synthetic `OPTIONS` button path does not call `check_object_permissions()` on `serializer.instance`.

## Why This Strategy

It directly addresses the reported crash with the smallest behavioral change.

It also preserves current behavior for methods where object-level permission checks are still meaningful:

- `PUT`
- `PATCH`
- `DELETE`

## Proposed Code Change

### Primary change

Refactor `BrowsableAPIRenderer.get_rendered_html_form()` so that `OPTIONS` is handled before `show_form_for_method(view, method, request, instance)` is called.

Recommended behavior:

1. Enter `override_method(view, request, method)` as today.
2. If `method == 'OPTIONS'`:
- return early when the method is not allowed
- run `view.check_permissions(request)`
- do not run `view.check_object_permissions(request, instance)`
- return `True` so the template can render the `OPTIONS` button
3. For all other methods, keep the existing `show_form_for_method()` flow.
4. Keep the existing `DELETE` truthy return after permission gating.

## Why Not Use `view.get_object()`

Avoid changing the renderer to retrieve a new object from the view.

Reasons:

- not every relevant view implements `get_object()`
- extra actions may intentionally not be tied to the routed object
- renderer-time object lookup can introduce extra queries and side effects
- it is a larger semantic change than needed for this bug

## Why Not Skip Checks For Both `DELETE` And `OPTIONS`

That would be simpler mechanically, but it risks changing existing UI authorization behavior for delete actions.

`OPTIONS` is different because the renderer does not build an object-bound form for it. It only needs enough permission context to decide whether the method is generally accessible.

## Suggested Test Plan

### Regression test

Add a renderer-focused regression test in `tests/test_renderers.py` near `BrowsableAPIRendererTests`.

Suggested test shape:

1. Define a `ViewSet` with browsable API enabled.
2. Add a detail extra action that returns `Response(serializer.data)`.
3. Use a serializer whose `instance` is a custom object without the attributes expected by object permissions.
4. Attach a permission class whose `has_object_permission()` would raise `AttributeError` if called with that custom object.
5. Request the extra action with `HTTP_ACCEPT='text/html'`.
6. Assert that the response renders successfully with status `200`.

### Guardrail test

Add a small test confirming that delete UI still respects the existing permission gate, or at minimum ensure no existing delete-related renderer test regresses when the new test is added.

### Optional unit test

If maintainers prefer tighter isolation, add a direct unit test around `get_rendered_html_form(..., method='OPTIONS', ...)` to assert the method returns truthy without consulting object permissions.

## Implementation Notes

- Keep the fix local to `BrowsableAPIRenderer`; avoid changing core permission APIs.
- Do not broaden exception handling to swallow arbitrary `AttributeError`. That would hide real bugs instead of correcting the invalid permission-check input.
- Preserve the current `many=True` behavior, where `instance` is already treated as `None`.

## Validation Steps

1. Run the new regression test.
2. Run existing browsable API renderer tests.
3. Verify no change in non-HTML renderer behavior.
4. Manually confirm that an authenticated browsable API extra action page now renders instead of crashing.

## Fallback Option

If reviewers want a more centralized fix, a secondary approach is to teach `show_form_for_method()` to skip object-permission checks only for `OPTIONS`. That is still acceptable, but the `get_rendered_html_form()`-level change keeps the special case closest to the synthetic-form behavior that causes the bug.