hacksoftware / django-styleguide Goto Github PK

if I used a nested serializer in API views is that doesn't bad for performance, code design, and clean code approach ??!!

There are the following serializers:

class ItemSerializer(serializers.Serializer):
    id = serializers.IntegerField()
    count = serializers.DecimalField(max_digits=12, decimal_places=2)
    is_removed = serializers.BooleanField()


class DocumentSerializer(serializers.Serializer):
    comment = serializers.CharField()
    items = ItemSerializer(many=True, required=True, allow_empty=False)

Or

class DocumentSerializer(serializers.Serializer):
    comment = serializers.CharField()
    items = inline_serializer(many=True, fields={
        'id': serializers.IntegerField(),
        'count': serializers.DecimalField(max_digits=12, decimal_places=2),
        'is_removed': serializers.BooleanField()
    })

I expected the following error formatting on an invalid request:

{
    "errors": [
        {
            "message": "This field is required.",
            "code": "required",
            "field": "items.0.is_removed"
        },
        {
            "message": "This field is required.",
            "code": "required",
            "field": "items.1.is_removed"
        }
    ]
}

But I got:

{
    "errors": [
        {
            "is_removed": [
                {
                    "message": "This field is required.",
                    "code": "required"
                }
            ],
            "field": "items"
        },
        {
            "is_removed": [
                {
                    "message": "This field is required.",
                    "code": "required"
                }
            ],
            "field": "items"
        }
    ]
}

What's wrong? thanks

Why are some of the URLs in the example model null=True, while others are not?

    repository = models.URLField(blank=True)
    video_channel = models.URLField(blank=True, null=True)
    facebook_group = models.URLField(blank=True, null=True)

After all, in general it's considered bad practice to have string fields with two distinct falsy values ('' and None for a nullable string) unless being able to differentiate between "no value set" and "empty value set" is a business requirement, or there's an uniqueness constraint on the field.

According to https://django-celery-beat.readthedocs.io/en/latest/ if we have multiple periodic tasks executing i.e every 10 sec, then they should all point to the same schedule object. Inside the source code of setup_periodic_tasks.py, we create new schedule object each time using .create instead of get_or_create.

Not sure if this has any significance on beat scheduler correctness but decided to make an issue here on Github anyway.

From looking through your guide I am trying to understand how you would handle updates within a service. From your example below:

class CourseUpdateApi(SomeAuthenticationMixin, APIView):
    class InputSerializer(serializers.Serializer):
        name = serializers.CharField(required=False)
        start_date = serializers.DateField(required=False)
        end_date = serializers.DateField(required=False)
    def post(self, request, course_id):
        serializer = self.InputSerializer(data=request.data)
        serializer.is_valid(raise_exception=True)
        update_course(course_id=course_id, **serializer.validated_data)
        return Response(status=status.HTTP_200_OK)

You advocate using keyword-only arguments so I would assume the method definition would be as so:

def update_course(
    *,
    course_id=course_id,
    name:str,
    start_date:datetime,
    end_date:datetime,
) -> Course:

In your input serializer not all fields are required, so they may not be available when calling the course_update from the API using the serializer data. So I am assuming you are going to give them a default of None inside of your service as so:

def update_course(
    *,
    course_id=course_id,
    name:str = None,
    start_date:datetime = None,
    end_date:datetime = None,
) -> Course:

If this is the case how do you handle the update? Do you perform checks on each of the arguments inside the update_course to see if they are None, to then update only those with values?

Cheers!

I browsing in the guide and found the code block.

class CourseDetailApi(SomeAuthenticationMixin, APIView):
    class OutputSerializer(serializers.ModelSerializer):
        class Meta:
            model = Course
            fields = ('id', 'name', 'start_date', 'end_date')

    def get(self, request, course_id):
        course = get_course(id=course_id)

        serializer = self.OutputSerializer(course)

        return Response(serializer.data)

And I'm wondering what does that get_course contains

I believe that get_course will be in selectors.py

and so

selectors.py

def get_course(*, id=None):
    try:
        course = Course.objects.get(id=id)
    except Course.DoesNotExists:
        raise Http404 # ??? is this right or another ValidationError?

As per the exception handler mixin

class ExceptionHandlerMixin:
    ...
    expected_exceptions = {
        ...
        PermissionError: rest_exceptions.PermissionDenied
    }

Shouldn't django.core.exceptions.PermissionDenied map to rest_exceptions.PermissionDenied?

Hi guys, great style guide ♥️!

I just wanted to propose an addition to the exception handling in the APIs section.

Using mixins is a great way to minimize code duplication but that means that all the APIs must inherit from the same mixin every time. To optimize even further a global exception handler can be used as described here -> https://www.django-rest-framework.org/api-guide/exceptions/#custom-exception-handling.

Using the code in the style guide, we can move the handle_exception method to another package and make some minor changes to the code.

from django.core.exceptions import ValidationError

from rest_framework.views import exception_handler
from rest_framework import exceptions as rest_exceptions

EXPECTED_EXCEPTIONS = {
    ValidationError: rest_exceptions.ValidationError
}


def handle_exception(exc, context):
    if isinstance(exc, tuple(expected_exceptions.keys())):
        drf_exception_class = EXPECTED_EXCEPTIONS[exc.__class__]
        drf_exception = drf_exception_class(get_error_message(exc))

        return exception_handler(drf_exception, context)

    return exception_handler(exc, context)

get_error_message is unchanged.

The last step is to register the exception handler:

REST_FRAMEWORK = {
    'EXCEPTION_HANDLER': 'path.to.handler.handle_exception'
}

And voilà!

This has been tested on Python 2.7 and everything works as expected, would love the feedback!

Cheers!

P.S. Venco 🔝

Hello,

How we can setup builtin rest framework documentation features?
For example Browsable API. Overriding get_serializer in every APIView sounds boring.

Thank you

Quite an important topic.

For Eg:
We have permission:
Only user can access to this resource, where and how to create the permission: can_access_resource(user)?

Currently, I am doing it in selectors it returns true or false and depending on that I raise an exception in Services.

Check comments here - #8

I watched @RadoRado presentation about Django Structure from EuroPython Conference 2018 and found it very interesting.
His talk centered around Django structure with an REST API but I was wondering if you have used graphql inside this structure? If not, could you provide some insight on how you would fit it in?

Hi,

A general use case for the product I'm building is returning different information for an endpoint depending on the type of user querying it. Before adopting this styleguide we handled this by creating different serializers and using them according to the user making the request.

How do you handle this use case?. Creating multiple OutputSerializers inside the view kind of pollutes it. Or maybe some kind of dynamic serializer is recommended?.

Thanks for the great work.

What do you think about moving all the logic from custom clean() methods on the model to the db constraints?
https://docs.djangoproject.com/en/2.2/ref/models/constraints/

Yes, some logic is not representable as a constraint, it could be an exception which could still reside inside the clean() method. But otherwise, constraints are always enforced, there's no need to remember to call full_clean() method. Also, there are usually faster than python validation.

In the docs there is this example:

def get_users(*, fetched_by: User) -> Iterable[User]:
    user_ids = get_visible_users_for(user=fetched_by)

    query = Q(id__in=user_ids)

    return User.objects.filter(query)

But also this example:

def get_items_for_user(
    *,
    user: User
) -> QuerySetType[Item]:
    return Item.objects.filter(payments__user=user)

What's the difference/distinction between the two? when should one prefer one over the other?

Hi, and thanks for the great work with the styleguide.

I was wonderig how is the recommended way to handle ordering according to the styleguide. I see filtering and pagination is specified, but not ordering. Should it be handled as a query_param with the FilterSerializer? In that case, I guess it's just a param to be taken out inside the service and used for ordering.

Thanks again.

Your style guide suggests that model properties should be written as selectors when properties spans multiple relations.
My query is that model properties can be easily written to fields in model serializers but how can I use these selectors with API views or serializers.

An example snippet of the possible scenario can help here.

Thanks!

Thanks for your great guide, inspired me a lot!

I'm a bit confused about two examples about services and tasks.

A. app/services.py via https://github.com/HackSoftware/Django-Styleguide#example-services

from django.contrib.auth.models import User
from django.core.exceptions import ValidationError

from django_styleguide.payments.selectors import get_items_for_user
from django_styleguide.payments.models import Item, Payment
from django_styleguide.payments.tasks import charge_payment


def buy_item(
    *,
    item: Item,
    user: User,
) -> Payment:
    if item in get_items_for_user(user=user):
        raise ValidationError(f'Item {item} already in {user} items.')

    payment = Payment.objects.create(
        item=item,
        user=user,
        successful=False
    )

    charge_payment.delay(payment_id=payment.id)

    return payment

B. app/tasks.py via https://github.com/HackSoftware/Django-Styleguide#celery

from celery import shared_task

from project.app.services import some_service_name as service


@shared_task
def some_service_name(*args, **kwargs):
    service(*args, **kwargs)

In example A, services call Celey task to execute one background task. In example B, one task is just another interface to our core logic - meaning - don't put business logic there..

However, if they live under same application, services.py imports tasks from tasks.py and vice vesa. It makes import loop.

Question:
Should we put normal tasks which are called by services in file tasks.py and create a new file cron_tasks.py for executing services from services.py periodically? Or I misunderstood.

Hello,

First, thank you for your styleguide, it helps me to be a better Django developer.

I am particularly interest by your custom error formatter and I think i have found a bug.

I think [0] is missing in _get_response_json_from_error_message

It is not bas because it works but mypy raise an exception.

def _get_response_json_from_error_message(
self, *, message: str = "", field: Optional[str] = None, code: str = "error")
-> Dict[str, List[Dict[str, str]]]:
        response_data: Dict[str, List[Dict[str, str]]] = {
            self.ERRORS: [{self.MESSAGE: message, self.CODE: code}]
        }

        if field:
            response_data[self.ERRORS][0][self.FIELD] = field

        return response_data

If it could help you, i will be happy.

Thanks for this guide. While I don't agree to everything, there's still a huge bunch of great ideas and concepts in here.

I'd like to copy the code of utils.py to one of our projects. But you don't have a license for the project or the file specified, so this is not really possible. Do you want to publish this under an open source license?

Hi there!

First of all, I would like to say thanks for the great idea of how to organize complex things.

But I want to share some thoughts about the way to organizing views & urls.

I have found the proposed approach to register views not quite RESTful because of
/update & /create aren't resources.

DRF's viewsets introduce a great way to do that but using them out-of-the-box contradicts to Do 1 API per operation rule.

But we still can write separate classes for each API and use viewsets to combine them together & register via DRF router.

Here the draft of viewset_factory function that will help with it.

def viewset_factory(*, name: str, viewset_class=viewsets.GenericViewSet, views_mapping: Mapping, **attrs):
    """Returns ViewSet class with proxy-methods got from views_mapping

    Example:

        Suppose, we have 2 views:
            FooListView(generics.ListAPIView):
                def list(request):
                    # impl.
                    pass

            FooCreateView(generics.CreateAPIView):
                def create(request:
                    # impl.
                    pass

        And we want to combine them into one viewset to make possible
        passing it to the `rest_framework` router

            FooViewSet = viewset_factory(
                name='FooViewSet',
                views_mapping={
                    'list': FooListView,
                    'create': FooCreateView,
                }
            )

        This is roughly equivalent to:

        class FooViewSet(viewsets.GenericViewSet):
            def list(self, request):
                view_class = FooListView(request=request)
                view_class.initial(request)
                return view_class.list(request)

            def create(self, request):
                view_class = FooCreateView(request=request)
                view_class.initial(request)
                return view_class.create(request)

    """

    def _create_viewset_method(view_class, method):
        def _method(cls, request, *args, **kwargs):
            view = view_class(request=request)
            view.args = args
            view.kwargs = kwargs
            view.initial(request)
            m = getattr(view, method)
            return m(request, *args, **kwargs)

        return _method

    viewset_class_name, viewset_class_attrs = name, {}

    for method, view_class in views_mapping.items():
        view_method = _create_viewset_method(view_class, method)
        viewset_class_attrs[method] = view_method

    viewset_class_attrs.update(attrs)

    viewset = type(viewset_class)(viewset_class_name, (viewset_class,), viewset_class_attrs)
    return viewset

And then we can just use DefaultRouter:

router = DefaultRouter()
router.register('foo', views.FooViewSet, basename='foo')

and have RESTful urls with actually separated views.

I'am having troubles with the inline_serializer for an OutputSerializer, let's suppose i have the following models:

class A(models.Model):
att_1=models.IntegerField()
model_b_reference=models.ForeignKey('B')
model_c_reference=models.ForeignKey('C')

class B(models.Model):
att_1=models.IntegerField()
att_2=models.IntegerField()

class C(models.Model):
att_1=models.IntegerField()
att_2=models.IntegerField()

How can i implement an OutputSerializer if i want to show in depth attributes from B model and not C; here it is an example of a desired json response for class A serializer:
{ "att_1":1, "model_b_reference":{ "att_1":1, "att_2":2 }, "model_c_reference":1 }

Hey,

Awesome project, really helped me structure my code in a favorable manor.

I was mimicking the behavior of the List and Detail API views. It seems that they return the actual serializer instead of the serializer's.

I did this:

It's a good idea to add few words about how to structure API responses so they are consistent.

This also includes:

• Working with filters
• Pagination
• How to properly format error responses

In my current project I have used selectors as defined in the guide.

I was wondering how you have implemented pagination, filtering and ordering. Normally when using rest framework this is done within the api by using the ListModelMixin, which under the hood will perform these on the queryset provided.

Do you handle these within your service instead or do implement a mechanism in the API like ListModelMixin. By leaving the services more generic and let the calling code handle these against a queryset returned by a selector.

Cheers

Hi I wanted to ask what would it be the best way to handle a situation when you have, let's say, 3 to 4 apps in your Django application

But then you'll need information from 2 apps that'll provide information for an endpoint

So where this new selector should be located?

New app that would read selectors from app 1 and 2, and then provide the information merged with an endpoint there acting like a parent?

Or maybe app 1 would have a selector and import a selector from app 2?

Odin was a good example back then. Now it's deprecated.

#38 Included the idea for an errors formatter class, to provide a common way for formatting errors.

The implementation is a bit rough & can use tests & after this - a bit of refactoring.

Hi,

I'm using your APIErrorsMixin and I found a little issue with the inheritance of an exception.

For example

class BaseException(Exception):
    pass

class ProblemException(BaseException):
    pass

I'm going to catch all child exceptions of BaseException so I should put on expected exceptions the BaseException

>> expected_exceptions = {
    ValueError: rest_exceptions.ValidationError,
    ValidationError: rest_exceptions.ValidationError,
    PermissionError: rest_exceptions.PermissionDenied,
    BaseException: rest_exceptions.APIException,
}

>> base_exception = BaseException()
>> problem_exception = ProblemException()
>> isinstance(problem_exception, ProblemException)
True
>> isinstance(problem_exception, BaseException)
True
>> problem_exception.__class__
__main__.ProblemException

If I raise the ProblemException, you're getting the exception using the class expected_exceptions[exc.__class__] and the result is a KeyError.

The solution for this problem should change a little bit the handle_exception method to:

def handle_exception(self, exc):
    for key in self.expected_exceptions.keys():
        if isinstance(exc, key):
            drf_exception_class = self.expected_exceptions[key]
            drf_exception = drf_exception_class(get_error_message(exc))
            return super().handle_exception(drf_exception)
    return super().handle_exception(exc)

Hi,

I would like to know how you would go about using SerializerMethodFields in an inline_serializer ?

I would rather not create a dedicated class for such use cases but unfortunately I haven't been able to think of or find an alternative to this...

Thanks !

Sorry about the dumb question, I couldn't find an answer.

Example:

def get_users(*, fetched_by: User) -> Iterable[User]:
    user_ids = get_visible_users_for(user=fetched_by)

    query = Q(id__in=user_ids)

    return User.objects.filter(query)

Hi, firstly, I want to say thanks for the great work with the style guide.

I was wondering why do you say that the InputSerializers should always extend from the plain Serializer, aren't you losing all the validations and other benefits we can obtain using a ModelSerializer in this case?

Thanks again.

Hi everyone,

I noticed that for updating, it's recommended to use a data dict with the fields to update (#57). I was wondering if you do something similar with creates. I have a creation service with a lot of fields, and it the method signature gets really long.

Do you do something similar to updates?

Thanks.

Hi thank you so much for Styleguide.
You can tell us where to place the working with database optimization logic. For example, prefetch_related/select_related?

An example snippet of the possible scenario can help here.

Thanks!

First of all, I think this style/layout of DRF is awesome, its clear and really helps organize a large project!
I'm starting to convert a project of mine to this style and I'm wondering if you ever use other HTTP methods. In the guide, you only use POST when the REST way of doing things for an update would be PATCH or PUT. Is there a reason for that? Furthermore, if you using POST for create and update what do your URLs end up looking like? Following your style guide you can't have URLs like this:

path("products",  ProductCreate.as_view()) 
path("products",  ProductList.as_view()) 
path("products/<int:product_id>",  ProductUpdate.as_view()) 
path("products/<int:product_id>",  ProductDelete.as_view())

They would have to look more like this:

path("products/create",  ProductCreate.as_view()) 
path("products",  ProductList.as_view()) 
path("products/<int:product_id>/update",  ProductUpdate.as_view()) 
path("products/<int:product_id>"/delete,  ProductDelete.as_view()) 

I haven't tried using the other HTTP method and this style so maybe you can have overlapping urls that way? Do you have any examples for all CURD operations or suggestions on how you structure your URLs?

Through using the guide I have implemented some model specific validation as recommended. I.e.

def clean(self):
        if self.start_date > self.end_date:
            raise ValidationError("End date cannot be before start date!")

After setting up the custom Exception Handling and Error Formatting I have noticed the output won't be:

{
  "errors": [
    {
      "message": "End date cannot be before start date!",
      "code": "Invalid",
    }
    ]
}

But instead:

{
  "errors": [
    {
      "message": "End date cannot be before start date!",
      "code": "Invalid",
      "field": "__all__",
    }
    ]
}

I assume that the field should be removed from the response if the validation is a non field error?

If this is the case it is down to key_is_non_field_errors = key == api_settings.NON_FIELD_ERRORS_KEY inside of the ErrorsFormatter class. The default key for DRF is non_field_errors where as the default django validation error key (NON_FIELD_ERRORS) is __all__.

To remove the field name from the response when it is a non field error I have implemented the following :

from django.core.exceptions import NON_FIELD_ERRORS as DJANGO_NON_FIELD_ERRORS_KEY

key_is_non_field_errors = key in [
                api_settings.NON_FIELD_ERRORS_KEY,
                DJANGO_NON_FIELD_ERRORS_KEY,
            ]

If this was not the intended behaviour then please ignore the above.

Cheers.

For my project I need a user defined id. I currently create this with a @Property but I have a hard time with this because this property also queries previous records for ids like: 20210001, 20210002, (...), 20220001

Would you leave this as property or what would be your alternative?

Hi, this project is a good initiative to incentive high quality software building with Django, but I wonder why procedural-like programming is used for services & selectors layer when Django itself uses an object-oriented approach with Managers and QuerySets.

I've adopted QuerySetType from this project's queryset_type.py and it works great for the most part. It was missing support for the logical OR opertaor (|) so I added the following method:

class QuerySetType(Generic[DjangoModel], extra=Iterable):
    ...
    def __or__(self, other) -> Optional[DjangoModel]:
        ...

I also noticed it's missing things like support for slicing (qs[:20]). Is there a deliberate reason for that? is your actual implementation using a more updated/fuller version? trying to understand if this is intentional and I should be wary of implementing it.

For example, to support slicing we may do this?

class QuerySetType(Generic[DjangoModel], extra=Iterable):
    ...
    #old
    # def __getitem__(self, index: int) -> DjangoModel: ...
    # new
    def __getitem__(self, index) -> Union[DjangoModel, "QuerySetType[DjangoModel]"]:
        ...

First of all, I think this style/layout of DRF is awesome, its clear and really helps organize a large project!

I'm starting to convert a project of mine to this style and I'm wondering if you ever use other HTTP methods. In the guide, you only use POST when the REST way of doing things for an update would be PATCH or PUT. Is there a reason for that? Furthermore, if you using POST for create and update what do your URLs end up looking like? Following your style guide you can't have URLs like this:

path("products",  ProductCreate.as_view()) 
path("products",  ProductList.as_view()) 
path("products/<int:product_id>",  ProductUpdate.as_view()) 
path("products/<int:product_id>",  ProductDelete.as_view()) 

They would have to look more like this:

path("products/create",  ProductCreate.as_view()) 
path("products",  ProductList.as_view()) 
path("products/<int:product_id>/update",  ProductUpdate.as_view()) 
path("products/<int:product_id>"/delete,  ProductDelete.as_view()) 

I haven't tried using the other HTTP method and this style so maybe you can have overlapping urls that way? Do you have any examples for all CURD operations or suggestions on how you structure your URLs?

This seems like a good styleguide.
How do you handle proper schema documentation similar to how viewsets generate the schema?

Sample Viewset:
  id:
    retrieve
    update
    delete
  list
  create

As well as proper filtering and ordering options. (using a serializer for filtering is awesome idea, but schemas don't understand that)
Thanks.

For services with arguments that represent model instances (e.g. ids) do you have a preference for whether to give the whole instance as argument or the instance's id?

def update_user(
  *,
  user: User,
  group: UserGroup,
  name: str
) -> User:
  ...

# vs.

def update_user(
  *,
  user_id: int,
  group_id: int,
  name: str
) -> User:
  ...

Hi,

Thanks for the styleguide. I have a question, what is the reason to indicate that the InputSerializer should always be a plain Serializer?.

I was going over my tests as I want to move them to the structure you propose. In the picture you have there's no __ init __.py under models and selectors. I imagine this isn't intentional. At least my IDE (PyCharm) couldn't find the tests if __ init __.py isn't present. I know this is a tiny thing, but it had me tricked for a while.

Hello, first of all, I think you have been doing a great job with styles for Django. However, I'm curious what do you think about the below concepts:

1. Large serialization, exactly creating/updating objects from complicated data. As I can see there is an example of very simple cases. I would like to know how you handle complicated services with many arguments (maybe you use domain entities to simplify it?) and how communication between these services looks.
2. Nested resources, e.g. /courses/1/comments/create for creating.
   Do you allow to use it in your company? If so, how would it look like from the serializer/validation/service side?

Adding a special section for the above concepts and similar would be helpful for me and I hope for others too.

Good reads:

• https://docs.djangoproject.com/en/3.0/topics/logging/
• https://docs.python.org/3/library/logging.config.html#logging-config-dictschema
• https://docs.python.org/3/library/logging.html#logrecord-attributes

Hi! Thanks for this awesome guide!

I see that you use type annotations with querysets and django.
I suggest to give https://github.com/typeddjango/django-stubs a try.

It support real QuerySet and model annotations. So, you won't use -> Iterable[MyModel] where QuerySet[MyModel] should be used.