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

Adds error handling for badly formed UUIDs sent to API endpoints #11009

Merged
merged 6 commits into from
Aug 4, 2023

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jul 25, 2023

Summary

  • Uses django_filters filtering where possible for filtering by UUID fields
  • Adds default UUIDFilter for morango UUIDField
  • Handles ValueErrors arising during database queries to return the appropriate response that isn't a 500

References

Fixes #10999 - actual Sentry reports includes multiple endpoints, all of which should be addressed here (some of which are addressed by previous work).

Reviewer guidance

Where needed I have added additional test coverage, but most of this is just defensive programming.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Jul 25, 2023
@rtibbles rtibbles requested review from jredrejo and akolson July 25, 2023 20:44
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: medium labels Jul 25, 2023
@rtibbles rtibbles changed the base branch from develop to release-v0.16.x August 2, 2023 19:09
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is fine, some parts not easy to understand for me as they're clearly a special and smart use of the Django features. Added tests are pretty clear though.
I see no problems in the code and everything went well in my tests. I haven't tested in Postgresql where UUID format is a bit different but I trust Django folks have kept that in mind.

# maps to this filter when using the UUIDField in a filter.
from morango.models import UUIDField

FilterSet.FILTER_DEFAULTS.update({UUIDField: {"filter_class": UUIDFilter}})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about the existence of FILTER_DEFAULTS nor I could find it in the documentation https://django-filter.readthedocs.io/
I only have found some references in the django-filter forums to underestand how it worked. Looking at the code makes sense, and also, if I understand it correctly the place in the code where you added it forces it to be active during the execution of the whole django request. is that interpretation correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - it does not appear to be documented, which is unfortunate, but it seemed to be the only way to set this global mapping from Field type to Filter class.

The other alternative would be to subclass FilterSet and then systematically use that for all our FilterSets (although, this would not work for automatically generated FilterSets using DRF's magic: https://www.django-rest-framework.org/api-guide/filtering/#djangofilterbackend as there doesn't seem to be a setting to set the default FilterSet class).

Setting it here seems to be early enough in the Django initialization sequence that it is now set for any derived class - because of the import path from morango, it needs to happen after Django app initialization, because of access to the models module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: medium TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle badly formed UUIDs as 404s for ContentNodeTree endpoint
2 participants