From 238893724a12aee8c7b9bd26f4d55745cee46116 Mon Sep 17 00:00:00 2001 From: Keith Walsh Date: Thu, 20 Feb 2020 08:20:04 -0500 Subject: [PATCH] Set default limit on /access/ to max limit when no pagination limit supplied In a recent (PR)[https://github.com/RedHatInsights/insights-rbac/pull/195] we removed the default limit on the `/access/` endpoint, to ensure all records were returned in a single request instead of using the default of 10. This meant in order to be backwards compatible, we'd also need to continue supporting pagination params. To do this, we updated the spec to include `oneOf` two possible valid responses, one paginated and one unpaginated. The `data` would remain the same, but the meta data for pagination would not be included unless a `limit` param was supplied in the request. The associated updates were made to the `openapi.json` spec, but due to an existing bug/issue in the OpenAPI Generator project [1,2], this breaks some client generation which is used by app teams and QE, even though it's valid per the spec. In order to avoid having a separate endpoint explicitly for a paginated responses, and to also avoid constructing false meta data for pagination, we've decided to set the default limit on the `/access/` endpoint equal to the max limit number when no `limit` is supplied, but continue to respect the `limit` pagination param when it is supplied. This will allow for client generation to continue to work, and will allow those using pagination by default to still be supported. For those not using pagination, the response `data` should again still be the same, and the default limit will not be a barrier to hitting pagination, as it will be the same as our max results. [1] https://github.com/OpenAPITools/openapi-generator/issues/15 [2] https://github.com/OpenAPITools/openapi-generator/issues/1709 --- docs/source/specs/openapi.json | 83 +--------------------------- rbac/management/access/view.py | 5 +- tests/management/access/test_view.py | 2 +- 3 files changed, 6 insertions(+), 84 deletions(-) diff --git a/docs/source/specs/openapi.json b/docs/source/specs/openapi.json index ae25dc295..9f195c018 100644 --- a/docs/source/specs/openapi.json +++ b/docs/source/specs/openapi.json @@ -1518,7 +1518,7 @@ } }, { - "$ref": "#/components/parameters/QueryLimitNoDefault" + "$ref": "#/components/parameters/QueryLimit" }, { "$ref": "#/components/parameters/QueryOffset" @@ -1526,26 +1526,11 @@ ], "responses": { "200": { - "description": "A list of access objects. By default, this request will not be paginated. To received a paginated response, include the `/?limit=` query parameter.", + "description": "A paginated list of access objects", "content": { "application/json": { "schema": { - "oneOf": [ - { - "$ref": "#/components/schemas/AccessNoPagination" - }, - { - "$ref": "#/components/schemas/AccessPagination" - } - ] - }, - "examples": { - "AccessNoPagination": { - "$ref": "#/components/examples/AccessNoPagination" - }, - "AccessPagination": { - "$ref": "#/components/examples/AccessPagination" - } + "$ref": "#/components/schemas/AccessPagination" } } } @@ -1607,17 +1592,6 @@ "maximum": 1000 } }, - "QueryLimitNoDefault": { - "in": "query", - "name": "limit", - "required": false, - "description": "Parameter for selecting the amount of data returned.", - "schema": { - "type": "integer", - "minimum": 1, - "maximum": 1000 - } - }, "NameFilter": { "in": "query", "name": "name", @@ -2345,24 +2319,6 @@ } ] }, - "AccessNoPagination": { - "allOf": [ - { - "type": "object", - "required": [ - "data" - ], - "properties": { - "data": { - "type": "array", - "items": { - "$ref": "#/components/schemas/Access" - } - } - } - } - ] - }, "Status": { "required": [ "api_version" @@ -2414,39 +2370,6 @@ } } } - }, - "examples": { - "AccessNoPagination": { - "value": { - "data": [ - { - "permission": "app-name:*:*", - "resourceDefinitions": [] - } - ] - } - }, - "AccessPagination": { - "value": { - "meta": { - "count": 1, - "limit": 10, - "offset": 0 - }, - "links": { - "first": "/access/?application=app-name&limit=10", - "next": null, - "previous": null, - "last": "/access/?application=app-name&limit=10" - }, - "data": [ - { - "permission": "app-name:*:*", - "resourceDefinitions": [] - } - ] - } - } } } } \ No newline at end of file diff --git a/rbac/management/access/view.py b/rbac/management/access/view.py index 7f2da69c8..40446b6a0 100644 --- a/rbac/management/access/view.py +++ b/rbac/management/access/view.py @@ -94,10 +94,9 @@ def get(self, request): def paginator(self): """Return the paginator instance associated with the view, or `None`.""" if not hasattr(self, '_paginator'): + self._paginator = self.pagination_class() if self.pagination_class is None or 'limit' not in self.request.query_params: - self._paginator = None - else: - self._paginator = self.pagination_class() + self._paginator.default_limit = self._paginator.max_limit return self._paginator def paginate_queryset(self, queryset): diff --git a/tests/management/access/test_view.py b/tests/management/access/test_view.py index 1a474780e..4d6dfe40a 100644 --- a/tests/management/access/test_view.py +++ b/tests/management/access/test_view.py @@ -123,7 +123,7 @@ def test_get_access_success(self): self.assertIsNotNone(response.data.get('data')) self.assertIsInstance(response.data.get('data'), list) self.assertEqual(len(response.data.get('data')), 1) - self.assertIsNone(response.data.get('meta')) + self.assertEqual(response.data.get('meta').get('limit'), 1000) self.assertEqual(self.access_data, response.data.get('data')[0]) def test_get_access_with_limit(self):