Merge pull request #114 from mdn/query_reject_1243221

fix bug 1243221 - Reject query parameters reserved by JSON API v1.0

docs/v1/doc_cases.json

@@ -90,7 +90,7 @@
     }, {
         "name": "browser-create-in-changeset-2",
         "method": "POST",
-        "endpoint": "browsers?changeset=4",
+        "endpoint": "browsers?use_changeset=4",
         "data": {
             "browsers": {
                 "slug": "nintendo-ds",
@@ -745,7 +745,7 @@
     }, {
         "name": "view-feature-update-parts-with-changeset-2",
         "method": "POST",
-        "endpoint": "versions?changeset=36",
+        "endpoint": "versions?use_changeset=36",
         "data": {
             "versions": {
                 "version": "2.0",
@@ -758,7 +758,7 @@
     }, {
         "name": "view-feature-update-parts-with-changeset-3",
         "method": "POST",
-        "endpoint": "supports?changeset=36",
+        "endpoint": "supports?use_changeset=36",
         "data": {
             "supports": {
                 "links": {

docs/v1/raw/browser-create-in-changeset-2-request-headers.txt

@@ -1,4 +1,4 @@
-POST /api/v1/browsers?changeset=4 HTTP/1.1
+POST /api/v1/browsers?use_changeset=4 HTTP/1.1
 Host: browsercompat.org
 Accept: application/vnd.api+json
 Authorization: Bearer xxQLNiTUFjRL5En8nBWzSDc5tLWkV2

docs/v1/raw/view-feature-update-parts-with-changeset-2-request-headers.txt

@@ -1,4 +1,4 @@
-POST /api/v1/versions?changeset=36 HTTP/1.1
+POST /api/v1/versions?use_changeset=36 HTTP/1.1
 Host: browsercompat.org
 Accept: application/vnd.api+json
 Authorization: Bearer xxQLNiTUFjRL5En8nBWzSDc5tLWkV2

docs/v1/raw/view-feature-update-parts-with-changeset-3-request-headers.txt

@@ -1,4 +1,4 @@
-POST /api/v1/supports?changeset=36 HTTP/1.1
+POST /api/v1/supports?use_changeset=36 HTTP/1.1
 Host: browsercompat.org
 Accept: application/vnd.api+json
 Authorization: Bearer xxQLNiTUFjRL5En8nBWzSDc5tLWkV2

docs/v2/doc_cases.json

@@ -141,7 +141,7 @@
     }, {
         "name": "browser-create-in-changeset-2",
         "method": "POST",
-        "endpoint": "browsers?changeset=4",
+        "endpoint": "browsers?use_changeset=4",
         "data": {
             "data": {
                 "type": "browsers",
@@ -1887,7 +1887,7 @@
     }, {
         "name": "view-feature-update-parts-with-changeset-2",
         "method": "POST",
-        "endpoint": "versions?changeset=55",
+        "endpoint": "versions?use_changeset=55",
         "data": {
             "data": {
                 "type": "versions",
@@ -1907,7 +1907,7 @@
     }, {
         "name": "view-feature-update-parts-with-changeset-3",
         "method": "POST",
-        "endpoint": "supports?changeset=55",
+        "endpoint": "supports?use_changeset=55",
         "data": {
             "data": {
                 "type": "supports",

docs/v2/implementation.rst

@@ -185,7 +185,7 @@ resources:
 .. literalinclude:: /v2/raw/browser-create-in-changeset-3-request-body.json
     :language: json
 
-The response included relationships to the items created in the changeset:
+The response includes relationships to the items created in the changeset:
 
 .. literalinclude:: /v2/raw/browser-create-in-changeset-3-response-headers.txt
     :language: http

docs/v2/raw/browser-create-in-changeset-2-request-headers.txt

@@ -1,4 +1,4 @@
-POST /api/v2/browsers?changeset=4 HTTP/1.1
+POST /api/v2/browsers?use_changeset=4 HTTP/1.1
 Host: browsercompat.org
 Accept: application/vnd.api+json
 Authorization: Bearer xxQLNiTUFjRL5En8nBWzSDc5tLWkV2

docs/v2/raw/browser-create-in-changeset-2-response-body.json

@@ -1,6 +1,6 @@
 {
     "links": {
-        "self": "https://browsercompat.org/api/v2/browsers?changeset=4"
+        "self": "https://browsercompat.org/api/v2/browsers?use_changeset=4"
     },
     "data": {
         "type": "browsers",

docs/v2/raw/view-feature-update-parts-with-changeset-2-request-headers.txt

@@ -1,4 +1,4 @@
-POST /api/v2/versions?changeset=55 HTTP/1.1
+POST /api/v2/versions?use_changeset=55 HTTP/1.1
 Host: browsercompat.org
 Accept: application/vnd.api+json
 Authorization: Bearer xxQLNiTUFjRL5En8nBWzSDc5tLWkV2

docs/v2/raw/view-feature-update-parts-with-changeset-2-response-body.json

@@ -1,6 +1,6 @@
 {
     "links": {
-        "self": "https://browsercompat.org/api/v2/versions?changeset=55"
+        "self": "https://browsercompat.org/api/v2/versions?use_changeset=55"
     },
     "data": {
         "type": "versions",

docs/v2/raw/view-feature-update-parts-with-changeset-3-request-headers.txt

@@ -1,4 +1,4 @@
-POST /api/v2/supports?changeset=55 HTTP/1.1
+POST /api/v2/supports?use_changeset=55 HTTP/1.1
 Host: browsercompat.org
 Accept: application/vnd.api+json
 Authorization: Bearer xxQLNiTUFjRL5En8nBWzSDc5tLWkV2

docs/v2/raw/view-feature-update-parts-with-changeset-3-response-body.json

@@ -1,6 +1,6 @@
 {
     "links": {
-        "self": "https://browsercompat.org/api/v2/supports?changeset=55"
+        "self": "https://browsercompat.org/api/v2/supports?use_changeset=55"
     },
     "data": {
         "type": "supports",

tools/client.py

@@ -61,7 +61,7 @@ def request(
         if method in change_methods and self.csrftoken:
             headers['X-CSRFToken'] = self.csrftoken
         if method in change_methods and self.changeset:
-            params['changeset'] = self.changeset
+            params['use_changeset'] = self.changeset
         if method in create_methods:
             expected_statuses = [201]
         elif method in delete_methods:

webplatformcompat/exceptions.py

@@ -0,0 +1,49 @@
+# -*- coding: utf-8 -*-
+"""API exceptions and exception handler."""
+
+from django.utils.translation import ugettext_lazy as _
+
+from rest_framework.exceptions import APIException
+from rest_framework.status import HTTP_400_BAD_REQUEST
+from rest_framework.views import exception_handler as base_handler
+
+
+class InvalidQueryParam(APIException):
+    """The request contained a bad query parameter.
+
+    JSON API v1.0 defines a structure (source/parameter) for specifying which
+    query parameter was bad.
+    http://jsonapi.org/format/1.0/#error-objects
+    """
+
+    detail_fmt = _('Query parameter "%(query_param)s" is invalid.')
+    status_code = HTTP_400_BAD_REQUEST
+
+    def __init__(self, query_param):
+        """Initialize the exception with the bad query parameter."""
+        self.parameter = query_param
+        self.detail = self.detail_fmt % {'query_param': query_param}
+
+
+class NotImplementedQueryParam(InvalidQueryParam):
+    """Request contained an query parameter that is not implemented.
+
+    JSON API v1.0 requires a 400 when the service doesn't support a documented
+    query parameter, such as 'include':
+    http://jsonapi.org/format/1.0/#fetching-includes
+    """
+
+    detail_fmt = _('Query parameter "%(query_param)s" is not implemented.')
+
+
+def handler(exc, context):
+    """
+    Return the response that should be used for any given exception.
+
+    If the v2 API is called, use the JSON API v1.0 exception handler.
+    Otherwise, use the default Django REST Framework handler.
+    """
+    response = base_handler(exc, context)
+    if response is not None:
+        response.exc = exc
+    return response

webplatformcompat/history.py

@@ -160,7 +160,7 @@ def process_request(self, request):
         # Default is to update cached objects as they are modified
         request.delay_cache = False
 
-        changeset_id = request.GET.get('changeset')
+        changeset_id = request.GET.get('use_changeset')
         if changeset_id:
             changeset = Changeset.objects.get(id=changeset_id)
             if changeset.user != request.user:

webplatformcompat/tests/test_exceptions.py

@@ -0,0 +1,23 @@
+# -*- coding: utf-8 -*-
+"""Tests for API exceptions."""
+
+from rest_framework.exceptions import ParseError
+
+from ..exceptions import handler
+from .base import TestCase
+
+
+class TestExceptionHandler(TestCase):
+    """Test the extended exception handler."""
+
+    def test_api_exception(self):
+        """Test the response includes 'standard' exceptions as an attribute."""
+        exception = ParseError()
+        response = handler(exception, {})
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.exc, exception)
+
+    def test_unexpected_exception(self):
+        """Test that unexpected responses return None."""
+        response = handler(ValueError('unexpected'), {})
+        self.assertIsNone(response)

webplatformcompat/tests/test_history.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 """Tests for simple_history extensions."""
 
-from json import loads
+from json import dumps, loads
 
 from django.contrib.auth.models import User
 
@@ -11,17 +11,29 @@
 from .base import APITestCase
 
 
-class TestMiddleware(APITestCase):
-    """Test the HistoryChangesetRequestMiddleware."""
+class TestBaseMiddleware(APITestCase):
+    """Test the HistoryChangesetRequestMiddleware.
+
+    This should be tested by a subclass that defines:
+    * namespace - the API namespace, such as v1
+    * api_data - A function returning data in the API format
+    """
 
     __test__ = False  # Don't test outside of a versioned API
+    content_type = 'application/vnd.api+json'
+
+    def url(self, changeset):
+        """Return the test URL."""
+        return (
+            self.api_reverse('browser-list') +
+            '?use_changeset=%s' % changeset.id)
 
     def test_post_with_changeset(self):
         self.login_user()
         changeset = Changeset.objects.create(user=self.user)
-        url = self.api_reverse('browser-list') + '?changeset=%s' % changeset.id
-        data = {'slug': 'firefox', 'name': '{"en": "Firefox"}'}
-        response = self.client.post(url, data)
+        url = self.url(changeset)
+        response = self.client.post(
+            url, dumps(self.api_data()), content_type=self.content_type)
         self.assertEqual(201, response.status_code, response.data)
         browser = Browser.objects.get()
         history = browser.history.get()
@@ -31,10 +43,9 @@ def test_post_with_changeset_wrong_user(self):
         self.login_user()
         other = User.objects.create(username='other')
         changeset = Changeset.objects.create(user=other)
-        url = self.api_reverse('browser-list') + '?changeset=%s' % changeset.id
-        data = {'slug': 'firefox', 'name': '{"en": "Firefox"}'}
+        url = self.url(changeset)
         response = self.client.post(
-            url, data, content_type='application/vnd.api+json')
+            url, dumps(self.api_data()), content_type=self.content_type)
         self.assertEqual(400, response.status_code)
         expected = {
             'errors': {
@@ -47,10 +58,9 @@ def test_post_with_changeset_wrong_user(self):
     def test_post_with_closed_changeset(self):
         self.login_user()
         changeset = Changeset.objects.create(user=self.user, closed=True)
-        url = self.api_reverse('browser-list') + '?changeset=%s' % changeset.id
-        data = {'slug': 'firefox', 'name': '{"en": "Firefox"}'}
+        url = self.url(changeset)
         response = self.client.post(
-            url, data, content_type='application/vnd.api+json')
+            url, dumps(self.api_data()), content_type=self.content_type)
         self.assertEqual(400, response.status_code)
         expected = {
             'errors': {
@@ -60,7 +70,7 @@ def test_post_with_closed_changeset(self):
     def test_post_with_error_not_json_api(self):
         self.login_user()
         changeset = Changeset.objects.create(user=self.user, closed=True)
-        url = self.api_reverse('browser-list') + '?changeset=%s' % changeset.id
+        url = self.url(changeset)
         data = {'slug': 'firefox', 'name': '{"en": "Firefox"}'}
         response = self.client.post(url, data)
         self.assertEqual(400, response.status_code)

webplatformcompat/tests/v1/test_history.py

@@ -1,11 +1,20 @@
 # -*- coding: utf-8 -*-
 """Tests for simple history overrides in v1 API."""
 
-from ..test_history import TestMiddleware
+from ..test_history import TestBaseMiddleware
+from .base import NamespaceMixin
 
 
-class TestMiddlewareInV1(TestMiddleware):
+class TestMiddleware(NamespaceMixin, TestBaseMiddleware):
     """Test the Middleware using v1 API endpoints."""
 
-    __test__ = True  # Run these tests
-    namespace = 'v1'  # Use the v1 API endpoints
+    def api_data(self):
+        """Return JSON data for creating a new browser."""
+        return {
+            'browsers': {
+                'slug': 'firefox',
+                'name': {
+                    'en': 'Firefox'
+                }
+            }
+        }

webplatformcompat/tests/v1/test_viewsets.py

@@ -216,7 +216,7 @@ def test_put_in_changeset(self, mock_update):
             }
         })
         url = self.api_reverse('browser-detail', pk=browser.pk)
-        url += '?changeset=%s' % changeset.pk
+        url += '?use_changeset=%s' % changeset.pk
         mock_update.reset_mock()
         mock_update.side_effect = Exception('not called')
         response = self.client.put(
@@ -277,7 +277,7 @@ def test_delete_in_changeset(self, mock_update):
             Browser, slug='internet_exploder',
             name={'en': 'Internet Exploder'})
         url = self.api_reverse('browser-detail', pk=browser.pk)
-        url += '?changeset=%d' % self.changeset.id
+        url += '?use_changeset=%d' % self.changeset.id
         mock_update.reset_mock()
         mock_update.side_effect = Exception('not called')
         response = self.client.delete(url)

webplatformcompat/tests/v2/test_history.py

@@ -1,9 +1,23 @@
 # -*- coding: utf-8 -*-
 """Tests for simple history overrides in v2 API."""
 
-from ..test_history import TestMiddleware
+from ..test_history import TestBaseMiddleware
 from .base import NamespaceMixin
 
 
-class TestMiddlewareInV2(TestMiddleware, NamespaceMixin):
-    """Test the Middleware using API endpoints."""
+class TestMiddleware(NamespaceMixin, TestBaseMiddleware):
+    """Test the Middleware using v2 API endpoints."""
+
+    def api_data(self):
+        """Return JSON data for creating a new browser."""
+        return {
+            'data': {
+                'type': 'browsers',
+                'attributes': {
+                    'slug': 'firefox',
+                    'name': {
+                        'en': 'Firefox'
+                    }
+                }
+            }
+        }