fixing unique together validator for fields with source

[anveshagarwal]

Description

As described in the issue below , the fields which are read_only + default are excluded from writable_fields and they are under _read_only_defaults function of serializer
https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py#L439
now in this function ,here in defaults orderdict() fields.field_name is set as key. So the problem is that if the field_name is different from the field in the model(with which the source keyword is filled), uniquetogether validator throws a field is required error as it is not able to map the source.

refs##7003

[rpkilby]

Hi @anveshagarwal. Could you add a test case that demonstrates the failure?

[anveshagarwal]

Hi @rpkilby i added the testcase

[rpkilby]

Hi @anveshagarwal. Please take this feedback constructively, but there is a lot wrong with the PR. I assume a lot of the issues below would have been caught, however, because test_read_only_default doesn't contain an __init__.py, it's not a proper python package. So, the test module isn't discovered and the tests aren't executed. This is why it's often suggested that you write your tests before the fix.

If you write the test first and see the failure, you know two things:

• the test was executed
• the test demonstrates the failure

If you fix the bug first then write the tests second, you can't guarantee the above.

[anveshagarwal]

@rpkilby thanks a lot for your valuable feedback. I learn a lot from it. I am pretty new in Development which led to these silly mistakes. This is my first error reporting to any open source. Will keep all the points you mentioned in my further approaches. And yes i had a time crunch to report these so i didnt approach it well enough. Thanks again.

[rpkilby]

Of course, happy to provide feedback.

[anveshagarwal]

@rpkilby i am not able to get the serializer class from which i can get the source name of the field

[anveshagarwal]

hey @rpkilby as you can see in my changes i am getting serializer calss from set_context function but in the only test failing filter_queryset is called directly so there i cant get the serializer class. But in the implementation set_context is always called first. So these changes are working(solves bugs) in my existing work but test is failing here.

[anveshagarwal]

need help on this. Want to resolve it.

[auvipy]

https://docs.djangoproject.com/en/dev/ref/models/options/#unique-together

Use UniqueConstraint with the constraints option instead.

UniqueConstraint provides more functionality than unique_together. unique_together may be deprecated in the future.

[anveshagarwal]

@auvipy but that is to be used in models? isnt it? How that will help me not to get this bug in serializer's uniquetogethervalidatior? How will i get serializer level validation without this bug from solution you suggested? Wont this bug will occur if i use UniqueConstraint with the constraints option instead?

[rpkilby]

@anveshagarwal The suggestion is about future-proofing the tests, not fixing the bug. i.e., if UniqueConstraint is preferred over Meta.unique_together, with the latter potentially being deprecated, it would make sense to go ahead and use the constraints option. That said, UniqueConstraint was only added in Django 2.2, so we're not able to use it yet since we still support Django 1.11

Also, I'm taking another stab at this, but am currently waiting on #7076 which is a followup to some validator changes.

[auvipy]

rebase needed

[rpkilby]

Hi @anveshagarwal. I'm closing this in favor of #7086. Thanks for getting this started - you definitely found quite the obscure bug 😄

[anveshagarwal]

@rpkilby hey thanks! and i really wanted to contribute by having some commits in the DRF :) But all i am happy to contribute in anyway. And if i would have looked to the changes done after removing set_context implementation ,I might have solved the part i was stuck on :)