When unique constraint is violated status code is 409

[vdusek]

closes #3846
https://pulp.plan.io/issues/3846

[pep8speaks]

Hello @vdusek! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on August 28, 2018 at 08:07 Hours UTC

[pulpbot]

Can one of the admins verify this patch?

[vdusek]

This PR defines new exception to return 409 for uniqueness error. The test fails in the is_valid method because it catches or re-raises only generic ValidationError. So the exception I introduced is not handled properly. One of the solutions is to define our own is_valid method. But it doesn't seem to be a good solution.
Any other thoughts or suggestions?

[dkliban]

@vdusek I would write a custom is_valid method tht checks for the 409 and reraises it. If not a 409 it should just call into the super is_valid.

[vdusek]

Now, when unique constraint is violated the response looks like this:

HTTP/1.1 409 Conflict
...
{
    "detail": "Conflict with the current state of the target resource."
}

Before the commit, with the ValidationError exception, it looked like this:

HTTP/1.1 400 Bad Request
...
{
    "name": [
        "This field must be unique."
    ]
}

So the different format of the detail message is probably the reason why this test didn't pass. I spent a lot of time on figuring out how to format it in a right way but unsuccessfully. So if anyone has any idea how to do it, I'll be glad.

[dparalen]

Hi,

thanks for the PR!
Please rebase it on master, either after your fix commit b1f45aa or on head

Thanks,
milan

[codecov]

Codecov Report

Merging #3569 into master will increase coverage by 0.62%.
The diff coverage is 91.83%.

@@            Coverage Diff             @@
##           master    #3569      +/-   ##
==========================================
+ Coverage    57.7%   58.33%   +0.62%     
==========================================
  Files          60       61       +1     
  Lines        2523     2568      +45     
==========================================
+ Hits         1456     1498      +42     
- Misses       1067     1070       +3

Impacted Files	Coverage Δ
pulpcore/pulpcore/app/serializers/repository.py	94.48% <100%> (ø)	⬆️
pulpcore/pulpcore/app/serializers/user.py	80% <100%> (ø)	⬆️
pulpcore/pulpcore/app/validators.py	100% <100%> (ø)
pulpcore/pulpcore/app/serializers/content.py	62.22% <50%> (ø)	⬆️
pulpcore/pulpcore/exceptions/http.py	73.07% <85.71%> (+14.74%)	⬆️
pulpcore/pulpcore/app/serializers/base.py	66.88% <95%> (+4.29%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a175c11...8eee132. Read the comment docs.

[vdusek]

Updated, checks finally passed. :) However it's not a final version, I'm going to open a PR on rest framework for a better solution.

[vdusek]

Opened issue - encode/django-rest-framework#6124 and PR - encode/django-rest-framework#6126 on DRF. However it doesn't seem like we're getting to a solution.

[CodeHeeler]

Thank you for pursuing the upstream option first, but as that isn't getting traction, your custom solution looks good, approved! (You'll still need someone w/ commit bit to approve in order to merge.)

[daviddavis]

I'm a bit hesitant on merging this PR. I don't think returning a 400 in cases where there is a uniqueness violation is incorrect. Moreover, I see a few downsides if we merge this:

• This adds complexity which we will have to carry and maintain in our codebase
• I think DRF upstream had a good point. What if there are two invalidations with one being a 409 and the other one being a 400?
• Lastly, I think plugin writers will overlook this and just end up returning 400s for uniqueness violations which will lead to inconsistencies. In fact, it doesn't look like this PR exposes PulpUniqueValidator as part of the plugin API.

[vdusek]

@daviddavis You're probably right. The code isn't good, added value is not much and it could cause troubles in the future. I spent lot of time working on this issue but at least I learned something new. :) So I'll close this PR and the issue.

[daviddavis]

@vdusek thanks for your hard work on this issue. Glad you were able to learn something too. 👍