Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release 7.0.0 breaks sparse fields request #1228

Closed
n2ygk opened this issue May 30, 2024 · 9 comments · Fixed by #1231
Closed

Release 7.0.0 breaks sparse fields request #1228

n2ygk opened this issue May 30, 2024 · 9 comments · Fixed by #1231
Labels

Comments

@n2ygk
Copy link
Contributor

n2ygk commented May 30, 2024

Description of the Bug Report

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

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:

* 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
@n2ygk n2ygk added the bug label May 30, 2024
@sliverc
Copy link
Member

sliverc commented May 31, 2024

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
Copy link
Contributor Author

n2ygk commented May 31, 2024 via email

@sliverc
Copy link
Member

sliverc commented May 31, 2024

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

@n2ygk
Copy link
Contributor Author

n2ygk commented May 31, 2024 via email

@sliverc
Copy link
Member

sliverc commented Jun 1, 2024

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

@n2ygk
Copy link
Contributor Author

n2ygk commented Jun 4, 2024

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

@sliverc
Copy link
Member

sliverc commented Jun 4, 2024

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
Copy link
Contributor Author

n2ygk commented Jun 4, 2024

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
Copy link
Member

sliverc commented Jun 4, 2024

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

sliverc added a commit to sliverc/django-rest-framework-json-api that referenced this issue Jun 6, 2024
@sliverc sliverc mentioned this issue Jun 6, 2024
5 tasks
n2ygk pushed a commit that referenced this issue Jun 6, 2024
New release as for regression #1228
n2ygk added a commit to columbia-it/django-jsonapi-training that referenced this issue Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants