Skip to content
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

Remove default limit on /access/ requests #195

Conversation

coderbydesign
Copy link
Contributor

In order to prevent default enforcement of pagination on the /access/ endpoint,
this will remove the default limit (currently 10) from /access/ requests, while
keeping it on all other endpoints.

When we get the paginator for the request, we'll now check to see if the ?limit
query parameter is sent in the request. If so, we'll continue to respect pagination
on the request, and return a paginated response. If ?limit does not exist in
the request, we will not set a paginator, which in tern will supply the raw
queryset to the serializer.

In order to keep a consistent API payload with and without pagination, we need to
explicitly return the serialized data nested under a data object.

With Pagination:
/api/rbac/v1/access/?application=ansible-automation&limit=20

{
  "meta": {
    "count": 1,
    "limit": 20,
    "offset": 0
  },
  "links": {
    "first": "/api/rbac/v1/access/?application=ansible-automation&limit=20&offset=0",
    "next": null,
    "previous": null,
    "last": "/api/rbac/v1/access/?application=ansible-automation&limit=20&offset=0"
  },
  "data": [
    {
      "permission": "ansible-automation:*:*",
      "resourceDefinitions": []
    }
  ]
}

Screen Shot 2020-02-04 at 9 49 02 AM

Without Pagination:
/api/rbac/v1/access/?application=ansible-automation

{
  "data": [
    {
      "permission": "ansible-automation:*:*",
      "resourceDefinitions": []
    }
  ]
}

Screen Shot 2020-02-04 at 9 48 53 AM

In order to prevent default enforcement of pagination on the `/access/` endpoint,
this will remove the default limit (currently 10) from `/access/` requests, while
keeping it on all other endpoints.

When we get the paginator for the request, we'll now check to see if the `?limit`
query parameter is sent in the request. If so, we'll continue to respect pagination
on the request, and return a paginated response. If `?limit` does not exist in
the request, we will not set a paginator, which in tern will supply the raw
queryset to the serializer.

In order to keep a consistent API payload with and without pagination, we need to
explicitly return the serialized data nested under a `data` object.

**With Pagination:**
_/api/rbac/v1/access/?application=ansible-automation&limit=20_
```
{
  "meta": {
    "count": 1,
    "limit": 20,
    "offset": 0
  },
  "links": {
    "first": "/api/rbac/v1/access/?application=ansible-automation&limit=20&offset=0",
    "next": null,
    "previous": null,
    "last": "/api/rbac/v1/access/?application=ansible-automation&limit=20&offset=0"
  },
  "data": [
    {
      "permission": "ansible-automation:*:*",
      "resourceDefinitions": []
    }
  ]
}
```

**Without Pagination:**
_/api/rbac/v1/access/?application=ansible-automation_
```
{
  "data": [
    {
      "permission": "ansible-automation:*:*",
      "resourceDefinitions": []
    }
  ]
}
```
We now have two different possible 200 responses for `/access/`, one with pagination,
and one without.

OpenAPI 3 supports the use of `anyOf` and `oneOf` for response body schemas [1].
This updates the spec to reflect those possible responses (with and without
pagination), and includes examples of each.

[1] https://swagger.io/docs/specification/describing-responses/
Copy link
Contributor

@astrozzc astrozzc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@wcmitchell wcmitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@coderbydesign coderbydesign merged commit c16707b into RedHatInsights:master Feb 5, 2020
coderbydesign added a commit to coderbydesign/insights-rbac that referenced this pull request Feb 20, 2020
…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
lpichler pushed a commit that referenced this pull request Nov 8, 2023
[GitHub] - Automated ConfigMap Generation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants