From ff9fb9bb067128309a151be16d760020e9a2fcaf Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 12 Jun 2017 15:10:25 -0400 Subject: [PATCH] Add POST method to CourseSummariesView POST method allows large number of course ID arguments to be passed as data, while GET method is restricted by URL length. EDUCATOR-464 --- AUTHORS | 1 + analytics_data_api/v0/tests/views/__init__.py | 36 +++++++++++----- .../v0/tests/views/test_course_summaries.py | 11 ++--- analytics_data_api/v0/views/__init__.py | 42 +++++++++++++++++-- .../v0/views/course_summaries.py | 34 ++++++++++++++- analyticsdataserver/tests.py | 10 ++++- 6 files changed, 112 insertions(+), 22 deletions(-) diff --git a/AUTHORS b/AUTHORS index dc3a9771..bd498c64 100644 --- a/AUTHORS +++ b/AUTHORS @@ -9,3 +9,4 @@ Jason Bau John Jarvis Dmitry Viskov Eric Fischer +Kyle McCormick diff --git a/analytics_data_api/v0/tests/views/__init__.py b/analytics_data_api/v0/tests/views/__init__.py index d10030fb..16f70b7e 100644 --- a/analytics_data_api/v0/tests/views/__init__.py +++ b/analytics_data_api/v0/tests/views/__init__.py @@ -100,15 +100,29 @@ class APIListViewTestMixin(object): list_name = 'list' default_ids = [] always_exclude = ['created'] + test_post_method = False - def path(self, ids=None, fields=None, exclude=None, **kwargs): - query_params = {} - for query_arg, data in zip([self.ids_param, 'fields', 'exclude'], [ids, fields, exclude]) + kwargs.items(): - if data: - query_params[query_arg] = ','.join(data) - query_string = '?{}'.format(urlencode(query_params)) + def path(self, query_data=None): + query_data = query_data or {} + concat_query_data = {param: ','.join(arg) for param, arg in query_data.items() if arg} + query_string = '?{}'.format(urlencode(concat_query_data)) if concat_query_data else '' return '/api/v0/{}/{}'.format(self.list_name, query_string) + def validated_request(self, ids=None, fields=None, exclude=None, **extra_args): + params = [self.ids_param, 'fields', 'exclude'] + args = [ids, fields, exclude] + data = {param: arg for param, arg in zip(params, args) if arg} + data.update(extra_args) + + get_response = self.authenticated_get(self.path(data)) + if self.test_post_method: + post_response = self.authenticated_post(self.path(), data=data) + self.assertEquals(get_response.status_code, post_response.status_code) + if 200 <= get_response.status_code < 300: + self.assertEquals(get_response.data, post_response.data) + + return get_response + def create_model(self, model_id, **kwargs): pass # implement in subclass @@ -134,19 +148,19 @@ def all_expected_results(self, ids=None, **kwargs): def _test_all_items(self, ids): self.generate_data() - response = self.authenticated_get(self.path(ids=ids, exclude=self.always_exclude)) + response = self.validated_request(ids=ids, exclude=self.always_exclude) self.assertEquals(response.status_code, 200) self.assertItemsEqual(response.data, self.all_expected_results(ids=ids)) def _test_one_item(self, item_id): self.generate_data() - response = self.authenticated_get(self.path(ids=[item_id], exclude=self.always_exclude)) + response = self.validated_request(ids=[item_id], exclude=self.always_exclude) self.assertEquals(response.status_code, 200) self.assertItemsEqual(response.data, [self.expected_result(item_id)]) def _test_fields(self, fields): self.generate_data() - response = self.authenticated_get(self.path(fields=fields)) + response = self.validated_request(fields=fields) self.assertEquals(response.status_code, 200) # remove fields not requested from expected results @@ -158,10 +172,10 @@ def _test_fields(self, fields): self.assertItemsEqual(response.data, expected_results) def test_no_items(self): - response = self.authenticated_get(self.path()) + response = self.validated_request() self.assertEquals(response.status_code, 404) def test_no_matching_items(self): self.generate_data() - response = self.authenticated_get(self.path(ids=['no/items/found'])) + response = self.validated_request(ids=['no/items/found']) self.assertEquals(response.status_code, 404) diff --git a/analytics_data_api/v0/tests/views/test_course_summaries.py b/analytics_data_api/v0/tests/views/test_course_summaries.py index 0e2dc9d4..37b01c63 100644 --- a/analytics_data_api/v0/tests/views/test_course_summaries.py +++ b/analytics_data_api/v0/tests/views/test_course_summaries.py @@ -22,6 +22,7 @@ class CourseSummariesViewTests(VerifyCourseIdMixin, TestCaseWithAuthentication, list_name = 'course_summaries' default_ids = CourseSamples.course_ids always_exclude = ['created', 'programs'] + test_post_method = True def setUp(self): super(CourseSummariesViewTests, self).setUp() @@ -135,7 +136,7 @@ def test_fields(self, fields): ) def test_empty_modes(self, modes): self.generate_data(modes=modes) - response = self.authenticated_get(self.path(exclude=self.always_exclude)) + response = self.validated_request(exclude=self.always_exclude) self.assertEquals(response.status_code, 200) self.assertItemsEqual(response.data, self.all_expected_results(modes=modes)) @@ -144,13 +145,13 @@ def test_empty_modes(self, modes): [CourseSamples.course_ids[0], 'malformed-course-id'], ) def test_bad_course_id(self, course_ids): - response = self.authenticated_get(self.path(ids=course_ids)) + response = self.validated_request(ids=course_ids) self.verify_bad_course_id(response) def test_collapse_upcoming(self): self.generate_data(availability='Starting Soon') self.generate_data(ids=['foo/bar/baz'], availability='Upcoming') - response = self.authenticated_get(self.path(exclude=self.always_exclude)) + response = self.validated_request(exclude=self.always_exclude) self.assertEquals(response.status_code, 200) expected_summaries = self.all_expected_results(availability='Upcoming') @@ -161,13 +162,13 @@ def test_collapse_upcoming(self): def test_programs(self): self.generate_data(programs=True) - response = self.authenticated_get(self.path(exclude=self.always_exclude[:1], programs=['True'])) + response = self.validated_request(exclude=self.always_exclude[:1], programs=['True']) self.assertEquals(response.status_code, 200) self.assertItemsEqual(response.data, self.all_expected_results(programs=True)) @ddt.data('passing_users', ) def test_exclude(self, field): self.generate_data() - response = self.authenticated_get(self.path(exclude=[field])) + response = self.validated_request(exclude=[field]) self.assertEquals(response.status_code, 200) self.assertEquals(str(response.data).count(field), 0) diff --git a/analytics_data_api/v0/views/__init__.py b/analytics_data_api/v0/views/__init__.py index 5b8591b3..652097c4 100644 --- a/analytics_data_api/v0/views/__init__.py +++ b/analytics_data_api/v0/views/__init__.py @@ -119,19 +119,33 @@ class APIListView(generics.ListAPIView): GET /api/v0/some_endpoint/ Returns full list of serialized models with all default fields. - GET /api/v0/some_endpoint/?ids={id},{id} + GET /api/v0/some_endpoint/?ids={id_1},{id_2} Returns list of serialized models with IDs that match an ID in the given `ids` query parameter with all default fields. - GET /api/v0/some_endpoint/?ids={id},{id}&fields={some_field},{some_field} + GET /api/v0/some_endpoint/?ids={id_1},{id_2}&fields={some_field_1},{some_field_2} Returns list of serialized models with IDs that match an ID in the given `ids` query parameter with only the fields in the given `fields` query parameter. - GET /api/v0/some_endpoint/?ids={id},{id}&exclude={some_field},{some_field} + GET /api/v0/some_endpoint/?ids={id_1},{id_2}&exclude={some_field_1},{some_field_2} Returns list of serialized models with IDs that match an ID in the given `ids` query parameter with all fields except those in the given `exclude` query parameter. + POST /api/v0/some_endpoint/ + { + "ids": [ + "{id_1}", + "{id_2}", + ... + "{id_200}" + ], + "fields": [ + "{some_field_1}", + "{some_field_2}" + ] + } + **Response Values** Since this is an abstract class, this view just returns an empty list. @@ -142,6 +156,9 @@ class APIListView(generics.ListAPIView): explicitly specifying the fields to include in each result with `fields` as well of the fields to exclude with `exclude`. + For GET requests, these parameters are passed in the query string. + For POST requests, these parameters are passed as a JSON dict in the request body. + ids -- The comma-separated list of identifiers for which results are filtered to. For example, 'edX/DemoX/Demo_Course,course-v1:edX+DemoX+Demo_2016'. Default is to return all courses. @@ -149,6 +166,12 @@ class APIListView(generics.ListAPIView): For example, 'course_id,created'. Default is to return all fields. exclude -- The comma-separated fields to exclude in the response. For example, 'course_id,created'. Default is to not exclude any fields. + + **Notes** + + * GET is usable when the number of IDs is relatively low + * POST is required when the number of course IDs would cause the URL to be too long. + * POST functions the same as GET here. It does not modify any state. """ ids = None fields = None @@ -175,6 +198,19 @@ def get(self, request, *args, **kwargs): return super(APIListView, self).get(request, *args, **kwargs) + def post(self, request, *args, **kwargs): + # self.request.data is a QueryDict. For keys with singleton lists as values, + # QueryDicts return the singleton element of the list instead of the list itself, + # which is undesirable. So, we convert to a normal dict. + request_data_dict = dict(request.data) + self.fields = request_data_dict.get('fields') + exclude = request_data_dict.get('exclude') + self.exclude = self.always_exclude + (exclude if exclude else []) + self.ids = request_data_dict.get(self.ids_param) + self.verify_ids() + + return super(APIListView, self).get(request, *args, **kwargs) + def verify_ids(self): """ Optionally raise an exception if any of the IDs set as self.ids are invalid. diff --git a/analytics_data_api/v0/views/course_summaries.py b/analytics_data_api/v0/views/course_summaries.py index 9161f5fe..431113e5 100644 --- a/analytics_data_api/v0/views/course_summaries.py +++ b/analytics_data_api/v0/views/course_summaries.py @@ -13,9 +13,19 @@ class CourseSummariesView(APIListView): """ Returns summary information for courses. - **Example Request** + **Example Requests** - GET /api/v0/course_summaries/?course_ids={course_id},{course_id} + GET /api/v0/course_summaries/?course_ids={course_id_1},{course_id_2} + + POST /api/v0/course_summaries/ + { + "course_ids": [ + "{course_id_1}", + "{course_id_2}", + ... + "{course_id_200}" + ] + } **Response Values** @@ -39,6 +49,9 @@ class CourseSummariesView(APIListView): Results can be filed to the course IDs specified or limited to the fields. + For GET requests, these parameters are passed in the query string. + For POST requests, these parameters are passed as a JSON dict in the request body. + course_ids -- The comma-separated course identifiers for which summaries are requested. For example, 'edX/DemoX/Demo_Course,course-v1:edX+DemoX+Demo_2016'. Default is to return all courses. @@ -48,6 +61,12 @@ class CourseSummariesView(APIListView): For example, 'course_id,created'. Default is to exclude the programs array. programs -- If included in the query parameters, will find each courses' program IDs and include them in the response. + + **Notes** + + * GET is usable when the number of course IDs is relatively low + * POST is required when the number of course IDs would cause the URL to be too long. + * POST functions the same as GET for this endpoint. It does not modify any state. """ serializer_class = serializers.CourseMetaSummaryEnrollmentSerializer programs_serializer_class = serializers.CourseProgramMetadataSerializer @@ -68,6 +87,17 @@ def get(self, request, *args, **kwargs): response = super(CourseSummariesView, self).get(request, *args, **kwargs) return response + def post(self, request, *args, **kwargs): + # self.request.data is a QueryDict. For keys with singleton lists as values, + # QueryDicts return the singleton element of the list instead of the list itself, + # which is undesirable. So, we convert to a normal dict. + request_data_dict = dict(self.request.data) + programs = request_data_dict.get('programs') + if not programs: + self.always_exclude = self.always_exclude + ['programs'] + response = super(CourseSummariesView, self).post(request, *args, **kwargs) + return response + def verify_ids(self): """ Raise an exception if any of the course IDs set as self.ids are invalid. diff --git a/analyticsdataserver/tests.py b/analyticsdataserver/tests.py index 3774d53f..9bb8b184 100644 --- a/analyticsdataserver/tests.py +++ b/analyticsdataserver/tests.py @@ -27,7 +27,15 @@ def setUp(self): def authenticated_get(self, path, data=None, follow=True, **extra): data = data or {} - return self.client.get(path, data, follow, HTTP_AUTHORIZATION='Token ' + self.token.key, **extra) + return self.client.get( + path=path, data=data, follow=follow, HTTP_AUTHORIZATION='Token ' + self.token.key, **extra + ) + + def authenticated_post(self, path, data=None, follow=True, **extra): + data = data or {} + return self.client.post( + path=path, data=data, follow=follow, HTTP_AUTHORIZATION='Token ' + self.token.key, **extra + ) @contextmanager