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

👋 ❓Proposal - marking parameters with annotations #746

Open
vitalik opened this issue Apr 20, 2023 · 12 comments
Open

👋 ❓Proposal - marking parameters with annotations #746

vitalik opened this issue Apr 20, 2023 · 12 comments
Labels
design API design

Comments

@vitalik
Copy link
Owner

vitalik commented Apr 20, 2023

@adriangb in xpresso framework created what I think a nicer approach for marking api function arguments. I think it should be the default way in django-ninja as well

basically this:

@api.post("/some")
def some(request, filters:Filters = Query(...), payload: SomeSchema = Form(...)):
    ...

can turn into this

@api.post("/some")
def some(request, filters: Query[Filters], payload: Form[SomeSchema]):
    ...

Optional params will turn from

foo: int | None = Query(None)

to

foo: Query[int | None] = None

passing OpenApi parameters:

foo: Query[int, OpenAPI(title="A Foo", example=42)]

Pros

  • somewhat more intuitive definitions
  • no weird "..." for required params (which I see lot of people are very surprised at first usage :) )

Cons

  • most likely not backward compatible
    • can become backward compatible with different naming like - FromQuery, FromBody, FromPath

Please share your thoughts

cc: @SmileyChris @OtherBarry @baseplate-admin @stephenrauch @jkeyes

@vitalik vitalik added the design API design label Apr 20, 2023
@vitalik vitalik pinned this issue Apr 20, 2023
@OtherBarry
Copy link
Contributor

First impression: I like the idea, it feels cleaner and more pythonic.

Some general notes:

  1. This has to still work nicely with type checkers / IDEs, ideally without having to add an extra package or mypy plugin or whatever. One of the best parts of django ninja is that type checkers 'just work'. I don't think the benefits of this system would outweigh the type checking loss if it doesn't work smoothly.
  2. I think it'd be ideal to keep this backwards compatible. Ideally this would be with the same class names, but I'd lean on the side of something like FromQuery or QueryVar over losing backwards compatibilty.
  3. Personally not a fan of the PEP593 style of adding openapi args, but I like the idea of being able to have different classses that extend the type, rather than having heaps of kwargs on one class.

Basically, its a good idea, I just don't want to lose the current way of doing things in order to gain it.

@adriangb
Copy link

This will work with IDEs. In fact, it will be better in a lot of ways, especially w.r.t. default values.

Backwards compatibility with the same name is unfortunately not possible. You can however have it if you use different names. I’d recommend keeping foo: int = Query(**kwargs) and making FromQuery = Annotated[T, Query()]

@adriangb
Copy link

foo: Query[int, OpenAPI(title="A Foo", example=42)]

I don’t think that will work. You’ll need to do one of:

  • foo: Annotated[int, Query(), OpenAPI(…)]
  • foo: FromQuery[Annotated[int, OpenAPI(…)]]`

IMO both are equally ugly and verbose, but they result in the same thing (internally you don’t have to handle them separately or anything).

@baseplate-admin
Copy link
Contributor

baseplate-admin commented Apr 20, 2023

So my first impression on this : It's a new way of approaching a rather complex problem.

Now my 2 cents on this topic is :

  1. I always thought of django-ninja as a replacement of fastapi. So those who wants the functionality, security and more specifically the admin panel and orm of django should switch over to django-ninja. Keeping the same API as fastapi meant the transition is smoother. It also has the bonus point that almost all patterns written for fastapi applies to django ( excluding dependency injection )
  2. Without codemod. This breaking change is not feasible for large codebases. My own project, i have over 7,000 LOC of python codes that would be very hard to migrate if we choose to drop old API.

no weird "..." for required params (which I see lot of people are very surprised at first usage :) )

Maybe we can follow current fastapi approach? ( eg : adding a title? )

@api.post("/some")
def some(request, filters:Filters = Query(...), payload: SomeSchema = Form(title="some special schema which does awesome things")):
    ...

Note that fastapi changed quite a lot in the recent times. For example old versions of fastapi ( to my knowledge ) used to have Query function. Now that's no longer the case


Overall the direction i want ninja to head in is to market itself as the fastapi of django eco-system ( but it could be a me problem here )..

Thanks for reading. It's great to see the development resuming. You have created a really awesome framework @vitalik

@adriangb
Copy link

FastAPI actually supports and recommends this approach now: https://github.com/tiangolo/fastapi/releases/tag/0.95.0.

also, this doesn’t have to be a breaking change

@baseplate-admin
Copy link
Contributor

FastAPI actually supports and recommends this approach now: https://github.com/tiangolo/fastapi/releases/tag/0.95.0.

Fair enough. As long as we have a step by step migration for django-ninja, i wont complain.

also, this doesn’t have to be a breaking change

But follow a deprecation like django handles it ?

@vitalik vitalik changed the title Proposal - marking parameters with annotations 👋 ❓Proposal - marking parameters with annotations Jun 15, 2023
@wu-clan
Copy link

wu-clan commented Sep 10, 2023

I checked Annotated's Test Code because the documentation hasn't been updated yet!

It's a little different from fastapi's, fastapi requires that parameters must set default values, e.g. Query Annotated

django-ninja seems to set them to None by default, but some additional test code is needed!

My point:

Regular typing and PEP593 is sufficient, it is also a standard, for this proposal, in the source code, Query... These are exported as functions and may not be appropriate for direct use in type annotations, it is better to use Type[class] constructs as type hints

@vitalik
Copy link
Owner Author

vitalik commented Sep 11, 2023

@wu-clan

not sure which part you see different, but this should work iudentical in django-ninja and fastapi:

q: Annotated[str, Query()],
# equals to
q: str = Query(...)

OR

q: Annotated[str, Query()] = "some-defatul",
# equals to 
q: str = Query("some-defatul")

but the Topic here to make it shorter:

q: Query[str] = "some-default"

with support of autocompletion and other stutff

@vitalik vitalik closed this as completed Sep 11, 2023
@vitalik vitalik reopened this Sep 11, 2023
@stinovlas
Copy link

I do like the new syntax a bit more, but I also don't want to change my annotations for every endpoint at once. Some backwards compatible change would be best (although I'd prefer the same naming schema, FromQuery is weird – on the other hand, you can always use import as).

Migration guide for django-ninja==1.0.0 would be greatly appreciated. Migration tool would be a great addition, but I consider the guide more important. pydantic itself has a good migration guide for v2, that could be referenced from the django-ninja migration guide.

@vitalik
Copy link
Owner Author

vitalik commented Sep 12, 2023

@stinovlas

Well the plan is actually to keep all these 3 in working (backwards compatible)

q: Query[str] = "some-defatul"
# is equal to
q: Annotated[str, Query()] = "some-defatul",
# and equal to 
q: str = Query("some-defatul")
# and all will work in the same codebase

it just the default documentation will suggest to use the new syntax by default

@c4ffein
Copy link
Contributor

c4ffein commented Sep 1, 2024

What is the current status of this issue?
I read the codebase to check if I would feel comfortable picking it up, and I think I do, but I'd rather first ask:

  • Do you still want it implemented as in your first and last comments?
  • Is it ok if I drop a PoC PR we can iterate on or did someone start working on it already?
  • Would you provide a list of UT I can duplicate to make it pass with both the old and new syntax (or files for which I should duplicate all applicable tests)?

@mathmul
Copy link

mathmul commented Sep 21, 2024

So right now I understand I can replace this:

def list_users(request, params: Get_UsersQueryParams = Query(...)):

with

def list_users(request, params: Query[Get_UsersQueryParams]):

but I do not understand, how I could use the annotations here:

def send_reset_password_email(request, user_id: str = Path(..., example="01c8ba9e-25d2-4ad5-9ddb-90eb3d15569c"), params: Put_Users_ResetPasswordEmailQueryParams = Query(...)):

If I replace the Query(...) part, I get the problem that I have a parameter with no default value after one with it. And I don't know how to replace the Path(..., example=...) part with the annotation without losing the example value in OpenAPI UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design API design
Projects
None yet
Development

No branches or pull requests

8 participants