Add setting for configuring optional URL prefix for /api #14939

TheRealHaoLiu · 2024-02-28T21:56:45Z

SUMMARY

Add OPTIONAL_API_URLPATTERN_PREFIX setting

examples:

if set to '' (empty string) API pattern will be /api
if set to 'controller' API pattern will be /api AND /api/controller

ISSUE TYPE

New or Enhanced Feature

COMPONENT NAME

API

AWX VERSION

awx: 23.9.1.dev4+gec1a556ba0.d20240228

ADDITIONAL INFORMATION

dmzoneill · 2024-02-28T22:53:07Z

@TheRealHaoLiu what is the deal with the swagger tests all failing? Looking at your changes and the errors in swagger, it looks to me like something is off with the URL patterns you have changed and the associated tests?
Could you provide clarification?

dmzoneill · 2024-02-29T13:13:34Z

mmm, it seems some new issues have been uncovered

TheRealHaoLiu · 2024-02-29T20:11:34Z

fixed the swagger sanity test failure

jbradberry · 2024-03-18T15:33:41Z

awx/api/urls/urls.py

-    re_path(r'^login/$', LoggedLoginView.as_view(template_name='rest_framework/login.html', extra_context={'inside_login_context': True}), name='login'),
-    re_path(r'^logout/$', LoggedLogoutView.as_view(next_page='/api/', redirect_field_name='next'), name='logout'),
-    re_path(r'^o/', include(oauth2_root_urls)),
+    re_path(r'^(' + settings.OPTIONAL_API_URLPATTERN_PREFIX + '/)?$', ApiRootView.as_view(), name='api_root_view'),


There is a better way.

chrismeyersfsu · 2024-03-18T15:42:47Z

The Djangoy way to solve this would be via ROOT_URLCONF + middleware that would dynamically set request.urlconf based on the incoming url path.

But there are complications. Our url building and registration happens on import. So what would we dynamically set urlconf to? Let's say you somehow had a urlconf_base and urlconf_with_extra_path where urlconf_base is what ROOT_URLCONF is and urlconf_with_extra_path is all of the urls in ROOT_URLCONF BUT with the api/<extra_path_here>/... modified. But where and how? We could hard-code it in the middelware and hard-code the fact that we want to only operate on api/<extra_path_here> urls but that isn't any better than this hack. So let's say we sprinkle in something to the url that we can inspect in the middleware.

Let's say this below.

 re_path(
        r'^(' + settings.OPTIONAL_API_URLPATTERN_PREFIX + '/)?logout/$', LoggedLogoutView.as_view(next_page='/api/', redirect_field_name='next'), name='logout'
    )

Becomes this:

 re_path(
        r'^logout/$', LoggedLogoutView.as_view(next_page='/api/', redirect_field_name='next'), name='logout'
    options_prepend_path='awx/')

Now the middleware can find options_prepend_path and use that version.

But is this any better than this hack? It does become more testable. I tried testing this hack version and it sucks to test because the urls are "built" and registered on import and mocking/changing things on import sucks.

I'd say this version is more discoverable. I'd say a middleware version is more hidden BUT I think other components could re-use this code if it were middleware.

Ultimately, it's a judgement call as to if it's better to do this the Django way or via this hack. I'm of the opinion that this hack is OK. I'd love someone to do it "proper" so that it is more testable, but I'm ok with this version.

jbradberry · 2024-03-18T18:56:49Z

My preferred idiomatic Django way would be for an upstream urls.py file to use this setting as its prefix for importing the awx/main urls.

jbradberry · 2024-03-18T19:01:35Z

awx/api/versioning.py

@@ -24,6 +24,10 @@ def drf_reverse(viewname, args=None, kwargs=None, request=None, format=None, **e
    else:
        url = _reverse(viewname, args, kwargs, request, format, **extra)

+    if settings.OPTIONAL_API_URLPATTERN_PREFIX and request:
+        if request._request.path.startswith(f"/api/{settings.OPTIONAL_API_URLPATTERN_PREFIX}/v2/"):


I'm kind of not sure what is intended here, but this almost certainly does not do whatever that is. The condition virtually guarantees that the replace will be a no-op.

jbradberry · 2024-03-18T19:05:50Z

awx/main/tests/docs/test_swagger_generation.py

-        assert set(list(sorted(paths['/api/v2/credentials/{id}/'].keys()))) == set(['delete', 'get', 'patch', 'put', 'parameters'])
-        assert set(list(paths['/api/v2/settings/'].keys())) == set(['get', 'parameters'])
-        assert set(list(paths['/api/v2/settings/{category_slug}/'].keys())) == set(['get', 'put', 'patch', 'delete', 'parameters'])
+        assert set(list(paths['/api/{var}'].keys())) == set(['get', 'parameters'])


What is this doing? {var} looks like string templating, but I see no f-string or .format() in the changeset here.

Add OPTIONAL_API_URLPATTERN_PREFIX setting examples: - if set to `''` (empty string) API pattern will be `/api` - if set to 'controller' API pattern will be `/api` AND `/api/controller`

TheRealHaoLiu · 2024-03-18T20:27:18Z

Addressed @jbradberry's review in a collab session

jbradberry · 2024-03-19T15:11:21Z

awx/api/versioning.py

@@ -24,6 +24,10 @@ def drf_reverse(viewname, args=None, kwargs=None, request=None, format=None, **e
    else:
        url = _reverse(viewname, args, kwargs, request, format, **extra)

+    if settings.OPTIONAL_API_URLPATTERN_PREFIX and request:
+        if request._request.path.startswith(f"/api/{settings.OPTIONAL_API_URLPATTERN_PREFIX}"):


Is request._request.path really necessary, or does request.path work?

yah ur right request.path works

* Add setting for configuring optional URL prefix for /api Add OPTIONAL_API_URLPATTERN_PREFIX setting examples: - if set to `''` (empty string) API pattern will be `/api` - if set to 'controller' API pattern will be `/api` AND `/api/controller`

github-actions bot added the component:api label Feb 28, 2024

TheRealHaoLiu changed the title ~~Add setting for url prefix~~ Add setting for configuring optional URL prefix for /api Feb 28, 2024

dmzoneill marked this pull request as draft February 28, 2024 22:04

TheRealHaoLiu force-pushed the optional_api_url_prefix branch from 029ae9c to 69d5841 Compare February 28, 2024 22:19

TheRealHaoLiu marked this pull request as ready for review February 28, 2024 22:20

dmzoneill marked this pull request as draft February 28, 2024 22:59

dmzoneill marked this pull request as ready for review February 28, 2024 23:00

TheRealHaoLiu force-pushed the optional_api_url_prefix branch from 69d5841 to 86a196d Compare February 28, 2024 23:25

dmzoneill marked this pull request as draft February 29, 2024 00:19

dmzoneill marked this pull request as ready for review February 29, 2024 12:43

TheRealHaoLiu force-pushed the optional_api_url_prefix branch from 86a196d to 15ab639 Compare February 29, 2024 20:11

TheRealHaoLiu requested review from john-westcott-iv and fosterseth February 29, 2024 20:13

TheRealHaoLiu force-pushed the optional_api_url_prefix branch 2 times, most recently from 90f2bbe to 8c7c0d3 Compare February 29, 2024 20:24

dmzoneill force-pushed the optional_api_url_prefix branch from 8c7c0d3 to 8484e8a Compare March 5, 2024 13:03

dmzoneill marked this pull request as draft March 6, 2024 00:22

TheRealHaoLiu force-pushed the optional_api_url_prefix branch from 8484e8a to c8e9776 Compare March 7, 2024 16:56

TheRealHaoLiu marked this pull request as ready for review March 7, 2024 16:57

kdelee approved these changes Mar 7, 2024

View reviewed changes

chrismeyersfsu force-pushed the optional_api_url_prefix branch from c8e9776 to 624c458 Compare March 15, 2024 14:13

gundalow requested a review from jbradberry March 18, 2024 15:30

jbradberry reviewed Mar 18, 2024

View reviewed changes

TheRealHaoLiu force-pushed the optional_api_url_prefix branch from 624c458 to 4918fe4 Compare March 18, 2024 15:39

jbradberry reviewed Mar 18, 2024

View reviewed changes

Add setting for configuring optional URL prefix for /api

cebc917

Add OPTIONAL_API_URLPATTERN_PREFIX setting examples: - if set to `''` (empty string) API pattern will be `/api` - if set to 'controller' API pattern will be `/api` AND `/api/controller`

TheRealHaoLiu force-pushed the optional_api_url_prefix branch from 4918fe4 to cebc917 Compare March 18, 2024 20:26

jbradberry reviewed Mar 19, 2024

View reviewed changes

Update versioning.py

b3daf90

jbradberry approved these changes Mar 19, 2024

View reviewed changes

TheRealHaoLiu enabled auto-merge (squash) March 19, 2024 15:43

TheRealHaoLiu merged commit ab593bd into ansible:devel Mar 19, 2024
19 of 20 checks passed

TheRealHaoLiu deleted the optional_api_url_prefix branch March 19, 2024 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add setting for configuring optional URL prefix for /api #14939

Add setting for configuring optional URL prefix for /api #14939

TheRealHaoLiu commented Feb 28, 2024

dmzoneill commented Feb 28, 2024 •

edited

Loading

dmzoneill commented Feb 29, 2024

TheRealHaoLiu commented Feb 29, 2024

jbradberry Mar 18, 2024

chrismeyersfsu commented Mar 18, 2024

jbradberry commented Mar 18, 2024

jbradberry Mar 18, 2024

jbradberry Mar 18, 2024

TheRealHaoLiu commented Mar 18, 2024

jbradberry Mar 19, 2024

TheRealHaoLiu Mar 19, 2024

TheRealHaoLiu Mar 19, 2024

Add setting for configuring optional URL prefix for /api #14939

Add setting for configuring optional URL prefix for /api #14939

Conversation

TheRealHaoLiu commented Feb 28, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

AWX VERSION

ADDITIONAL INFORMATION

dmzoneill commented Feb 28, 2024 • edited Loading

dmzoneill commented Feb 29, 2024

TheRealHaoLiu commented Feb 29, 2024

jbradberry Mar 18, 2024

Choose a reason for hiding this comment

chrismeyersfsu commented Mar 18, 2024

jbradberry commented Mar 18, 2024

jbradberry Mar 18, 2024

Choose a reason for hiding this comment

jbradberry Mar 18, 2024

Choose a reason for hiding this comment

TheRealHaoLiu commented Mar 18, 2024

jbradberry Mar 19, 2024

Choose a reason for hiding this comment

TheRealHaoLiu Mar 19, 2024

Choose a reason for hiding this comment

TheRealHaoLiu Mar 19, 2024

Choose a reason for hiding this comment

dmzoneill commented Feb 28, 2024 •

edited

Loading