Fix OpenAPI docs generation to include missing response headers when pagination is used #579
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #579 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 14 14
Lines 885 885
Branches 192 192
=======================================
Hits 884 884
Partials 1 1 ☔ View full report in Codecov by Sentry. |
flask_smorest/pagination.py
Outdated
| } | ||
| if "headers" not in resp_doc: | ||
| resp_doc["headers"] = {} | ||
| resp_doc["headers"].update( |
There was a problem hiding this comment.
I prefer the setdefault form. That's the form I use everywhere in the code.
resp_doc.setdefault("headers", {}).update(...)
tests/test_spec.py
Outdated
| @@ -178,6 +178,29 @@ def test_get(val): | |||
| "schema": {"$ref": "#/components/schemas/PaginationMetadata"}, | |||
| } | |||
|
|
|||
| def test_all_headers_are_documented(self, app): | |||
There was a problem hiding this comment.
I'm tempted to add a comment such as
# Non-regression test for https://github.com/marshmallow-code/flask-smorest/issues/578to make the test intent more explicit.
There was a problem hiding this comment.
I did add a #noqa: E501 to stop flake8 complaining about the length of that line.
tests/test_spec.py
Outdated
| @@ -178,6 +178,29 @@ def test_get(val): | |||
| "schema": {"$ref": "#/components/schemas/PaginationMetadata"}, | |||
| } | |||
|
|
|||
| def test_all_headers_are_documented(self, app): | |||
There was a problem hiding this comment.
We could put this in pagination tests and make the name reflect the fact that we're checking other headers are not dropped as a non-reg test for this specific issue.
I'd be happy to enlarge the scope to check all sorts of combination and leave the test here. But this is not what we do here and I'm not sure it's worth trying.
For now, I'd move this to test_pagination.py and call it something like test_pagination_doc_preservers_other_headers_doc.
There was a problem hiding this comment.
Funilly enough, I first had that test in test_pagination.py, but then moved it because I felt it better fit in test_spec.py since it was about documentation. In any case, I moved and renamed it.
|
I have applied all suggestions, but did not yet squash the branch so you can check it more easily. I'll do that once you are happy with the changes. |
tests/test_pagination.py
Outdated
| @@ -479,6 +479,39 @@ def func(): | |||
| assert parameters[3]["schema"]["minimum"] == 1 | |||
| assert parameters[3]["schema"]["maximum"] == 100 | |||
|
|
|||
| # Non-regression test for https://github.com/marshmallow-code/flask-smorest/issues/578 # noqa: E501 | |||
| @pytest.mark.parametrize("openapi_version", ("2.0", "3.0.2")) | |||
| def test_pagination_doc_preservers_other_headers_doc(self, app, openapi_version): | |||
…pagination is used
drcpu-github
force-pushed
the
openapi-doc-list-headers
branch
from
ebe9bcc to
05e34a2
Compare
Done! |
|
Released in 0.42.3. Thanks! |
PR for issue #578.
Added the code I showed in the issue and a simple test to show that it works. Not sure if you expect more from a test than this, but let me know.