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

Let Request.url_for lookup routes in mounted app first #1416

Closed
wants to merge 3 commits into from

Conversation

plankthom
Copy link

@plankthom plankthom commented Jan 15, 2022

Proposed fix for #814

This modifies the lookup of routes by name to first look in the routes of the mounted app before resolving routes in the root app.

Motivation

As mentioned by @jussiarpalahti in by #814 (comment), this lookup algorithm makes more sense if the mounted sub-application is maintained as an independent module.

In that case you would not expect the sub-application to know under what name (if any) it is mounted.

With this feature, you can even mount the same application twice under different routes, without any need to configure the mount prefix as an application property.

@aminalaee
Copy link
Member

@plankthom Thank you for the PR. Can you please explain a bit about this?

As far as I could see this was not really a bug, this was the intended behaviour.

For doing reverse URL lookup on sub-mounted apps you'd need request.url_for('sub:<name>') and calling request.url_for(<name>) should not work.

@plankthom
Copy link
Author

plankthom commented Jan 15, 2022

@aminalaee : I`ve added some motivation in the PR description.

A FastApi example
test/trinkets.py

from fastapi import FastAPI, Request

app = FastAPI(title='Trinkets')
trinkets = [ {'name': 'nice'}, {'name': 'colorful'} ]

@app.get('/')
def info(request: Request):
    return {
        '_links: {'trinkets': {'href':  request.url_for('list_trinkets')}}
    }

@app.get('/trinket')
def list_trinkets(request: Request):
    return [
        {
            **trinket,
            '_links': {'self': {'href': request.url_for('get_trinket', trinket_name=trinket['name'])}}
        }
        for trinket in trinkets
    ]


@app.get('/trinket/{trinket_name}')
def get_trinket(trinket_name: str, request: Request):
    return next(t for t in trinkets if t['name'] == trinket_name)

test/main.py

import fastapi
import trinket

app = fastapi.FastAPI(title='Trinkets and Treasures')

@app.get('/')
def info(request: fastapi.Request):
    return { 
        'trinkets_v1' :  request.url_for('v1:list_trinkets'),
        'trinkets_v2' :  request.url_for('v2:list_trinkets')
    }

app.mount('/api/v1', app=trinket.app, name= 'v1')
app.mount('/api/v2', app=trinket.app, name= 'v2')

curl http://localhost:8000 | jq

{
  "trinkets_v1": "http://localhost:8000/api/v1/trinket",
  "trinkets_v2": "http://localhost:8000/api/v2/trinket"
}

curl curl http://localhost:8000/api/v2/trinket

[
  {
    "name": "nice",
    "_links": {
      "self": {
        "href": "http://localhost:8000/api/v2/trinket/nice"
      }
    }
  },
  {
    "name": "colorful",
    "_links": {
      "self": {
        "href": "http://localhost:8000/api/v2/trinket/colorful"
      }
    }
  }
]

@plankthom
Copy link
Author

plankthom commented Jan 15, 2022

Note that current fastapi treats these sub-apps independently in their documentation (e.g. documented with the local paths):

http://localhost:8000/api/v1/docs
image

(the current fastapi docs on that topic lag a bit behind)

@jussiarpalahti
Copy link

Hi. Original issue author here. Thank you @plankthom for looking into this.

My original use case was to make several small, reusable ASGI apps and combine them into larger app instances. Their URLs and URL namespace could differ depending on needs of the main app. There shouldn't be a need for any of the sub-apps to know where they reside, if ASGI framework can manage their URLs. Examples of such apps I had was authorization, and pub/sub messaging.

@plankthom plankthom changed the title Fix/url for on subapp Let Request.url_for lookup routes in mounted app first Jan 17, 2022
@adriangb adriangb added the feature New feature or request label Feb 2, 2022
@plankthom
Copy link
Author

@aminalaee did previous comments clarify the intent of this PR? If so would you consider reviewing it?

@Kludex Kludex mentioned this pull request Oct 3, 2022
11 tasks
@alex-oleshkevich
Copy link
Member

@Kludex I want this too. This is smth I suffer too.

Here is my example: https://github.com/alex-oleshkevich/ohmyadmin/blob/master/ohmyadmin/resources.py#L485
App can have many Resource classes mounted under different paths. And it is impossible to generate a valid URL path to the route within the mounted resource by name. I had to wrap it in a hackish name=self.url_name('edit') to add a unique prefix.

See

class Resource(Router):
    def __init__():
        super().__init__(routes=[
			Route('/', self.index_view, name='index'),
			Route('/edit', self.edit_view, name='edit'),
       ])

app = Starlette(routes=[
	Mount('/app/products', Resource()),
	Mount('/app/users', Resource()),
	Mount('...', Resource()),
])

There is no easy way to generate a URL path for edit from index and vice versa:

def index_view(self, request):
	return self.request.url_path(???) # 'edit' is a global name

You have to prefix route names to make them unique:

Route('/', self.index_view, name='products_index'),
Route('/', self.index_view, name='users_index'),

And do this for all. This is error-prone and too verbose.

If you decide to namespace URL in Mount everything worsens. You cannot generate URL to /app/users without knowing the namespace name defined by the user (is it 'users' in this example).

Mount('/app/users', Resource(), name='users'),

# in view code of 3rd-party library
def index_view(self, request):
	return self.request.url_path(namespace???:routename???) # 'edit' is a global name

I think we can do something like this:

self.request.url_path('.edit')
self.request.url_path('.index')

If route name starts with dot, then we look up the current ASGI app first (inner router for example) and then iterate parents"

self.request.url_path('.edit') # works, returns `/api/products/edit`
self.request.url_path('.index') # works, returns `/api/products`

self.request.url_path('home') # works, returns `/`
self.request.url_path('index') # works, returns url for `index` route name, note it is not prefixed with dot.

The Flask does it in the same way with blueprints - https://flask.palletsprojects.com/en/2.2.x/blueprints/#building-urls

@plankthom
Copy link
Author

plankthom commented Oct 5, 2022

@Kludex @alex-oleshkevich I can update this PR with the proposal (use . prefixed paths to have resolution local to the inner router) ... This will indeed improve backward compatibility, although IMO there is very little risk at incompatibility in the current proposal (other than that previously broken lookups would work now).

@Kludex Kludex mentioned this pull request Feb 14, 2023
8 tasks
@Kludex
Copy link
Sponsor Member

Kludex commented Mar 16, 2023

Can someone reply this? Then we can proceed...

@Kludex
Copy link
Sponsor Member

Kludex commented May 31, 2023

Closing it as stale since no one replied my last question.

Happy to reopen. 👍

@Kludex Kludex closed this May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants