Django objects uniqueness hell with M2M fields

Question

class Badge(SafeDeleteModel):
    owner = models.ForeignKey(settings.AUTH_USER_MODEL,
                              blank=True, null=True,
                              on_delete=models.PROTECT)
    restaurants = models.ManyToManyField(Restaurant)
    identifier = models.CharField(max_length=2048)  # not unique at a DB level!

I want to ensure that for any badge, for a given restaurant, it must have a unique identifier. Here are the 4 ideas I have had:

• idea #1: using unique_together -> Does not work with M2M fields as explained [in documentation] (https://docs.djangoproject.com/en/2.1/ref/models/options/#unique-together)
• idea #2: overriding save() method. Does not fully work with M2M, because when calling add or remove method, save() is not called.

• idea #3: using an explicite through model, but since I'm live in production, I'd like to avoid taking risks on migrating important structures like theses. EDIT: after thinking of it, I don't see how it could help actually.

• idea #4: Using a m2m_changedsignal to check the uniqueness anytime the add() method is called.

I ended up with the idea 4 and thought everything was OK, with this signal...

@receiver(m2m_changed, sender=Badge.restaurants.through)
def check_uniqueness(sender, **kwargs):
    badge = kwargs.get('instance', None)
    action = kwargs.get('action', None)
    restaurant_pks = kwargs.get('pk_set', None)

    if action == 'pre_add':
        for restaurant_pk in restaurant_pks:
            if Badge.objects.filter(identifier=badge.identifier).filter(restaurants=restaurant_pk):
                raise BadgeNotUnique(MSG_BADGE_NOT_UNIQUE.format(
                    identifier=badge.identifier,
                    restaurant=Restaurant.objects.get(pk=restaurant_pk)
                ))

...until today when I found in my database lots of badges with the same identifier but no restaurant (should not happend at the business level) I understood there is no atomicity between the save() and the signal. Which means, if the user have an error about uniqueness when trying to create a badge, the badge is created but without restaurants linked to it.

So, the question is: how do you ensure at the model level that if the signal raises an Error, the save() is not commited?

Thanks!

Haslam

Why not make an IdentifiedBadge with a m2m to restaurant. In such way you can guarantee this by design. Usually it is better to enforce things by design then aiming to patch these. Signals, .save(), etc. can be bypassed (for example when updating in bulk). – Judgemade

That could be a 5th idea indeed thanks. Yet, I don't like the idea of adding a new class just to handle other class models integrity. How this solution would be superior of idea #3? – Haslam

save shouldn't catch error that raised in signal, this is source code where signal was send github.com/django/django/blob/stable/2.1.x/django/db/models/… as you can see it wrapped to transaction atomic and doesn't catch errors. Are you sure that you signal is executed? – Photoelectrotype

Is it a hard requirement that the identifier not be unique at a database level? Or is that just a preference? – Laplace

@Laplace hard requirement – Haslam

@DavidD. how is the badge identifier generated? – Seaweed

@TGO entered by human. – Haslam

It seems like you're gonna have to bite the bullet and go with idea 3. Proceed cautiously and test often before deployment – Laplace

@Laplace That's exactly what I'm gonna do except if someone find a more elegant solution in between :D – Haslam

Answer 1

I see two separate issues here:

1. You want to enforce a particular constraint on your data.

2. If the constraint is violated, you want to revert previous operations. In particular, you want to revert the creation of the Badge instance if any Restaurants are added in the same request that violate the constraint.

Regarding 1, your constraint is complicated because it involves multiple tables. That rules out database constraints (well, you could probably do it with a trigger) or simple model-level validation.

Your code above is apparently effective at preventing adds that violate the constraint. Note, though, that this constraint could also be violated if the identifier of an existing Badge is changed. Presumably you want to prevent that as well? If so, you need to add similar validation to Badge (e.g. in Badge.clean()).

Regarding 2, if you want the creation of the Badge instance to be reverted when the constraint is violated, you need to make sure the operations are wrapped in a database transaction. You haven't told us about the views where these objects area created (custom views? Django admin?) so it's hard to give specific advice. Essentially, you want to have this:

with transaction.atomic():
    badge_instance.save()
    badge_instance.add(...)

If you do, an exception thrown by your M2M pre_add signal will rollback the transaction, and you won't get the leftover Badge in your database. Note that admin views are run in a transaction by default, so this should already be happening if you're using the admin.

Another approach is to do the validation before the Badge object is created. See, for example, this answer about using ModelForm validation in the Django admin.

Eustoliaeutectic

Thanks for your answer. I didn’t talk about the views because I definitely want this integrity rule to be handled at the model level. I can’t rely on views to ensure that role. May I ask you what do you think of the suggested #idea3 as a solution? – Haslam

@DavidD.: The integrity rule (point 1) can be handled at the model level, but you also want to make atomic a series of operations on different database tables (point 2). The Badge object has to exist in the database before you can try (and potentially fail) to add any Restaurants to it. As for idea #3, could you edit your question to be more explicit? What would the through model look like and how would it solve the problem? – Eustoliaeutectic

I think you're right, thinking of idea #3, it seems id does not solve the problem. I wanted to use unique_together but Django does not allow to write something like unique_together=(badge__identifier, restaurant). – Haslam

I have a new idea #5: what about deleting the badge itself in the signal? It's kind of dirty because an object would be created then deleted just after, but it should work as expected I guess... ? – Haslam

@DavidD.: Deleting the badge might work, though you'd have to be careful to construct your condition in such a way that you only do so on new Badges (as opposed to adding Restaurants to existing Badges). There may be unexpected interactions with the signal system, and you'd miss out on the benefits of using Django's validation system. I wouldn't recommend it, but it might work. – Eustoliaeutectic

Your answer allowed me to understand that it is more a view related problem that a model one. In the end, I just used a with transaction.atomic() block in my form_valid CBV method. It solved the problem. I just need to be careful when working at model level. Thanks. Happy bounty. – Haslam

Answer 2

You can specify your own connecting model for your M2M-models, and then add a unique_together constraint in the meta class of the membership model

class Badge(SafeDeleteModel):
    ...
    restaurants = models.ManyToManyField(Restaurant, through='BadgeMembership')

class BadgeMembership(models.Model):
    restaurant = models.ForeignKey(Restaurant, null=False, blank=False, on_delete=models.CASCADE)
    badge = models.ForeignKey(Badge, null=False, blank=False, on_delete=models.CASCADE)

    class Meta:
        unique_together = (("restaurant", "badge"),)

This creates an object that's between the Badge and Restaurant which will be unique for each badge per restaurant.

Optional: Save check

You can also add a custom save function where you can manually check for uniqueness. In this way you can manually raise an exception.

class BadgeMembership(models.Model):
    restaurant = models.ForeignKey(Restaurant, null=False, blank=False, on_delete=models.CASCADE)
    badge = models.ForeignKey(Badge, null=False, blank=False, on_delete=models.CASCADE)

    def save(self, *args, **kwargs):
        # Only save if the object is new, updating won't do anything
        if self.pk is None:
            membershipCount = BadgeMembership.objects.filter(
                Q(restaurant=self.restaurant) &
                Q(badge=self.badge)
            ).count()
            if membershipCount > 0:
                raise BadgeNotUnique(...);
            super(BadgeMembership, self).save(*args, **kwargs)

Stockman

Hi Johan, thanks for your help. However, I'm not sure unique_together = (("restaurant", "badge"),) statement will ensure uniqueness. It's remains possible to create 2 badges with the same identifier and the same restaurants. I would need something like unique_together = (("restaurant", "badge__identifier"),) but it's not possible to write this. – Haslam

@DavidD. Have you tried with the unique_together on the membership model that connects Badge and Restaurant? (ie not on the Badge or the Restaurant model). – Stockman

Answer 3

I'm afraid the correct way to achieve this really is by adapting the "through" model. But remember that at database level this "through" model already exists, and therefore your migration would simply be adding a unique constraint. It's a rather simple operation, and it doesn't really involve any real migrations, we do it often in production environments.

Take a look at this example, it pretty much sums everything you need.

Nowhither

Can you please elaborate on how the "through" model would help here to ensure integrity at the model level? For me, even with this intermediary model, we will still be able to createa badge with no restaurant linked because of atomicity (see the initial problem in the question). How is this solution better than the signals I'm already using? – Haslam

@DavidD. I have updated my answer on how a custom through model will solve your problem. – Stockman