[proposal] Class Based Operations

[vitalik]

[proposal] Class Based Operations

[lorddaedra]

Just use functions...

[FJLendinez]

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)

[ksamuel]

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 self are 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.

[adambudziak]

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 should¹ 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:

1. each function in the service can be async if needed
2. each function can use caching mechanism
3. each function can be called only when needed
4. for testing purposes you can slightly rework the constructor in the view to allow injecting a service
5. you get proper responsibility separation which makes it easier to maintain
6. there's basically no repetition
7. 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 way²; 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)

---

¹ 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.

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

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.

[denizdogan]

Regarding the proposal:

Please, don't do it. In my opinion, the introduction of class-based views in Django was a huge mistake.

• Classes carry state, making code with classes inherently difficult to reason about. (What's the state of the object at any given time?)
• Functions don't carry state, making them comparatively easy to understand.

Of course, in Python you're allowed to do whatever the heck you want, so this is not 100% true in this case, but this is generally how we're used to thinking about things.

In your (admittedly contrived) example, the issue is that we need to repeat this code:

user_projects = request.user.project_set
project = get_object_or_404(user_projects, id=project_id)

What's so bad about this solution?

# utils.py
def get_project_or_404(request, project_id):
  user_projects = request.user.project_set
  return get_object_or_404(user_projects, id=project_id)

Adding classes for something like this just overcomplicates things. Code repetition is only bad when it's excessive. Sometimes repeating yourself is a good thing.

In your proposed solution, things look nice and dandy, but you're completely glossing over the new shared state between the routes. A lot of programmers will be asking themselves exactly when the class is instantiated, who owns the object, and when it will be destroyed. And will some outsider, perhaps the object owner, modify the state of it at any point? What if there's suddenly a growing ecosystem of Django Ninja plugins to extend its functionality, what are those allowed to do with the state? (At least we should be ourselves asking these things...)

After having written this out, I see that @ksamuel raised pretty much the exact same concerns, so I'll stop here and refer to their comment.

[vitalik]

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...

[vitalik]

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

[toudi]

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.

[baseplate-admin]

@vitalik have you considered looking to Django-ninja-extra??

I think parts of the code should be merged into django-ninja itself

[wesleykendall]

I'm very much aligned with the sentiments expressed by @denizdogan. Class-based views were a huge mistake in Django, and the patterns were copied in DRF, even allowing people to create and update models from serializers 🤮

IMO the proposal encourages anti-patterns at organizations without having a meaningful impact on simplifying or reducing code that can be implemented with other approaches. Let's go back to the initial code example:

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

If this is allowed in django-ninja, the first thing that will happen is you'll have people misuse the pattern:

@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('/{task_id}/', response=TaskOut)
    def details(self, request, task_id: int):
        return get_object_or_404(self.tasks, id=task_id)

The class above is useless for one endpoint, but this is what will ultimately end up happening at a larger organization where people copy/paste API endpoints as starting points and repeat patterns. They'll stuff things in __init__, they'll do huge prefetches and joins that are useless for the majority of endpoints, and it will just be DRF ViewSets all over again.

Please don't let this in this beautiful library. I think @ksamuel 's outlined approach is much more explicit, easier to test, and harder to abuse.

[Adrian-at-CrimsonAzure]

The class above is useless for one endpoint, but this is what will ultimately end up happening at a larger organization where people copy/paste API endpoints as starting points and repeat patterns. They'll stuff things in __init__, they'll do huge prefetches and joins that are useless for the majority of endpoints, and it will just be DRF ViewSets all over again.

I don't see "people can misuse this to write bad code" as an argument against adding a useful design paradigm. If you're following proper REST or CRUD principles both DRF ViewSets and Django Ninja Extra Controllers are just as easy to maintain and test as the function-based approach. People misuse Django Signals and Messages all the time, but that doesn't mean they're any less useful when the task at hand benefits from them.

I personally use Ninja Extra and, other than a few quirks, quite enjoy it. I don't think it should be directly merged into this project, but instead should be reimplemented to allow both function and class-based APIs to take advantage of the features it offers.

[vitalik]

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

[denizdogan]

@monkut It's fair to want to "group" functionality into logical pieces, but you definitely don't need classes for it. You can do exactly that using just functions, more specifically higher-order functions, such as for example decorators. It is easier to test, easier to maintain, simpler to understand, etc.

I don't agree with the sentiment behind saying you'd have to "additionally add decorators everywhere". I don't understand how introducing classes would be any better. At some point you will have to write some code that does something.

[femiir]

@vitalik have you considered looking to Django-ninja-extra??

I think parts of the code should be merged into django-ninja itself

same thing....I think he should be a contributor and it will help bring Django-ninja to a bigger audience he has things like ninja-jwt which will go a long way in making people consider ninja as their goto option... I have decided to start using Ninja for all my api's in Django and python...

and will love to sometimes have the flexibility of knowing I can use the class based options for certain tasks without writing separate functions or separate classes with methods...

[rhzs]

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

[vitalik]

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...

[skalecki-imblox]

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

[vitalik]

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

[omidraha]

There is a package that implement this:
https://github.com/eadwinCode/django-ninja-extra

[femiir]

@vitalik have you considered looking to Django-ninja-extra??

[baseplate-admin]

that's exactly why I feel it will make a lot of sense to merge them both and then continue from there

I am against this. Because django-ninja-extras is very rest-framework-y. We have to extract the modules and then add them here

[rajaiswal]

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

[vitalik]

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

[rajaiswal]

@vitalik First option init (without _ ) sounds good since that is what we're trying to replace.

[lqx131499]

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.

[erhuabushuo]

It is a nice feature, When does it come out?

[Kojiro20]

I solved this in a pretty nice way for FastAPI but they just give me 👎 and no discussion @tiangolo please consider it 🦗: tiangolo/fastapi#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.

[Kojiro20]

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).

[Kojiro20]

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.

[Kojiro20]

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.

[adambudziak]

Yes, certainly

[toudi]

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.

[lqx131499]

你的邮件我已收到，谢谢。如有必要，请联系：13794627738

[Niraj-Kamdar]

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

[nuarhu]

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_view for url specific class configuration (in api context, as_route/as_router could be used)
• dispatch as 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 methods http_method_not_allowed, options and others.
• get_queryset - a convention that has spread beyond the class based views, I'd say.
• get_object for single objects with pk_url_kwarg/slug_url_kwarg, see django.views.generic.detail.SingleObjectMixin
• get_context_data as last gate to pimp the response - in an api context, this name is not fitting though.
• paginate_by/page/page_kwarg/ordering from 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!

[lqx131499]

你的邮件我已收到，谢谢。如有必要，请联系：13794627738

[lqx131499]

你的邮件我已收到，谢谢。如有必要，请联系：13794627738

[stinovlas]

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.

[sebastian-philipp]

👍 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)

[monkut]

I'm coming from the DRF world, and looking for something a little ligther.

Django Ninja looks appealing, but I have a big API, and really want to be able to group operations on a single resource together in a class without having to additionally add decorators everywhere. I'd rather subclass a base class and have the base class handle all the heavy lifting by providing overridable sensible defaults.

@FJLendinez proposal looks the most appealing to me.

But, I think routing and auth could/should be applied externally to the class.

I really want to be able to write something like this:

class CustomerAddressAPI(ApiModel):
    queryset = CustomerAddress.objects.all()
    schema = CustomerAddressSchema

[lewoudar]

Hi everyone, just to add grist to the mill, I want to share how starlite handles class-based operations. This project also adopts a different strategy regarding routing.
After I agree that we could manage without it, I organize my code very well with FastAPI and the dependency injection system.

Edit: I also discovered the project django-ninja-extra. I think we can take inspiration from it for class-based operations and permissions. 🙃

[JoshYuJump]

I like it

[baseplate-admin]

and will love to sometimes have the flexibility of knowing I can use the class based options for certain tasks without writing separate functions or separate classes with methods...

I dont have a very strong opinion about this. I think for now django-ninja-extra is a good enough option.

What i really am interested in is utility classes from django-ninja-extra

[femiir]

and will love to sometimes have the flexibility of knowing I can use the class based options for certain tasks without writing separate functions or separate classes with methods...

I dont have a very strong opinion about this. I think for now django-ninja-extra is a good enough option.

What i really am interested in is utility classes from django-ninja-extra

that's exactly why I feel it will make a lot of sense to merge them both and then continue from there

[femiir]

that's exactly why I feel it will make a lot of sense to merge them both and then continue from there

I am against this. Because django-ninja-extras is very rest-framework-y. We have to extract the modules and then add them here

I didn't get this can you explain what you mean by is very rest-framework-y

[baseplate-admin]

I didn't get this can you explain what you mean by is very rest-framework-y

django-ninja-extras is configured from settings.py. While most of NINJA doesn't require modifying the params from settings.py but instead passing items as decorator.

[adambudziak]

If you're following proper REST or CRUD

I don't really know what a proper CRUD is, but I'm pretty sure "properness" of REST is quite orthogonal to the mess you can make implementing it (and I hope you mean returning HTML from REST endpoints and not JSON).

Anyway, I don't really see why people love class-based views so much, could you provide an actual example of something that is unachievable with DI and routers? It can be anything, something you consider more elegant, more cool, more efficient, more maintainable.

[changja88]

omg. this design should be applied in hurry.
Currently, django-ninja has no native way to capsule realated api defs.

[acuriel]

I honestly don't like the idea, because it brings the following problem.
Let's suppose we want to create a set of CRUD operations for a model, and we want to encapsulate the common logic in the __init__ or any of the proposals.
First problem. There is no need to query the database for instances if the action is being create, delete, update. And even if we guarantee that this logic is executed only for some actions, then it doesn't have sense to have it in a common place
Second problem: Let's suppose we have a real common logic between those actions. We use this feature, but then the logic for one of those actions changed, and it's not so common. That means we have to split again the class operation into functions, and honestly, most people will just find an ugly workaround like if action == 'action_name'... because it takes less time.
It might be better just to encapsulate from the beginning the logic in common function. Ideally the endpoint functions shouldn't even deal with models directly.
I know the use of this is optional, but one of the main reason I'm loving this library so much, is because is not making the same mistakes DRF made, and I hate DRF :D

[lqx131499]

你的邮件我已收到，谢谢。如有必要，请联系：13794627738