DAL creates new objects without validating them

[dilyanpalauzov]

When a model field has validators=[...] and an object is created in src/dal/create_object() , get_queryset().create() is called and the validators are bypassed.

The validators shall be called in any case.

[jpic]

Agreed!

Looking forward to review a patch for this ;)

[dilyanpalauzov]

My proposal:

diff --git a/src/dal/views.py b/src/dal/views.py
--- a/src/dal/views.py
+++ b/src/dal/views.py
@@ -98,6 +98,7 @@ class BaseQuerySetView(ViewMixin, BaseListView):
 
     def create_object(self, text):
         """Create an object given a text."""
+        self.get_queryset().model(**{self.create_field: text}).clean_fields()
         return self.get_queryset().get_or_create(
             **{self.create_field: text})[0]
 
@@ -128,7 +129,10 @@ class BaseQuerySetView(ViewMixin, BaseListView):
         if text is None:
             return http.HttpResponseBadRequest()
 
-        result = self.create_object(text)
+        try:
+            result = self.create_object(text)
+        except Exception as e:
+            return http.JsonResponse({'validation_error': ", ".join(e.messages)})
 
         return http.JsonResponse({
             'id': result.pk,
diff --git a/src/dal_select2/static/autocomplete_light/select2.js b/src/dal_select2/static/autocomplete_light/select2.js
--- a/src/dal_select2/static/autocomplete_light/select2.js
+++ b/src/dal_select2/static/autocomplete_light/select2.js
@@ -95,6 +95,10 @@
                     xhr.setRequestHeader("X-CSRFToken", document.csrftoken);
                 },
                 success: function(data, textStatus, jqXHR ) {
+                    if (data.validation_error != undefined) {
+                        alert("Validation error: " + data.validation_error);
+                        return;
+                    }
                     select.append(
                         $('<option>', {value: data.id, text: data.text, selected: true})
                     );

[jpic]

Nice ! Do you think it would be better to run validators before adding the create option instead / also ?

[dilyanpalauzov]

I am not sure I understand the question.

Generally, running the validators if the object is already in the database is pointless, and running the validators right after the object was inserted in the database is also wrong.

Weigthing up I put the call before get_or_create().

In my use case the validator ensures, that the new element cannot end or start with a space. The users type an element with a terminating space. The idea is that the users get a notification when they request creating impossible things.

If there is no Create option, right after the space is typed, the user will be confused. The whole change is about notifying the user. Running the validator and informing with alerts the user, before she clicks on Create is also wrong, as the user might continue typing, in my case having a space in the middle is fine.

To sum up, before the user clicks on Create , no validation shall be made and only afterwards she shall see alerts.

[jpic]

We already check if the user has permission before showing the create option: https://github.com/yourlabs/django-autocomplete-light/blob/master/src/dal_select2/views.py#L45

Did you consider also adding validation at this level ? Perhaps we could display the validation error there too, instead of the Create option, somehow ?

Otherwise the alert works for me you know, just exploring other possibilities ;)

[dilyanpalauzov]

I have not considered adding validation at that level.

I have no preference between substituting Create ... xyz with an error message, and alert(), providing that a version of django-autocomplete-light is released, that does inform the user about failed validations.

It is probably better to cache the result of self.get_queryset() in a variable, rather than calling the method twice in create_object().

Invoking the validators on each keystroke might be expensive, and from this perspective an error generated in Select2ViewMixin.get_create_option() might be suboptimal.

[jpic]

Agreed, feel free to fiddle with both ideas and submit a PR from what you prefer.

If you have unit tests, you'll have better garantee that next contributor won't break it ;)

[dilyanpalauzov]

Here the variant without alert():

diff --git a/src/dal_select2/views.py b/src/dal_select2/views.py
--- a/src/dal_select2/views.py
+++ b/src/dal_select2/views.py
@@ -7,7 +7,7 @@ from dal.views import BaseQuerySetView, ViewMixin
 
 from django import http
 from django.core.exceptions import ImproperlyConfigured
-from django.utils import six
+from django.utils import html, six
 from django.utils.translation import ugettext as _
 from django.views.generic.list import View
 
@@ -27,7 +27,6 @@ class Select2ViewMixin(object):
 
     def get_create_option(self, context, q):
         """Form the correct create_option to append to results."""
-        create_option = []
         display_create_option = False
         if self.create_field and q:
             page_obj = context.get('page_obj', None)
@@ -41,13 +40,18 @@ class Select2ViewMixin(object):
             if q.lower() in existing_options:
                 display_create_option = False
 
-        if display_create_option and self.has_add_permission(self.request):
-            create_option = [{
-                'id': q,
-                'text': _('Create "%(new_value)s"') % {'new_value': q},
-                'create_id': True,
-            }]
-        return create_option
+            if display_create_option and self.has_add_permission(self.request):
+                try:
+                    self.get_queryset().model(**{self.create_field: q}).clean_fields()
+                except Exception as e:
+                    return [{'text': '{} cannot be created: '.format(html.escape(q)) + ', '.join(e.messages), 'disabled': True}]
+
+                return [{
+                    'id': q,
+                    'text': _('Create "%(new_value)s"') % {'new_value': html.escape(q)},
+                    'create_id': True,
+                }]
+        return []
 
     def render_to_response(self, context):
         """Return a JSON response in Select2 format."""

[jpic]

OK @dilyanpalauzov, can you at least push this in a branch please ?

[jpic]

Which do you think makes a better user experience ?

[dilyanpalauzov]

I think storing the object in a variable and rendering the variable via self.get_result_label() is the best form, as it presents the object to the the user as nice as possible, both when the validation fails and succeesses. alert() cannot do this.

diff --git a/src/dal_select2/views.py b/src/dal_select2/views.py
--- a/src/dal_select2/views.py
+++ b/src/dal_select2/views.py
@@ -41,13 +40,18 @@ class Select2ViewMixin(object):
             if q.lower() in existing_options:
                 display_create_option = False
 
-        if display_create_option and self.has_add_permission(self.request):
-            create_option = [{
-                'id': q,
-                'text': _('Create "%(new_value)s"') % {'new_value': q},
-                'create_id': True,
-            }]
-        return create_option
+            if display_create_option and self.has_add_permission(self.request):
+                try:
+                    m = self.get_queryset().model(**{self.create_field: q})
+                    m.clean_fields()
+                except Exception as e:
+                    return [{'text': '{} cannot be created: '.format(self.get_result_label(m)) + ', '.join(e.messages), 'disabled': True}]
+                return [{
+                    'id': q,
+                    'text': _('Create "%(new_value)s"') % {'new_value': self.get_result_label(m)},
+                    'create_id': True,
+                 }]
+        return []
 
     def render_to_response(self, context):
         """Return a JSON response in Select2 format."""

[jorgeorpinel]

Hi all. Do you know whether it's possible to not even show the "Create ..." option in the drop-down depending on certain validations? For example if the same name already exists.

p.s. I created https://github.com/yourlabs/django-autocomplete-light/issues/1063 for this.

[piscvau]

Hello Plus one for the need to have a validation before creating new objects. Last proposed solution by dilyanpalauzov. works fine. Too bad it was not included in the package as it is simpler than using django addanother.

[jpic]

@piscvau well nobody cared about it enough to open a pull request but it does look pretty nice I agree!