fields kwarg conflicts with a FK NestedField with accept_pk kwarg when updating

[resurrexi]

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]

Does your EmployeeSerializer inherit DynamicFieldsMixin?.

[resurrexi]

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]

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

[resurrexi]

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]

The tests passed to me.

[resurrexi]

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]

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]

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]

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]

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

django-restql/django_restql/mixins.py

Lines 745 to 757 in 25e1a5c

	# Get nested field serializer
	serializer = nested_fields[field]
	serializer_class = type(serializer)
	kwargs = serializer.validation_kwargs
	nested_obj = getattr(instance, field)
	serializer = serializer_class(
	nested_obj,
	**kwargs,
	data=values,
	context=self.context,
	partial=serializer.is_partial
	)
	serializer.is_valid()

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]

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]

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]

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]

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]

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]

djangorestframework-filters==0.11.1

django-restql/tox.ini

Line 24 in 25e1a5c

djangorestframework-filters>=1.0.0.dev0

This is the version of djangorestframework-filter used in testing

[resurrexi]

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. 😱

[yezyilomo]

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]

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 😆

[yezyilomo]

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.