-
Notifications
You must be signed in to change notification settings - Fork 60
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
RHCLOUD-30751 Principal Links endpoint fixes #1248
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
/retest |
rbac/management/principal/view.py
Outdated
@@ -188,7 +187,7 @@ def get(self, request): | |||
"first": f"{path}?limit={limit}&offset=0{usernames_filter}", | |||
"next": f"{path}?limit={limit}&offset={offset + limit}{usernames_filter}", | |||
"previous": f"{path}?limit={limit}&offset={previous_offset}{usernames_filter}", | |||
"last": None, | |||
"last": f"{path}?limit={limit}&offset={int(limit - int(count))}{usernames_filter}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if limit < count then offset will be negative
maybe we can reuse the get_paginated_response()
from rbac/api/common/pagination.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EvanCasey13 is it possible to leverage get_paginated_response
somehow ?
it looks there some cases which needs to covered.
rbac/management/principal/view.py
Outdated
@@ -183,12 +182,13 @@ def get(self, request): | |||
count = len(data) | |||
else: | |||
count = None | |||
last_link = int(count) - int(limit) if (int(count) - int(limit)) >= 0 else 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider better name for the variable, it should describe the value that is inside
rbac/management/principal/view.py
Outdated
response_data["meta"] = {"count": count, "limit": limit, "offset": offset} | ||
response_data["links"] = { | ||
"first": f"{path}?limit={limit}&offset=0{usernames_filter}", | ||
"next": f"{path}?limit={limit}&offset={offset + limit}{usernames_filter}", | ||
"previous": f"{path}?limit={limit}&offset={previous_offset}{usernames_filter}", | ||
"last": None, | ||
"last": f"{path}?limit={limit}&offset={last_link}{usernames_filter}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is needed to fix all links here to work in same way as for other endpoints like GET /groups/
for example GET /principals/
for limit=10 and offset=0 (default values) the previous link should be null
/retest |
Link(s) to Jira
https://issues.redhat.com/browse/RHCLOUD-30751
Description of Intent of Change(s)
Fixed bug where incorrect links were being returned for the principals endpoint
Local Testing
How can the feature be exercised?
How can the bug be exploited and fix confirmed?
Is any special local setup required?
Checklist
Secure Coding Practices Checklist Link
Secure Coding Practices Checklist