1

TL;DR both my model and my form calculate the value of the field number_as_char. Can I avoid the double work, but still check uniqueness when using the model without the form?

I use Python 3 and Django 1.11


My model looks as follows:

class Account(models.Model):
    parent_account = models.ForeignKey(
        to='self',
        on_delete=models.PROTECT,
        null=True,
        blank=True)
    number_suffix = models.PositiveIntegerField()
    number_as_char = models.CharField(
        max_length=100,
        blank=True,
        default='',
        unique=True)

    @classmethod
    def get_number_as_char(cls, parent_account, number_suffix):
        # iterate over all parents
        suffix_list = [str(number_suffix), ]
        parent = parent_account
        while parent is not None:
            suffix_list.insert(0, str(parent.number_suffix))
            parent = parent.parent_account

        return '-'.join(suffix_list)

    def save(self, *args, **kwargs):
        self.number_as_char = self.get_number_as_char(
            self.parent_account, self.number_suffix)
        super().save(*args, **kwargs)

The field number_as_char is not supposed to be set by the user because it is calculated based on the selected parent_account: it is obtained by chaining the values of the field number_suffix of all the parent accounts and the current instance.

Here is an example with three accounts:

ac1 = Account()
ac1.parent_account = None
ac1.number_suffix = 2
ac1.save()
# ac1.number_as_char is '2'

ac2 = Account()
ac2.parent_account = ac1
ac2.number_suffix = 5
ac2.save()
# ac2.number_as_char is '2-5'

ac3 = Account()
ac3.parent_account = ac2
ac3.number_suffix = 1
ac3.save()
# ac3.number_as_char is '2-5-1'

It is NOT an option to drop the field and use a model property instead, because I need to ensure uniqueness and also use that field for sorting querysets with order_by().


My form looks as follows:

class AccountForm(forms.ModelForm):

    class Meta:
        model = Account
        fields = [
            'parent_account', 'number_suffix', 'number_as_char',
        ]
        widgets = {
            'number_as_char': forms.TextInput(attrs={'readonly': True}),
        }

    def clean(self):
        super().clean()
        self.cleaned_data['number_as_char'] = self.instance.get_number_as_char(
            self.cleaned_data['parent_account'], self.cleaned_data['number_suffix'])

I included number_as_char in the form with widget attribute readonly and I use the forms clean() method to calculate number_as_char (it has to be calculated before validating uniqueness).


This all works (the model and the form), but after validating the form, the value of number_as_char will be calculated again by the models save() method. Its not a big problem, but is there a way to avoid this double calculation?

  1. If I remove the calculation from the forms clean() method, then the uniqueness will not be validated with the new value (it will only check the old value).
  2. I don't want to remove the calculation entirely from the model because I use the model in other parts without the form.

Do you have any suggestions what could be done differently to avoid double calculation of the field?

Ralf
  • 16,086
  • 4
  • 44
  • 68
  • The way your model is built, you could ensure uniqueness by checking that the pair `(parent, suffix)` is unique, but it would probably be more expensive than what you're doing. It would not require double calculation of the `number_as_char` but I don't think you'd win much, if anything. This is a super common headache with django... – Laurent S May 04 '18 at 16:41
  • @LaurentS Using `unique_together` like you suggest is not a complete solution either, because the rows with `parent_account=None` can be duplicated (SQL NULL does not equal another SQL NULL). See [this question](https://stackoverflow.com/questions/33307892/django-unique-together-with-nullable-foreignkey) for that issue. – Ralf May 04 '18 at 16:55
  • Ah, sorry, I missed the fact that `parent_account` could be `None`... Could you possibly setup a "root" account that all accounts with no parent link to? Depending on your application, this might help in getting the `unique_together` to work? – Laurent S May 04 '18 at 16:58
  • @LaurentS a pseudo root account sounds a bit complicated. I would have to exclude that one when displaying the list to avoid confusing the end user. It is possible, but it sounds more complicated than my current solution. – Ralf May 04 '18 at 17:48
  • It won't help with the validation, but are there any reasons against making `number_as_char` a property. As it is your model is denormalized. Using a property you could normalize it and perform the calculation only when needed, not every time when it's saved. – cezar May 07 '18 at 08:28
  • @cezar I think I need `number_as_char` as a field to make sorting more performant (using `order_by()`). As far as I know I cannot use a property for database sorting. – Ralf May 07 '18 at 11:02
  • Right, it makes then sense to store it in the database. – cezar May 07 '18 at 11:05
  • I think this is X/Y problem https://en.wikipedia.org/wiki/XY_problem. And you probably need to look into https://github.com/django-mptt/django-mptt – Sardorbek Imomaliev May 07 '18 at 14:13

4 Answers4

1

I can't see any way around doing this in two places (save() and clean()) given that you need it to work for non-form-based saves as well).

However I can offer two efficiency improvements to your get_number_as_char method:

  1. Make it a cached_property so that the second time it is called, you simply return a cached value and eliminate double-calculation. Obviously you need to be careful that this isn't called before an instance is updated, otherwise the old number_as_char will be cached. This should be fine as long as get_number_as_char() is only called during a save/clean.

  2. Based on the information you've provided above you shouldn't have to iterate over all the ancestors, but can simply take the number_as_char for the parent and append to it.

The following incorporates both:

@cached_property
def get_number_as_char(self, parent_account, number_suffix):
    number_as_char = str(number_suffix)
    if parent_account is not None:
        number_as_char = '{}-{}'.format(parent_account.number_as_char, number_as_char)

    return number_as_char

To be sure that the caching doesn't cause problems you could just clear the cached value after you're done saving:

def save(self, *args, **kwargs):
    self.number_as_char = self.get_number_as_char(
        self.parent_account, self.number_suffix)
    super().save(*args, **kwargs)
    # Clear the cache, in case something edits this object again.
    del self.get_number_as_char
solarissmoke
  • 30,039
  • 14
  • 71
  • 73
1

I tinkered with it a bit, and I think I found a better way.

By using the disabled property on the number_as_char field of your model form, you can entirely ignore users input (and make the field disabled in a single step).

Your model already calculates the number_as_char attribute in the save method. However, if the Unique constraint fails, then your admin UI will throw a 500 error. However, you can move your field calculation to the clean() method, leaving the save() method as it is.

So the full example will look similar to this:

The form:

class AccountForm(forms.ModelForm):

    class Meta:
        model = Account
        fields = [
            'parent_account', 'number_suffix', 'number_as_char',
        ]
        widgets = {
            'number_as_char': forms.TextInput(attrs={'disabled': True}),
        }

The model:

class Account(models.Model):
    # ...

    def clean(self):
        self.number_as_char = self.get_number_as_char(
            self.parent_account, self.number_suffix
        )
        super().clean()

That way anything that generates form based on your model will throw a nice validation error (provided that it uses the built-in model validation, which is the case for Model Forms).

The only downside to this is that if you save a model that triggers the validation error, you will see an empty field instead of the value that failed the validation - but I guess there is some nice way to fix this as well - I'll edit my answer if I also find a solution to this.

samu
  • 2,870
  • 16
  • 28
  • Sounds interresting. I will try it out and get back to you by the end of the week (before the bounty expires). – Ralf May 07 '18 at 13:20
  • Your idea did not solve the problem completly, but pointed me in the right direction. I added an answer with my final solution (and marked it as accepted), but the bounty goes for you. Thanks. – Ralf May 09 '18 at 13:42
1

After reading all the answers and doing some more digging through the docs, I ended up using the following:

  1. @samu suggested using the models clean() method and @Laurent S suggested using unique_together for (parent_account, number_suffix). Since only using unique_together doesn't work for me because parent_account can be null, I opted for combining the two ideas: checking for existing (parent_account, number_suffix) combinations in the models clean() method.
  2. As a consecuence, I removed number_as_char from the form and it is now only calculated in the save() method. By the way: thanks to @solarissmoke for suggesting to calculated it based on the first parent only, not iterating all the way to the top of the chain.
  3. Another consecuence is that I now need to explicitly call the models full_clean() method to validate uniqueness when using the model without the form (otherwise I will get the database IntegrityError), but I can live with that.

So, now my model looks like this:

class Account(models.Model):
    parent_account = models.ForeignKey(
        to='self',
        on_delete=models.PROTECT,
        null=True,
        blank=True)
    number_suffix = models.PositiveIntegerField()
    number_as_char = models.CharField(
        max_length=100,
        default='0',
        unique=True)

    def save(self, *args, **kwargs):
        if self.parent_account is not None:
            self.number_as_char = '{}-{}'.format(
                self.parent_account.number_as_char,
                self.number_suffix)
        else:
            self.number_as_char = str(self.number_suffix)
        super().save(*args, **kwargs)

    def clean(self):
        qs = self._meta.model.objects.exclude(pk=self.pk)
        qs = qs.filter(
            parent_account=self.parent_account,
            number_suffix=self.number_suffix)
        if qs.exists():
            raise ValidationError('... some message ...')

And my form ends up like this:

class AccountForm(forms.ModelForm):
    class Meta:
        model = Account
        fields = [
            'parent_account', 'number_suffix',
        ]

EDIT

I'll mark my own answer as accepted, because non of the suggestions fully suited my needs.

However, the bounty goes to @samus answer for pointing me in the right direction with using the clean() method.

Ralf
  • 16,086
  • 4
  • 44
  • 68
  • Thanks, this is IMHO the simpler and nicer solution. Doing the clean on the model itself completely makes sense for this use case, which is very similar to the problem that brought me to this SO question. – Augusto Destrero Nov 15 '18 at 10:39
0

Another way - probably not as good though - would be to use Django signals. You could make a pre_save signal that would set the correct value for number_as_char field on the instance that's about to get saved.

That way you don't have to have it done in a save() method of your model, OR in the clean() method of your ModelForm.

Using signals should ensure that any operation that uses the ORM to manipulate your data (which, by extend, should mean all ModelForms as well) will trigger your signal.

The disadvantage to this approach is that it is not clear from the code directly how is this property generated. One has to stumble upon the signal definition in order to discover that it's even there. If you can live with it though, I'd go with signals.

samu
  • 2,870
  • 16
  • 28
  • 1
    Hm... I'm not sure about signals. As I understand it, form validation would not catch any `ValidationError` I raise from the signal. – Ralf May 07 '18 at 11:36
  • Right, I forgot about the validation error. Could you also provide the validation code in your original question? – samu May 07 '18 at 11:49
  • So far I have no other validation code besides whats shown in my question; the validation I'm interrested in is that the field `number_as_char` is unique. – Ralf May 07 '18 at 12:23
  • I don't see any validation in the code you posted in your original question. I assume that you're setting this field in your form, because your form does not ignore users input on that field. Try using the `disabled` (https://docs.djangoproject.com/en/2.0/ref/forms/fields/#disabled) attribute for `number_as_char` field on your ModelForm. If your `save()` method already does the calculation,so by setting it you should be able to push the responsibility to calculate that field to the model entirely. Have a look if that helps. If it does, I can re-post this to a separate answer. – samu May 07 '18 at 12:32
  • TL;DR: if you set `disabled` on your `ModelForm` class, you should not have to set the `number_as_char` value in the form class at all. – samu May 07 '18 at 12:36
  • If I leave the calculation of `number_as_char` for the `save()` method and the resulting value of `number_as_char` is duplicated, then I get an error page (http status code 500) caused by an `IntegrityError` instead of a `ValidationError` that renders the error message nicely. – Ralf May 07 '18 at 12:57