👋 ❓Proposal - marking parameters with annotations

[vitalik]

@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

[OtherBarry]

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]

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]

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]

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]

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]

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 ?

[wu-clan]

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]

@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

[stinovlas]

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]

@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]

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]

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.