Improve handling of status codes. #573

ngnpope · 2021-10-14T14:37:32Z

I've bumped into a number of things related to status codes that I think could work better.

Status codes are largely passed around as strings.
In some places only strings are accepted, e.g. OpenApiExample's status_codes list.
OpenApiExample requires status_codes to be defined to something to work with non-200/201 statuses in OpenApiResponse which doesn't make sense and seems unnecessary if the example can only be for that defined response and its status code.

These changes do the following:

Allow string, integer (including rest_framework.status.* constants) or enum (e.g. HTTPStatus) to work in:
- The keys of the responses dict of @extend_schema.
- The status_codes list of OpenApiExample.
Allow the status_codes list to be left undefined so that the examples in OpenApiResponse assume they work for the status code for that response.

This probably isn't perfect and you'll want to fix some bits I'm sure, but I thought there was enough here to be getting on with.

codecov · 2021-10-14T14:44:20Z

Codecov Report

Merging #573 (8f28e40) into master (59f9749) will decrease coverage by 0.02%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
- Coverage   98.80%   98.78%   -0.03%     
==========================================
  Files          58       58              
  Lines        6702     6724      +22     
==========================================
+ Hits         6622     6642      +20     
- Misses         80       82       +2

Impacted Files	Coverage Δ
drf_spectacular/openapi.py	`96.97% <90.90%> (-0.22%)`	⬇️
drf_spectacular/utils.py	`98.99% <100.00%> (+<0.01%)`	⬆️
tests/test_examples.py	`100.00% <100.00%> (ø)`
tests/test_regressions.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59f9749...8f28e40. Read the comment docs.

ngnpope

@tfranzel I've left some thoughts on some of this for you.

ngnpope · 2021-10-14T14:38:15Z

docs/drf_yasg.rst

@@ -150,8 +150,7 @@ Parameter Location
 - :py:data:`~drf_yasg.openapi.IN_QUERY` is called :py:attr:`~drf_spectacular.utils.OpenApiParameter.QUERY`
 - :py:data:`~drf_yasg.openapi.IN_HEADER` is called :py:attr:`~drf_spectacular.utils.OpenApiParameter.HEADER`
 - :py:data:`~drf_yasg.openapi.IN_BODY` and :py:data:`~drf_yasg.openapi.IN_FORM` have no direct equivalent.
-  Instead you can use ``@extend_schema(request={"<media-type>": ...})`` or
-  ``@extend_schema(request={("<status-code>", "<media-type"): ...})``.


We both missed that this didn't make sense for requests! 🤦🏻

ngnpope · 2021-10-14T14:39:37Z

drf_spectacular/openapi.py

@@ -1019,7 +1020,7 @@ def _get_examples(self, serializer, direction, media_type, status_code=None, ext
                continue
            if media_type and media_type != example.media_type:
                continue
-            if status_code and status_code not in example.status_codes:
+            if status_code and status_code not in (example.status_codes or [200, 201]):


By postponing the default values here, we allow OpenApiExample to be unshackled such that it can be defined globally and reused for different status codes in multiple operations.

ngnpope · 2021-10-14T14:40:12Z

drf_spectacular/openapi.py

-                if code in responses:
-                    responses[code]['content'].update(content_response['content'])
+                    media_types = None
+                status_code = int(status_code)


Casting to int allows for http.HTTPStatus to work.

unfortunately it would be a regression

ngnpope · 2021-10-14T14:42:39Z

drf_spectacular/openapi.py

-                    responses[code] = content_response
-            return responses
+                    responses[status_code] = content_response
+            return {str(k): v for k, v in responses.items()}


This could be removed if we wanted to pass back int here too, but that would require updating all of the tests which currently have things like ...['responses']['200'] to have ...['responses'][200]. You could choose to do this - it's not difficult - but I didn't want to introduce too much noise for the purposes of this PR.

ngnpope · 2021-10-14T14:45:35Z

drf_spectacular/openapi.py

+                if example.status_codes is None:
+                    example.status_codes = [status_code]


My main concern here is isolation as we're modifying the example. We might need to clone the object or otherwise pass through a special flag to _get_examples() to say that this is in an OpenApiResponse so we can assume that all is well if status_codes is None...

ngnpope · 2021-10-14T14:52:08Z

drf_spectacular/openapi.py

+                elif status_code not in example.status_codes:
+                    warn(
+                        f'example in response with status code {status_code} had'
+                        f'status_codes set to {example.status_codes!r}'
+                    )


I didn't test this, but it struck me that if we're allowing status_codes to be None, or they can be pre-defined as the same value as the status code of the response, they could also have the wrong value.

Again, maybe this check should be pushed down into _get_examples() to be done when in this context of an OpenApiResponse to highlight that there is something wrong instead of silently ignoring it.

This check would be especially handy for the following (untested, hypothetical) situation:

unauthenticated_example = OpenApiExample( "UnauthenticatedExample", value={"error": "You are not authenticated."}, status_codes=[401, 403], ) @extend_schema( ..., responses={ 200: OpenApiResponse(description="...", examples=[unauthenticated_example]), # Should raise warning... 401: OpenApiResponse(description="...", examples=[unauthenticated_example]), 403: OpenApiResponse(description="...", examples=[unauthenticated_example]), ) @api_view(["POST"]) def view(request): ...

ngnpope · 2021-10-14T14:52:56Z

drf_spectacular/openapi.py

@@ -1193,7 +1203,7 @@ def _get_response_for_code(self, serializer, status_code, media_types=None):
        if (
            self._is_list_view(serializer)
            and get_override(serializer, 'many') is not False
-            and ('200' <= status_code < '300' or spectacular_settings.ENABLE_LIST_MECHANICS_ON_NON_2XX)


I mean, this worked, but it's sort of a fluke. 😄

this actually works quite well given that 2XX is also a valid value. unless you go totally crazy there, this always does what it is supposed to do.

ngnpope · 2021-10-14T14:53:56Z

drf_spectacular/utils.py

@@ -123,7 +124,7 @@ def __init__(
        self.response_only = response_only
        self.parameter_only = parameter_only
        self.media_type = media_type
-        self.status_codes = status_codes or ['200', '201']
+        self.status_codes = list(map(int, status_codes)) if status_codes else None


Forcing list[int] | None early makes everything much easier downstream.

ngnpope · 2021-10-15T08:29:01Z

Rebased to resolve conflicts.

I added the (status-code, media-type) version as requested in this comment: tfranzel#527 (comment) But, of course, status codes do not apply to requests, only responses!

Currently status codes are allowed to be integers in some places, but not others. It seems more natural that a status code be represented as an integer than a string.

When passing an example in `examples` for `OpenApiResponse`, assume that the `status_codes` of the `OpenApiExample` should match that of the response and don't default to `[200, 201]`.

tfranzel

Hey @ngnpope, sry I had to manage my time a bit, but finally got around looking at this.

I was a bit surprised that the wildcard status codes were not covered by the tests at all. Initially the code was dealing in ints until I found out that status codes can also literally be 2XX. This breaks a lot of changes you made there. I added a regression test for this with 36b970e.

I do like some aspect of this like delayed defaults for examples ['200', '201']. HTTPStatus seems nice but users should likely prefer using DRF's native definitions with rest_framwork.status, which should work out of the box already.

A friendly remark: I'm thankful for your awesome contributions! However, mixing together several issues/refactorings in one large PR makes it harder for me to quickly review it in between my other work. It might also mean more work for you, if I need to reject some parts of it.
One more thing. If you're thinking about doing a bigger change that might be controversial, why not create a 1-2 sentence issue first, so that I can provide you with some preliminary feedback. It may save you some time in the future.

tfranzel · 2021-10-31T22:10:35Z

drf_spectacular/openapi.py

-                if code in responses:
-                    responses[code]['content'].update(content_response['content'])
+                    media_types = None
+                status_code = int(status_code)


unfortunately it would be a regression

tfranzel · 2021-10-31T22:11:27Z

drf_spectacular/openapi.py

@@ -1193,7 +1203,7 @@ def _get_response_for_code(self, serializer, status_code, media_types=None):
        if (
            self._is_list_view(serializer)
            and get_override(serializer, 'many') is not False
-            and ('200' <= status_code < '300' or spectacular_settings.ENABLE_LIST_MECHANICS_ON_NON_2XX)


this actually works quite well given that 2XX is also a valid value. unless you go totally crazy there, this always does what it is supposed to do.

ngnpope · 2021-11-01T00:10:33Z

I was a bit surprised that the wildcard status codes were not covered by the tests at all. Initially the code was dealing in ints until I found out that status codes can also literally be 2XX. This breaks a lot of changes you made there. I added a regression test for this with 36b970e.

I'd completely forgotten that wildcard status codes was even a thing. (Mentioned here in the specification.)

I do like some aspect of this like delayed defaults for examples ['200', '201']. HTTPStatus seems nice but users should likely prefer using DRF's native definitions with rest_framwork.status, which should work out of the box already.

Yes. One of the main things I was wanting to avoid was the need to specify the status codes where OpenApiExample is passed to OpenApiResponse which already has status codes defined. (Although that might need some thought if we have a wildcard status code on the response.) Another thing is that, currently, the status codes for OpenApiExample must be a string which is confusing if passing integers in the responses dict.

A friendly remark: I'm thankful for your awesome contributions! However, mixing together several issues/refactorings in one large PR makes it harder for me to quickly review it in between my other work. It might also mean more work for you, if I need to reject some parts of it.

Sorry - comes from time pressures and changes depending on each other. Will endeavour to split things up in the future.

tfranzel · 2022-04-21T18:22:15Z

@ngnpope since this was kind of a feature down the wrong path, I think we can close this. I'm cleaning up, but if there is something still to consider here let me know.

ngnpope commented Oct 14, 2021

View reviewed changes

ngnpope force-pushed the fix-status-codes branch from 92bc3bb to 4ab934c Compare October 15, 2021 08:28

ngnpope added 6 commits October 22, 2021 11:43

Remove invalid documentation in drf-yasg migration guide.

9d4ba5f

I added the (status-code, media-type) version as requested in this comment: tfranzel#527 (comment) But, of course, status codes do not apply to requests, only responses!

Delay default status code selection for examples.

1e9fd0f

Pass status codes around as integers instead of strings.

51a9867

Currently status codes are allowed to be integers in some places, but not others. It seems more natural that a status code be represented as an integer than a string.

Add tests for status codes in enums, e.g. http.HTTPStatus.

60b4f07

Make use of DRF status helper functions.

e9f1ec1

Support examples for responses without specifying status codes.

8f28e40

When passing an example in `examples` for `OpenApiResponse`, assume that the `status_codes` of the `OpenApiExample` should match that of the response and don't default to `[200, 201]`.

ngnpope force-pushed the fix-status-codes branch from 4ab934c to 8f28e40 Compare October 22, 2021 10:44

tfranzel added a commit that referenced this pull request Oct 31, 2021

add regression test for catch-all status codes #573

36b970e

tfranzel reviewed Oct 31, 2021

View reviewed changes

tfranzel closed this Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of status codes. #573

Improve handling of status codes. #573

ngnpope commented Oct 14, 2021

codecov bot commented Oct 14, 2021 •

edited

Loading

ngnpope left a comment

ngnpope Oct 14, 2021

ngnpope Oct 14, 2021

ngnpope Oct 14, 2021

tfranzel Oct 31, 2021

ngnpope Oct 14, 2021

ngnpope Oct 14, 2021

ngnpope Oct 14, 2021

ngnpope Oct 14, 2021 •

edited

Loading

tfranzel Oct 31, 2021

ngnpope Oct 14, 2021

ngnpope commented Oct 15, 2021

tfranzel left a comment

tfranzel Oct 31, 2021

tfranzel Oct 31, 2021

ngnpope commented Nov 1, 2021

tfranzel commented Apr 21, 2022

		if example.status_codes is None:
		example.status_codes = [status_code]

Improve handling of status codes. #573

Improve handling of status codes. #573

Conversation

ngnpope commented Oct 14, 2021

codecov bot commented Oct 14, 2021 • edited Loading

Codecov Report

ngnpope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngnpope Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngnpope commented Oct 15, 2021

tfranzel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngnpope commented Nov 1, 2021

tfranzel commented Apr 21, 2022

codecov bot commented Oct 14, 2021 •

edited

Loading

ngnpope Oct 14, 2021 •

edited

Loading