Skip to content

Commit

Permalink
Set default limit on /access/ to max limit when no pagination limit s…
Browse files Browse the repository at this point in the history
…upplied

In a recent (PR)[RedHatInsights#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] OpenAPITools/openapi-generator#15
[2] OpenAPITools/openapi-generator#1709
  • Loading branch information
coderbydesign committed Feb 20, 2020
1 parent 36c19e4 commit 2388937
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 84 deletions.
83 changes: 3 additions & 80 deletions docs/source/specs/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -1518,34 +1518,19 @@
}
},
{
"$ref": "#/components/parameters/QueryLimitNoDefault"
"$ref": "#/components/parameters/QueryLimit"
},
{
"$ref": "#/components/parameters/QueryOffset"
}
],
"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"
}
}
}
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -2345,24 +2319,6 @@
}
]
},
"AccessNoPagination": {
"allOf": [
{
"type": "object",
"required": [
"data"
],
"properties": {
"data": {
"type": "array",
"items": {
"$ref": "#/components/schemas/Access"
}
}
}
}
]
},
"Status": {
"required": [
"api_version"
Expand Down Expand Up @@ -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": []
}
]
}
}
}
}
}
5 changes: 2 additions & 3 deletions rbac/management/access/view.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion tests/management/access/test_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down

0 comments on commit 2388937

Please sign in to comment.