Call full_clean() in save(), so we don't need django-fullclean (#128)

At least, for Binder models this is the case.  All models managed by
Binder views should inherit from BinderModel, and if not, the user
should code validation error checking in _store().

In all our projects we use django-fullclean because being able to
accidentally save invalid models is a huge mind fuck.  Thus, it is
pointless to call full_clean() in our views, as it means we'll be
running potentially expensive validations twice.  For example,
relation fields check that the foreign relation exists, which requires
a query per relation field.

It is technically a breaking change, but for most projects this will
be a non-issue.  The test suite of Boekestijn passed except for one
test which was specifically for its User view, which uses a non-Binder
model.  This was relatively easy to convert to use a full_clean() in
its _store() method.

CHANGELOG.md

@@ -13,6 +13,16 @@ Using `Q()` objects instead of querysets allows making queries in
 `with_ids` without requiring a subquery on filtered relations, which
 can be a big performance win on large tables.
 
+Now, `full_clean` is automatically called upon `save()` of a
+`BinderModel`.  This should not be a huge breaking change because most
+projects "in the wild" are already using the
+[django-fullclean](https://github.com/fish-ball/django-fullclean)
+plugin.  This should reduce the number of checks done while saving
+objects.  However, it also means that if a non-binder model is saved
+it will no longer be validated.  If you want this to happen you need
+to override `_store` to do some checking.
+
+
 ## Version 1.4.0
 
 ### Features

binder/models.py

@@ -1,5 +1,6 @@
 import re
 import warnings
+from collections import defaultdict
 from datetime import date, datetime, time
 from contextlib import suppress
 
@@ -470,6 +471,57 @@ def annotations(cls):
 		return getattr(cls, ann_name)
 
 
+	def save(self, *args, **kwargs):
+		self.full_clean() # Never allow saving invalid models!
+		return super().save(*args, **kwargs)
+
+
+	def full_clean(self, *args, **kwargs):
+		# Determine if the field needs an extra nullability check.
+		# Expects the field object (not the field name)
+		def field_needs_nullability_check(field):
+			if isinstance(field, (models.CharField, models.TextField, models.BooleanField)):
+				if field.blank and not field.null:
+					return True
+
+			return False
+
+
+		validation_errors = defaultdict(list)
+
+		try:
+			res = super().full_clean(*args, **kwargs)
+		except ValidationError as ve:
+			if hasattr(ve, 'error_dict'):
+				for key, value in ve.error_dict.items():
+					validation_errors[key] += value
+			elif hasattr(ve, 'error_list'):
+				for e in ve.error_list:
+					validation_errors['null'].append(e) # XXX
+
+		# Django's standard full_clean() doesn't complain about some
+		# not-NULL fields being None.  This causes save() to explode
+		# with a django.db.IntegrityError because the column is NOT
+		# NULL. Tyvm, Django.  So we perform an extra NULL check for
+		# some cases. See #66, T2989, T9646.
+		for f in self._meta.fields:
+			if field_needs_nullability_check(f):
+				# gettattr on a foreignkey foo gets the related model, while foo_id just gets the id.
+				# We don't need or want the model (nor the DB query), we'll take the id thankyouverymuch.
+				name = f.name + ('_id' if isinstance(f, models.ForeignKey) else '')
+
+				if getattr(self, name) is None and getattr(self, f.name) is None:
+					validation_errors[f.name].append(ValidationError(
+						'This field cannot be null.',
+						code='null',
+					))
+
+		if validation_errors:
+			raise ValidationError(validation_errors)
+		else:
+			return res
+
+
 def history_obj_post_init(sender, instance, **kwargs):
 	instance._history = instance.binder_concrete_fields_as_dict(skip_deferred_fields=True)
 

binder/views.py

@@ -1076,37 +1076,20 @@ def get(self, request, pk=None, withs=None):
 
 
 
-	# Determine if the field needs an extra nullability check.
-	# Expects the field object (not the field name)
-	def field_needs_nullability_check(self, field):
-		if isinstance(field, (models.CharField, models.TextField, models.BooleanField)):
-			if field.blank and not field.null:
-				return True
-
-		return False
-
-
-
-	def binder_clean(self, obj, pk=None):
-		try:
-			res = obj.full_clean()
-		except ValidationError as ve:
-			model_name = self.get_model_view(obj.__class__)._model_name()
-
-			raise BinderValidationError({
-				model_name: {
-					obj.pk if pk is None else pk: {
-						f: [
-							{'code': e.code, 'message': e.messages[0]}
-							for e in el
-						]
-						for f, el in ve.error_dict.items()
-					}
+	def binder_validation_error(self, obj, validation_error, pk=None):
+		model_name = self.get_model_view(obj.__class__)._model_name()
+
+		return BinderValidationError({
+			model_name: {
+				obj.pk if pk is None else pk: {
+					f: [
+						{'code': e.code, 'message': e.messages[0]}
+						for e in el
+					]
+					for f, el in validation_error.error_dict.items()
 				}
-			})
-		else:
-			return res
-
+			}
+		})
 
 
 	# Deserialize JSON to Django Model objects.
@@ -1150,39 +1133,15 @@ def store_m2m_field(obj, field, value, request):
 			except BinderValidationError as e:
 				validation_errors.append(e)
 
-		try:
-			self.binder_clean(obj, pk=pk)
-		except BinderValidationError as bve:
-			validation_errors.append(bve)
-
-
-		# full_clean() doesn't complain about some not-NULL fields being None.
-		# This causes save() to explode with a django.db.IntegrityError because the
-		# column is NOT NULL. Tyvm, Django.
-		# So we perform an extra NULL check for some cases. See #66, T2989, T9646.
-		for f in obj._meta.fields:
-			if self.field_needs_nullability_check(f):
-				# gettattr on a foreignkey foo gets the related model, while foo_id just gets the id.
-				# We don't need or want the model (nor the DB query), we'll take the id thankyouverymuch.
-				name = f.name + ('_id' if isinstance(f, models.ForeignKey) else '')
-
-				if getattr(obj, name) is None:
-					e = BinderValidationError({
-						self._model_name(): {
-							obj.pk if pk is None else pk: {
-								f.name: [{
-									'code': 'null',
-									'message': 'This field cannot be null.'
-								}]
-							}
-						}
-					})
-					validation_errors.append(e)
 
 		if validation_errors:
 			raise sum(validation_errors, None)
 
-		obj.save()
+		try:
+			obj.save()
+		except ValidationError as ve:
+			validation_errors.append(self.binder_validation_error(obj, ve, pk=pk))
+
 
 		for field, value in deferred_m2ms.items():
 			try:
@@ -1253,20 +1212,18 @@ def _store_m2m_field(self, obj, field, value, request):
 			for addobj in obj_field.model.objects.filter(id__in=new_ids - old_ids):
 				setattr(addobj, obj_field.field.name, obj)
 				try:
-					self.binder_clean(addobj)
-				except BinderValidationError as bve:
-					validation_errors.append(bve)
+					addobj.save()
+				except ValidationError as ve:
+					validation_errors.append(self.binder_validation_error(addobj, ve))
 				else:
 					addobj.save()
 		elif getattr(obj._meta.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor:
 			remote_obj = obj._meta.get_field(field).related_model.objects.get(pk=value[0])
 			setattr(obj, field, remote_obj)
 			try:
-				self.binder_clean(remote_obj)
-			except BinderValidationError as bve:
-				validation_errors.append(bve)
-			else:
 				remote_obj.save()
+			except ValidationError as ve:
+				validation_errors.append(self.binder_validation_error(remote_obj, ve))
 		elif any(f.name == field for f in self._get_reverse_relations()):
 			#### XXX FIXME XXX ugly quick fix for reverse relation + multiput issue
 			if any(v for v in value if v < 0):
@@ -1370,6 +1327,10 @@ def _store_field(self, obj, field, value, request, pk=None):
 					except TypeError:
 						raise BinderFieldTypeError(self.model.__name__, field)
 					except ValidationError as ve:
+						# This would be nice, but this particular validation error
+						# has no error dict... (TODO FIXME)
+						# raise self.binder_validation_error(obj, ve, pk=pk)
+
 						model_name = self.get_model_view(obj.__class__)._model_name()
 						raise BinderValidationError({
 							model_name: {
@@ -1961,8 +1922,10 @@ def soft_delete(self, obj, undelete, request):
 				return
 
 		obj.deleted = not undelete
-		self.binder_clean(obj, pk=obj.pk)
-		obj.save()
+		try:
+			obj.save()
+		except ValidationError as ve:
+			raise self.binder_validation_error(obj, ve)
 
 
 
@@ -2112,19 +2075,7 @@ def dispatch_file_field(self, request, pk=None, file_field=None):
 				return JsonResponse( {"data": {file_field_name: path}} )
 
 			except ValidationError as ve:
-				model_name = self.get_model_view(obj.__class__)._model_name()
-
-				raise BinderValidationError({
-					model_name: {
-						obj.pk if pk is None else pk: {
-							f: [
-								{'code': e.code, 'message': e.messages[0]}
-								for e in el
-							]
-							for f, el in ve.error_dict.items()
-						}
-					}
-				})
+				raise self.binder_validation_error(obj, ve, pk=pk)
 
 		if request.method == 'DELETE':
 			self._require_model_perm('change', request)

tests/filters/test_uuid_field_filtering.py

@@ -25,19 +25,15 @@ def setUp(self):
 		self.artis.save()
 
 		fabbby = Caretaker(name='fabbby')
-		fabbby.full_clean()
 		fabbby.save()
 
 		door1 = Gate(zoo=self.gaia, keeper=fabbby, serial_number=None)
-		door1.full_clean()
 		door1.save()
 
 		door2 = Gate(zoo=self.emmen, keeper=fabbby, serial_number='{2e93ec15-2d68-477d-960f-52779ef6198b}')
-		door2.full_clean()
 		door2.save()
 
 		door3 = Gate(zoo=self.artis, keeper=fabbby, serial_number='3e93ec15-2d68-477d-960f-52779ef6198b')
-		door3.full_clean()
 		door3.save()
 
 

tests/plugins/test_csvexport.py

@@ -25,7 +25,7 @@ def temp_imagefile(width, height, format):
 		return f
 
 	def setUp(self):
-		animal = Animal()
+		animal = Animal(name='test')
 		animal.save()
 
 		self.pictures = []

tests/plugins/test_imageview.py

@@ -37,7 +37,7 @@ def temp_imagefile(width, height, format):
 		return f
 
 	def _get_picture(self, width, height):
-		animal = Animal()
+		animal = Animal(name='test')
 		animal.save()
 
 		file = ImageTest.temp_imagefile(width, height, 'jpeg')

tests/test_history.py

@@ -59,7 +59,6 @@ def test_model_with_history_creates_changes_on_creation(self):
 
 	def test_model_with_history_creates_changes_on_update_but_only_for_changed_fields(self):
 		daffy = Animal(name='Daffy Duck')
-		daffy.full_clean()
 		daffy.save()
 
 		# Model changes outside the HTTP API aren't recorded (should they be?)
@@ -94,10 +93,8 @@ def test_model_with_history_creates_changes_on_update_but_only_for_changed_field
 
 	def test_model_with_related_history_model_creates_changes_on_the_same_changeset(self):
 		mickey = Caretaker(name='Mickey')
-		mickey.full_clean()
 		mickey.save()
 		pluto = Animal(name='Pluto')
-		pluto.full_clean()
 		pluto.save()
 
 		# Model changes outside the HTTP API aren't recorded (should they be?)