Dynamically populate constraints in Field.contribute_to_class

Django Internals ORM
1

Hey !

I’m writing a custom model field and would like it to come with a database CHECK constraint. The actual use case is DB validation of Django’s choices, to avoid invalid values (since the data can be edited from outside of Django).

I thought that could be done with contribute_to_class, and indeed it’s almost possible.

Here’s what I got so far:

class ChoiceField(models.CharField):
    def contribute_to_class(self, cls, name, private_only=False):
        super().contribute_to_class(cls, name, private_only)
        accepted_values = [c[0] for c in self.choices]
        cls._meta.constraints.append(
            models.CheckConstraint(
                check=models.Q(**{f"{name}__in": accepted_values}),
                name=f"%(app_label)s_%(class)s_{name}_valid_choices"
            )
        )

It works exactly as I’d like when I specify an empty constraints list in my model’s Meta (meaning makemigrations will create the constraints).

But for some reason, if I don’t specify an empty list, or if my model has no Meta at all, the constraints aren’t picked up (makemigrations doesn’t create the constraints).

I stumbled about this ticket, which seems very similar, and where it was suggested to ask here (in that case, it was about “indexes”, and strangely they seem to have the exact opposite issue, where it works as expected without Meta, but the index is added twice with the Meta).

Does anyone understand what could be the cause of the issue ?
Wouldn’t it be nice for Django to support this ? DB data validation is really a must when you can’t rely on python validation (migrating large amount of data, DB operated by other clients, etc…)

Cheers !!

Olivier

1 Like
2

The easiest way to create those types of constraints is to define those choices in a “code table” and use a foreign key to relate the base table to it. (That’s what we do to enforce the integrity of those types of selections.)

3

Thanks for the suggestion ! Indeed that’s an approach, but that forces to store the values in the DB (and hence somehow sync them with the code), and also doesn’t work for more complex use cases (such as multiselect, where you’d store the selection as a postgres array of strings).

4

No, there’s no “code” to be synced, because the data exists in the database. It eliminates the need for the code.

Yes it does - it works quite fine. From a relational database perspective, the “multiselect” would be represented by a many-to-many relationship.

This is an exceedingly common technique used across numerous domains and frameworks. In fact, architecturally, I’ll maintain that it’s far superior to the code based method - precisely for the reasons you identify, among others.

It’s the only way that we allow choice options to be defined.

5

I’d quite like this to work. To generate check constraints from choices would be great.

I’d be inclined to reduce that to a test case. Much easier to get people to comment against running code.

Q: does it work generating migrations when you change the choices? (Showing that in a test too would be handy.)

6

I’m definitely :+1: to having a clear, clean api for defining custom fields that set constraints. I agree with @carltongibson that we should add some tests to Django to prove it works and protect against regressions. I also think it’s worth documenting the pattern if it works reliably.

7

we use a subclass of TextChoices as an helper to define Constraints, but indeed a native API should be more cleaner and error-proof.

So definitely :+1: for me

However, I propose to implement the ChoiceField as a separate class accepting the real field as a parameter (like postgres ArrayField for example). In this way we could have char choices, int choices, etc…

8

Ok I have this written as a test case (for now just an arbitrary constraint not involving choices, I think we need that to work reliably first before applying it to the ChoiceField use case) :

  • sqlite / with Meta: works as expected
  • sqlite / without Meta: the constraint work, but not picked up by makemigrations
  • postgres / with Meta: fails with DuplicateObject: constraint "test_constraint_model" for relation "modelwithmeta_model" already exists
  • postgres / without Meta: the constraint work, but not picked up by makemigrations

I’m not 100% sure what’s happening, in particular how it can be that the constraint work if not picked by makemigrations. How can they even be created in the database without a migration ?!

I think there’s nothing fishy going on with the test themselves (e.g. cross-test interference) since I get consistent results when running each test individually, and the issue resembles the similar ticket I was mentionning (btw it was 28888 not 2888).

Any pointer at what could be at play ?

Once this simple case is sorted, I can add tests for more advanced scenarios too (inherited models, abstract models, etc).

1 Like
9

I know the goal of this forum post is to make the general case works, but I figured I’d mention that django-taggit solves this use case perfectly.

1 Like
10

I’ve been doing some R&D to understand the original issue presented in this post. I think there are two main things to discuss, perhaps separately:

  1. Why/how the constraints added by a field to its model’s constraint list (via contribute_to_class) is not picked up in the generated migration but it’s present in the DB (the original forum post).

  2. A new feature that we could summarize using @Lily-Foote’s words as [Let’s define] a clear, clean api for defining custom fields that set constraints.

Regarding item 1, the core of the issue is that makemigrations would not pick up the field-defined constraint but migrate will apply it. Specifically, for a simple model like (taken from the linked repo):

from django.db import models


class ConstraintField(models.CharField):
    def contribute_to_class(self, cls, name, private_only=False):
        super().contribute_to_class(cls, name, private_only)
        cls._meta.constraints.append(
            models.CheckConstraint(
                check=models.Q(**{f"{name}__lte": 10}),
                name=f"test_constraint_{cls.__name__.lower()}_lte_value",
            )
        )


class DynamicModelWithConstraintField(models.Model):

    constrained_field = ConstraintField(max_length=10)

The generated migration is:

    operations = [
        migrations.CreateModel(
            name='DynamicModelWithConstraintField',
            fields=[
                ('id', models.BigAutoField(...)), # reduced for brevity
                ('constrained_field', ticket_dynamic_constraints.models.ConstraintField(max_length=10)),
            ],
        ),
    ]

But sqlmigrate shows:

BEGIN;
--
-- Create model DynamicModelWithConstraintField
--
CREATE TABLE "ticket_dynamic_constraints_dynamicmodelwithconstraintfield" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "constrained_field" varchar(10) NOT NULL, CONSTRAINT "test_constraint_dynamicmodelwithconstraintfield_lte_value" CHECK ("constrained_field" <= '10'));
COMMIT;

So clearly the constraint is (to be) added to the DB, but the migration machinery is not aware of it so further changes to the constraint result in all kinds of errors on future operations over those constraints.
I realized that the code in the migrations’ state module, when calling ModelState.from_model, the model._meta.original_attrs is used to build the ModelState instance that represents the model, but this original_attrs is populated in Options.contribute_to_class and that’s not accounting/including what the Field has added to the model’s constraints (more debugging on this is needed, but for starter, there is a if self.meta that does not do any magic if Meta is not defined.

So for this item, the main question is do we consider this a bug? From #28888 likely not since there is no documented support for adding indexes (or constraints) in Field.contribute_to_class, but it’s worth asking the question.

What about item 2? We could discuss whether a get_constraints would make sense as a new public method in the Field class. This could be a way to provide an API to solve the use case but I’m not sure if there is a similar precedent that would guide this decision making.

1 Like
11

Thanks @nessita for looking into this !!

I’ve pushed a new commit demonstrating the actual use case of the “ChoiceField”. It shows that when choices change, the constraints are dropped and recreated in the migrations (due to 1/ this test only passes in sqlite and requires Meta.constraints = [] to be defined). So it seems we are really almost there already.

With that in mind, I would argue that contribute_to_class is actually a good candidate for 2/, and to treat 1/ as a bug ? Otherwise we’d very soon need get_indexes (the use case of #28888), and then maybe others as well. But of course that’s just my 2 cents, I don’t know much about Django’s internals and would be very happy with a get_constraints.

12

FWIW, I’ve been using Python for over a decade but mostly Flask/SQLAlchemy based projects. I recently started working with Django and needed to build a new model. One of the first things I did was add a Field mixin that would automatically enforce blank=True at the DB level. It used contribute_to_class() to add a constraint to cls._meta.constraints() which worked during testing with pytest --nomigrations but failed when using the migration. I’d also like to automatically create constraints for choice types as discussed above, just haven’t got there yet.

I eventually tracked this down to the constraints not being created in the migration if Meta.constraints was not defined on the model.

The documentation for contribute_to_class() is very minimal. I’ve only found a wiki page and a reference to it in the model instance reference. But, it is mentioned and it is indicated that it can be used by developers, i.e. its not a private implementation.

Additionally, given a) that ._meta is almost universally documented as the way to interact with Meta once the class has been created and b) constraints added by contribute_to_class() are recognized by other Django machinery as evidenced by their creation when using pytest --nomigrations, I believe it should be considered a bug that makemigrations does not create them.

Thanks for your consideration.

13

I think there’s a case for this. E.g. adding a check constraint for (Text) Choices (say) would be in line with lots of the recent changes about pushing work down into the DB.

1 Like
14

Another reason to consider it a bug. If you add constraints = () to Meta but then add the constraints dynamically to _meta.constraints then makemigrations will generate them. This resulted for me in a very weird scenario where one model’s dynamic constraints were being generated just fine (because it defined other static constraints in Meta) and another model’s dynamic constraints missing (b/c it defined no static constraints).

FWIW, I’m running v3.2.18 so its possible more recent versions of Django behave differently now.

I’ve worked around it by enforcing the presence of Meta.constraints:

class TWTModelBase(models.base.ModelBase):
    def __new__(cls, name, bases, attrs):

        # Ensures that TWModel instances have a Meta class with a 'constraints' attribute.  Django
        # migrations have a bug that results in constraints not being generated if Meta or 
        # Meta.constraints is not present.
        attr_meta = attrs.get('Meta', type('Meta', (), {}))
        if not hasattr(attr_meta, 'constraints'):
            attr_meta.constraints = ()
            attrs['Meta'] = attr_meta

        new_cls = super().__new__(cls, name, bases, attrs)
        return new_cls


class TWTModel(models.Model, metaclass=TWTModelBase):
    created_at = models.DateTimeField(auto_now_add=True)
    updated_at = models.DateTimeField(auto_now=True)

    objects = Manager()

    class Meta:
        abstract = True
15

It’s not clear to me that dynamically editing constraints yourself is supported. (Like, it’s possible but we may not want to draw conclusions from it.)

16

I’ve worked around it by enforcing the presence of Meta.constraints:

Turns out that even with the presence of Meta.constraints, I ran into a weird issue where running a migration that looked correct resulted in “constraint already exists” error for one, but only one of about five, constraints that were added dynamically.

I decided to punt on this implementation and just do it all with meta programming so that the constraints were added to Meta.constraints before ModelBase.__new__() is called.

17

Do you have any examples?
I’m stuck on this problem.

18

This is the solution I came up with, so far I have no problems with it and it works.

class PositivoComun:

    @staticmethod
    def get_constrain(cls, name, minimo=0):
        return models.CheckConstraint(
                check=models.Q(**{f"{name}__gte": minimo}),
                name=f"check_{cls.lower()}_{name}_gte_{minimo}"
            )


class PositivoDecimalField(models.DecimalField, PositivoComun):
    """Campo de tipo decimal que solo permite números iguales o mayores a cero.
    """
    description = "Número decimal positivo"
    valor_minimo = 0

    def __init__(self, *args, **kwargs):
        if 'valor_minimo' in kwargs:
            self.valor_minimo = kwargs['valor_minimo']
            del kwargs['valor_minimo']
        if 'default' not in kwargs:
            kwargs["default"] = 0
        if 'max_digits' not in kwargs:
            kwargs["max_digits"] = 18
        if 'decimal_places' not in kwargs:
            kwargs["decimal_places"] = 6
        if 'validators' not in kwargs:
            kwargs['validators'] = [MinValueValidator(self.valor_minimo), MaxValueValidator(999999999999)]
        super().__init__(*args, **kwargs)

class CustomModelMetaClass(ModelBase):
    def __new__(cls, name, bases, attrs):
        klas = super().__new__(cls, name, bases, attrs)
        for f in attrs:
            if hasattr(attrs[f], "get_constrain") and callable(attrs[f].get_constrain):
                if hasattr(attrs[f], "valor_minimo"):
                    klas._meta.constraints.append(
                        attrs[f].get_constrain(klas.__name__, f, attrs[f].valor_minimo)
                    )
                else:
                    klas._meta.constraints.append(
                        attrs[f].get_constrain(klas.__name__, f)
                    )

        return klas

class AlmacenProducto(Modelos, metaclass=CustomModelMetaClass):
    existencia = PositivoDecimalField(db_column='existencia', blank=False, null=False, verbose_name='existencia')
19

the combination of metaclass + addind field.get_constrain() works fine for me as well


class CustomModelMetaClass(ModelBase):
    def __new__(cls, name, bases, attrs):
        klass = super().__new__(cls, name, bases, attrs)
        for field in attrs:
            if hasattr(attrs[field], "get_constrain") and callable(
                attrs[field].get_constrain
            ):
                klass._meta.constraints.append(
                    attrs[field].get_constrain(
                        klass._meta.app_label, klass._meta.model_name, field
                    )
                )
        return klass


class CustomFieldWithConstraint(Field):
    @staticmethod
    def get_constrain(app_label, model_name, field_name):
        return CheckConstraint(
            name=f"{app_label}__{model_name}__{field_name}_...",
            check=... # whatever
        )


class Pole(Model, metaclass=CustomModelMetaClass):
    location = CustomFieldWithConstraint()

this create the correct migration with explicit DB constraint for the model