Adding django-ninja options to building REST APIs

TGoddessana commented 8 months ago

Description

I'd suggest to add the django-ninja option to the project creation options.

Instead of using the use_drf option in the current template, it would be nice to have an option like the one below.

in cookiecutter.json:

...
"api_framework": ["Django REST Framework", "Django ninja"],

The current template has a good separation of the api-related code under api, so I think we could substitute schemas.py for serializers.py and so on to put the django ninja related code there.

Rationale

Now that version 1.0 of django-ninja has been released, and it seems to have matured quite a bit, I think it's worth adding it to the template.

foarsitter commented 8 months ago

A PR is more than welcome

TGoddessana commented 8 months ago

I wanted to get the maintainers' views before I started working on it, so I'll get to it :)

browniebroke commented 7 months ago

Although I don't have any experience with Django-ninja, I've heard about it in a few places and it looks really good. I would be interested to learn more about it and see what it would like to reproduce our simple DRF endpoints.

Accepting such change would however depend on how maintainable it is in the long run, so it would be ideally be covered by automated tests and not too intertwined with the DRF code.

If it's easier for you, perhaps try to generate a vanilla cookiecutter-django project with DRF, push it GitHub somewhere, open a pull request to migrate it to django-ninja and send us the link here? That would give us an idea of the differences between the 2 frameworks and maybe we could make a more informed decision?

TGoddessana commented 7 months ago

Great. I've been looking into what the current DRF implementation looks like for endpoint reproduction, and it appears that token authentication is not provided by ninja.

I'll try to create a simple demo and upload it soon :)

TGoddessana commented 7 months ago

OK, i created some demo for this.

In my opinion, the biggest decision to make about adding options is how much you want to implement the APIs that are currently in your template.

django ninja does not provide token-based authentication as provided by DRF.
django ninja doesn't have anything like viewset, so you'll need to test against the current implementation.
django ninja and DRF both provide ways to handle errors, but to make them exactly the same, code changes would need to be made in either framework.
Minor code changes would need to be made to align minor changes to the api endpoints with the current DRF implementation.
Beyond that, there are some very subtle differences.

So, I would like to suggest that...

How about making the test code test the API interface instead of being associated with DRF, for example, one of the test codes in the current template is to remove the code that tests queryset on ViewSet.
For maintainability, how about strengthening the test code for the USERS API? The current template does not provide test code for all the endpoints that ViewSet provides, but ninja should provide test code for all http methods since there is no such thing as viewset.

The other issue, in my opinion, is that..

the template is titled "Best Practices", so it seems like we should be discussing best practices and making changes. For simple example, what is the best practices to error handling in django ninja?

foarsitter commented 7 months ago

From my point of perspective there is no need to align the DRF functionality with the one provided by django-ninja. What is important to me is that there is 100% test-coverage for what is provided out of the box.

What kind of authentication you would suggest to replace the token auth?

TGoddessana commented 7 months ago

https://django-ninja.dev/guides/authentication/

There are several third-party packages that support authentication in django ninja, but personally I think we should wait until they are mature enough and can be evaluated.

My suggestion is to use Django Session based authentication, which is supported by Django Ninja.

for example, view can see like below:

@api.get("/pets", auth=django_auth)
def pets(request):
    return f"Authenticated user {request.auth}"

browniebroke commented 7 months ago

That's a great start, although it would have been a bit easier to follow the changes if you had a pull request with ALL the changes required. The PR that you linked seems to build upon other changes that added django-ninja already. Also, if the PR was still open we could collaborate and suggest improvements...

From my perspective, I don't really mind if the API responses are slighly different between DRF and django-ninja, or that error are reported differently. I'd rather have a project as close as psosible to what each framework considers the "best practice".

Once the project is generated, if the user opted for django-ninja, they shouldn't have any custom code that is just to make it look like DRF (or vise-versa).

TGoddessana commented 7 months ago

it would have been a bit easier to follow the changes if you had a pull request with ALL the changes required.

Can you tell me a little bit more about this? First of all, I reverted the merged PR and recreated it.

From my perspective, I don't really mind if the API responses are slighly different between DRF and django-ninja, or that error are reported differently. I'd rather have a project as close as psosible to what each framework considers the "best practice".

I agree with the above comment. Making the responses, URL namespaces, etc. the same as DRF provides adds unnecessary and minor changes.

The package itself is mature, but finding best practices for using it is not as easy as I thought it would be. I'd love to discuss this with some of you who are more experienced - certainly better developers than I am.

browniebroke commented 7 months ago

Can you tell me a little bit more about this? First of all, I reverted the merged PR and recreated it.

The PR changes the file config/ninja_api.py, but clearly this file isn't present with DRF. So I would expect this file to be created by your PR, not edited. Basically having a PR with installation and configuration of django-ninja, not just adding the endpoints. If we add it to the template, we'll have to maintain the whole thing... No need to change it now though. Thanks for opening this.

TGoddessana commented 7 months ago

(Add ninja_api file, remove api_router file) https://github.com/TGoddessana/cookiecutter_django_ninja_example/commit/77fedd74fa0d7f448657719059657af952cf30fb

(Add requirements) https://github.com/TGoddessana/cookiecutter_django_ninja_example/commit/1938cacec2bd0d7546efa7b71f57e88ee9732dfb

The only commits I made to install Django ninja were adding requirements and creating the ninja_api file. you can check the main branch commits 😀

Me too, thanks for looking into the suggestion

bgervan commented 2 months ago

Now the allauth package provides endpoints out of the box, so making django-ninja extension to cookiecutter-django is much easier. Is there any ongoing progress on this topic since the last discussions?

cookiecutter / cookiecutter-django