[proposal] Class Based Operations #15
Comments
|
@vitalik About the 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 |
|
yes, this seems the only option... (without the classmethod), the onlyl thing I struggle how to name that method: my shortlist so far
|
|
@vitalik First option |
|
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 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.querysetUsage:
|
|
Just use functions... |
|
It is a nice feature, When does it come out? |
|
I solved this in a pretty nice way for FastAPI but they just give me 👎 and no discussion @tiangolo please consider it 🦗: fastapi/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.textThe 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 |
|
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
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 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. |
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
endIt'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 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 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 (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 taskAnd 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 @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 taskand the arguments is no longer readable and you have to force(or forced by 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 taskBut 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:
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 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 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:
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:
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 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 |
|
@adambudziak 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) |
|
Yes, certainly |
|
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 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 ( 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. |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
|
@vitalik I am using |
|
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?
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! |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
|
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. |
|
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: If this is allowed in django-ninja, the first thing that will happen is you'll have people misuse the pattern: 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 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. |
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. |
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. |
|
omg. this design should be applied in hurry. |
|
I honestly don't like the idea, because it brings the following problem. |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
|
Has it been considered to solve this problem from the level of APIView that comes with django, when identifying resources with an API, writing as a class is a good solution. |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
|
Object oriented programming it's not necesary when doing an api for a 9 to 5 job, but is for creating libraries and reusable stuff. This feature is what will make me switch from fastapi to django ninja as tiagnolo refuses to include it in his library. |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
|
@vitalik |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
|
Updates? |
Because tiangolo is smart. Including this is basically giving everyone the tools to break most good practices. Class based views are not needed at all ever, If you need to encapsulate common logic, just use Repository pattern or service classes, anything. Api endpoints should basically only do:
That should be the base of each of the endpoints. And for this we don't need class based views. |
|
This would be great. I have various permissions mixins on my CBVs which I now have to rewrite somehow to make them compatible with view functions. It'd be much simpler if I could just reuse them wholesale, or at least keep a similar approach. As it is I'll have to have multiple decorators on my functions which will be jarring to others working on the code. I hope this gets added. |
|
你的邮件我已收到，谢谢。如有必要，请联系：13794627738
|
A few months later just realized that all of you were right. There is no need for class based views. As you said, the api should only be a thin layer on top of a repository and should not contain any state or logic so classes are not necessary. |
|
To provide an alternative viewpoint - thin views and class based views are not mutually exclusive. Nothing forces or encourages you to have large class views. You can have class views that are extremely thin, with all business logic happening in service layers based on a class variable set on the child using a rigid service layer API. The only thing the views do are the three steps pointed out above. I usually see more bad code with business logic jumbled up in function based views than class based; bad code is easy to write in both and I would argue it is easier to fall into that trap with function based views, especially for junior devs. Class based just gives people additional code structure options to allow them to scale as they choose. |
yes, and to make code unreadable in more ways. If you give a gun to a monkey, and it shoots you, is it its fault or yours? Vitalik implementing class based views is like giving a gun to us (monkeys). Of course it's possible to make bad implementation for function-based views, but it's still nowhere near the horror that you can make with multiple inheritance, where each layer overwrites part of superclass methods, adds a few extra, builds an implicit state in the constructor that's changed all over the place, and then on top of that all, you have two extra mixin classes that also add some stuff. Changing just one thing requires reading the documentation or digging into the code to understand how it all works. If your use case is not documented then you can be sure to spend a few hours on some simple stuff. And I'm not even exaggerating, that's how Django Rest Framework looks like even before you start sprinkling extra libraries on top of it. |
|
Nice first step. Then please implement the CRUD mixins, like in DRF. |
Please don't. I always disliked those implicitly defined APIs where you always have to check the source code of the base classes you want to use. |
Frankly, if a feature exists, nobody forces anyone to use it. There are plenty of people who find it useful. You won't argue with that, I hope. Everything else is a matter of personal preference. |
|
This is a great idea! In my current project for example, I have many endpoints that share common path parameters and permission based logic driven by those path parameters. E.g. API users have different permissions at different facilities. At Facility X, User A might have admin privileges, and only basic privileges at Facility Y. /facilities/{facility_id}/operators/{operator_id}/tasks This is what you could call a "class of endpoints" in the common sense. What @vitalik is proposing is a VERY elegant way of supporting a common RESTful practice by using what classes are meant for. Having shared code that authorizes a user based on a class of endpoints would be DRY and really reduce headaches. Is there a way to accomplish code sharing elegantly right now without a class-based approach? Is there a hierarchical way to define path params that every child operation uses? And logic that gets run for an entire set of endpoints baed on those common path params? Dogmatic claims about OOP and class-based approaches are really absurd. Use whatever approach you want. Do not strain a framework because some developers don't follow best practices for whatever reason. This isn't even "OOP" anyway. In any trade, there is disagreements about which tools are best. That doesn't mean you shouldn't make tools that some know how to take advantage of correctly because some tradespeople prefer different ones or are less experienced to make good use of them. Many frameworks out there explain that certain approaches should only be used in certain circumstances, or have "more advanced" approaches hidden in submenus in the docs etc. If you are very concerned about many users of the framework adding endpoints to a class of endpoints that doesn't actually benefit from the shared logic, you could put a warning in the docs about this. Even that IMO is unnecessary because it's just a principle of software design. If something doesn't relate to a class, it doesn't belong in the class. You aren't being forced to migrate everything to a class-based approach, you are just given the option. And if your workplace allows developers to copy and paste code without understanding the consequences or doesn't have proper code review processes to make sure that e.g. initializer methods are light weight, then thats a different issue. |
While it's true that nobody forces anybody to use anything, there's only limited amount of time and effort that open source maintainers can and want to invest into their projects. By working on class based operations, something else will inevitably be put on the backseat. I think that it would be ideal if group of voluteers that want class based operations in |
It couldn't be more simple than this brilliant implementation, that could be simply adapted/accepted as a part of a core. https://github.com/hbakri/django-ninja-crud ATM, without it, it's just too much code, which means more tests to write, more maintenance. That's why DRF is quite comfortable to work with, but unfortunately it does not (and will never) support async, which forces me to look elsewhere (and thus back to this package). You could of course offer everything as a third-party package, but that increases the dependency-hell quite drastically. Honestly, I don't see how/why CRUD and class-based views are hard to maintain. Every mature framework has it. Django has it too. Why not here? |
There is already django ninja extra which I think fulfils this role perfectly :) |
|
I also think that a separate package would be a better approach. personally, I'd love to see something like django-core that would only have the apps ecosystem and management commands with the orm and migrations (so no admin panel, no templating system, no forms, etc). On the other hand, somebody with a thicker ockham's razor might say that not every project uses a database and migrations and that they would not belong in a "core" package :D my point is that the situation with Django's admin panel and templates is similar as this discussion - it is there, in the framework and nobody is forcing anybody to use it but this does not change the fact that you are still downloading it each time you start a project, even though chances are you will not use it. |
and others
Read full proposal here - http://django-ninja.rest-framework.com/proposals/cbv/
To allow incapsulate common logic into class methods
The text was updated successfully, but these errors were encountered: