django-ninja
django-ninja copied to clipboard
vitalik
[proposal] Class Based Operations
Read full proposal here - http://django-ninja.rest-framework.com/proposals/cbv/
To allow incapsulate common logic into class methods
from ninja import Router
router = Router()
@router.path('/project/{project_id}/tasks')
class Tasks:
def __init__(self, request, project_id=int):
user_projects = request.user.project_set
self.project = get_object_or_404(user_projects, id=project_id))
self.tasks = self.project.task_set.all()
@router.get('/', response=List[TaskOut])
def task_list(self, request):
return self.tasks
@router.get('/{task_id}/', response=TaskOut)
def details(self, request, task_id: int):
return get_object_or_404(self.tasks, id=task_id)
@vitalik About the async def __init__(): issue, this seems like a good workaround:
import asyncio
dsn = "..."
class Foo(object):
@classmethod
async def create(cls, settings):
self = Foo()
self.settings = settings
self.pool = await create_pool(dsn)
return self
async def main(settings):
settings = "..."
foo = await Foo.create(settings)
Source: https://stackoverflow.com/questions/33128325/how-to-set-class-attribute-with-await-in-init/33134213 Couple other workarounds are also proposed in this post that we could look into.
yes, this seems the only option... (without the classmethod), the onlyl thing I struggle how to name that method:
my shortlist so far
- init (without _ )
- create
- prepare
- before_request
@vitalik First option init (without _ ) sounds good since that is what we're trying to replace.
I don't think we need this design. The design of NINJA is very good now. It can transplant the logic code to other frameworks, such as FASTAPI and FLASK.
Hi @vitalik , why do we need to use __init__ ?
Can we use middleware and rails like approach?
from ninja import Router
router = Router()
@router.path('/project/{project_id}/tasks', before_action=['get_all_tasks'], after_action=['after_action'], around_action=['around_action'])
class Tasks:
def get_all_tasks(self, request, project_id=int):
user_projects = request.user.project_set
self.project = get_object_or_404(user_projects, id=project_id))
self.tasks = self.project.task_set.all()
@router.get('/', response=List[TaskOut])
def task_list(self, request):
return self.tasks
@router.get('/{task_id}/', response=TaskOut)
def details(self, request, task_id: int):
return get_object_or_404(self.tasks, id=task_id)
Or you may want to write like this..
@router.path('/project/{project_id}/tasks, around_action=['around_action'])
class Tasks:
@router.before_action()
def get_all_tasks(self, request, project_id=int):
user_projects = request.user.project_set
self.project = get_object_or_404(user_projects, id=project_id))
self.tasks = self.project.task_set.all()
@router.after_action()
def log_action(self):
print("hello")
@router.get('/', response=List[TaskOut])
def task_list(self, request):
return self.tasks
@router.get('/{task_id}/', response=TaskOut)
def details(self, request, task_id: int):
return get_object_or_404(self.tasks, id=task_id)
Ref: https://guides.rubyonrails.org/action_controller_overview.html
Here the implementation of my proposal:
from functools import partial
from typing import List
from django.shortcuts import get_object_or_404
from ninja import Router
def update_object(obj, payload):
for attr, value in payload.dict().items():
if value:
setattr(obj, attr, value)
return obj
class ApiModel:
RETRIEVE = 'retrieve'
LIST = 'list'
CREATE = 'create'
UPDATE = 'update'
DELETE = 'delete'
views = [RETRIEVE, LIST, CREATE, UPDATE, DELETE]
auth = None
response_schema = None
create_schema = None
update_schema = None
router: Router = None
queryset = None
def __init__(self):
if not self.router:
raise Exception("Router is necessary")
if not self.response_schema:
raise Exception("Response schema is necessary")
if self.LIST in self.views:
self.router.add_api_operation(path="/", methods=["GET"],
auth=self.auth,
view_func=self.get_list_func(),
response=List[self.response_schema])
if self.RETRIEVE in self.views:
self.router.add_api_operation(path="/{pk}", methods=["GET"],
auth=self.auth,
view_func=self.get_retrieve_func(),
response=self.response_schema)
if self.UPDATE in self.views:
self.router.add_api_operation(path="/{pk}", methods=["PUT", "PATCH"],
auth=self.auth,
view_func=self.get_put_path_func(),
response=self.response_schema)
if self.CREATE in self.views:
self.router.add_api_operation(path="/", methods=["POST"],
auth=self.auth,
view_func=self.get_post_func(),
response=self.response_schema)
if self.DELETE in self.views:
self.router.add_api_operation(path="/{pk}", methods=["DELETE"],
auth=self.auth,
view_func=self.get_delete_func())
def get_list_func(self):
model = self.queryset.model
def list_func(get_queryset, request):
return get_queryset(request)
list_func = partial(list_func, self.get_queryset)
list_func.__name__ = f"list_{model._meta.model_name}"
return list_func
def get_retrieve_func(self):
model = self.queryset.model
def retrieve_func(get_queryset, request, pk):
return get_object_or_404(get_queryset(request), pk=pk)
retrieve_func = partial(retrieve_func, self.get_queryset)
retrieve_func.__name__ = f"retrieve_{model._meta.model_name}"
return retrieve_func
def get_post_func(self):
create_schema = self.create_schema
model = self.queryset.model
def post_func(request, payload: create_schema):
return self.queryset.model.objects.create(**payload.dict())
post_func.__name__ = f"create_{model._meta.model_name}"
return post_func
def get_put_path_func(self):
update_schema = self.update_schema
model = self.queryset.model
def put_path_func(request, pk, payload: update_schema):
return update_object(
get_object_or_404(self.get_queryset(request), pk=pk), payload)
put_path_func.__name__ = f"update_{model._meta.model_name}"
return put_path_func
def get_delete_func(self):
model = self.queryset.model
def delete_func(request, pk):
obj = get_object_or_404(self.get_queryset(request), pk=pk)
obj.delete()
return
delete_func.__name__ = f"delete_{model._meta.model_name}"
return delete_func
def get_queryset(self, request):
return self.queryset
Usage:
router = Router()
class CustomerAddressAPI(ApiModel):
queryset = CustomerAddress.objects.all()
auth = KnoxAuth() # https://github.com/FJLendinez/django-ninja-knox
create_schema = CreateCustomerAddressSchema
update_schema = UpdateCustomerAddressSchema
response_schema = OutCustomerAddressSchema
router = router
# api.add_router('/customer-addresses/', CustomerAddressAPI().router)
I solved this in a pretty nice way for FastAPI but they just give me 👎 and no discussion @tiangolo please consider it 🦗: https://github.com/tiangolo/fastapi/pull/2626
My approach solves both issues - so you can actually use the class in multiple instances like this:
from fastapi import FastAPI
from fastapi.routing import ViewAPIRouter
from fastapi.testclient import TestClient
from pydantic import BaseModel
view_router = ViewAPIRouter()
app = FastAPI()
class Item(BaseModel):
message_update: str
@view_router.bind_to_class()
class Messages(ViewAPIRouter.View):
def __init__(self, message: str):
self.message = message
@view_router.get("/message")
def get_message(self) -> str:
return self.message
@view_router.post("/message")
def post_message(self, item: Item) -> str:
self.message = item.message_update
em_instance = Messages(message="👋")
pt_instance = Messages(message="olá")
en_instance = Messages(message="hello")
app.include_router(em_instance.router, prefix="/em")
app.include_router(pt_instance.router, prefix="/pt")
app.include_router(en_instance.router, prefix="/en")
client = TestClient(app)
# verify route inclusion
response = client.get("/em/message")
assert response.status_code == 200, response.text
assert em_instance.message in response.text
response = client.get("/pt/message")
assert response.status_code == 200, response.text
assert pt_instance.message in response.text
response = client.get("/en/message")
assert response.status_code == 200, response.text
assert en_instance.message in response.text
# change state in an instance
item = Item(message_update="✨")
response = client.post("/em/message", json=item.dict())
assert response.status_code == 200, response.text
response = client.get("/em/message")
assert response.status_code == 200, response.text
assert item.message_update in response.text
The above code is functional in my fork and all the docs work as expected. The change is relatively simple - adds maybe 100 lines to encapsulate the class decorator and store the route decorations. Then it injects the 'self' parameter for each class instance on top of the provided values. It is a relatively small change with very high value.
@vitalik would you be interested in a similar PR to this project?
I think all the people who 👎 without much thought are missing the value of encapsulation and testability that this brings to the code. It's very pythonic to drop everything into global scope – but you lose the ability to instantiate fresh during tests, instead you need complex dependency mocks or tests without mocks. This does not scale in a large project with many contributors.
Hi @Kojiro20
Well, what you do here is a bit different from what the goal is you create instances on declaration time, while goal is that class instance created on each request, so that during instance initialisation some common attributes can be set to avoid code duplications
I understand what you're trying to do - but if you'll bear with me a little - I'll try to explain what I meant by 'it solves both problems' above.
Django (and python at large) are very much biased toward whiz-bang trickery and auto-magic injection of things. This, still, despite guido leaving and everyone supposedly caring about the 'Principle of Least Astonishment'. The hot-take is that you wouldn't be trying to find a name for your post-init initializer if Django hadn't already created a paradigm where everything is side-loaded from config and confusingly-injected at runtime everywhere in the platform.
The only reason you need to decide between "init, create, prepare, before_request" is because Django has stolen the init function in its frenzied attempt to make WSGI scale. But, if you don't yield your init power to begin with, you won't need to reclaim it later.
This can be done if you init with resolve and re-use the instance you started with. Django actually allows this (which is why I'm not using FastAPI for my latest gig) but none of the niceties come along with it (so far). If you instantiated a class and made a "urls.py-concatatable" set of urls (and maybe even throw in some sane method semantics (lol, django has no sane method filtration semantics outside cbvs after 15yrs?!?) as a bonus) you could instantiate once and then concat something onto the urls list.
Sorry this is a bit of a rant. I just regret taking a contract that requires python. Since Guido left, everyone is breaking back-compat with wild abandon at the same time, while doubling down on the "Just use functions..." mentality. 30yrs in this industry and honestly the worst code that exists is written in c# (and derivatives (or perhaps superlatives?)) and python. Don't get me wrong, Java can be awful, but it's never as bad as others at their worst (nor as good when they're at their best).
well still... looking at your example I do not see how can it solve the issue when:
you have a bunch of API urls which start with the same path/params
-
GET /project/<proj-id>/tasks -
POST /project/<proj-id>/tasks -
GET /project/<proj-id>/tasks/<task-id> -
PATCH /project/<proj-id>/tasks/<task-id> -
DELETE /project/<proj-id>/tasks/<task-id> -
POST /project/<proj-id>/tasks/<task-id>/complete
as you can see a little todo app - there are 6 methods that share the same logic for checking permission (if user have access to project)
user_projects = request.user.project_set
self.project = get_object_or_404(user_projects, id=project_id))
self.tasks = self.project.task_set.all()
repeating these in every operation will lead to tons of code duplications...
That sounds better suited to a middleware decorator IMO.
To use the single-instance class version tho you could instantiate your authorization validator during app initialization and provide it to your router instance. So, instead of calling a globally imported get_object_or_404 function you could call something like self.send_404_if_not_authorized(...).
Am I completely misunderstanding your point? I think my high-level goal is to get as far away as possible from global functions as they make it harder to scale out a project. Even in FastAPI this plays out with things like sqlAlchemy providing a global db session object because there's no way to have an instance-based router.
That sounds better suited to a middleware decorator IMO.
No.. func decorator this case will make as well 6x line duplication, and potential security issue if some one forgets to decorate operation...
I've been using middleware for authentication for all requests that match a path prefix. For authorization the rules become more specific - but are usually also grouped with a path-prefix. One declaration and regex match - applied to all requests.
But, even if there's no common prefix or regex match possible, you could add it in the function call that initializes the class decorator. If you took my implementation for FastAPI - you would have an opportunity to pass such a middleware definition at this stage of the decorator lifecycle: https://github.com/tiangolo/fastapi/pull/2626/files#diff-4d573079004a9f3d148baa4658e68e82b8a3d1a95d603fee8177daa92cf65c93R1174.
Then, as you create an instance of the class - apply the requirements to all decorated class functions here: https://github.com/tiangolo/fastapi/pull/2626/files#diff-4d573079004a9f3d148baa4658e68e82b8a3d1a95d603fee8177daa92cf65c93R1141. Note, when this line executes you will have a reference to a fully initialized instance of the view/class that was decorated. So, if you wanted to pass a specific authorization handler into each instance of the class it would be doable and usable for each decorated method on that instance.
@vitalik I'm adding another idea to the mix. The best solution to routing problems I saw was provided by Elixir / Phoenix approach, you can find it here: https://hexdocs.pm/phoenix/routing.html
defmodule HelloWeb.Router do
use HelloWeb, :router
pipeline :browser do
plug :accepts, ["html"]
plug :fetch_session
plug :fetch_flash
plug :protect_from_forgery
plug :put_secure_browser_headers
end
pipeline :api do
plug :accepts, ["json"]
end
scope "/", HelloWeb do
pipe_through :browser
get "/", PageController, :index
end
# Other scopes may use custom stacks.
# scope "/api", HelloWeb do
# pipe_through :api
# end
end
It's like a composable middleware, where we can select which middleware should be used for each view.
It also provides a way to map multiple functions from a module in a standardized way: https://hexdocs.pm/phoenix/routing.html#resources
Maybe it would be possible to somehow utilize that approach here? Do you think something similar would be possible in python? If not, then maybe we could go a bit more into the direction of Django Rest Framework, with ViewSets and standardized routes?
On the other hand... dependencies should make it possible to implement permisions?
Sorry, but your problem with __init__ is due to the fact that you want to use wrong tool to solve a problem of duplication. The point of a constructor is to initialize an instance so that it can function at all, not to reduce duplication in its methods.
What will happen if you make a class with 5 methods and the sixth actually needs a slightly different state? Will you initialize it in the method? This will make the calls in the constructor redundant. Will you make an if in the constructor? This will get ridiculously convoluted in short time when next methods are added. Will you actually go back to a function-based view? Then what's the point of the class in the first place?
Look, the main reason DRF's design is so bad is because they wanted to give too many tools to reduce the duplication. You got class-based views, then viewsets, and serializers that serve five different purposes (at least). If you match the case predicted by the developers then it's cool and pretty, but in reality you have to hack your way through at least 5 different classes to implement what you need.
And when designing a library you always need to keep in mind that people will abuse its mechanics. You'll add classes and in two years someone will go to a new job and find a ginormous class with 30 methods, an __init__ that's impossible to read and they'll open github and create a new tool for REST APIs django-samurai that will not have this problem, mostly due to lack of class-based views.
(Yeah, I may be biased)
The API is basically the "UI" of your backend application. In any big project this layer should be as thin as possible and only call things down the dependency chain. Using class-based views will again encourage the harmful approach of implementing the whole business logic in one place.
What you need to reduce the duplication in your example is Dependency Injection, plain and simple. (precisely how it'd be done in FastAPI).
This is pseudo-code btw
router = Router()
def get_project(request: Request, project_id: Path[int]):
return get_object_or_404(request.user.project_set, id=project_id)
def get_project_tasks(project = Depends(get_project)):
return project.task_set.all()
def get_project_task(task_id: int, project_tasks = Depends(get_project_tasks)):
return get_object_or_404(project_tasks, id=task_id)
@router.get('/project/{project_id}/tasks/', response=List[TaskOut])
def task_list(request, project_tasks = Depends(get_project_tasks)):
return project_tasks
@router.get('/project/{project_id}/tasks/{task_id}/', response=TaskOut)
def details(request, task = Depends(get_project_task)):
return task
@router.post('/project/{project_id}/tasks/{task_id}/complete', response=TaskOut)
def complete(request, task = Depends(get_project_task)):
task.completed = True
task.save()
return task
And you have literally zero problems with anything I mentioned before and you can make a dependency async.
Hi @adambudziak
Thank you for your thoughts
Well the FastAPI approach with dependancy leads to a way "wider" code base that is even harder to read
like let's say in the complete method you also need a project (f.i. to notify all the project owners about completion)
@router.post('/project/{project_id}/tasks/{task_id}/complete', response=TaskOut)
def complete(request, project_id: int, task_id: int, task = Depends(get_project_task), project = Depends(get_project)):
task.completed = True
task.save()
project.notify(task)
return task
and the arguments is no longer readable and you have to force(or forced by black) yourself split arguments into multiple lines, which now sort of looks like class but it's a function... well I do not see this redable
compare it with
class Tasks:
...
@router.post('/{task_id}/', response=TaskOut)
def complete(self, request, task_id: int):
task = get_object_or_404(self.tasks, id=task_id)
task.completed = True
task.save()
project.notify(task)
return task
But indeed - the fact that some one will create a class with 30 methods (which can be actual case if you have 30 methods that operate tasks and project) is definitely path to a not-maintainable code
on the other hand - in case when a single task have lots of methods then it should be extracted to a separate class(es):
@router.path('/project/{project_id}/tasks')
class TaskListActions:
def __init__
def list_tasks(...
def create_task(...
def bulk_action1(...
def bulk_action1(...
@router.path('/project/{project_id}/tasks/{task_id}')
class SingleTaskActions(TaskListActions):
def __init__
def modify(...
def delete(...
def complete(...
def reassign(...
def split(...
@router.path('/project/{project_id}/tasks/{task_id}/attachments')
class TaksAttachments(SingleTaskActions):
def __init__
def list_attachments(...
def add_attachment(...
def remove_attachment(...
so this simple project/task/attachment management case has like 12 methods
and all depends on:
- project (permission)
- task(permission) - subset
- almost every where you should have project on hands to check some settings or logic
the FastAPI approach with dependencies leads to ALOT of arguments in each method and IMHO leads to harder times to maintain (and even read) that
So yeah.. this is controversial topic on a proposal - everyone is welcome to pour thoughts
PS: maybe Class based utility also should be on a separate package
Well, ending up with 12 parameters of a function is certainly an issue. However, whether it's worse or better than a hierarchy of classes is a matter of opinion. My biggest problem with classes is that it's much harder to read a single method in isolation: you always have to keep the implicit state in mind, checkout the constructor, maybe see the parent class(es) etc.
With a plain function you always know that everything that matters is under your def and in the dependencies (parameters) that, if designed well, should be pure functions and easy to understand.
Have a look from a testing perspective: to test a function endpoint with dependencies you don't need to mock anything because the dependencies are just parameters. You pass the parameters and check return value: done.
With a class-based view, when you want to test a single endpoint, you always need to instantiate the class and perform all the burden in __init__. This in most cases will require mocking or patching (because you can't mock the constructor) and will be much harder to understand.
Now, I believe it's all a matter of trade-offs. If you need 10 things to perform an action, then you need them either stated explicitly as a function parameter or also explicitly but in the constructor. The work has to be done somewhere, and in my opinion the class-based approach in the form stated in the issue is just a specialization of DI, have a look at that:
router = Router()
class Context:
def __init__(self, request, project_id: int):
user_projects = request.user.project_set
self.project = get_object_or_404(user_projects, id=project_id))
self.tasks = self.project.task_set.all()
def get_context(request, project_id) -> Context:
return Context(
request, project_id
)
@router.get('/project/{project_id}/tasks/', response=List[TaskOut])
def task_list(context = Depends(get_context)):
return context.tasks
@router.get('/project/{project_id}/tasks/{task_id}/', response=TaskOut)
def details(context = Depends(get_context), task_id):
return get_object_or_404(context.tasks, task_id=task_id)
While I don't recommend this, it's basically equivalent to the functionality provided by a class-based approach. Yes, you still have to declare the parameter in a slightly uglier way, but semantically there's no difference. The main upside of this solution is that now you actually can "mock the constructor".
With DI, you can arrange your dependencies in any manner you wish, you don't have to have 12 one-liner dependencies that are all passed to a function; when it makes sense you can group them together or come up with your own mechanism. The thing is that you as a library creator leave this decision to the user who should1 know best.
By the way, the problem with a non-async constructor is only one of many, I'll try listing them now:
- the constructor has to get all the parameters possible for any of it's methods
- two methods may require different initialization what should lead to separate classes as you said @vitalik
- the constructor performs all the queries in the beginning even though only one of them may be useful for the actual method that's called
Well, to be honest, in an extreme case you would need one class per method to keep this clean.
But all these classes (and this pattern from the proposal in general) violate the single responsibility principle and this is actually the whole root of the problem with the class-based approach.
Let's exercise the following example
router = Router()
class ProjectTasksService:
def __init__(self, user, project_id):
self.user = user
self.project_id = project_id
def get_user_projects(self):
return self.user.project_set.all()
def get_user_project(self):
return get_object_or_404(self.get_user_projects(), id=self.project_id)
def get_tasks(self):
return self.get_user_project().task_set.all()
def get_task(self, task_id):
return get_object_or_404(self.get_tasks(), id=task_id)
@router.path('/project/{project_id}/tasks')
class Tasks:
def __init__(self, request, project_id):
self.service = ProjectTasksService(request.user, project_id)
@router.get('/', response=List[TaskOut])
def task_list(self, request):
return self.service.get_tasks()
@router.get('/{task_id}/', response=TaskOut)
def details(self, request, task_id: int):
return self.service.get_task(task_id)
Now, the advantages:
- each function in the service can be async if needed
- each function can use caching mechanism
- each function can be called only when needed
- for testing purposes you can slightly rework the constructor in the view to allow injecting a service
- you get proper responsibility separation which makes it easier to maintain
- there's basically no repetition
- you can reuse the service in multiple views
But, if you let class-based views in, then you cannot guarantee that users will actually use them in this way2; more likely they will go for the straight-to-hell approach.
Now, the thing is that this Service approach works as well with DI and plain functions:
# ... service without changes
def get_service(request, project_id: int):
return ProjectTasksService(request.user, project_id)
@router.get('/project/{project_id}/tasks/', response=List[TaskOut])
def task_list(service = Depends(get_service)):
return service.get_tasks()
@router.get('/project/{project_id}/tasks/{task_id}/', response=TaskOut)
def details(service = Depends(get_service), task_id: int = Path()):
return service.get_task(task_id)
1 it's very important to me that a library encourages good coding practices. DRF is the opposite of that. If you go for a canonical DRF code design then you basically violate all SOLID principles. If you give programmers tools to write garbage easily or good code the hard way, then all codebases will be terrible.
2 Maybe you could encourage it with expecting a service class in the view base class constructor, but I don't know what a generic service class could look like.
@adambudziak Yeah, the service class passed as dependency looks interesting...
do you think it make sense to automatically detected dependency based on annotation ?
class ProjectTasksService:
def __init__(self, request, user, project_id):
...
@router.get('/project/{project_id}/tasks/{task_id}/', response=TaskOut)
def details(task_id: int, service: ProjectTasksService = Depends()):
return service.get_task(task_id)
hello.
I don't know whether this issue is still up for the discussion but I was wondering on an approach that would be somewhat similar to previously mentioned, but would separate permissions from context. I'll use pseudocode from now on for brevity
so let's start with your example - a set of projects in which we want the current user to only be able to see his own projects
# just the types for autocompletion
class ProjectContext(TypedDict):
project: Project
class RequestWithContext(Request):
context: ProjectContext
# context processor
def retrieve_project(request, project_id: int) -> RequestContext:
return RequestContext(project=Project.objects.get(id=project_id)
# permission middleware
def check_permissions(request: RequestWithContext) -> None:
user = request.auth
project = request.context["project"]
if project.author != user:
raise Http401
router = Router("/projects/{project_id}", middleware=check_permissions, context_resolver=retrieve_project)
@router.get("/")
def project_details(request: RequestWithContext):
project = request.context["project"]
@router.delete("/")
def project_delete(request: RequestWithContext):
...
so here's a brief description of how would this work. we could attach these context resolvers on either a path, or a router and then, prior to visiting a path, a context would be resolved, very similarly to how it's possible to resolve authentication (i.e. by populating request.auth). Then, a middleware would kick in that could either do nothing (if permissions would be met) or it would raise an exception that could be dealt with on the base of api object (exactly like there's a recipe for dealing with server errors in the docs)
the whole RequestWithContext is made purely so that there would be autocompletion support in the editor but as you can see the only thing it does is it just populates an extra property of a request (context) with objects retrieved from the database.
In order to make just a single query (and not 2) I thought that context could be processed prior to checking permissions. otherwise (i.e. if you would be checking permissions prior to resolving context) you'd have to query for project details for the second time just to check it's properties.
@vitalik I am using __call__ for async __init__ feature in my async-files library: https://github.com/Niraj-Kamdar/async-files. This may not be ideal but it's less verbose then other approach and intuitive (or atleast i think it is). In my case though since people generally using it with context aenter...aexit takes care of initialization, the __call__ approach is just for completeness.
Hi there,
@adambudziak 's arguments are very convincing though I have to admit that I'm quite the fan of Django's class based views. What I like about them is there "convention over configuration" aspect.
Maybe following some of Django's class based view naming+usage conventions could be an option for Ninja's class based views?
-
as_viewfor url specific class configuration (in api context,as_route/as_routercould be used) -
dispatchas the main method that handles the request response chain (not__init__...) and all implemented HTTP methods, see django/views/generic/base.py. This works in combination with the methodshttp_method_not_allowed,optionsand others. -
get_queryset- a convention that has spread beyond the class based views, I'd say. -
get_objectfor single objects withpk_url_kwarg/slug_url_kwarg, see django.views.generic.detail.SingleObjectMixin -
get_context_dataas last gate to pimp the response - in an api context, this name is not fitting though. -
paginate_by/page/page_kwarg/orderingfrom django.views.generic.list.MultipleObjectMixin
I understand that sticking to old conveniences can hinder finding new and better solutions. It is a difficult decision.
Thank you for your hard work!
if anybody is interested, here's an implementation that I made that can be used with current ninja codebase:
from functools import wraps
from typing import Callable, List, Optional
from ninja import Router as BaseRouter
class Router(BaseRouter):
middleware: Optional[Callable] = None
context_resolver: Optional[Callable] = None
def __init__(
self,
middleware: Optional[Callable] = None,
context_resolver: Optional[Callable] = None,
*args,
**kwargs,
):
self.middleware = middleware
self.context_resolver = context_resolver
super().__init__(*args, **kwargs)
def add_api_operation(
self,
path: str,
methods: List[str],
view_func: Callable,
*args,
**kwargs,
) -> None:
def run_context_resolver_and_middleware(view_func):
@wraps(view_func)
def inner(request, *args, **kwargs):
if self.context_resolver is not None:
context = self.context_resolver(request, *args, **kwargs)
request.context = context
if self.middleware is not None:
self.middleware(request, *args, **kwargs)
return view_func(request, *args, **kwargs)
return inner
view_func = run_context_resolver_and_middleware(view_func)
return super().add_api_operation(path, methods, view_func, *args, **kwargs)
thanks to this, I can now add middleware and context processors within the router itself:
def check_some_permissions(request, *args, **kwargs):
user, _ = request.auth
if not user.has_perm('my_app.permission_name'):
raise SomeCustomException
def populate_request_context(request, *args, **kwargs):
return {"queryset": MyModel.objects.all()}
my_router = Router(middleware=check_some_permissions, context_resolver=populate_request_context)
@my_router.get("/")
def objects_get(request, *args, **kwargs):
queryset = request.context["queryset"]
you can add typing if you want but I wanted to keep it short and self-explainatory
because routers can be chained and nested I don't think it's a huge issue that the solution still keeps functions as first-class citizens of the API, as opposed to the classes. It's always a compromise, but the main benefit is that with this solution we can do it with the current codebase :)
cheers.
Class base operations take a break from some of the best features from Fast API, en hence Django-ninja:
-
typing is not declarative anymore. Even in your simple example in the first post, all the attributes you used attached to
selfare not typed, because that's just not as convenient to do. -
it's based on side effects: you get to modify the instance attributes to pass data around. This makes things harder to test, and it will likely result in some edge case about references down the road.
But more than that, CBV were one of the biggest Django mistake IMO. They are almost always more verbose and complicated, except for toy examples. They are hard to compose, and reuse. You can't opt in for part of their behavior when you don't need the whole thing. And their worst problem is that you have to know the graph of implicit method calls to override any part of their behavior.
Now you may suggest that class based operations avoid this problem by just offering __init__ as an entry point, but you have an active community, and they will want to provide libs and reusable hooks. Soon you will have an ecosystem with reusable class based operations that will tell users to inherit from this or that, and end up with the same problem. Only you won't be able to fix it, because it won't be in your code base, but in the ecosystem.
I think DRY for API endpoints are best offered using a mix of 3 systems that are independent, separated, but that can be used together if needed:
1 - decorators for any check prior or clean up after the view (auth, permission, etc) like django offers (E.G: @login_required). The decorator should be required to use @functools.wraps to maintain the view annotations so that django-ninja can introspect them, and to transpose *args, **kwargs to the wrapped function. The later point is now possible with Python 3.10 param spec (https://www.python.org/dev/peps/pep-0612/), as it will play well with types. But it's possible to provide a wrapper to hide that from the user behind an API similar to pytest @fixture.
Mock example:
from ninja import viewdecorator
from my_code import user_owns_projects_or_403
# Easy to use, no need to declare or inherit from a class
# Setup and tear down are in the same place
# No side effects, it's easier to tests and see where things
# come from and go to
@viewdecorator
def user_owns_project(request, project_id: int) :
# this runs before the view
user_own_projects_or_403(request, project_id)
response = yield
# this runs after the view
return response
# You can compose decorators by stacking them, no MRO
# Order is clear
@api.get('/project/{project_id}/tasks', response=List[TaskOut]))
@user_owns_project()
def task_list(request, project_id: int)
...
2 - sub dependencies from the DI system for anything that needs to be calculated and returned (E.G: https://fastapi.tiangolo.com/tutorial/dependencies/sub-dependencies/). This is very flexible, it composes infinity without the risk of creating a diamond graph like inheritance, and you can see the injection entry point and the chain of causality, so it's explicit.
Mock example:
from ninja import Depends
# This part is easy to test, no side effect
def projects_and_tasks(
request, project_id=int
):
user_projects = request.user.project_set
project = get_object_or_404(user_projects, id=project_id))
tasks = project.task_set.all()
return project, tasks
# You know where things come from, the call graph is unidirectional
@api.get('/project/{project_id}/tasks', response=List[TaskOut]))
def task_list(request, projects_and_tasks: tuple[Project, Task] = Depends(projects_and_tasks))
projects, tasks = projects_and_tasks # this can be made even 2 separate DI, I was just lazy
return tasks
# No new API to learn, it works as the rest of django-ninja
@api.get('/project/{project_id}/tasks/{task_id}/', response=TaskOut))
def task_list(request, task_id: int, projects_and_tasks: tuple[Project, Task] = Depends(projects_and_tasks))
projects, tasks = projects_and_tasks
return get_object_or_404(tasks, id=task_id)
3 - a routing system to group API calls with the same url if you really don't want to repeat a part of the url. We can use a context manager for it, or something else (E.G: curring).
Mock example:
# If you just want to reuse part of the URL, you can just use this part
# No need for a whole class if that's just DRY for the URL you wish for
with api.route('/project/{project_id}/tasks') as task_route:
@task_route.get(response=List[TaskOut]))
def task_list(request, projects_and_tasks: tuple[Project, Task] = Depends(projects_and_tasks))
return tasks
@task_route.get('{task_id}/', response=TaskOut))
def task_list(request, task_id: int, projects_and_tasks: tuple[Project, Task] = Depends(projects_and_tasks))
return get_object_or_404(self.tasks, id=task_id)
Since they solve different problems, you don't force the user to pay the full price for everything by using a class, if you just want to say: reuse some code, or just reuse the URL. You can use the context manager or the injection or the decorator for just one problem. Because the 3 systems are independent, they are also easier to compose the way you want. And you can combine the 3 to get exactly the behavior that the class provides.
If you use either separately, they are incomplete. But if you use all of them, you cover all main needs, with a system that is way less complicated for the end user.
The coder providing the injection or decorator still have to do some work, but that's always the case, even with class base operations. Plus, it's possible to provide tooling to make it easier. But the end users can just say "give me this", I'll do some calculation, and reuse it. It has worked great for FastAPI and pytest so far.
No need for state diagrams to understand what's going on, no fear of hitting some MRO edge case, and a more declarative API. And best of all, when an error will crash the server, it will be much easier to put a breakpoint where the problem occurs than to figure out where the heck you should do somewhere in the maze of methods and parents.
Is it really necessary to do any async stuff in __init__? I typically want __init__ to be fast and therefore don't benefit from the asynchronous functionality. For everything else, why not just use async method or property? Applied to the example in the first post, this would look like this:
from ninja import Router
router = Router()
@router.path('/project/{project_id}/tasks')
class Tasks:
@property
async def tasks(self):
if not hasattr(self, '_tasks'):
self._tasks = await get_tasks_asynchronously()
return self._tasks
@router.get('/', response=List[TaskOut])
async def task_list(self, request):
return await self.tasks
@router.get('/{task_id}/', response=TaskOut)
async def details(self, request, task_id: int):
return get_object_or_404(await self.tasks, id=task_id)
Note: functools.cache_property cannot be used here because coroutine can't be awaited multiple times. That's why I added the caching manually.
:+1: for using DI. Right now, I'm looking for a replacement for get_queryset:
from ninja import Depends
def get_queryset(request) -> QuerySet[Project]:
return Project.objects.filter(owner=request.user)
@router.get("/", response=List[Project])
def list_ip_transit(request, get_queryset: QuerySet[Project] = Depends(get_queryset)):
return get_queryset
@router.get("/{project_id}", response=Project)
def list_ip_transit(request, get_queryset: QuerySet[Project] = Depends(get_queryset), project_id: int):
return get_queryset.get(id=project_id)
There is a package that implement this: https://github.com/eadwinCode/django-ninja-extra
