search

yezyilomo / django-restql

Turn your API made with Django REST Framework(DRF) into a GraphQL like API.
https://yezyilomo.github.io/django-restql
MIT License
616 stars 43 forks source link

`fields` kwarg conflicts with a FK NestedField with `accept_pk` kwarg when updating #169

Closed resurrexi closed 4 years ago

resurrexi commented 4 years ago

I'm running into an issue when PATCHing or PUTing a resource with a nested serializer. For brevity, here's an example:

class EmployeeJobDetailSerializer(DynamicFieldsMixin, NestedModelSerializer):
    employee = NestedField(
        EmployeeSerializer,
        accept_pk=True,
        fields=['employee_id', 'first_name', 'last_name']
    )

    class Meta:
        model = EmployeeJobDetail
        fields = ...

If I'm not mistaken, accept_pk=True essentially converts the nested serializer to a PrimaryKeyRelatedField when creating/updating. However, PrimaryKeyRelatedField does not recognize the fields argument so it throws an error: TypeError: __init__() got an unexpected keyword argument 'fields'.

Is there a way to somehow ignore fields when updating with accept_pk=True? GET works perfectly and only selects the fields that I have specified for the kwarg.

yezyilomo commented 4 years ago

Does your EmployeeSerializer inherit DynamicFieldsMixin?.

resurrexi commented 4 years ago

Does your EmployeeSerializer inherit DynamicFieldsMixin?.

Yes

I actually looked through the source code, and I think the fix is here. There's actually a comment a couple lines above to find all non-validation kwargs, and those were just a few. Adding fields and exclude to the list seemed to have resolved the issue, and all my other API tests still passed.

Can't say for sure if the change will affect the tests associated with the package. I could try and make the fix on the package and run its tests.

yezyilomo commented 4 years ago

Ooooh yeah, now I remember I marked it as TODO :laughing:, We should add fields, exclude, return_pk & disable_dynamic_fields kwargs there.

resurrexi commented 4 years ago

Wait a second, I take back what I just said. The tests passed because I had commented out the fields kwarg in the nested serializer. Adding fields and exclude didn't resolve the issue, but I'm looking at another line that may potentially be the issue.

yezyilomo commented 4 years ago

The tests passed to me.

resurrexi commented 4 years ago

Ok, I'm so stupid... Apparently, I forgot I was running my app in docker, and my tests were still using the old code... Silly me.

Anyways, while looking through the code again, I noticed PrimaryKeyRelatedField appears in line 94 without the **self.validation_kwargs argument but appears in line 243. Is there a reason for the difference?

resurrexi commented 4 years ago

So I added fields and exclude to the list and I got a new error now. Full stacktrace:

Traceback (most recent call last):
  File "/opt/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/opt/venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 115, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/opt/venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 113, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/opt/venv/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/viewsets.py", line 114, in view
    return self.dispatch(request, *args, **kwargs)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/views.py", line 505, in dispatch
    response = self.handle_exception(exc)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/views.py", line 465, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/views.py", line 476, in raise_uncaught_exception
    raise exc
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/views.py", line 502, in dispatch
    response = handler(request, *args, **kwargs)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/mixins.py", line 82, in partial_update
    return self.update(request, *args, **kwargs)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/mixins.py", line 68, in update
    self.perform_update(serializer)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/mixins.py", line 78, in perform_update
    serializer.save()
  File "/code/erp/api/serializers.py", line 19, in save
    return super().save(updated_by=current_user)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/serializers.py", line 207, in save
    self.instance = self.update(self.instance, validated_data)
  File "/opt/venv/lib/python3.8/site-packages/django_restql/mixins.py", line 939, in update
    return super().update(instance, validated_data)
  File "/opt/venv/lib/python3.8/site-packages/rest_framework/serializers.py", line 990, in update
    setattr(instance, attr, value)
  File "/opt/venv/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 206, in __set__
    raise ValueError(
ValueError: Cannot assign "<class 'rest_framework.fields.empty'>": "EmployeeJobDetail.employee" must be a "Employee" instance.

Note, I'm PATCHing the resource, so I'm only providing some fields, not all fields. employee was not one of the fields that I provided since it didn't need to be updated.

resurrexi commented 4 years ago

Update! Finally got it working! The fix was to include fields and exclude, along with the other kwargs you mentioned!

For some reason, I had to copy your mixins.py and serializers.py files and reference the classes from the copied files in my code to get rid of that second error!

If you want, I can make the fixes and add those nonvalidation kwargs and make a PR. :)

andis-roze commented 4 years ago

Nice to see an activity here. On an unrelated topic: should I post a bug report if I have problem which doesn't necessarily is a bug? Sorry once more for the off-topic.

yezyilomo commented 4 years ago

Update! Finally got it working! The fix was to include fields and exclude, along with the other kwargs you mentioned!

Good, just to make it clear, below is a snapshot of one part which use validation_kwargs https://github.com/yezyilomo/django-restql/blob/25e1a5c276efc23622614055453a6f75d0d1d0f4/django_restql/mixins.py#L745-L757 As you can see, we are calling the actual serializer there, and pass data, context & partial so they shouldn't be part of kwargs that's why we were removing them, we should also remove all kwargs introduced by Django RESTQL(DynamicFieldsMixin) because the serializer doesn't recognize them.

yezyilomo commented 4 years ago

If you want, I can make the fixes and add those nonvalidation kwargs and make a PR. :)

Please do, will gladly accept it, but don't remove this comment # TODO: Find all non validation related kwargs to remove(below are just few) because am still not sure if we've included all non validation related fields.

yezyilomo commented 4 years ago

should I post a bug report if I have problem which doesn't necessarily is a bug?

Just open an issue and explain if it's a bug, feature suggestion or anything else. You are welcome @andis-roze

resurrexi commented 4 years ago

Please do, will gladly accept it, but don't remove this comment # TODO: Find all non validation related kwargs to remove(below are just few) because am still not sure if we've included all non validation related fields.

Will do.

On a side note, would you have any gripes if the code ended up being formatted by black? My code editor is currently configured to run black on save. It will still be PEP8 compliant and line length won't exceed 79 for regular code and 72 for comments/docstrings.

resurrexi commented 4 years ago

I made my changes to the code but I'm having a little bit of trouble running runtests.py.

ImportError: Could not import 'rest_framework_filters.backends.RestFrameworkFilterBackend' for API setting 'DEFAULT_FILTER_BACKENDS'. ImportError: cannot import name 'QUERY_TERMS' from 'django.db.models.sql.constants' (/home/leeekwon/.cache/pypoetry/virtualenvs/django-restql-GkQ9am-6-py3.7/lib/python3.7/site-packages/django/db/models/sql/constants.py).

Not too sure what I'm doing wrong here. Here is my env info:

python==3.7.6 django==2.2 djangorestframework==3.11.0 djangorestframework-filters==0.11.1 pypeg2==2.15.2

yezyilomo commented 4 years ago

On a side note, would you have any gripes if the code ended up being formatted by black? My code editor is currently configured to run black on save. It will still be PEP8 compliant and line length won't exceed 79 for regular code and 72 for comments/docstrings.

In my opinion black is great and yes it makes code consistent but sometimes it makes readability suffer because it enforces even things which are not rules(i.e PEP 8 does not enforce them). It would be great if they could at least allow configuring it so that people could ignore some checks, but it would go against being consistent which is their selling point. I prefer using flake8 and autopep8 which gives me the freedom to choose what to check and reformat.

yezyilomo commented 4 years ago

djangorestframework-filters==0.11.1

https://github.com/yezyilomo/django-restql/blob/25e1a5c276efc23622614055453a6f75d0d1d0f4/tox.ini#L24

This is the version of djangorestframework-filter used in testing

resurrexi commented 4 years ago

djangorestframework-filters==0.11.1

Gotcha. I was looking at the requirements.txt file. I just ran the tests and everything passed but it looks like you already made a commit to fix this issue. However, I noticed in your commit, you missed the query kwargs. I'm not sure if that's still being used the DynamicFieldsMixin, but I saw it being referenced in a comment.

In my opinion black is great and yes it makes code consistent but sometimes it makes readability suffer because it enforces even things which are not rules(i.e PEP 8 does not enforce them). It would be great if they could at least allow configuring it so that people could ignore some checks, but it would go against being consistent which is their selling point. I prefer using flake8 and autopep8 which gives me the freedom to choose what to check and reformat.

Thats true. Black can be rather opinionated with how it formats. However, I haven't run into problems where it made readability suffer. :scream:

yezyilomo commented 4 years ago

However, I noticed in your commit, you missed the query kwargs. I'm not sure if that's still being used the DynamicFieldsMixin,

Oooh I forgot that, let me fix it, thanks for reminding me.

resurrexi commented 4 years ago

No problem! Is it possible to publish the updated package to pypi? That way, I don't have to use my local edits to the copy-and-pasted files :laughing:

yezyilomo commented 4 years ago

No problem! Is it possible to publish the updated package to pypi? That way, I don't have to use my local edits to the copy-and-pasted files

Yeah fixes should be published right away. am on it.