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

Validate 'sort' field in API requests #1869

Merged
merged 2 commits into from May 10, 2018

Conversation

Projects
None yet
2 participants
@toolness
Contributor

toolness commented May 8, 2018

Fixes #257.

Notes

  • This works by introspecting on the db_index attribute of every field in the Contract model, ensuring that whatever we're indexing is a sortable field.

  • We're raising a Django REST Framework ValidationError when an invalid sort field is provided. This is a bit funky: as DRF's documentation states, it's supposed to only be raised by DRF serializers and field validators, but because CALC 1's view code is very unconventional, it doesn't use either. So we're just raising it in the actual view, which nonetheless seems to work out okay (tests verify this, so we'll be notified if there's ever a regression).

  • I've added indexes on idv_piid and vendor_name. We'll probably need the latter anyways for supporting vendor search (which I think was brought up as a potential feature, though I can't find a GH issue for it at the moment). However, idv_piid was only really added to ensure tests didn't break, and I'm not sure whether it'll be useful in practice or not. That said, there is a comment next to the field that says "index this field", despite the fact that it wasn't indexed before, so 🤷‍♂️ . Both fields are definitely needed because the front-end allows them both to be sorted!

@toolness toolness requested a review from hbillings May 8, 2018

@@ -2,15 +2,15 @@
from django.test import TestCase
from .test_rates_api import GetRatesTests
from . import test_rates_api

This comment has been minimized.

@toolness

toolness May 8, 2018

Contributor

Changed this to import the module rather than the class: when we import the class, pytest thinks it needs to be run as a test suite, which means that the tests are run twice.

@toolness toolness referenced this pull request May 10, 2018

Merged

Add data_quality_report management command and web view. #1875

4 of 6 tasks complete
@hbillings

👌

resp = self.client.get(f'{self.path}/?sort=blarg')
self.assertEqual(resp.status_code, 400)
self.assertEqual(resp.json(), [
'"blarg" is not a valid field to sort on'

This comment has been minimized.

@hbillings

hbillings May 10, 2018

Member

why not?? poor blargh never has any fun.

@@ -216,11 +217,21 @@ def get_contracts_queryset(request_params, wage_field):
price = request_params.get('price', None)
price__gte = request_params.get('price__gte')
price__lte = request_params.get('price__lte')
sort = request_params.get('sort', wage_field)
sort = request_params.get('sort', wage_field).split(',')

This comment has been minimized.

@hbillings

hbillings May 10, 2018

Member

I'm a little unsure of what the split is doing here. (I looked at a sample request at api/docs, and I didn't see a wage_field, er, field returned, so I'm not sure what it's supposed to be.)

This comment has been minimized.

@toolness

toolness May 10, 2018

Contributor

Good catch--the docs for sort just say "the column to sort on", but in reality this can be multiple fields, separated by commas. So we should update the docs to reflect this. Filed #1889 to address it later.

@toolness toolness merged commit d34cd29 into develop May 10, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 92% (0.1% change)
Details

@toolness toolness deleted the api-sort-validation branch May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment