Skip to content

Empty string("") as a path parameter default value. #11313

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

Closed
wants to merge 2 commits into from
Closed

Empty string("") as a path parameter default value. #11313

wants to merge 2 commits into from

Conversation

osangu
Copy link

@osangu osangu commented Mar 19, 2024

Hello!

This PR is about set empty string("") as a default value of path.

I think @app.get() is more elegance than @app.get(""),
beacause of below reasones.

  • if ()and ("") do same works, may be "" is not important information.
  1. we don't have to do same job of typing "".
  2. When using with other parameters, you can focus on other parameters.

before

from http import HTTPStatus
from fastapi import FastAPI, APIRouter

app = FastAPI()
router = APIRouter(prefix="/users")

@router.get("")
def foo():
    pass

@router.post(
    "", # or path=""
    response_model=Bar,
    status_code=HTTPStatus.CREATED
)
def bar():
    return Bar()

app.include_router(router)

after

from http import HTTPStatus
from fastapi import FastAPI, APIRouter

app = FastAPI()
router = APIRouter(prefix="/users")

@router.get()
def foo():
    pass

@router.post(
    response_model=Bar,
    status_code=HTTPStatus.OK
)
def bar():
    return Bar()

app.include_router(router)

And i think about problems after this change and how to prevent.

  1. backward compatibility: It is just a default, so people who using with "" or "/" will not trap in any problem.
  2. Not expected path by default: show warning log or option about want to use default value.

So i made this changes : )


P. S
this is just my opinion and i also want to listen about other people thinking!

@codespearhead
Copy link

codespearhead commented Mar 24, 2024

I'd argue we should be careful with this PR as is because it'd be a slippery slope to this and this.

The default value should be a trailing forward slash in my opinion.

Also, there should be a mechanism to prevent users from creating endpoints ending with file extensions to prevent this.

@osangu
Copy link
Author

osangu commented Mar 25, 2024

Thkas for your comment!
I'm really happy to talking about this PR.

I read about your links, and summarize like below.

please check about summaries tight, cause i'm not good at english. 😢
I hope they will correct with link context.

  • This change can cause many cost and problems.
  • CVE-2023-4853 Security Problem: access to secure endpoint by using multi slash.
  • Files shouldn’t end in a slash.

But, i guess those problems caused when using trailing forward slash as default.
So i want to know why you recommend slash as default.
By using empty string, can't we prevent problems?


P. S
This PR is for a default path, that make developers can skip writing none path.
I also known and thought about side effects.

And this is my opinion.

The latest version and previous version, path was necessary parameter.
So path must exist in there code, and this change will not bring effects to them.

It only works on new colde.

...

I hope my thking is share to you kindly.
Please, ask me if you have any questions!

@codespearhead
Copy link

codespearhead commented Mar 31, 2024

The use of a trailing slash in URIs, or lack thereof, is largely inconsistent across service providers, because some will add it when it's missing, whereas others will just ignore it when it's there. This results in a brittle deployment since that behavior is out of our control.

Besides, this lets the consumers of your API choose either flavor and run the risk of being affected without warrant, as illustrated in that Spring Boot thread. Imagine the disaster if that change was in the gateway of your cloud services provider instead of in the framework you're using?

Adding a trailing slash by default will spare us the hassle of having to make that choice down the line, as concluded here, because adding it in URIs if it wasn't there, unless the URI ended with a file extension, used to be a common practice, and changing the behavior of legacy codebases is something that's generally out of our control.

@tiangolo
Copy link
Member

tiangolo commented Apr 2, 2024

So, the usage of an empty string for a route/path operation is already a strange corner case. I prefer to keep it explicit that it is taking an empty path than remove it altogether. I'm also gonna refactor how routers work, and that change could affect this, so for now I'll rather pass on this.

But thanks for the interest! ☕

@tiangolo tiangolo closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants