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

Adding django-ninja options to building REST APIs #4923

Open
TGoddessana opened this issue Mar 13, 2024 · 11 comments
Open

Adding django-ninja options to building REST APIs #4923

TGoddessana opened this issue Mar 13, 2024 · 11 comments

Comments

@TGoddessana
Copy link
Contributor

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
Copy link
Collaborator

A PR is more than welcome

@TGoddessana
Copy link
Contributor Author

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

@browniebroke
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

TGoddessana commented Mar 24, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

TGoddessana commented Mar 25, 2024

(Add ninja_api file, remove api_router file)
TGoddessana/cookiecutter_django_ninja_example@77fedd7

(Add requirements)
TGoddessana/cookiecutter_django_ninja_example@1938cac

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

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

No branches or pull requests

3 participants