JSON:API 1.1: return empty 'included' array when ?include= is present #1301
Conversation
Fixes django-json-api#1109. Ensures top-level 'included' is returned as [] when the query param ?include= is provided but no related resources exist. Updates integration tests to reflect JSON:API v1.1 spec compliance.
|
Fixed tox lint config issue. Also, verified locally with tox -e lint and all checks passing. |
There was a problem hiding this comment.
Thanks for working on this PR.
I would recommend being careful when you use AI. Always verify what it does, before accepting it.
For instance the pull request template and its checklist is actually made for human beings and not AI.
In the checklist it states to add changelog entry and add yourself to authors. Great if you could follow up on those.
Let me know if you have any questions.
.gitignore
Outdated
| @@ -43,6 +43,9 @@ coverage.xml | |||
|
|
|||
| # VirtualEnv | |||
| .venv/ | |||
| venv/ | |||
There was a problem hiding this comment.
Please revert changes to .gitignore. The DJA documentation states to use env folder. Actually .venv is already additional, I want to avoid that we add more.
In case you want to use venv naming locally, you can always ignore it in your local in the file .git/info/exclude.
.gitignore
Outdated
| @@ -51,3 +54,7 @@ manage.py | |||
|
|
|||
| # example database | |||
| drf_example | |||
|
|
|||
| pytest.ini | |||
There was a problem hiding this comment.
not sure why this is added please revert.
rest_framework_json_api/renderers.py
Outdated
| @@ -659,6 +659,10 @@ def render(self, data, accepted_media_type=None, renderer_context=None): | |||
| render_data["included"].append( | |||
| included_cache[included_type][included_id] | |||
| ) | |||
| else: | |||
There was a problem hiding this comment.
there is actually another feature with includes resources by default which was not mentioned in the original issues, but also needs to be considered when deciding whether an empty include array should be added or not.
Actually the whole code can be simplified. This else is not needed but in case the if needs to be adjusted to
if included_resources:
this will cover the cases of includes per default and per request query param as well.
setup.cfg
Outdated
| @@ -20,8 +20,9 @@ exclude = | |||
| build/lib, | |||
| .eggs | |||
| .tox, | |||
| env | |||
| .venv | |||
| env, | |||
There was a problem hiding this comment.
as mentioned above please revert.
tox.ini
Outdated
| @@ -47,3 +47,6 @@ deps = | |||
| -rrequirements/requirements-documentation.txt | |||
| commands = | |||
| sphinx-build -W -b html -d docs/_build/doctrees docs docs/_build/html | |||
|
|
|||
There was a problem hiding this comment.
not so sure what empty lines shoud be fixed here? I have the feeling they rather should be reverted.
|
Thanks for the feedback @sliverc. I’ll make sure to review my changes instead of relying on AI. |
|
Thanks for following up. You need to adjust the test_search_keywords test because this test relies on a view which has default included resources, so there should be an empty-included array. |
Fixes #1109
This PR fixes handling of the included array so that it correctly respects both JSONAPIMeta.included_resources defaults and explicit include query parameters.
Description of the Change
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS