HackSoftware / Django-Styleguide

Django styleguide used in HackSoft projects
MIT License
5.12k stars 520 forks source link

Why Overloaded POST over PUT/PATCH/DELETE? #148

Closed frontendschmacktend closed 5 months ago

frontendschmacktend commented 9 months ago

As a startup founder halfway through building my first fully productionized Django API, I just want to say this might be the single most useful Django-related document on the internet right now, major kudos for putting this together from your battle scars over the years. I've read through all the rebuttals to this styleguide and they all miss the point: simplicity scales. Because of this, we are very careful to make sure we understand why you make certain choices before deviating from them when they don't make sense to us.

HomaDev commented 9 months ago

@frontendschmacktend Using only Post and Get really suspicious, and I want to know why to. But did I understand correctly, that you want to include all methods for one model under one ApiView instead of concept "one veiw - one endpoint"?

frontendschmacktend commented 9 months ago

Yes that's correct, any thoughts on the pros/cons of that if we still keep each method's code minimal in the views.py file as is in the styleguide?

HomaDev commented 9 months ago

Yes that's correct, any thoughts on the pros/cons of that if we still keep each method's code minimal in the views.py file as is in the styleguide?

  1. Isolation - you are isolating one api from another and don't have strong connection to model, if you are making big project I guaranty you will have endpoint that don't fit in model-view paradigm where you combine different models, nest objects in different way or something like this. It will separate your app on different styles some endpoint will be "model based" and others will look like strange add-hoc methods, and even after 15-20 endpoints program probable will look messy.
  2. You will have to use ViewSet, because with APIView you can only have one of each methods in one class, because of it you will have to create way more than one APIView, and it will be the same as in design but with no straight structure. For example you need 4 get methods to retrieve item, to list items, and list items without particular fields, and one more with a lot of nested objects. It's not a big problem with base ViewSet, you will have 4 methods, like list, retrieve, list_minimal, list_nested (just for example didn't think on naming to much), that will be routed automaticly. I don't know about abstruction here, they will or will not cause some problems in the future.
  3. Problem with a lot of methods in one ViewSet. In my app that is just being developed, we have endpoint like this:

    class ItemListApi(APIView, ItemSelector):
    class Pagination(LimitOffsetPagination):
        default_limit = 1
    
    class FilterSerializer(serializers.Serializer):
        stock = serializers.ListField(
            child=serializers.CharField(),
            required=False
        )
        price_max = serializers.IntegerField(required=False)
        price_min = serializers.IntegerField(required=False)
    
        order_by = serializers.CharField(default='price')
    
        category__id = serializers.ListField(
            child=serializers.IntegerField(),
            required=False
        )
    
    class OutputSerializer(serializers.Serializer):
        id = serializers.IntegerField()
        name = serializers.CharField(max_length=128)
        price = serializers.FloatField()
        discounted_price = serializers.FloatField(allow_null=True, required=False)
        length = serializers.FloatField(allow_null=True, required=False)
        height = serializers.FloatField(allow_null=True, required=False)
        width = serializers.FloatField(allow_null=True, required=False)
        stock = serializers.ChoiceField(choices=ItemModel.STOCK_STATUS_CHOICES)
        mini_description = serializers.CharField(max_length=2500)
        image_set = inline_serializer(many=True, fields={
            'id': serializers.IntegerField(),
            'image': serializers.ImageField()
        })
        category = inline_serializer(fields={
            'id': serializers.IntegerField()
        })
        mini_image = serializers.ImageField(allow_null=True, required=False)
        slug = serializers.SlugField(max_length=100)
        stars = serializers.FloatField(default=0)
        review_count = serializers.IntegerField(default=0)
    
        class Meta:
            ref_name = 'shop.ItemListOutputSerializer'
    
    @extend_schema(
        responses={200: OutputSerializer()},
        parameters=[
            OpenApiParameter(
                name='category__id',
                location=OpenApiParameter.QUERY,
                description='category id',
                required=False,
                type=int,
                many=True
            ),
            OpenApiParameter(
                name='stock',
                location=OpenApiParameter.QUERY,
                description='stock state of item (you can select more then one by holding control button',
                required=False,
                type=str,
                many=True,
                enum=('IN_STOCK', 'OUT_OF_STOCK', 'BACKORDER', 'SPECIFIC_ORDER')
            ),
            OpenApiParameter(
                name='price_min',
                location=OpenApiParameter.QUERY,
                description='minimal price',
                required=False,
                type=int,
                many=False
            ),
            OpenApiParameter(
                name='price_max',
                location=OpenApiParameter.QUERY,
                description='max price',
                required=False,
                type=int,
                many=False
            ),
            OpenApiParameter(
                name='order_by',
                location=OpenApiParameter.QUERY,
                description='order by something',
                required=False,
                type=str,
                enum=('price', '-price')
            ),
            OpenApiParameter(
                name='limit',
                location=OpenApiParameter.QUERY,
                description='how many objects you get',
                required=False,
                type=int
            ),
            OpenApiParameter(
                name='offset',
                location=OpenApiParameter.QUERY,
                description='page of objects',
                required=False,
                type=int
            ),
        ],
    )
    def get(self, request):
        filters_serializer = self.FilterSerializer(data=request.query_params)
        filters_serializer.is_valid(raise_exception=True)
        print(filters_serializer.validated_data)
        items = self.item_list(filters=filters_serializer.validated_data)
    
        return get_paginated_response(
            pagination_class=self.Pagination,
            serializer_class=self.OutputSerializer,
            queryset=items,
            request=request,
            view=self
        )

    Even it is just developed api it is quite big and with time it will became a lot larger. Do you really want something like 20-40 of this in one ViewSet class? Of course with no schema it will be not that big. Also you will need to declare Pagination, Filter, Input and Output serializer classes inside of methods, and I don't know how oop standards feels about it.

HomaDev commented 9 months ago

@frontendschmacktend (forgot to point in upper comment)

HomaDev commented 9 months ago

But really @RadoRado, why should't we use all needed methods? post on CreateApis, patch on UpdateApis, delete on DeleteApis?

RadoRado commented 9 months ago

@frontendschmacktend / @HomaDev I just saw this issue :see_no_evil:

Reading everything & answering accordingly.

RadoRado commented 9 months ago

The biggest one is the format you chose for your endpoints, to create many many endpoints that are only either GET or POST endpoints vs keeping the number of endpoints low and having multiple methods (like GET/POST/PUT/PATCH/DELETE) per endpoint. Here are a few reasons we are choosing not to follow you on this and have the methods (still each with its own serializer) under one view, please help us understand your reasoning on this topic:

I think this is most often the case with folks checking the styleguide.

we have no problems with either approach.

Perhaps, our single biggest reason to stick to the "1 API per action per endpoint" is because it's simple - the API is doing 1 thing.

The other one is your preference of Serializers over ModelSerializers. What is the benefit of having to manually change a field across many serializers vs just inheriting from the model where you can change it once and it changes everywhere? This way you know each API is going to read/write this field to the database in the right format.

If you find ModelSerializer more convenient - simply stick to it!

The way we reason about serializers is that they define the input / output interface of an API, and we don't necessarily want to have that 1:1 relation with a model.

Sometimes - it's a model. Other times - it's something else. So in order to keep a clear distinction - we prefer the pure Serializer class.

As an aside, any best practices you have around automating drf-spectacular to generate OpenAPI specifications in SwaggerUI would be appreciated. For example we're working on adding example request/response JSON bodies to each endpoint so our frontend team already have a full example working when getting started.

I think this issues gives the best approach:

  1. https://github.com/HackSoftware/Django-Styleguide/issues/134
  2. https://github.com/HackSoftware/Django-Styleguide/issues/105
RadoRado commented 9 months ago

@HomaDev About the HTTP verbs <-> APIs - it's a matter of taste. As long as you keep things consistent - use whatever makes sense to you.

For me, personally, unless I'm building an API that needs to be RESTful and consumed by unknown clients - I tend to stick to GET / POST, since this covers the "read data" vs. "write data" paradigm.

RadoRado commented 5 months ago

I'm closing this issue for now. Feel free to reopen or simply open a new one :raised_hands: