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

Set default limit on /access/ to max limit when no pagination limit supplied #214

Merged

Conversation

coderbydesign
Copy link
Contributor

@coderbydesign coderbydesign commented Feb 20, 2020

In a recent PR 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

…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
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.

LGTM!

@coderbydesign coderbydesign merged commit 489ee4d into RedHatInsights:master Feb 20, 2020
mkanoor added a commit to mkanoor/insights-rbac-api-client-ruby that referenced this pull request Feb 20, 2020
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.

None yet

4 participants