Add names to routes #106

gaganpreet · 2021-09-28T21:17:20Z

Very helpful project, thanks!

I came across a small issue: I use this recipe to simplify the operation ids generated in the OpenAPI schema. A caveat of applying this simplification is that route names have to be unique.

Unfortunately, if no explicit route name is specified, the fallback is endpoint.__name__ (endpoint here being the endpoint function), which results from this starlette function. In CRUDRouter's case, everything ends up being the same boring value called route.

This PR uses the route prefix to assign a unique name to every route. Grammatically, it's not always correct. eg: If your SQLAlchemy table is named users, get_one_user is correct, but get_all_user isn't. I'm not sure if there's already a way to handle pluralisation in the project, maybe that warrants a discussion first?

vercel · 2021-09-28T21:17:26Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/flortz/fastapi-crudrouter/CNzfmvhSrsUUGtrvaiGt8KHKghQq
✅ Preview: https://fastapi-crudrouter-git-fork-gaganpreet-unique-rou-96d782-flortz.vercel.app

awtkns

Hi @gaganpreet, thanks for this! Its a small yet important fix that will improve integration with openapi. 🚀

As for pluralization, I think it would be tricky to get it 100% grammatically correct. I'm sure you know it is not as simple as just adding an 's' to the end of any string. I am hesitant to bring in another package to handle pluralization more completely. However a crude implementation that covers most cases should be possible. Thoughts?

awtkns · 2021-10-02T06:09:23Z

fastapi_crudrouter/core/_base.py

@@ -60,6 +61,7 @@ def __init__(
                response_model=Optional[List[self.schema]],  # type: ignore
                summary="Get All",
                dependencies=get_all_route,
+                name=f"get_all_{item_name}",


Suggested change

name=f"get_all_{item_name}",

name=f"get_all_{prefix}",

We need item_name because prefix is a path prefix for the API and includes slashes. Which made me realise that using prefix for item might also end up with slashes, so I'll change item name to the lowercased schema class name.

awtkns · 2021-10-02T06:09:39Z

fastapi_crudrouter/core/_base.py

@@ -80,6 +83,7 @@ def __init__(
                response_model=Optional[List[self.schema]],  # type: ignore
                summary="Delete All",
                dependencies=delete_all_route,
+                name=f"delete_all_{item_name}",


Suggested change

name=f"delete_all_{item_name}",

name=f"delete_all_{prefix}",

awtkns · 2021-10-02T06:09:50Z

fastapi_crudrouter/core/_base.py

@@ -102,6 +107,7 @@ def __init__(
                summary="Update One",
                dependencies=update_route,
                error_responses=[NOT_FOUND],
+                name=f"update_one_{item_name}",


Suggested change

name=f"update_one_{item_name}",

name=f"update_one_{prefix}",

awtkns · 2021-10-02T06:10:08Z

fastapi_crudrouter/core/_base.py

@@ -91,6 +95,7 @@ def __init__(
                summary="Get One",
                dependencies=get_one_route,
                error_responses=[NOT_FOUND],
+                name=f"get_one_{item_name}",


Suggested change

name=f"get_one_{item_name}",

name=f"get_one_{prefix}",

awtkns · 2021-10-02T06:10:17Z

fastapi_crudrouter/core/_base.py

@@ -113,6 +119,7 @@ def __init__(
                summary="Delete One",
                dependencies=delete_one_route,
                error_responses=[NOT_FOUND],
+                name=f"delete_one_{item_name}",


Suggested change

name=f"delete_one_{item_name}",

name=f"delete_one_{prefix}",

awtkns · 2021-10-02T06:10:41Z

fastapi_crudrouter/core/_base.py

@@ -47,6 +47,7 @@ def __init__(
        )

        prefix = str(prefix if prefix else self.schema.__name__).lower()
+        item_name = prefix


Suggested change

item_name = prefix

awtkns · 2021-10-02T07:00:31Z

@gaganpreet @grigorov to further this, do you have any objections to directly setting the operation id?

some sample operation_id / names could be:

GET /users -> get_all_users
POST /users -> create_user
POST /user/item_id -> update_user_by_id
...

gaganpreet

However a crude implementation that covers most cases should be possible. Thoughts?

A possible solution could be what Django does, it uses the class name and simply appends "s" to the name by default, and lets you specify verbose_name and verbose_name_plural as part of model options to customise this behaviour.

do you have any objections to directly setting the operation id?

We shouldn't set operation ids by default: they need to be unique across the whole application and if we set operation ids for users by default we'd need an option to disable the feature in case it results in conflicts in their app.

gaganpreet · 2021-10-02T15:50:11Z

fastapi_crudrouter/core/_base.py

@@ -60,6 +61,7 @@ def __init__(
                response_model=Optional[List[self.schema]],  # type: ignore
                summary="Get All",
                dependencies=get_all_route,
+                name=f"get_all_{item_name}",


We need item_name because prefix is a path prefix for the API and includes slashes. Which made me realise that using prefix for item might also end up with slashes, so I'll change item name to the lowercased schema class name.

awtkns · 2021-10-05T06:16:40Z

Hi @gaganpreet. I ran the master vs this PR and compared the generated openapi spec. You can see this from the /openapi.json route by default. As you can see below, operationIds are already set Fastapi (or something under it). Your changes alter both the operationId and title in the openapi spec.

Before we merge this PR. I think it would be best if alter the operation_id and title to make them more readble.

Method	Path	Field	Old	New
GET	`/potato`	operationId	route_potato_get	get_all_potato_potato_get
GET	`/potato`	title	Response Route Potato Get	Response Get All Potato Potato Get
POST	`/potato`	operationId	route_potato_post	create_one_potato_potato_post
...

Thoughts?

Full diff:

 diff master.json unique-route-names.json
14c14
<         "operationId": "get_all_potato_potato_get",
---
>         "operationId": "route_potato_get",
43c43
<                   "title": "Response Get All Potato Potato Get",
---
>                   "title": "Response Route Potato Get",
69c69
<         "operationId": "create_one_potato_potato_post",
---
>         "operationId": "route_potato_post",
108c108
<         "operationId": "delete_all_potato_potato_delete",
---
>         "operationId": "route_potato_delete",
115c115
<                   "title": "Response Delete All Potato Potato Delete",
---
>                   "title": "Response Route Potato Delete",
133c133
<         "operationId": "get_one_potato_potato__item_id__get",
---
>         "operationId": "route_potato__item_id__get",
176c176
<         "operationId": "update_one_potato_potato__item_id__put",
---
>         "operationId": "route_potato__item_id__put",
229c229
<         "operationId": "delete_one_potato_potato__item_id__delete",
---
>         "operationId": "route_potato__item_id__delete",
275c275
<         "operationId": "get_all_carrot_carrot_get",
---
>         "operationId": "route_carrot_get",
303c303
<                   "title": "Response Get All Carrot Carrot Get",
---
>                   "title": "Response Route Carrot Get",
330c330
<         "operationId": "create_one_carrot_carrot_post",
---
>         "operationId": "route_carrot_post",
370c370
<         "operationId": "delete_all_carrot_carrot_delete",
---
>         "operationId": "route_carrot_delete",
377c377
<                   "title": "Response Delete All Carrot Carrot Delete",
---
>                   "title": "Response Route Carrot Delete",
396c396
<         "operationId": "get_one_carrot_carrot__item_id__get",
---
>         "operationId": "route_carrot__item_id__get",
440c440
<         "operationId": "update_one_carrot_carrot__item_id__put",
---
>         "operationId": "route_carrot__item_id__put",
494c494
<         "operationId": "delete_one_carrot_carrot__item_id__delete",
---
>         "operationId": "route_carrot__item_id__delete",

gaganpreet · 2021-10-13T20:27:45Z

Hey Adam, busy schedule got in the way. I just looked into the two points you mentioned.

First of all, title: it's generated in this part of FastAPI. It looks like a generated field and I don't see a way to override that with something more pleasant. ☹️

With regards to operation id, it's going through a similar transformation. We can change it to the same value as name (so operation id will look like get_all_potato, create_one_potato etc.), with the caveat that if someone uses duplicate schema names to create FastAPI CRUD routers, things will break (I don't know how it will break as I haven't tried it yet).

Default FastAPI operation ids aren't usually pretty which is why I use the simplify operation ids transformation (which prompted this PR in the first place).

paxtonBravo

I'm very need this for my new project

Add names to routes

7ff95f6

vercel bot deployed to Preview September 28, 2021 21:17 View deployment

grigorov approved these changes Oct 2, 2021

View reviewed changes

awtkns approved these changes Oct 2, 2021

View reviewed changes

awtkns added the enhancement New feature or request label Oct 2, 2021

gaganpreet commented Oct 2, 2021

View reviewed changes

Use schema name as item name because prefix contains slashes

aa489fe

vercel bot deployed to Preview October 2, 2021 20:19 View deployment

paxtonBravo suggested changes Jul 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add names to routes #106

Add names to routes #106

gaganpreet commented Sep 28, 2021

vercel bot commented Sep 28, 2021 •

edited

Loading

awtkns left a comment •

edited

Loading

awtkns Oct 2, 2021

gaganpreet Oct 2, 2021

awtkns Oct 2, 2021

awtkns Oct 2, 2021

awtkns Oct 2, 2021

awtkns Oct 2, 2021

awtkns Oct 2, 2021

awtkns commented Oct 2, 2021 •

edited

Loading

gaganpreet left a comment

gaganpreet Oct 2, 2021

awtkns commented Oct 5, 2021 •

edited

Loading

gaganpreet commented Oct 13, 2021

paxtonBravo left a comment

Add names to routes #106

Are you sure you want to change the base?

Add names to routes #106

Conversation

gaganpreet commented Sep 28, 2021

vercel bot commented Sep 28, 2021 • edited Loading

awtkns left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awtkns commented Oct 2, 2021 • edited Loading

gaganpreet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awtkns commented Oct 5, 2021 • edited Loading

gaganpreet commented Oct 13, 2021

paxtonBravo left a comment

Choose a reason for hiding this comment

vercel bot commented Sep 28, 2021 •

edited

Loading

awtkns left a comment •

edited

Loading

awtkns commented Oct 2, 2021 •

edited

Loading

awtkns commented Oct 5, 2021 •

edited

Loading