New optional settings - disable bulk_create in all managers and call self.full_clean() before saving the models

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

New optional settings - disable bulk_create in all managers and call self.full_clean() before saving the models

אורי
Hi,

I added this new ticket today and I would like to know what is your feedback or comments?


We are using Django for Speedy Net and Speedy Match (currently Django 1.11.18, we can't upgrade to a newer version of Django because of one of our requirements, django-modeltranslation). I want to validate each model before saving it to the database, so I'm using class ValidateModelMixin as the base of each model.

For this reason, I want to disable bulk_create in all managers in all of our models. So I added this code:

class ValidateModelMixin(object):
    def save(self, *args, **kwargs):
        """Call `full_clean` before saving."""
        self.full_clean()
        return super().save(*args, **kwargs)


class ManagerMixin(object):
    def bulk_create(self, *args, **kwargs):
        raise NotImplementedError("bulk_create is not implemented.")


class BaseModel(ValidateModelMixin, models.Model):
    def save(self, *args, **kwargs):
        try:
            field = self._meta.get_field('id')
            if ((not (self.id)) and (hasattr(field, 'id_generator'))):
                self.id = field.id_generator()
                while (self._meta.model.objects.filter(id=self.id).exists()):
                    self.id = field.id_generator()
        except FieldDoesNotExist:
            pass
        return super().save(*args, **kwargs)

    class Meta:
        abstract = True


class TimeStampedModel(BaseModel):
    date_created = models.DateTimeField(auto_now_add=True, db_index=True)
    date_updated = models.DateTimeField(auto_now=True, db_index=True)

    class Meta:
        abstract = True


class BaseManager(ManagerMixin, models.Manager):
    pass


class BaseUserManager(ManagerMixin, DjangoBaseUserManager):
    pass


I thought maybe it's good to add these settings - disable bulk_create in all managers and call self.full_clean() before saving the models - as an optional settings both in the project and also in each model (maybe in class Meta) so it will be possible to override Django's default both per-project and also for any specific model. I understand that the default is not to call self.full_clean() before saving the models and to allow bulk_create in the managers, but I suspect this may lead to invalid data in the database of the projects.

The current code in the master is on https://github.com/speedy-net/speedy-net/blob/master/speedy/core/base/models.py, and the code in the branch I'm currently working on is on https://github.com/speedy-net/speedy-net/blob/uri_merge_with_master_2019-01-05_a/speedy/core/base/models.py.

Thanks,
אורי (Uri)

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeHZNA94pOFcWbxbeP5%2BShE1XMZuqx-Rcfit%2BSYvHz-2qQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: New optional settings - disable bulk_create in all managers and call self.full_clean() before saving the models

Aymeric Augustin
Hello,

Your ValidateModelMixin and ManagerMixin look perfectly reasonable. You wanted to customize two aspects of Django's behavior. You managed to do so in four lines and three lines respectively. (I'm leaving out `id_generator` which is another concern.)

Preventing database writes without model validation would require further changes. You'd have to disable `update`, `bulk_update`, `raw` and perhaps others. Even then it would still be possible to use `django.db.connection` to make arbitrary queries, etc.

What I'm saying here is — I'm not sure there's a useful line that Django can draw and that would work for most projects. I wouldn't use the same constraints if I was selling theater tickets or managing access control for health records.

Generally speaking, developers write code that makes (hopefully correct) changes to the database. Each team devises best practices to guarantee a suitable level of quality. Models and managers mixins are a good technique for that.

Best regards,

-- 
Aymeric.



On 5 Jan 2019, at 22:14, ⁨אורי⁩ <⁨[hidden email]⁩> wrote:

Hi,

I added this new ticket today and I would like to know what is your feedback or comments?


We are using Django for Speedy Net and Speedy Match (currently Django 1.11.18, we can't upgrade to a newer version of Django because of one of our requirements, django-modeltranslation). I want to validate each model before saving it to the database, so I'm using class ValidateModelMixin as the base of each model.

For this reason, I want to disable bulk_create in all managers in all of our models. So I added this code:

class ValidateModelMixin(object):
    def save(self, *args, **kwargs):
        """Call `full_clean` before saving."""
        self.full_clean()
        return super().save(*args, **kwargs)


class ManagerMixin(object):
    def bulk_create(self, *args, **kwargs):
        raise NotImplementedError("bulk_create is not implemented.")


class BaseModel(ValidateModelMixin, models.Model):
    def save(self, *args, **kwargs):
        try:
            field = self._meta.get_field('id')
            if ((not (self.id)) and (hasattr(field, 'id_generator'))):
                self.id = field.id_generator()
                while (self._meta.model.objects.filter(id=self.id).exists()):
                    self.id = field.id_generator()
        except FieldDoesNotExist:
            pass
        return super().save(*args, **kwargs)

    class Meta:
        abstract = True


class TimeStampedModel(BaseModel):
    date_created = models.DateTimeField(auto_now_add=True, db_index=True)
    date_updated = models.DateTimeField(auto_now=True, db_index=True)

    class Meta:
        abstract = True


class BaseManager(ManagerMixin, models.Manager):
    pass


class BaseUserManager(ManagerMixin, DjangoBaseUserManager):
    pass


I thought maybe it's good to add these settings - disable bulk_create in all managers and call self.full_clean() before saving the models - as an optional settings both in the project and also in each model (maybe in class Meta) so it will be possible to override Django's default both per-project and also for any specific model. I understand that the default is not to call self.full_clean() before saving the models and to allow bulk_create in the managers, but I suspect this may lead to invalid data in the database of the projects.

The current code in the master is on https://github.com/speedy-net/speedy-net/blob/master/speedy/core/base/models.py, and the code in the branch I'm currently working on is on https://github.com/speedy-net/speedy-net/blob/uri_merge_with_master_2019-01-05_a/speedy/core/base/models.py.

Thanks,
אורי (Uri)

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeHZNA94pOFcWbxbeP5%2BShE1XMZuqx-Rcfit%2BSYvHz-2qQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/A185BD40-3B39-4595-9F46-7A95710A7CEB%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: New optional settings - disable bulk_create in all managers and call self.full_clean() before saving the models

אורי
Thank you, Aymeric. I appreciate your feedback.


On Sun, Jan 6, 2019 at 12:24 AM Aymeric Augustin <[hidden email]> wrote:
Hello,

Your ValidateModelMixin and ManagerMixin look perfectly reasonable. You wanted to customize two aspects of Django's behavior. You managed to do so in four lines and three lines respectively. (I'm leaving out `id_generator` which is another concern.)

Preventing database writes without model validation would require further changes. You'd have to disable `update`, `bulk_update`, `raw` and perhaps others. Even then it would still be possible to use `django.db.connection` to make arbitrary queries, etc.

What I'm saying here is — I'm not sure there's a useful line that Django can draw and that would work for most projects. I wouldn't use the same constraints if I was selling theater tickets or managing access control for health records.

Generally speaking, developers write code that makes (hopefully correct) changes to the database. Each team devises best practices to guarantee a suitable level of quality. Models and managers mixins are a good technique for that.

Best regards,

-- 
Aymeric.



On 5 Jan 2019, at 22:14, ⁨אורי⁩ <⁨[hidden email]⁩> wrote:

Hi,

I added this new ticket today and I would like to know what is your feedback or comments?


We are using Django for Speedy Net and Speedy Match (currently Django 1.11.18, we can't upgrade to a newer version of Django because of one of our requirements, django-modeltranslation). I want to validate each model before saving it to the database, so I'm using class ValidateModelMixin as the base of each model.

For this reason, I want to disable bulk_create in all managers in all of our models. So I added this code:

class ValidateModelMixin(object):
    def save(self, *args, **kwargs):
        """Call `full_clean` before saving."""
        self.full_clean()
        return super().save(*args, **kwargs)


class ManagerMixin(object):
    def bulk_create(self, *args, **kwargs):
        raise NotImplementedError("bulk_create is not implemented.")


class BaseModel(ValidateModelMixin, models.Model):
    def save(self, *args, **kwargs):
        try:
            field = self._meta.get_field('id')
            if ((not (self.id)) and (hasattr(field, 'id_generator'))):
                self.id = field.id_generator()
                while (self._meta.model.objects.filter(id=self.id).exists()):
                    self.id = field.id_generator()
        except FieldDoesNotExist:
            pass
        return super().save(*args, **kwargs)

    class Meta:
        abstract = True


class TimeStampedModel(BaseModel):
    date_created = models.DateTimeField(auto_now_add=True, db_index=True)
    date_updated = models.DateTimeField(auto_now=True, db_index=True)

    class Meta:
        abstract = True


class BaseManager(ManagerMixin, models.Manager):
    pass


class BaseUserManager(ManagerMixin, DjangoBaseUserManager):
    pass


I thought maybe it's good to add these settings - disable bulk_create in all managers and call self.full_clean() before saving the models - as an optional settings both in the project and also in each model (maybe in class Meta) so it will be possible to override Django's default both per-project and also for any specific model. I understand that the default is not to call self.full_clean() before saving the models and to allow bulk_create in the managers, but I suspect this may lead to invalid data in the database of the projects.

The current code in the master is on https://github.com/speedy-net/speedy-net/blob/master/speedy/core/base/models.py, and the code in the branch I'm currently working on is on https://github.com/speedy-net/speedy-net/blob/uri_merge_with_master_2019-01-05_a/speedy/core/base/models.py.

Thanks,
אורי (Uri)

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeHZNA94pOFcWbxbeP5%2BShE1XMZuqx-Rcfit%2BSYvHz-2qQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/A185BD40-3B39-4595-9F46-7A95710A7CEB%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeGLp%3DxfC0%3D3dwSfSY9WDwWvC8WiLuX9ZQY_4wJSwyy8nQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.