3
django 2.0

I have a django model, with different slug fields:

from django.core.validators import validate_slug

class MyModel(models.Model):
     # with slug field
     slug_field = models.SlugField(max_length=200)

     # or charfield with slug validator (should be exactly the same)
     char_field = models.CharField(max_length=200, validators=[validate_slug])

The first problem i have, in my form i have a clean method, to validate the values of multiple fields, not individually. This method should theoretically be called after the clean_fields method, but it is called even if clean_fields raise an error.

My forms.py:

class MyForm(forms.ModelForm):
    class Meta:
        model = MyModel
        fields = '__all__'

    def clean(self):
        cleaned_data = super().clean()
        print(cleaned_data.get('slug_field'))  # > None
        print(cleaned_data.get('char_field'))  # > ééé; uncleaned data
        print(self.errors)  # only from slug_field
        return cleaned_data

With SlugField, slug_field is not set in cleaned_data, when it's invalid, and after the error is raised and returned to the user by the form. (I don't see why clean() is even reached, because clean_fields() have raised the error before)

The problem is that with the CharField with any custom validator (validate_slug or a self made one), the uncleaned value is returned in cleaned_data. However, the validation error is still raised, but after.

This is quite dangerous for me, because i used to trust cleaned_data, to modify data not saved inside the model.

Guillaume Lebreton
  • 2,586
  • 16
  • 25
  • 1
    Using a slug field as a primary key is unusual. Unless you have a good reason to do this, I would `alias = models.SlugField(max_length=200, unique=True)`. Your slug field will still be unique, and Django will create the primary key field automatically. – Alasdair Feb 19 '18 at 09:59
  • It's a bad idea to use `SlugField` as primary key, which eventually is not the case here, but only your misunderstanding. The `SlugField` should rather be generated by some business logic, preferably in the `save` method. The biggest danger you face here is in front of your screen. – cezar Feb 28 '18 at 14:28
  • @cezar If the Slug field is correctly cleaned, i don't see any major issue. Anyway, i modified the question to hide any mention to this primary key, and i tested the code without primary key explecitly set, still the same. – Guillaume Lebreton Feb 28 '18 at 18:11
  • 1
    Now that you've simplified your example, I can reproduce your example in Django 1.11. I'm not sure why it's happening, you'd have to step through the source code to see why the two fields behaved differently. It looks like it might be a bug, however I don't think it's a security issue. Calling `form.is_valid()` returns `False`, and outside the `clean()` method the `char_field` value is not in `form.cleaned_data`. – Alasdair Mar 01 '18 at 13:49
  • The security issue can be in the clean method, which can be used to modify other fields or perform operations not directly related to the database, like generating some files. Its simpler doing this kind of operations just before data is saved (with `clean()`) than after its saved in the database. And again, it breaks the trust, uncleaned data should **never** be on the cleaned data array. – Guillaume Lebreton Mar 01 '18 at 14:14

1 Answers1

5

The clean() method is called after the field's validator. If the alias is invalid, then it won't be in cleaned_data. Your clean method should handle this case, for example:

def clean():
    cleaned_data = super().clean()
    print(self.errors)  # You should see the alias error here
    if 'alias' in cleaned_data:
        print(cleaned_data['alias'])
        # do any code that relies on cleaned_data['alias'] here
    return cleaned_data

See the docs on cleaning fields that depend on each other for more info.

Alasdair
  • 298,606
  • 55
  • 578
  • 516
  • Thanks, but that's not my problem, the problem is that not validation error was raised, although the alias was invalid. And again, CharField with validate_slugs works perfectly. I don't want to handle this in the clean method because it should have been handle in the clean_fields – Guillaume Lebreton Feb 19 '18 at 10:01
  • Look at the example in the docs I linked to. You have to handle the case that the field is not in `cleaned_data`. If you change your code so that it doesn't raise a `KeyError`, then you will see the error from the validator when you render the forms. Check `self.errors` at the beginning of the `clean` method if you don't believe me. – Alasdair Feb 19 '18 at 10:08
  • They check is the key exists because they are maybe not required fields. As mandatory field (PK), 'alias' sould always been there, or if invalid, instead a **Field** validation should have been raised. My question is not about how make it work, but why there is a diffrent behaviour between the 'SlugField' and 'CharField' with 'validate_slug'. I know the validation error will be fired if i trow the key error, but it sould have been fired before. – Guillaume Lebreton Feb 19 '18 at 10:21
  • The subject field *is* required in that example. For the third and final time, you can't assume a field exists in `cleaned_data`. Your code needs to handle the case that the field is invalid and not in `cleaned_data`. That is the case whether you are using a `SlugField` or a `CharField`. – Alasdair Feb 19 '18 at 10:28
  • Ok, but still i can't understand 2 things; the field validation worked, but why the error is not raised at this moment ? Why i don't have this behavior with CharField with `validate_slug` ? When im using CharField and i don't check if alias exists, i don't have problem because the error is raised before, as expected. – Guillaume Lebreton Feb 19 '18 at 12:48
  • And i found out why: with CharField, the error is raised but instead returning None in `cleaned_data` it returns the invalid value. Why then.. – Guillaume Lebreton Feb 19 '18 at 12:51
  • I can't see why the CharField would give you that behaviour. Perhaps it related to you having `primary_key=True` - as I said in the comment above that's an unusual approach. – Alasdair Feb 19 '18 at 13:38
  • No, i tried with another field, not unique and no PK. Still the same. I have pk as slug to ease my work with DRF – Guillaume Lebreton Feb 19 '18 at 13:50
  • @GuillaumeLebreton -- using your exact example... I was not able to reproduce your problem. I see `None` and `None` for both the slug field and char field. Perhaps there's something else in your code not shown here causing the problem. – sytech Feb 27 '18 at 18:07
  • That's strange, thanks for the feedback i will try to reproduce it in another project. Did you use django 2.0 ? – Guillaume Lebreton Feb 28 '18 at 15:42
  • @sytech I confirm that i was able to reproduce it with django 1.11 too, with nothing more than shown in the code here. – Guillaume Lebreton Feb 28 '18 at 16:23