Better surfacing of validation errors in UI / optional model instance validation

[ababic]

Hi @bmihelac,

Related issues:

• replace widget's validation by Django field's validation #64
• Validation when using ModelResource #176
• What is the best way to do validation data with postgresql as the database before import?  #415
• Fields without blank=True that are blank should error #601

Here's where I've got to so far. I thought it would be good to get some feedback before getting stuck into writing new tests, but the current ones are passing, so at least nothing is broken :)

When enabled, instance.full_clean() is called for each row, and any resulting validation errors surface themselves in the preview table like so:

The errors are hidden visually at first, then become visible when you hover over a row's first column, in such a way that it doesn't hide any of the values for the current row. It's pretty crude, but it's a starting point. Some examples:

Anyone wanting to give this a whirl for themselves should be able to install from git using:

pip install git+https://github.com/ababic/django-import-export.git@modelresource-validation-error-handling

Any/all feedback welcome :)

[coveralls]

Coverage increased (+0.9%) to 93.277% when pulling 55cb6f3 on ababic:modelresource-validation-error-handling into ec568b5 on django-import-export:master.

[ababic]

So, I've just realised the app has it's own layer of validation (powered by widgets), which is a little at odds with this approach.

I'm certain there are use cases where instance.full_clean() would be the preferred validation option (e.g. for simple model imports, where custom validation may have already been added at the model level), but It's also clear that there's a need for the custom widget approach, because you may have validation logic that is only relevant for imports, or might only be relevant to certain columns etc.

It feels like we need a way to combine the validation errors from both methods somehow, so that they can be displayed in a similar manner. Any thoughts on this?

[mgrdcm]

@ababic I've not had a chance to look at this much (and won't for a few days) but I'm excited by this work you're doing! I think this is great. I've definitely had use for seeing both types of errors surfaced better.

[ababic]

Okay, I've now got the field and model validation error handling playing nicely together, like I wanted. However, there are a few other things I was wondering about:

1. When a Field value is invalid, how can we go about still showing the invalid value in the preview table? Currently, the column appears blank, which I don't think works so well.
2. Is it a problem that import_obj() now raises ValidationError instead of ValueError? Could this cause issues with backwards compatibility?
3. Because field validation errors are grouped by column/field name, can we ditch the "Column 'field_name': " text that Field.clean() prepends to the original ValueError message? Can you think of any reason NOT to do this?
4. How do you feel about altering Field.clean() to raise ValidationError instead of ValueError, and we catch both types in Resource.import_obj() (to retain backwards compatibility for a while)? It feels odd to use ValidationError everywhere except Field.clean() - especially when ValidationError feels like a better choice.

[bmihelac]

@ababic in test_import_data_raises_unicode_validation_errors_without_erroring test widget would raise validation error but this error goes to row_errors, that is why test fails. Here are inspected values in debugger:

(django-import-export) ➜  django-import-export git:(modelresource-validation-error-handling) ✗ PYTHONPATH=".:tests:$PYTHONPATH" django-admin.py test  --settings=settings core.tests.test_resources.ModelResourceTest.test_import_data_raises_unicode_validation_errors_without_erroring

> /Users/bmihelac/dev/django/dev/django-import-export/tests/core/tests/test_resources.py(339)test_import_data_raises_unicode_validation_errors_without_erroring()
-> self.assertTrue(result.has_validation_errors())
(Pdb) result.row_errors()
[(1, [<import_export.results.Error object at 0x10992fc90>])]

Here is commit that passes test:

bmihelac@f3f00b2

Does this makes sense?

[ababic]

Thanks for looking at it @bmihelac. That is a surprise - even without clean_model_instances set, I would still expect import_obj() to catch the ValueError raised by the widget here:

django-import-export/import_export/resources.py

Line 380 in 158b0ee

except ValueError as e:

And re-raise it (along with other field errors for that row) as a ValidationError.

Something I can't yet explain yet is going on here. I'll investigate some more.

[ababic]

@bmihelac Okay - now we're cooking :) Python 2 tests are failing as expected - which will no doubt be due to a UnicodeDecodeError being raised. Now we can apply the force_text fix, and the test should now pass in Python 2.

[ababic]

Test suite wise, I'm thinking we should cover the following:

• Model instance validation working as expected (when the clean_model_instances option is used).
• Model instance validation errors and widget validation errors being combined, and invalid fields being excluded by object.full_clean(), so as not to repeat errors.
• Simple unit tests for InvalidRow methods.

@bmihelac Can you think of anything else?

[bmihelac]

@ababic this sounds great

[ababic]

Okay, cool. I won't get chance to finish things up this weekend due to other commitments, but I'll get this done in the next week or two (max).

[bmihelac]

Cool, let me know if anything is needed from me.

[ababic]

Hi @bmihelac, I'm now happy with these changes :)

[bmihelac]

@ababic great news, I'll check it ASAP. Many thanks for your work and patience :)

[ababic]

@bmihelac Thank you. You too :)

[bmihelac]

def validate_instance(self, instance, import_validation_errors=None, validate_unique=True):
    if import_validation_errors is None:
        errors = {}
    else:
        erros = import_validation_errors.copy()

Suggestion - using None instead of mutable dict.
As per https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html

[ababic]

@bmihelac Fully agree - that's a better solution :)

[bmihelac]

Rebased and merged in bd5ce92.

A new release will follow soon.

Thanks @ababic

[edgabaldi]

I'm testing this feature and it's not catching a DoesNotExist exception.

Model:

class Category(models.Model):
    name = models.CharField(max_length=100)

    def __str__(self):
        return self.name

class Post(models.Model):

    category = models.ForeignKey(
        to='app.Category',
        on_delete=models.CASCADE)

    title = models.CharField(max_length=100)

    content = models.TextField()

    def __str__(self):
        return self.title

Resource:

class PostResource(resources.ModelResource):

    class Meta:
        model = Post
        clean_model_instances = True

Test:

>>> resource = PostResource()
>>> dataset=tablib.Dataset(['', '3', 'foo', 'bar'], headers=['id', 'category', 'title', 'content'])
>>> result = resource.import_data(dataset, dry_run=True)
>>> result.has_errors()
True
>>> result.has_validation_errors()
False
>>> pprint.pprint(result.row_errors()[0][1][0].__dict__)
{'error': DoesNotExist('Category matching query does not exist.'),
 'row': OrderedDict([('id', ''),
                     ('category', '3'),
                     ('title', 'foo'),
                     ('content', 'bar')]),
 'traceback': 'Traceback (most recent call last):\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/src/django-import-export/import_export/resources.py", '
              'line 512, in import_row\n'
              '    self.import_obj(instance, row, dry_run)\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/src/django-import-export/import_export/resources.py", '
              'line 377, in import_obj\n'
              '    self.import_field(field, obj, data)\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/src/django-import-export/import_export/resources.py", '
              'line 360, in import_field\n'
              '    field.save(obj, data, is_m2m)\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/src/django-import-export/import_export/fields.py", '
              'line 113, in save\n'
              '    cleaned = self.clean(data)\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/src/django-import-export/import_export/fields.py", '
              'line 69, in clean\n'
              '    value = self.widget.clean(value, row=data)\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/src/django-import-export/import_export/widgets.py", '
              'line 352, in clean\n'
              '    return self.get_queryset(value, row, *args, '
              '**kwargs).get(**{self.field: val})\n'
              '  File '
              '"/Users/edgargabaldi/.virtualenvs/teste/lib/python3.7/site-packages/django/db/models/query.py", '
              'line 399, in get\n'
              '    self.model._meta.object_name\n'
              'app.models.Category.DoesNotExist: Category matching query does '
              'not exist.\n'}```

[ababic]

Hi @edgabaldi. If DoesNotExist is intended to be surfaced a ValidationError, then it's really the responsibility of the widget to catch that and re-raise it as a ValueError. This particular bit of functionality could get very messy indeed if it starts trying to capture all of the different types of exceptions that widgets could raise when cleaning.