From e62793933c0628ce8963dc8981c8b7b62dac7436 Mon Sep 17 00:00:00 2001 From: Keith Walsh Date: Fri, 6 Sep 2024 13:07:38 -0400 Subject: [PATCH 1/3] Ensure paginated links in v2 APIs are supported/consistent with v1 Currently `links` are rendering like so for v2 endpoints: ``` { "links": { "first": "http://localhost:8000/api/rbac/v2/workspaces/?limit=1&offset=0", "last": "http://localhost:8000//api/rbac/v2/workspaces/?limit=1&offset=3", "next": "http://localhost:8000//api/rbac/v2/workspaces/?limit=1&offset=2", "previous": "http://localhost:8000//api/rbac/v2/workspaces/?limit=1" } } ``` And we need them to be consistent with v1 like: ``` { "links": { "first": "/api/rbac/v2/workspaces/?limit=1&offset=0", "last": "/api/rbac/v2/workspaces/?limit=1&offset=3", "next": "/api/rbac/v2/workspaces/?limit=1&offset=2", "previous": "/api/rbac/v2/workspaces/?limit=1" } } ``` In order to do this, it requires we change how we patch the `link_rewrite` method. This could be made simpler if we didn't have the current test cases expecting links to work for URLs without a `/api/v[0-9]` prefix. This PR adds an explicit test case for our prefix, and continues backward support for the existing test cases, though we may want to consider changing these since I don't believe they're valid use-cases today, in order to simplify the link logic in the paginator. --- rbac/api/common/pagination.py | 19 ++++++++----------- tests/api/common/test_pagination.py | 9 +++++++++ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/rbac/api/common/pagination.py b/rbac/api/common/pagination.py index 4747d28ea..b824bb647 100644 --- a/rbac/api/common/pagination.py +++ b/rbac/api/common/pagination.py @@ -17,13 +17,13 @@ """Common pagination class.""" import logging +from urllib.parse import urlparse +import re from rest_framework.pagination import LimitOffsetPagination from rest_framework.response import Response from rest_framework.utils.urls import replace_query_param -from api import API_VERSION - PATH_INFO = "PATH_INFO" logger = logging.getLogger(__name__) # pylint: disable=invalid-name @@ -38,16 +38,13 @@ class StandardResultsSetPagination(LimitOffsetPagination): def link_rewrite(request, link): """Rewrite the link based on the path header to only provide partial url.""" url = link - version = "v{}/".format(API_VERSION) if PATH_INFO in request.META: - try: - local_api_index = link.index(version) - path = request.META.get(PATH_INFO) - path_api_index = path.index(version) - path_link = "{}{}" - url = path_link.format(path[:path_api_index], link[local_api_index:]) - except ValueError: - logger.warning('Unable to rewrite link as "{}" was not found.'.format(version)) + url_components = urlparse(link) + path_and_query = url_components.path + (f"?{url_components.query}" if url_components.query else "") + if bool(re.search("/v[0-9]/", path_and_query)): + url = path_and_query + else: + logger.warning(f"Unable to rewrite link as no version was not found in {path_and_query}.") return url def get_first_link(self): diff --git a/tests/api/common/test_pagination.py b/tests/api/common/test_pagination.py index d5ad8e652..9a7a4e879 100644 --- a/tests/api/common/test_pagination.py +++ b/tests/api/common/test_pagination.py @@ -34,6 +34,15 @@ def test_link_rewrite(self): result = StandardResultsSetPagination.link_rewrite(request, link) self.assertEqual(expected, result) + def test_link_rewrite_rbac_prefix(self): + """Test the link rewrite for RBAC prefix.""" + request = Mock() + request.META = {PATH_INFO: "/api/rbac/v1/roles/"} + link = "http://localhost:8000/api/rbac/v1/roles/?limit=10&offset=0" + expected = "/api/rbac/v1/roles/?limit=10&offset=0" + result = StandardResultsSetPagination.link_rewrite(request, link) + self.assertEqual(expected, result) + def test_link_rewrite_err(self): """Test the link rewrite.""" request = Mock() From 4cc1815ad63301b00ead46cd9d026a674f63bfe2 Mon Sep 17 00:00:00 2001 From: Keith Walsh Date: Fri, 6 Sep 2024 13:28:24 -0400 Subject: [PATCH 2/3] Test case added for links in v2 APIs specifically --- tests/api/common/test_pagination.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/api/common/test_pagination.py b/tests/api/common/test_pagination.py index 9a7a4e879..54b743484 100644 --- a/tests/api/common/test_pagination.py +++ b/tests/api/common/test_pagination.py @@ -43,6 +43,15 @@ def test_link_rewrite_rbac_prefix(self): result = StandardResultsSetPagination.link_rewrite(request, link) self.assertEqual(expected, result) + def test_link_rewrite_rbac_prefix_v2(self): + """Test the link rewrite for RBAC prefix for v2 APIs.""" + request = Mock() + request.META = {PATH_INFO: "/api/rbac/v2/workspaces/"} + link = "http://localhost:8000/api/rbac/v2/workspaces/?limit=10&offset=0" + expected = "/api/rbac/v2/workspaces/?limit=10&offset=0" + result = StandardResultsSetPagination.link_rewrite(request, link) + self.assertEqual(expected, result) + def test_link_rewrite_err(self): """Test the link rewrite.""" request = Mock() From 4ffe9d187a28d35cf5978e7b6aab3bafc5e9f56e Mon Sep 17 00:00:00 2001 From: Keith Walsh Date: Fri, 6 Sep 2024 13:42:06 -0400 Subject: [PATCH 3/3] Appease the linters --- rbac/api/common/pagination.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rbac/api/common/pagination.py b/rbac/api/common/pagination.py index b824bb647..82d41eb52 100644 --- a/rbac/api/common/pagination.py +++ b/rbac/api/common/pagination.py @@ -17,8 +17,8 @@ """Common pagination class.""" import logging -from urllib.parse import urlparse import re +from urllib.parse import urlparse from rest_framework.pagination import LimitOffsetPagination from rest_framework.response import Response