django-json-api/django-rest-framework-json-api

Release 7.0.0 breaks sparse fields request

n2ygk opened this issue · 9 comments

n2ygk commented

Description of the Bug Report

As of release 7.0.0 a sparse fields request throws a KeyError for url at

django-rest-framework-json-api/rest_framework_json_api/renderers.py

Lines 485 to 486 in 366769e

if api_settings.URL_FIELD_NAME in resource and isinstance(
fields[api_settings.URL_FIELD_NAME], relations.RelatedField
This was working up through release 6.1.0.

I expect that this is related to this change identified in the CHANGELOG:

django-rest-framework-json-api/CHANGELOG.md

Line 27 in 366769e

* Adjusted that sparse fields properly removes meta fields when not defined.

How to reproduce:

git clone https://github.com/columbia-it/django-jsonapi-training
cd django-jsonapi-training
python -m venv venv
source venv/bin/activate
pip install -r requirements.txt
pip freeze | grep json
# see the version is djangorestframework-jsonapi==7.0.0
./manage.py migrate
./manage.py loaddata myapp/fixtures/*yaml
openssl genrsa -out oidc.key 4096
OIDC_RSA_PRIVATE_KEY_FILE=oidc.key ./manage.py runserver

Use Postman with OAuth2 as described here:

Token Name: *pick a name*
Grant Type: Authorization Code (With PKCE)
Callback URL: https://www.getpostman.com/oauth2/callback
Auth URL: http://localhost:8000/authorize/
Access Token URL: http://localhost:8000/token/
Client ID: demo_djt_web_client
Client Secret: demo_djt_web_secret
Scope: auth-columbia demo-djt-sla-bronze read openid profile email https://api.columbia.edu/scope/group
Client Authentication: Send as Basic Auth header

Click Get New Access Token and login as user admin pass admin123 and Authorize then proceed and Use Token

Send GET http://localhost:8000/v1/courses/?fields[courses]=course_name

observe this error:

  File "/Users/ac45/src/django-jsonapi-training/venv/lib/python3.12/site-packages/rest_framework_json_api/renderers.py", line 486, in build_json_resource_obj
    fields[api_settings.URL_FIELD_NAME], relations.RelatedField
    ~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'url'

Then send GET http://localhost:8000//v1/courses/?fields[courses]=course_name,url

and get back a correct JSONAPI response.

Now ^C the runserver, downgrade to v6.1.0, and try again.

pip install djangorestframework-jsonapi==6.1.0
OIDC_RSA_PRIVATE_KEY_FILE=oidc.key ./manage.py runserver

Send GET http://localhost:8000/v1/courses/?fields[courses]=course_name

and get back a correct JSONAPI response.

## Checklist

- [x] Certain that this is a bug (if unsure or you have a question use [discussions](https://github.com/django-json-api/django-rest-framework-json-api/discussions) instead)
- [x] Code snippet or unit test added to reproduce bug
sliverc commented

Thanks for the heads up. It seems in PR #1221 the url gets filtered out in the renderer but kept in the readable fields on the serialzer. Also actually id should be kept for overwriting id on a serialzer basis and not being filtered out when using sparse fields.

Strange that the tests did not catch it. Do you see what the difference of your view and our sparse fieldset test could be?

I will certainly need to analyze it more closely.

n2ygk commented
  • 👍1
sliverc commented

I think the issue is the HyperlinkedModelSerializer which is not tested in our test suite :sniff:

n2ygk commented
sliverc commented

sure sounds good. In case I will get to it earlier I will let you know.

n2ygk commented

@sliverc I've been staring at these tests for a while and can't come up with a proper breaking test. Any suggestions?

sliverc commented

In the tests/tests_views.py we could add another serializer/view at the bottom to the test routing whereas the serializer is derived from HyperlinkedModelSerializer, which I would hope should expose the issue?

I might also see whether I get some more time to look into it later on today.

n2ygk commented

In the tests/tests_views.py we could add another serializer/view at the bottom to the test routing whereas the serializer is derived from HyperlinkedModelSerializer, which I would hope should expose the issue?

I might also see whether I get some more time to look into it later on today.

Yeah I tried a few permutations of that but I think it also has to have a ResourceRelatedField and a ForeignKey model and using sparse fieldsets query.... I can try some more.

sliverc commented

I have some time at hand, let me give it a shot and I will let you know.

  • 👍1