From 6a96bef3a09ca33b83c0d266ed082b89f472954c Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Mon, 29 Mar 2021 11:22:05 +0200 Subject: [PATCH 01/34] Fix horrible typo --- binder/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/views.py b/binder/views.py index e42dd221..e4c188fb 100644 --- a/binder/views.py +++ b/binder/views.py @@ -2052,7 +2052,7 @@ def _multi_put_deletions(self, deletions, new_id_map, request): def multi_put(self, request): - logger.info('ACTIVATING THE MULTI-PUT!!!1!') + logger.info('ACTIVATING THE MULTI-PUT!!!!!') # Hack to communicate to _store() that we're not interested in # the new data (for perf reasons). From fd3c0499c5ce1f7cd562c1fe6a3c528e82d92378 Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Mon, 29 Mar 2021 13:27:38 +0200 Subject: [PATCH 02/34] Add standalone validation feature --- binder/exceptions.py | 10 ++++++++++ binder/views.py | 37 ++++++++++++++++++++++++++++++++----- docs/api.md | 11 ++++++++++- 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/binder/exceptions.py b/binder/exceptions.py index d5aec299..fbf9bd6f 100644 --- a/binder/exceptions.py +++ b/binder/exceptions.py @@ -235,3 +235,13 @@ def __add__(self, other): else: errors[model] = other.errors[model] return BinderValidationError(errors) + + +class BinderSkipSave(BinderException): + """Used to abort the database transaction when performing a non-save validation request.""" + http_code = 200 + code = 'SkipSave' + + def __init__(self): + super().__init__() + self.fields['message'] = 'No validation errors were encountered.' diff --git a/binder/views.py b/binder/views.py index e4c188fb..ad7086e0 100644 --- a/binder/views.py +++ b/binder/views.py @@ -25,7 +25,11 @@ from django.db.models.fields.reverse_related import ForeignObjectRel -from .exceptions import BinderException, BinderFieldTypeError, BinderFileSizeExceeded, BinderForbidden, BinderImageError, BinderImageSizeExceeded, BinderInvalidField, BinderIsDeleted, BinderIsNotDeleted, BinderMethodNotAllowed, BinderNotAuthenticated, BinderNotFound, BinderReadOnlyFieldError, BinderRequestError, BinderValidationError, BinderFileTypeIncorrect, BinderInvalidURI +from .exceptions import ( + BinderException, BinderFieldTypeError, BinderFileSizeExceeded, BinderForbidden, BinderImageError, BinderImageSizeExceeded, + BinderInvalidField, BinderIsDeleted, BinderIsNotDeleted, BinderMethodNotAllowed, BinderNotAuthenticated, BinderNotFound, + BinderReadOnlyFieldError, BinderRequestError, BinderValidationError, BinderFileTypeIncorrect, BinderInvalidURI, BinderSkipSave +) from . import history from .orderable_agg import OrderableArrayAgg, GroupConcat from .models import FieldFilter, BinderModel, ContextAnnotation, OptionalAnnotation, BinderFileField @@ -263,6 +267,9 @@ class ModelView(View): # NOTE: custom _store__foo() methods will still be called for unupdatable fields. unupdatable_fields = [] + # Allow validation without saving. + allow_standalone_validation = False + # Fields to use for ?search=foo. Empty tuple for disabled search. # NOTE: only string fields and 'id' are supported. # id is hardcoded to be treated as an integer. @@ -1349,6 +1356,17 @@ def binder_validation_error(self, obj, validation_error, pk=None): }) + + def _abort_when_standalone_validation(self, request): + """Raise a `BinderSkipSave` exception when this is a standalone request.""" + if self.allow_standalone_validation: + if 'validate' in request.GET: + raise BinderSkipSave + else: + raise BinderException('Standalone validation not enabled. You must enable this feature explicitly.') + + + # Deserialize JSON to Django Model objects. # obj: Model object to update (for PUT), newly created object (for POST) # values: Python dict of {field name: value} (parsed JSON) @@ -2060,13 +2078,15 @@ def multi_put(self, request): data, deletions = self._multi_put_parse_request(request) objects = self._multi_put_collect_objects(data) - objects, overrides = self._multi_put_override_superclass(objects) + objects, overrides = self._multi_put_override_superclass(objects) # model inheritance objects = self._multi_put_convert_backref_to_forwardref(objects) dependencies = self._multi_put_calculate_dependencies(objects) ordered_objects = self._multi_put_order_dependencies(dependencies) - new_id_map = self._multi_put_save_objects(ordered_objects, objects, request) - self._multi_put_id_map_add_overrides(new_id_map, overrides) - new_id_map = self._multi_put_deletions(deletions, new_id_map, request) + new_id_map = self._multi_put_save_objects(ordered_objects, objects, request) # may raise validation errors + self._multi_put_id_map_add_overrides(new_id_map, overrides) # model inheritance + new_id_map = self._multi_put_deletions(deletions, new_id_map, request) # may raise validation errors + + self._abort_when_standalone_validation(request) output = defaultdict(list) for (model, oid), nid in new_id_map.items(): @@ -2102,6 +2122,8 @@ def put(self, request, pk=None): data = self._store(obj, values, request) + self._abort_when_standalone_validation(request) + new = dict(data) new.pop('_meta', None) @@ -2132,6 +2154,8 @@ def post(self, request, pk=None): data = self._store(self.model(), values, request) + self._abort_when_standalone_validation(request) + new = dict(data) new.pop('_meta', None) @@ -2169,6 +2193,9 @@ def delete(self, request, pk=None, undelete=False, skip_body_check=False): raise BinderNotFound() self.delete_obj(obj, undelete, request) + + self._abort_when_standalone_validation(request) + logger.info('{}DELETEd {} #{}'.format('UN' if undelete else '', self._model_name(), pk)) return HttpResponse(status=204) # No content diff --git a/docs/api.md b/docs/api.md index 31b0cbf1..51ba97be 100644 --- a/docs/api.md +++ b/docs/api.md @@ -67,7 +67,7 @@ Ordering is a simple matter of enumerating the fields in the `order_by` query pa The default sort order is ascending. If you want to sort in descending order, simply prefix the attribute name with a minus sign. This honors the scoping, so `api/animal?order_by=-name,id` will sort by `name` in descending order and by `id` in ascending order. -### Saving a model +### Saving or updating a model Creating a new model is possible with `POST api/animal/`, and updating a model with `PUT api/animal/`. Both requests accept a JSON body, like this: @@ -161,6 +161,15 @@ If this request succeeds, you'll get back a mapping of the fake ids and the real It is also possible to update existing models with multi PUT. If you use a "real" id instead of a fake one, the model will be updated instead of created. + +#### Standalone Validation (without saving models) + +Sometimes you want to validate the model that you are going to save without actually saving it. This is useful, for example, when you want to inform the user of validation errors on the frontend, without having to implement the validation logic again. You may check for validation errors by sending a `POST`, `PUT` or `PATCH` request with an additional query parameter `validate`. + +Currently this is implemented by raising an `BinderValidateOnly` exception, which makes sure that the atomic database transaction is aborted. Ideally, you would only want to call the validation logic on the models, so only calling validation for fields and validation for model (`clean()`). But for now, we do it this way, at the cost of a performance penalty. + +It is important to realize that in this way, the normal `save()` function is called on a model, so it is possible that possible side effects are triggered, when these are implemented directly in `save()`, as opposed to in a signal method, which would be preferable. In other words, we cannot guarantee that the request will be idempotent. Therefore, the validation only feature is disabled by default and must be enabled by setting `allow_standalone_validation=True` on the view. + ### Uploading files To upload a file, you have to add it to the `file_fields` of the `ModelView`: From ea061e6d2f28121e6512f9ddb1a8b5b33a162c25 Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Mon, 29 Mar 2021 14:17:05 +0200 Subject: [PATCH 03/34] Correctly extract querystring parameter for PUT --- binder/views.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/binder/views.py b/binder/views.py index ad7086e0..e7cc5c6d 100644 --- a/binder/views.py +++ b/binder/views.py @@ -15,7 +15,7 @@ from django.views.generic import View from django.core.exceptions import ObjectDoesNotExist, FieldError, ValidationError, FieldDoesNotExist from django.http import HttpResponse, StreamingHttpResponse, HttpResponseForbidden -from django.http.request import RawPostDataException +from django.http.request import RawPostDataException, QueryDict from django.db import models, connections from django.db.models import Q, F from django.db.models.lookups import Transform @@ -1360,7 +1360,8 @@ def binder_validation_error(self, obj, validation_error, pk=None): def _abort_when_standalone_validation(self, request): """Raise a `BinderSkipSave` exception when this is a standalone request.""" if self.allow_standalone_validation: - if 'validate' in request.GET: + params = QueryDict(request.body) + if 'validate' in params: raise BinderSkipSave else: raise BinderException('Standalone validation not enabled. You must enable this feature explicitly.') From 796d32320276bcb1b8660af078eb916037944618 Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Mon, 29 Mar 2021 14:42:13 +0200 Subject: [PATCH 04/34] Raise the correct exception when flag is not set --- binder/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/views.py b/binder/views.py index e7cc5c6d..63367893 100644 --- a/binder/views.py +++ b/binder/views.py @@ -1364,7 +1364,7 @@ def _abort_when_standalone_validation(self, request): if 'validate' in params: raise BinderSkipSave else: - raise BinderException('Standalone validation not enabled. You must enable this feature explicitly.') + raise BinderRequestError('Standalone validation not enabled. You must enable this feature explicitly.') From dccd770dfb29fe43c58ecbfed078225e1bbd9e79 Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Fri, 25 Jun 2021 12:11:54 +0200 Subject: [PATCH 05/34] Improve comment --- binder/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/exceptions.py b/binder/exceptions.py index fbf9bd6f..2844f488 100644 --- a/binder/exceptions.py +++ b/binder/exceptions.py @@ -238,7 +238,7 @@ def __add__(self, other): class BinderSkipSave(BinderException): - """Used to abort the database transaction when performing a non-save validation request.""" + """Used to abort the database transaction when non-save validation was successfull.""" http_code = 200 code = 'SkipSave' From 42808415227a65713c911860a6a6c1f209643e81 Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Mon, 29 Mar 2021 14:52:39 +0200 Subject: [PATCH 06/34] Fix stupid logic --- binder/views.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/binder/views.py b/binder/views.py index 63367893..1bd57ab7 100644 --- a/binder/views.py +++ b/binder/views.py @@ -1359,12 +1359,12 @@ def binder_validation_error(self, obj, validation_error, pk=None): def _abort_when_standalone_validation(self, request): """Raise a `BinderSkipSave` exception when this is a standalone request.""" - if self.allow_standalone_validation: - params = QueryDict(request.body) - if 'validate' in params: + if 'validate' in params: + if self.allow_standalone_validation: + params = QueryDict(request.body) raise BinderSkipSave - else: - raise BinderRequestError('Standalone validation not enabled. You must enable this feature explicitly.') + else: + raise BinderRequestError('Standalone validation not enabled. You must enable this feature explicitly.') From b83003cb74da240740d63eae3293b44668c3baaa Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Fri, 25 Jun 2021 14:53:22 +0200 Subject: [PATCH 07/34] Stop earlier when flag not set --- binder/views.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/binder/views.py b/binder/views.py index 599eebec..5df3ceaf 100644 --- a/binder/views.py +++ b/binder/views.py @@ -378,6 +378,10 @@ def dispatch(self, request, *args, **kwargs): response = None try: + # only allow standalone validation if you know what you are doing + if 'validate' in request.GET and request.GET['validate'] == 'true' and not self.allow_standalone_validation: + raise BinderRequestError('Standalone validation not enabled. You must enable this feature explicitly.') + #### START TRANSACTION with ExitStack() as stack, history.atomic(source='http', user=request.user, uuid=request.request_id): transaction_dbs = ['default'] @@ -1375,11 +1379,12 @@ def binder_validation_error(self, obj, validation_error, pk=None): def _abort_when_standalone_validation(self, request): """Raise a `BinderSkipSave` exception when this is a standalone request.""" - if 'validate' in params: + if 'validate' in request.GET and request.GET['validate'] == 'true': if self.allow_standalone_validation: params = QueryDict(request.body) raise BinderSkipSave else: + print('validate not enabled') raise BinderRequestError('Standalone validation not enabled. You must enable this feature explicitly.') @@ -2142,6 +2147,9 @@ def put(self, request, pk=None): if hasattr(obj, 'deleted') and obj.deleted: raise BinderIsDeleted() + + logger.info('storing') + data = self._store(obj, values, request) self._abort_when_standalone_validation(request) From 31a61bfcb67d7b1aaf1c60ff34a2c6c246f3a8a3 Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Wed, 30 Jun 2021 14:26:01 +0200 Subject: [PATCH 08/34] Add tests for validation flow. We check if the action is not performed, and thus aborted internally by a BinderSkipSave exception, for POST, PUT, MULTI-PUT and DELETE. --- tests/test_model_validation.py | 228 ++++++++++++++++++++++++++ tests/testapp/models/animal.py | 2 +- tests/testapp/views/caretaker.py | 3 + tests/testapp/views/contact_person.py | 3 + 4 files changed, 235 insertions(+), 1 deletion(-) create mode 100644 tests/test_model_validation.py diff --git a/tests/test_model_validation.py b/tests/test_model_validation.py new file mode 100644 index 00000000..59dac762 --- /dev/null +++ b/tests/test_model_validation.py @@ -0,0 +1,228 @@ +from re import I +from tests.testapp.models import contact_person +from tests.testapp.models.contact_person import ContactPerson +from django.test import TestCase, Client + +import json +from binder.json import jsonloads +from django.contrib.auth.models import User +from .testapp.models import Animal, Caretaker, ContactPerson + + +class TestModelValidation(TestCase): + """ + Test the validate-only functionality. + + We check that the validation is executed as normal, but that the models + are not created when the validate query paramter is set to true. + + We check validation for: + - post + - put + - multi-put + - delete + """ + + + def setUp(self): + super().setUp() + u = User(username='testuser', is_active=True, is_superuser=True) + u.set_password('test') + u.save() + self.client = Client() + r = self.client.login(username='testuser', password='test') + self.assertTrue(r) + + # some update payload + self.model_data_with_error = { + 'name': 'very_special_forbidden_contact_person_name', # see `contact_person.py` + } + self.model_data = { + 'name': 'Scooooooby', + } + + + ### helpers ### + + + def assert_validation_error(self, response, person_id=None): + if person_id is None: + person_id = 'null' # for post + + self.assertEqual(response.status_code, 400) + + returned_data = jsonloads(response.content) + + # check that there were validation errors + self.assertEqual(returned_data.get('code'), 'ValidationError') + + # check that the validation error is present + validation_error = returned_data.get('errors').get('contact_person').get(str(person_id)).get('__all__')[0] + self.assertEqual(validation_error.get('code'), 'invalid') + self.assertEqual(validation_error.get('message'), 'Very special validation check that we need in `tests.M2MStoreErrorsTest`.') + + + def assert_multi_put_validation_error(self, response): + self.assertEqual(response.status_code, 400) + + returned_data = jsonloads(response.content) + + # check that there were validation errors + self.assertEqual(returned_data.get('code'), 'ValidationError') + + # check that all (two) the validation errors are present + for error in returned_data.get('errors').get('contact_person').values(): + validation_error = error.get('__all__')[0] + self.assertEqual(validation_error.get('code'), 'invalid') + self.assertEqual(validation_error.get('message'), 'Very special validation check that we need in `tests.M2MStoreErrorsTest`.') + + + ### tests ### + + + def assert_no_validation_error(self, response): + self.assertEqual(response.status_code, 200) + + # check that the validation was successful + returned_data = jsonloads(response.content) + self.assertEqual(returned_data.get('code'), 'SkipSave') + self.assertEqual(returned_data.get('message'), 'No validation errors were encountered.') + + + def test_validate_on_post(self): + self.assertEqual(0, ContactPerson.objects.count()) + + # trigger a validation error + response = self.client.post('/contact_person/?validate=true', data=json.dumps(self.model_data_with_error), content_type='application/json') + self.assert_validation_error(response) + self.assertEqual(0, ContactPerson.objects.count()) + + # now without validation errors + response = self.client.post('/contact_person/?validate=true', data=json.dumps(self.model_data), content_type='application/json') + self.assert_no_validation_error(response) + self.assertEqual(0, ContactPerson.objects.count()) + + # now for real + response = self.client.post('/contact_person/', data=json.dumps(self.model_data), content_type='application/json') + self.assertEqual(response.status_code, 200) + self.assertEqual('Scooooooby', ContactPerson.objects.first().name) + + + def test_validate_on_put(self): + person_id = ContactPerson.objects.create(name='Scooby Doo').id + self.assertEqual('Scooby Doo', ContactPerson.objects.first().name) + + # trigger a validation error + response = self.client.put(f'/contact_person/{person_id}/?validate=true', data=json.dumps(self.model_data_with_error), content_type='application/json') + self.assert_validation_error(response, person_id) + self.assertEqual('Scooby Doo', ContactPerson.objects.first().name) + + # now without validation errors + response = self.client.put(f'/contact_person/{person_id}/?validate=true', data=json.dumps(self.model_data), content_type='application/json') + self.assert_no_validation_error(response) + self.assertEqual('Scooby Doo', ContactPerson.objects.first().name) + + # now for real + response = self.client.put(f'/contact_person/{person_id}/', data=json.dumps(self.model_data), content_type='application/json') + self.assertEqual(response.status_code, 200) + self.assertEqual('Scooooooby', ContactPerson.objects.first().name) + + + def test_validate_on_multiput(self): + person_1_id = ContactPerson.objects.create(name='Scooby Doo 1').id + person_2_id = ContactPerson.objects.create(name='Scooby Doo 2').id + + multi_put_data = {'data': [ + { + 'id': person_1_id, + 'name': 'New Scooby', + }, + { + 'id': person_2_id, + 'name': 'New Doo' + } + ]} + + multi_put_data_with_error = {'data': [ + { + 'id': person_1_id, + 'name': 'very_special_forbidden_contact_person_name', + }, + { + 'id': person_2_id, + 'name': 'very_special_forbidden_contact_person_name' + } + ]} + + # trigger a validation error + response = self.client.put(f'/contact_person/?validate=true', data=json.dumps(multi_put_data_with_error), content_type='application/json') + self.assert_multi_put_validation_error(response) + self.assertEqual('Scooby Doo 1', ContactPerson.objects.get(id=person_1_id).name) + self.assertEqual('Scooby Doo 2', ContactPerson.objects.get(id=person_2_id).name) + + + # now without validation error + response = self.client.put(f'/contact_person/?validate=true', data=json.dumps(multi_put_data), content_type='application/json') + self.assert_no_validation_error(response) + self.assertEqual('Scooby Doo 1', ContactPerson.objects.get(id=person_1_id).name) + self.assertEqual('Scooby Doo 2', ContactPerson.objects.get(id=person_2_id).name) + + # now for real + response = self.client.put(f'/contact_person/', data=json.dumps(multi_put_data), content_type='application/json') + self.assertEqual(response.status_code, 200) + self.assertEqual('New Scooby', ContactPerson.objects.get(id=person_1_id).name) + self.assertEqual('New Doo', ContactPerson.objects.get(id=person_2_id).name) + + + def test_validate_on_delete(self): + '''Check if deletion is cancelled when we only attempt to validate + the delete operation. This test only covers validation of the + on_delete=PROTECT constraint of a fk.''' + + def is_deleted(obj): + '''Whether the obj was soft-deleted, so when the 'deleted' + attribute is not present, or when it is True.''' + + try: + obj.refresh_from_db() + except obj.DoesNotExist: + return True # hard-deleted + return animal.__dict__.get('deleted') or False + + + # animal has a fk to caretaker with on_delete=PROTECT + caretaker = Caretaker.objects.create(name='Connie Care') + animal = Animal.objects.create(name='Pony', caretaker=caretaker) + + + ### with validation error + + response = self.client.delete(f'/caretaker/{caretaker.id}/?validate=true') + # assert validation error + # and check that it was about the PROTECTED constraint + self.assertEqual(response.status_code, 400) + returned_data = jsonloads(response.content) + self.assertEqual(returned_data.get('code'), 'ValidationError') + self.assertEqual(returned_data.get('errors').get('caretaker').get(str(caretaker.id)).get('id')[0].get('code'), 'protected') + + self.assertFalse(is_deleted(caretaker)) + + + ### without validation error + + # now we delete the animal to make sure that deletion is possible + # note that soft-deleting will of course not remove the validation error + animal.delete() + + # now no validation error should be trown + response = self.client.delete(f'/caretaker/{caretaker.id}/?validate=true') + print(response.content) + self.assert_no_validation_error(response) + + self.assertFalse(is_deleted(caretaker)) + + + ### now for real + + response = self.client.delete(f'/caretaker/{caretaker.id}/') + self.assertTrue(is_deleted(caretaker)) diff --git a/tests/testapp/models/animal.py b/tests/testapp/models/animal.py index 85d844e0..49d48987 100644 --- a/tests/testapp/models/animal.py +++ b/tests/testapp/models/animal.py @@ -14,7 +14,7 @@ class Animal(LoadedValuesMixin, BinderModel): name = models.TextField(max_length=64) zoo = models.ForeignKey('Zoo', on_delete=models.CASCADE, related_name='animals', blank=True, null=True) zoo_of_birth = models.ForeignKey('Zoo', on_delete=models.CASCADE, related_name='+', blank=True, null=True) # might've been born outside captivity - caretaker = models.ForeignKey('Caretaker', on_delete=models.PROTECT, related_name='animals', blank=True, null=True) + caretaker = models.ForeignKey('Caretaker', on_delete=models.PROTECT, related_name='animals', blank=True, null=True) # we use the fact that this one is PROTECT in `test_model_validation.py` deleted = models.BooleanField(default=False) # Softdelete def __str__(self): diff --git a/tests/testapp/views/caretaker.py b/tests/testapp/views/caretaker.py index b74006b4..0db69683 100644 --- a/tests/testapp/views/caretaker.py +++ b/tests/testapp/views/caretaker.py @@ -7,3 +7,6 @@ class CaretakerView(ModelView): unwritable_fields = ['last_seen'] unupdatable_fields = ['first_seen'] model = Caretaker + + # see `test_model_validation.py` + allow_standalone_validation = True diff --git a/tests/testapp/views/contact_person.py b/tests/testapp/views/contact_person.py index c1e90c5b..bc2f25fe 100644 --- a/tests/testapp/views/contact_person.py +++ b/tests/testapp/views/contact_person.py @@ -6,3 +6,6 @@ class ContactPersonView(ModelView): model = ContactPerson m2m_fields = ['zoos'] unwritable_fields = ['created_at', 'updated_at'] + + # see `test_model_validation.py` + allow_standalone_validation = True From 9be35a226e4f296522951e45b05272ad2f4aab1d Mon Sep 17 00:00:00 2001 From: Jeroen van Riel Date: Wed, 30 Jun 2021 14:30:59 +0200 Subject: [PATCH 09/34] Some cleanup --- binder/exceptions.py | 4 +++- binder/views.py | 6 +----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/binder/exceptions.py b/binder/exceptions.py index 2844f488..75e4e572 100644 --- a/binder/exceptions.py +++ b/binder/exceptions.py @@ -238,7 +238,9 @@ def __add__(self, other): class BinderSkipSave(BinderException): - """Used to abort the database transaction when non-save validation was successfull.""" + """Used to abort the database transaction when validation was successfull. + Validation is possible when saving (post, put, multi-put) or deleting models.""" + http_code = 200 code = 'SkipSave' diff --git a/binder/views.py b/binder/views.py index 5df3ceaf..8d486cb8 100644 --- a/binder/views.py +++ b/binder/views.py @@ -1378,13 +1378,12 @@ def binder_validation_error(self, obj, validation_error, pk=None): def _abort_when_standalone_validation(self, request): - """Raise a `BinderSkipSave` exception when this is a standalone request.""" + """Raise a `BinderSkipSave` exception when this is a validation request.""" if 'validate' in request.GET and request.GET['validate'] == 'true': if self.allow_standalone_validation: params = QueryDict(request.body) raise BinderSkipSave else: - print('validate not enabled') raise BinderRequestError('Standalone validation not enabled. You must enable this feature explicitly.') @@ -2147,9 +2146,6 @@ def put(self, request, pk=None): if hasattr(obj, 'deleted') and obj.deleted: raise BinderIsDeleted() - - logger.info('storing') - data = self._store(obj, values, request) self._abort_when_standalone_validation(request) From 32996a4d637bf94a09dda3ee0d1d0c0f4e56c821 Mon Sep 17 00:00:00 2001 From: robin Date: Thu, 22 Jul 2021 11:34:28 +0200 Subject: [PATCH 10/34] Test _validation_model field for clean --- binder/models.py | 8 ++++++-- binder/views.py | 22 ++++++++++++++++------ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/binder/models.py b/binder/models.py index ba6665b4..007c869c 100644 --- a/binder/models.py +++ b/binder/models.py @@ -403,8 +403,12 @@ class Meta: abstract = True ordering = ['pk'] - def save(self, *args, **kwargs): - self.full_clean() # Never allow saving invalid models! + def save(self, *args, only_validate=False, **kwargs): + # A validation model might not require all validation checks as it is not a full model + # _validation_model can be used to skip validation checks that are meant for complete models that are actually being saved + self._validation_model = only_validate # Set the model as a validation model when we only want to validate the model + + self.full_clean(only_validate=only_validate) # Never allow saving invalid models! return super().save(*args, **kwargs) diff --git a/binder/views.py b/binder/views.py index 8d486cb8..aec0f62f 100644 --- a/binder/views.py +++ b/binder/views.py @@ -1397,6 +1397,9 @@ def _store(self, obj, values, request, ignore_unknown_fields=False, pk=None): ignored_fields = [] validation_errors = [] + # When only validating and not saving we attach a parameter so that we can skip or add validation checks + only_validate = request.GET.get('validate') == 'true' or request.GET.get('validate') == 'True' + if obj.pk is None: self._require_model_perm('add', request, obj.pk) else: @@ -1434,8 +1437,8 @@ def store_m2m_field(obj, field, value, request): raise sum(validation_errors, None) try: - obj.save() - assert(obj.pk is not None) # At this point, the object must have been created. + obj.save(only_validate=only_validate) + assert(obj.pk is not None) # At this point, the object must have been created. except ValidationError as ve: validation_errors.append(self.binder_validation_error(obj, ve, pk=pk)) @@ -1477,6 +1480,9 @@ def store_m2m_field(obj, field, value, request): def _store_m2m_field(self, obj, field, value, request): validation_errors = [] + # When only validating and not saving we attach a parameter so that we can skip or add validation checks + only_validate = request.GET.get('validate') == 'true' or request.GET.get('validate') == 'True' + # Can't use isinstance() because apparantly ManyToManyDescriptor is a subclass of # ReverseManyToOneDescriptor. Yes, really. if getattr(obj._meta.model, field).__class__ == models.fields.related.ReverseManyToOneDescriptor: @@ -1519,11 +1525,11 @@ 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: - addobj.save() + addobj.save(only_validate=only_validate) except ValidationError as ve: validation_errors.append(self.binder_validation_error(addobj, ve)) else: - addobj.save() + addobj.save(only_validate=only_validate) elif getattr(obj._meta.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor: #### XXX FIXME XXX ugly quick fix for reverse relation + multiput issue if any(v for v in value if v is not None and v < 0): @@ -1539,7 +1545,7 @@ def _store_m2m_field(self, obj, field, value, request): remote_obj = field_descriptor.related.remote_field.model.objects.get(pk=value[0]) setattr(remote_obj, field_descriptor.related.remote_field.name, obj) try: - remote_obj.save() + remote_obj.save(only_validate=only_validate) remote_obj.refresh_from_db() except ValidationError as ve: validation_errors.append(self.binder_validation_error(remote_obj, ve)) @@ -2234,6 +2240,10 @@ def delete_obj(self, obj, undelete, request): def soft_delete(self, obj, undelete, request): + + # When only validating and not saving we attach a parameter so that we can skip or add validation checks + only_validate = request.GET.get('validate') == 'true' or request.GET.get('validate') == 'True' + # Not only for soft delets, actually handles all deletions try: if obj.deleted and not undelete: @@ -2265,7 +2275,7 @@ def soft_delete(self, obj, undelete, request): obj.deleted = not undelete try: - obj.save() + obj.save(only_validate=only_validate) except ValidationError as ve: raise self.binder_validation_error(obj, ve) From be297adf850b64a9df85faef99fa477994cc8f84 Mon Sep 17 00:00:00 2001 From: robin Date: Thu, 22 Jul 2021 11:54:23 +0200 Subject: [PATCH 11/34] Remove incorrect argument from full_clean --- binder/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/models.py b/binder/models.py index 007c869c..85a9b33a 100644 --- a/binder/models.py +++ b/binder/models.py @@ -408,7 +408,7 @@ def save(self, *args, only_validate=False, **kwargs): # _validation_model can be used to skip validation checks that are meant for complete models that are actually being saved self._validation_model = only_validate # Set the model as a validation model when we only want to validate the model - self.full_clean(only_validate=only_validate) # Never allow saving invalid models! + self.full_clean() # Never allow saving invalid models! return super().save(*args, **kwargs) From 32b6e2d758005af2436f39e71ac0ab085ac861b9 Mon Sep 17 00:00:00 2001 From: robin Date: Thu, 22 Jul 2021 14:46:37 +0200 Subject: [PATCH 12/34] Add tests --- tests/test_model_validation.py | 45 +++++++++++++++++++++++++- tests/testapp/models/contact_person.py | 7 ++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/test_model_validation.py b/tests/test_model_validation.py index 59dac762..b05dba5a 100644 --- a/tests/test_model_validation.py +++ b/tests/test_model_validation.py @@ -35,7 +35,10 @@ def setUp(self): # some update payload self.model_data_with_error = { - 'name': 'very_special_forbidden_contact_person_name', # see `contact_person.py` + 'name': 'very_special_forbidden_contact_person_name', # see `contact_person.py` + } + self.model_data_with_non_validation_error = { + 'name': 'very_special_validation_contact_person_name', # see `contact_person.py` } self.model_data = { 'name': 'Scooooooby', @@ -127,6 +130,21 @@ def test_validate_on_put(self): self.assertEqual(response.status_code, 200) self.assertEqual('Scooooooby', ContactPerson.objects.first().name) + def test_validate_model_validation_whitelisting(self): + person_id = ContactPerson.objects.create(name='Scooby Doo').id + self.assertEqual('Scooby Doo', ContactPerson.objects.first().name) + + # the normal request should give a validation error + response = self.client.put(f'/contact_person/{person_id}/', data=json.dumps(self.model_data_with_non_validation_error), content_type='application/json') + self.assert_validation_error(response, person_id) + self.assertEqual('Scooby Doo', ContactPerson.objects.first().name) + + # when just validating we want to ignore this validation error, so with validation it should not throw an error + response = self.client.put(f'/contact_person/{person_id}/?validate=true', data=json.dumps(self.model_data), content_type='application/json') + self.assert_no_validation_error(response) + self.assertEqual('Scooby Doo', ContactPerson.objects.first().name) + + def test_validate_on_multiput(self): person_1_id = ContactPerson.objects.create(name='Scooby Doo 1').id @@ -154,6 +172,17 @@ def test_validate_on_multiput(self): } ]} + multi_put_data_with_validation_whitelist = {'data': [ + { + 'id': person_1_id, + 'name': 'very_special_validation_contact_person_name', + }, + { + 'id': person_2_id, + 'name': 'very_special_validation_contact_person_other_name' + } + ]} + # trigger a validation error response = self.client.put(f'/contact_person/?validate=true', data=json.dumps(multi_put_data_with_error), content_type='application/json') self.assert_multi_put_validation_error(response) @@ -167,6 +196,20 @@ def test_validate_on_multiput(self): self.assertEqual('Scooby Doo 1', ContactPerson.objects.get(id=person_1_id).name) self.assertEqual('Scooby Doo 2', ContactPerson.objects.get(id=person_2_id).name) + # multi put validation whitelist test + response = self.client.put(f'/contact_person/?validate=true', data=json.dumps(multi_put_data_with_validation_whitelist), content_type='application/json') + self.assert_no_validation_error(response) + self.assertEqual('Scooby Doo 1', ContactPerson.objects.get(id=person_1_id).name) + self.assertEqual('Scooby Doo 2', ContactPerson.objects.get(id=person_2_id).name) + + # multi put non validation whitelist test error + response = self.client.put(f'/contact_person/', + data=json.dumps(multi_put_data_with_validation_whitelist), + content_type='application/json') + self.assert_multi_put_validation_error(response) + self.assertEqual('Scooby Doo 1', ContactPerson.objects.get(id=person_1_id).name) + self.assertEqual('Scooby Doo 2', ContactPerson.objects.get(id=person_2_id).name) + # now for real response = self.client.put(f'/contact_person/', data=json.dumps(multi_put_data), content_type='application/json') self.assertEqual(response.status_code, 200) diff --git a/tests/testapp/models/contact_person.py b/tests/testapp/models/contact_person.py index 60db9897..d810c745 100644 --- a/tests/testapp/models/contact_person.py +++ b/tests/testapp/models/contact_person.py @@ -16,3 +16,10 @@ def clean(self): code='invalid', message='Very special validation check that we need in `tests.M2MStoreErrorsTest`.' ) + + # Should only give an error when model is not a validation model + if (self.name == 'very_special_validation_contact_person_name' or self.name == 'very_special_validation_contact_person_other_name') and not self._validation_model: + raise ValidationError( + code='invalid', + message='Very special validation check that we need in `tests.M2MStoreErrorsTest`.' + ) From d14787fa6515a1392636846be69ea863db5e4b17 Mon Sep 17 00:00:00 2001 From: stefanmajoor Date: Fri, 23 Jul 2021 10:18:07 +0200 Subject: [PATCH 13/34] do not write to a new worksheet. Take the default worksheet instead --- binder/plugins/views/csvexport.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/plugins/views/csvexport.py b/binder/plugins/views/csvexport.py index 8c5efd88..7485ae58 100644 --- a/binder/plugins/views/csvexport.py +++ b/binder/plugins/views/csvexport.py @@ -104,7 +104,7 @@ def __init__(self, request: HttpRequest, csv_settings: 'CsvExportView.CsvExportS # self.writer = self.pandas.ExcelWriter(self.response) self.work_book = self.openpyxl.Workbook() - self.sheet = self.work_book.create_sheet() + self.sheet = self.work_book._sheets[0] # The row number we are currently writing to self._row_number = 0 From 70622fd975c5b674a9bd7aa88a31c1bc74a2949c Mon Sep 17 00:00:00 2001 From: robin Date: Fri, 23 Jul 2021 10:49:51 +0200 Subject: [PATCH 14/34] update doc --- docs/api.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/api.md b/docs/api.md index 51ba97be..3a20d850 100644 --- a/docs/api.md +++ b/docs/api.md @@ -170,6 +170,8 @@ Currently this is implemented by raising an `BinderValidateOnly` exception, whic It is important to realize that in this way, the normal `save()` function is called on a model, so it is possible that possible side effects are triggered, when these are implemented directly in `save()`, as opposed to in a signal method, which would be preferable. In other words, we cannot guarantee that the request will be idempotent. Therefore, the validation only feature is disabled by default and must be enabled by setting `allow_standalone_validation=True` on the view. +When a model is being validated and not actually being saved the `_validation_model property` of the binder model is set to True. This allows whitelisting of certain validation checks such as with certain relations that are not included with the validation model. + ### Uploading files To upload a file, you have to add it to the `file_fields` of the `ModelView`: From d673457526d29a542f0798c9c06ef9708388ec85 Mon Sep 17 00:00:00 2001 From: stefanmajoor Date: Mon, 26 Jul 2021 09:44:46 +0200 Subject: [PATCH 15/34] Add first attempt at getting mssql support working --- .github/workflows/ci.yml | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9d2f4423..2a251e69 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,9 +8,9 @@ jobs: strategy: matrix: - python-version: ["3.7", "3.8"] - django-version: ["2.1.1", "3.1.4"] - database-engine: ["postgres", "mysql"] + python-version: [ "3.7", "3.8" ] + django-version: [ "2.1.1", "3.1.4" ] + database-engine: [ "postgres", "mysql", "mssql"] services: postgres: @@ -37,6 +37,14 @@ jobs: ports: - 3306:3306 + + mssqldb: + image: mcr.microsoft.com/mssql/server:2017-latest + env: + ACCEPT_EULA: y + SA_PASSWORD: Test + + steps: - name: Checkout code uses: actions/checkout@v2 From cfa6fa60256dce1b31a470360f980640998e8d20 Mon Sep 17 00:00:00 2001 From: stefanmajoor Date: Fri, 17 Sep 2021 13:42:55 +0200 Subject: [PATCH 16/34] add support fo nullable foreign keys in CSVexport plugin --- binder/plugins/views/csvexport.py | 7 ++++++- tests/plugins/test_csvexport.py | 30 +++++++++++++++++++++++++++--- tests/testapp/models/__init__.py | 2 +- tests/testapp/models/picture.py | 13 +++++++++++++ tests/testapp/views/__init__.py | 2 +- tests/testapp/views/picture.py | 7 +++++-- 6 files changed, 53 insertions(+), 8 deletions(-) diff --git a/binder/plugins/views/csvexport.py b/binder/plugins/views/csvexport.py index 6e66d2ff..294bd369 100644 --- a/binder/plugins/views/csvexport.py +++ b/binder/plugins/views/csvexport.py @@ -284,7 +284,12 @@ def get_datum(data, key, prefix=''): else: # Assume that we have a mapping now fk_ids = data[head_key] - if type(fk_ids) != list: + + if fk_ids is None: + # This case happens if we have a nullable foreign key that is null. Treat this as a many + # to one relation with no values. + fk_ids = [] + elif type(fk_ids) != list: fk_ids = [fk_ids] # if head_key not in key_mapping: diff --git a/tests/plugins/test_csvexport.py b/tests/plugins/test_csvexport.py index 52c0b406..d2d4d010 100644 --- a/tests/plugins/test_csvexport.py +++ b/tests/plugins/test_csvexport.py @@ -7,7 +7,7 @@ from django.core.files import File from django.contrib.auth.models import User -from ..testapp.models import Picture, Animal +from ..testapp.models import Picture, Animal, PictureBook import csv import openpyxl @@ -31,8 +31,10 @@ def setUp(self): self.pictures = [] + picture_book = PictureBook.objects.create(name='Holiday 2012') + for i in range(3): - picture = Picture(animal=animal) + picture = Picture(animal=animal, picture_book=picture_book) file = CsvExportTest.temp_imagefile(50, 50, 'jpeg') picture.file.save('picture.jpg', File(file), save=False) picture.original_file.save('picture_copy.jpg', File(file), save=False) @@ -48,6 +50,7 @@ def setUp(self): def test_csv_download(self): response = self.client.get('/picture/download_csv/') + print(response.content) self.assertEqual(200, response.status_code) response_data = csv.reader(io.StringIO(response.content.decode("utf-8"))) @@ -123,4 +126,25 @@ def test_context_aware_download_xlsx(self): self.assertEqual(list(_values[2]), [self.pictures[1].id, str(self.pictures[1].animal_id), (self.pictures[1].id ** 2)]) self.assertEqual(list(_values[3]), - [self.pictures[2].id, str(self.pictures[2].animal_id), (self.pictures[2].id ** 2)]) \ No newline at end of file + [self.pictures[2].id, str(self.pictures[2].animal_id), (self.pictures[2].id ** 2)]) + + def test_none_foreign_key(self): + """ + If we have a relation that we have to include which is nullable, and we have a foreign key, we do not want the + csv export to crash. I.e. we also want for a picture to export the picture book name, even though not all pioctures + belong to a picture book + :return: + """ + self.pictures[0].picture_book = None + self.pictures[0].save() + + response = self.client.get('/picture/download/') + self.assertEqual(200, response.status_code) + response_data = csv.reader(io.StringIO(response.content.decode("utf-8"))) + + data = list(response_data) + content = data[1:] + + picture_books = [c[-1] for c in content] + + self.assertEqual(['', 'Holiday 2012', 'Holiday 2012'], picture_books) \ No newline at end of file diff --git a/tests/testapp/models/__init__.py b/tests/testapp/models/__init__.py index ed41bbe5..7ccddcf7 100644 --- a/tests/testapp/models/__init__.py +++ b/tests/testapp/models/__init__.py @@ -9,7 +9,7 @@ from .gate import Gate from .nickname import Nickname, NullableNickname from .lion import Lion -from .picture import Picture +from .picture import Picture, PictureBook from .zoo import Zoo from .zoo_employee import ZooEmployee from .city import City, CityState, PermanentCity diff --git a/tests/testapp/models/picture.py b/tests/testapp/models/picture.py index ffbe59bb..862bfb3e 100644 --- a/tests/testapp/models/picture.py +++ b/tests/testapp/models/picture.py @@ -13,6 +13,18 @@ def delete_files(sender, instance=None, **kwargs): except Exception: pass + +class PictureBook(BinderModel): + """ + Sometimes customers like to commemorate their visit to the zoo. Of course there are always some shitty pictures that + we do not want in a picture album + + """ + + name = models.TextField() + + + # At the website of the zoo there are some pictures of animals. This model links the picture to an animal. # # A picture has two files, the original uploaded file, and the modified file. This model is used for testing the @@ -21,6 +33,7 @@ class Picture(BinderModel): animal = models.ForeignKey('Animal', on_delete=models.CASCADE, related_name='picture') file = models.ImageField(upload_to='floor-plans') original_file = models.ImageField(upload_to='floor-plans') + picture_book = models.ForeignKey('PictureBook', on_delete=models.CASCADE, null=True, blank=True) def __str__(self): return 'picture %d: (Picture for animal %s)' % (self.pk or 0, self.animal.name) diff --git a/tests/testapp/views/__init__.py b/tests/testapp/views/__init__.py index 7dd1f4f0..84989ceb 100644 --- a/tests/testapp/views/__init__.py +++ b/tests/testapp/views/__init__.py @@ -15,7 +15,7 @@ from .gate import GateView from .lion import LionView from .nickname import NicknameView -from .picture import PictureView +from .picture import PictureView, PictureBookView from .user import UserView from .zoo import ZooView from .zoo_employee import ZooEmployeeView diff --git a/tests/testapp/views/picture.py b/tests/testapp/views/picture.py index 7dadfe99..0ba35f99 100644 --- a/tests/testapp/views/picture.py +++ b/tests/testapp/views/picture.py @@ -3,16 +3,19 @@ from binder.views import ModelView from binder.plugins.views import ImageView, CsvExportView -from ..models import Picture +from ..models import Picture, PictureBook +class PictureBookView(ModelView): + model = PictureBook class PictureView(ModelView, ImageView, CsvExportView): model = Picture file_fields = ['file', 'original_file'] - csv_settings = CsvExportView.CsvExportSettings(['animal'], [ + csv_settings = CsvExportView.CsvExportSettings(['animal', 'picture_book'], [ ('id', 'picture identifier'), ('animal.id', 'animal identifier'), ('id', 'squared picture identifier', lambda id, row, mapping: id**2), + ('picture_book.name', 'Picturebook name') ]) @list_route(name='download_csv', methods=['GET']) From 3c8b239d1150e73411914e1a08994594b310cc7e Mon Sep 17 00:00:00 2001 From: Daan van der Kallen Date: Tue, 18 Jan 2022 23:30:21 +0100 Subject: [PATCH 17/34] Do 1 query per with instead of a big combined one to prevent exponential join scaling --- binder/views.py | 49 +++++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/binder/views.py b/binder/views.py index 59952a74..86495198 100644 --- a/binder/views.py +++ b/binder/views.py @@ -907,7 +907,6 @@ def _follow_related(self, fieldspec): def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): result = {} - annotations = {} singular_fields = set() rel_ids_by_field_by_id = defaultdict(lambda: defaultdict(list)) virtual_fields = set() @@ -947,7 +946,7 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): # This does mean you can't filter on this relation # unless you write a custom filter, too. if isinstance(virtual_annotation, Q): - annotations[field_alias] = Agg(virtual_annotation, filter=q, ordering=orders) + annotation = Agg(virtual_annotation, filter=q, ordering=orders) else: try: func = getattr(self, virtual_annotation) @@ -956,6 +955,7 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): self.model.__name__, field, virtual_annotation )) rel_ids_by_field_by_id[field] = func(request, pks, q) + continue # Actual relation else: if (getattr(self.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor or @@ -964,34 +964,27 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): if Agg != GroupConcat: # HACKK (GROUP_CONCAT can't filter and excludes NULL already) q &= Q(**{field+'__pk__isnull': False}) - annotations[field_alias] = Agg(field+'__pk', filter=q, ordering=orders) - - - qs = self.model.objects.filter(pk__in=pks).values('pk').annotate(**annotations) - for record in qs: - for field in with_map: - field_alias = field+'___annotation' if field in virtual_fields else field - - if field_alias in annotations: - value = record[field_alias] - - # Make the values distinct. We can't do this in - # the Agg() call, because then we get an error - # regarding order by and values needing to be the - # same :( - # We also can't just put it in a set, because we - # need to preserve the ordering. So we use a set - # to keep track of what we've seen and only add - # new items. - seen_values = set() - distinct_values = [] - for v in value: - if v not in seen_values: - distinct_values.append(v) + annotation = Agg(field+'__pk', filter=q, ordering=orders) + + qs = self.model.objects.filter(pk__in=pks).values('pk').annotate(**{field_alias: annotation}) + + for record in qs: + value = record[field_alias] + + # Make the values distinct. We can't do this in + # the Agg() call, because then we get an error + # regarding order by and values needing to be the + # same :( + # We also can't just put it in a set, because we + # need to preserve the ordering. So we use a set + # to keep track of what we've seen and only add + # new items. + seen_values = set() + for v in value: + if v not in seen_values: + rel_ids_by_field_by_id[field][record['pk']].append(v) seen_values.add(v) - rel_ids_by_field_by_id[field][record['pk']] += distinct_values - for field, sub_fields in with_map.items(): next = self._follow_related(field)[0].model From 292d7397209f839327e5fc6701c80935dfa14cc4 Mon Sep 17 00:00:00 2001 From: Daan van der Kallen Date: Wed, 19 Jan 2022 00:00:23 +0100 Subject: [PATCH 18/34] Do not use aggregates at all --- binder/views.py | 63 +++++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 41 deletions(-) diff --git a/binder/views.py b/binder/views.py index 86495198..0c3ce18a 100644 --- a/binder/views.py +++ b/binder/views.py @@ -911,8 +911,6 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): rel_ids_by_field_by_id = defaultdict(lambda: defaultdict(list)) virtual_fields = set() - Agg = self.AggStrategy - for field in with_map: vr = self.virtual_relations.get(field, None) @@ -924,12 +922,6 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): if rel == field or rel.startswith(field + '.') }) - # Model default orders (this sometimes matters) - orders = [] - field_alias = field + '___annotation' if vr else field - for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): - orders.append(prefix_db_expression(o, field_alias)) - # Virtual relation if vr: virtual_fields.add(field) @@ -945,45 +937,34 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): # annotations, so allow for fetching of ids, instead. # This does mean you can't filter on this relation # unless you write a custom filter, too. - if isinstance(virtual_annotation, Q): - annotation = Agg(virtual_annotation, filter=q, ordering=orders) - else: - try: - func = getattr(self, virtual_annotation) - except AttributeError: - raise BinderRequestError('Annotation for virtual relation {{{}}}.{{{}}} is {{{}}}, but no method by that name exists.'.format( - self.model.__name__, field, virtual_annotation - )) - rel_ids_by_field_by_id[field] = func(request, pks, q) - continue + try: + func = getattr(self, virtual_annotation) + except AttributeError: + raise BinderRequestError('Annotation for virtual relation {{{}}}.{{{}}} is {{{}}}, but no method by that name exists.'.format( + self.model.__name__, field, virtual_annotation + )) + rel_ids_by_field_by_id[field] = func(request, pks, q) # Actual relation else: if (getattr(self.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor or not any(f.name == field for f in (list(self.model._meta.many_to_many) + list(self._get_reverse_relations())))): singular_fields.add(field) - if Agg != GroupConcat: # HACKK (GROUP_CONCAT can't filter and excludes NULL already) - q &= Q(**{field+'__pk__isnull': False}) - annotation = Agg(field+'__pk', filter=q, ordering=orders) - - qs = self.model.objects.filter(pk__in=pks).values('pk').annotate(**{field_alias: annotation}) - - for record in qs: - value = record[field_alias] - - # Make the values distinct. We can't do this in - # the Agg() call, because then we get an error - # regarding order by and values needing to be the - # same :( - # We also can't just put it in a set, because we - # need to preserve the ordering. So we use a set - # to keep track of what we've seen and only add - # new items. - seen_values = set() - for v in value: - if v not in seen_values: - rel_ids_by_field_by_id[field][record['pk']].append(v) - seen_values.add(v) + # Model default orders (this sometimes matters) + orders = [] + for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): + orders.append(prefix_db_expression(o, field)) + + query = ( + self.model.objects + .order_by(*orders) + .filter(q, pk__in=pks, **{field + '__isnull': False}) + .values_list('pk', field + '__pk') + .distinct() + ) + + for pk, rel_pk in query: + rel_ids_by_field_by_id[field][pk].append(rel_pk) for field, sub_fields in with_map.items(): next = self._follow_related(field)[0].model From 528d776d950796d1bd681a006098887b57d279bc Mon Sep 17 00:00:00 2001 From: Daan van der Kallen Date: Wed, 19 Jan 2022 01:37:51 +0100 Subject: [PATCH 19/34] Do not use join for reverse relations --- binder/views.py | 52 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/binder/views.py b/binder/views.py index 0c3ce18a..2e1d2dc9 100644 --- a/binder/views.py +++ b/binder/views.py @@ -253,6 +253,21 @@ def prefix_db_expression(value, prefix): raise ValueError('Unknown expression type, cannot apply db prefix: %s', value) +# Prefix a q expression by adding a prefix to all filters. You can also supply +# an 'antiprefix', if the filter starts with this value it will be removed +# instead of the prefix being added. This is useful for reversed fields. +def prefix_q_expression(value, prefix, antiprefix=None): + children = [] + for child in value.children: + if isinstance(child, Q): + children.append(prefix_q_expression(child, prefix, antiprefix)) + elif antiprefix is not None and child[0].startswith(antiprefix + '__'): + children.append((child[0][len(antiprefix) + 2:], child[1])) + else: + children.append((prefix + '__' + child[0], child[1])) + return Q(*children, _negated=value.negated) + + class ModelView(View): # Model this is a view for. Use None for views not tied to a particular model. model = None @@ -946,22 +961,31 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): rel_ids_by_field_by_id[field] = func(request, pks, q) # Actual relation else: - if (getattr(self.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor or - not any(f.name == field for f in (list(self.model._meta.many_to_many) + list(self._get_reverse_relations())))): + f = self.model._meta.get_field(field) + + if f.one_to_one or f.many_to_one: singular_fields.add(field) - # Model default orders (this sometimes matters) - orders = [] - for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): - orders.append(prefix_db_expression(o, field)) - - query = ( - self.model.objects - .order_by(*orders) - .filter(q, pk__in=pks, **{field + '__isnull': False}) - .values_list('pk', field + '__pk') - .distinct() - ) + if any(f.name == field for f in self._get_reverse_relations()): + rev_field = f.remote_field.name + query = ( + view.model.objects + .filter(prefix_q_expression(q, rev_field, field), **{rev_field + '__in': pks}) + .values_list(rev_field + '__pk', 'pk') + .distinct() + ) + else: + # Model default orders (this sometimes matters) + orders = [] + for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): + orders.append(prefix_db_expression(o, field)) + query = ( + self.model.objects + .order_by(*orders) + .filter(q, pk__in=pks, **{field + '__isnull': False}) + .values_list('pk', field + '__pk') + .distinct() + ) for pk, rel_pk in query: rel_ids_by_field_by_id[field][pk].append(rel_pk) From 5cfc8dc88dd452dcd75a8f80df18b63396c960c6 Mon Sep 17 00:00:00 2001 From: Daan van der Kallen Date: Tue, 18 Jan 2022 23:30:21 +0100 Subject: [PATCH 20/34] Do 1 query per with instead of a big combined one to prevent exponential join scaling --- binder/views.py | 49 +++++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/binder/views.py b/binder/views.py index 80289aef..4e35a538 100644 --- a/binder/views.py +++ b/binder/views.py @@ -918,7 +918,6 @@ def _follow_related(self, fieldspec): def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): result = {} - annotations = {} singular_fields = set() rel_ids_by_field_by_id = defaultdict(lambda: defaultdict(list)) virtual_fields = set() @@ -958,7 +957,7 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): # This does mean you can't filter on this relation # unless you write a custom filter, too. if isinstance(virtual_annotation, Q): - annotations[field_alias] = Agg(virtual_annotation, filter=q, ordering=orders) + annotation = Agg(virtual_annotation, filter=q, ordering=orders) else: try: func = getattr(self, virtual_annotation) @@ -967,6 +966,7 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): self.model.__name__, field, virtual_annotation )) rel_ids_by_field_by_id[field] = func(request, pks, q) + continue # Actual relation else: if (getattr(self.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor or @@ -975,34 +975,27 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): if Agg != GroupConcat: # HACKK (GROUP_CONCAT can't filter and excludes NULL already) q &= Q(**{field+'__pk__isnull': False}) - annotations[field_alias] = Agg(field+'__pk', filter=q, ordering=orders) - - - qs = self.model.objects.filter(pk__in=pks).values('pk').annotate(**annotations) - for record in qs: - for field in with_map: - field_alias = field+'___annotation' if field in virtual_fields else field - - if field_alias in annotations: - value = record[field_alias] - - # Make the values distinct. We can't do this in - # the Agg() call, because then we get an error - # regarding order by and values needing to be the - # same :( - # We also can't just put it in a set, because we - # need to preserve the ordering. So we use a set - # to keep track of what we've seen and only add - # new items. - seen_values = set() - distinct_values = [] - for v in value: - if v not in seen_values: - distinct_values.append(v) + annotation = Agg(field+'__pk', filter=q, ordering=orders) + + qs = self.model.objects.filter(pk__in=pks).values('pk').annotate(**{field_alias: annotation}) + + for record in qs: + value = record[field_alias] + + # Make the values distinct. We can't do this in + # the Agg() call, because then we get an error + # regarding order by and values needing to be the + # same :( + # We also can't just put it in a set, because we + # need to preserve the ordering. So we use a set + # to keep track of what we've seen and only add + # new items. + seen_values = set() + for v in value: + if v not in seen_values: + rel_ids_by_field_by_id[field][record['pk']].append(v) seen_values.add(v) - rel_ids_by_field_by_id[field][record['pk']] += distinct_values - for field, sub_fields in with_map.items(): next = self._follow_related(field)[0].model From aae913f43d0ac1588bf95db0ae27845af700ec31 Mon Sep 17 00:00:00 2001 From: Daan van der Kallen Date: Wed, 19 Jan 2022 00:00:23 +0100 Subject: [PATCH 21/34] Do not use aggregates at all --- binder/views.py | 63 +++++++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 41 deletions(-) diff --git a/binder/views.py b/binder/views.py index 4e35a538..63850784 100644 --- a/binder/views.py +++ b/binder/views.py @@ -922,8 +922,6 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): rel_ids_by_field_by_id = defaultdict(lambda: defaultdict(list)) virtual_fields = set() - Agg = self.AggStrategy - for field in with_map: vr = self.virtual_relations.get(field, None) @@ -935,12 +933,6 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): if rel == field or rel.startswith(field + '.') }) - # Model default orders (this sometimes matters) - orders = [] - field_alias = field + '___annotation' if vr else field - for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): - orders.append(prefix_db_expression(o, field_alias)) - # Virtual relation if vr: virtual_fields.add(field) @@ -956,45 +948,34 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): # annotations, so allow for fetching of ids, instead. # This does mean you can't filter on this relation # unless you write a custom filter, too. - if isinstance(virtual_annotation, Q): - annotation = Agg(virtual_annotation, filter=q, ordering=orders) - else: - try: - func = getattr(self, virtual_annotation) - except AttributeError: - raise BinderRequestError('Annotation for virtual relation {{{}}}.{{{}}} is {{{}}}, but no method by that name exists.'.format( - self.model.__name__, field, virtual_annotation - )) - rel_ids_by_field_by_id[field] = func(request, pks, q) - continue + try: + func = getattr(self, virtual_annotation) + except AttributeError: + raise BinderRequestError('Annotation for virtual relation {{{}}}.{{{}}} is {{{}}}, but no method by that name exists.'.format( + self.model.__name__, field, virtual_annotation + )) + rel_ids_by_field_by_id[field] = func(request, pks, q) # Actual relation else: if (getattr(self.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor or not any(f.name == field for f in (list(self.model._meta.many_to_many) + list(self._get_reverse_relations())))): singular_fields.add(field) - if Agg != GroupConcat: # HACKK (GROUP_CONCAT can't filter and excludes NULL already) - q &= Q(**{field+'__pk__isnull': False}) - annotation = Agg(field+'__pk', filter=q, ordering=orders) - - qs = self.model.objects.filter(pk__in=pks).values('pk').annotate(**{field_alias: annotation}) - - for record in qs: - value = record[field_alias] - - # Make the values distinct. We can't do this in - # the Agg() call, because then we get an error - # regarding order by and values needing to be the - # same :( - # We also can't just put it in a set, because we - # need to preserve the ordering. So we use a set - # to keep track of what we've seen and only add - # new items. - seen_values = set() - for v in value: - if v not in seen_values: - rel_ids_by_field_by_id[field][record['pk']].append(v) - seen_values.add(v) + # Model default orders (this sometimes matters) + orders = [] + for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): + orders.append(prefix_db_expression(o, field)) + + query = ( + self.model.objects + .order_by(*orders) + .filter(q, pk__in=pks, **{field + '__isnull': False}) + .values_list('pk', field + '__pk') + .distinct() + ) + + for pk, rel_pk in query: + rel_ids_by_field_by_id[field][pk].append(rel_pk) for field, sub_fields in with_map.items(): next = self._follow_related(field)[0].model From 034a13c902174f3ffd0a60a31fac1064534722a6 Mon Sep 17 00:00:00 2001 From: Daan van der Kallen Date: Wed, 19 Jan 2022 01:37:51 +0100 Subject: [PATCH 22/34] Do not use join for reverse relations --- binder/views.py | 52 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/binder/views.py b/binder/views.py index 63850784..e85f863a 100644 --- a/binder/views.py +++ b/binder/views.py @@ -257,6 +257,21 @@ def prefix_db_expression(value, prefix): raise ValueError('Unknown expression type, cannot apply db prefix: %s', value) +# Prefix a q expression by adding a prefix to all filters. You can also supply +# an 'antiprefix', if the filter starts with this value it will be removed +# instead of the prefix being added. This is useful for reversed fields. +def prefix_q_expression(value, prefix, antiprefix=None): + children = [] + for child in value.children: + if isinstance(child, Q): + children.append(prefix_q_expression(child, prefix, antiprefix)) + elif antiprefix is not None and child[0].startswith(antiprefix + '__'): + children.append((child[0][len(antiprefix) + 2:], child[1])) + else: + children.append((prefix + '__' + child[0], child[1])) + return Q(*children, _negated=value.negated) + + class ModelView(View): # Model this is a view for. Use None for views not tied to a particular model. model = None @@ -957,22 +972,31 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): rel_ids_by_field_by_id[field] = func(request, pks, q) # Actual relation else: - if (getattr(self.model, field).__class__ == models.fields.related.ReverseOneToOneDescriptor or - not any(f.name == field for f in (list(self.model._meta.many_to_many) + list(self._get_reverse_relations())))): + f = self.model._meta.get_field(field) + + if f.one_to_one or f.many_to_one: singular_fields.add(field) - # Model default orders (this sometimes matters) - orders = [] - for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): - orders.append(prefix_db_expression(o, field)) - - query = ( - self.model.objects - .order_by(*orders) - .filter(q, pk__in=pks, **{field + '__isnull': False}) - .values_list('pk', field + '__pk') - .distinct() - ) + if any(f.name == field for f in self._get_reverse_relations()): + rev_field = f.remote_field.name + query = ( + view.model.objects + .filter(prefix_q_expression(q, rev_field, field), **{rev_field + '__in': pks}) + .values_list(rev_field + '__pk', 'pk') + .distinct() + ) + else: + # Model default orders (this sometimes matters) + orders = [] + for o in (view.model._meta.ordering if view.model._meta.ordering else BinderModel.Meta.ordering): + orders.append(prefix_db_expression(o, field)) + query = ( + self.model.objects + .order_by(*orders) + .filter(q, pk__in=pks, **{field + '__isnull': False}) + .values_list('pk', field + '__pk') + .distinct() + ) for pk, rel_pk in query: rel_ids_by_field_by_id[field][pk].append(rel_pk) From 4db24f8c8f9a4b3beaac20f25e042468957a3b1c Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Fri, 18 Feb 2022 11:01:44 +0100 Subject: [PATCH 23/34] Add webpage model --- binder/plugins/models/__init__.py | 1 + binder/plugins/models/html_field.py | 11 +++++++++++ tests/test_html_field.py | 26 ++++++++++++++++++++++++++ tests/testapp/models/__init__.py | 2 ++ tests/testapp/models/web_page.py | 13 +++++++++++++ 5 files changed, 53 insertions(+) create mode 100644 binder/plugins/models/__init__.py create mode 100644 binder/plugins/models/html_field.py create mode 100644 tests/test_html_field.py create mode 100644 tests/testapp/models/web_page.py diff --git a/binder/plugins/models/__init__.py b/binder/plugins/models/__init__.py new file mode 100644 index 00000000..5ad29194 --- /dev/null +++ b/binder/plugins/models/__init__.py @@ -0,0 +1 @@ +from .html_field import HtmlField diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py new file mode 100644 index 00000000..2ede5568 --- /dev/null +++ b/binder/plugins/models/html_field.py @@ -0,0 +1,11 @@ +from django.db.models import TextField + + +class HtmlField(TextField): + """ + Determine a safe way to save "secure" user provided HTML input, and prevent + """ + + + def validate(self, value, _): + pass diff --git a/tests/test_html_field.py b/tests/test_html_field.py new file mode 100644 index 00000000..8caa40be --- /dev/null +++ b/tests/test_html_field.py @@ -0,0 +1,26 @@ +from django.contrib.auth.models import User +from django.test import TestCase, Client + +from project.testapp.models import Zoo, WebPage + + +class AnnotationTestCase(TestCase): + + def setUp(self): + super().setUp() + u = User(username='testuser', is_active=True, is_superuser=True) + u.set_password('test') + u.save() + self.client = Client() + r = self.client.login(username='testuser', password='test') + self.assertTrue(r) + + self.zoo = Zoo(name='Apenheul') + self.zoo.save() + + self.webpage = WebPage(zoo=self.zoo, content='') + + + def test_save_normal_text_ok(self): + self.webpage = WebPage(zoo=self.zoo, content='Artis') + diff --git a/tests/testapp/models/__init__.py b/tests/testapp/models/__init__.py index ed41bbe5..48e0e08f 100644 --- a/tests/testapp/models/__init__.py +++ b/tests/testapp/models/__init__.py @@ -14,6 +14,8 @@ from .zoo_employee import ZooEmployee from .city import City, CityState, PermanentCity from .country import Country +from .web_page import WebPage + # This is Postgres-specific if os.environ.get('BINDER_TEST_MYSQL', '0') != '1': from .timetable import TimeTable diff --git a/tests/testapp/models/web_page.py b/tests/testapp/models/web_page.py new file mode 100644 index 00000000..811cdeb6 --- /dev/null +++ b/tests/testapp/models/web_page.py @@ -0,0 +1,13 @@ + +from binder.models import BinderModel +from django.db import models + +from binder.plugins.models import HtmlField + + +class WebPage(BinderModel): + """ + Every zoo has a webpage containing some details about the zoo + """ + zoo = models.OneToOneField('Zoo', related_name='web_page', on_delete=models.CASCADE) + content = HtmlField() From 846a54fc6f4fa79bf81823855d86a2fce43e03a2 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Fri, 18 Feb 2022 12:14:54 +0100 Subject: [PATCH 24/34] Implement html field --- binder/plugins/models/html_field.py | 75 ++++++++++++++++++++++++++++- tests/test_html_field.py | 48 ++++++++++++++++-- tests/testapp/views/__init__.py | 1 + tests/testapp/views/web_page.py | 7 +++ 4 files changed, 124 insertions(+), 7 deletions(-) create mode 100644 tests/testapp/views/web_page.py diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index 2ede5568..4c519e61 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -1,4 +1,74 @@ from django.db.models import TextField +from html.parser import HTMLParser +from django.core import exceptions + + +def link_validator(tag, attribute_name, attribute_value): + if not attribute_value.startswith('http://') and not attribute_value.startswith('https://'): + raise exceptions.ValidationError( + 'Link is not valid', + code='invalid_tag', + params={ + 'tag': tag, + }, + ) + +class HtmlValidator(HTMLParser): + allowed_tags = [ + # General setup + 'p', 'br', + # Headers + 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'h7', + + # text decoration + 'b', 'strong', 'i', 'em', 'u', + # Lists + 'ol', 'ul', 'li', + + # Special + 'a', + ] + + allowed_attributes = { + 'a': ['href', 'rel', 'target'] + } + + special_validators = { + ('a', 'href'): link_validator + } + + error_messages = { + 'invalid_tag': 'Tag %(tag)s is not allowed', + 'invalid_attribute': 'Attribute %(attribute)s not allowed for tag %(tag)s' + } + + + + def handle_starttag(self, tag: str, attrs: list) -> None: + if tag not in self.allowed_tags: + raise exceptions.ValidationError( + self.error_messages['invalid_tag'], + code='invalid_tag', + params={ + 'tag': tag + }, + ) + + allowed_attributes_for_tag = self.allowed_attributes.get(tag,[]) + + for (attribute_name, attribute_content) in attrs: + if attribute_name not in allowed_attributes_for_tag: + raise exceptions.ValidationError( + self.error_messages['invalid_attribute'], + code='invalid_tag', + params={ + 'tag': tag, + 'attribute': attribute_name + }, + ) + if (tag, attribute_name) in self.special_validators: + self.special_validators[(tag, attribute_name)](tag, attribute_name, attribute_content) + class HtmlField(TextField): @@ -6,6 +76,7 @@ class HtmlField(TextField): Determine a safe way to save "secure" user provided HTML input, and prevent """ - def validate(self, value, _): - pass + # Validate all html tags + validator = HtmlValidator() + validator.feed(value) diff --git a/tests/test_html_field.py b/tests/test_html_field.py index 8caa40be..d0b2d283 100644 --- a/tests/test_html_field.py +++ b/tests/test_html_field.py @@ -1,10 +1,11 @@ from django.contrib.auth.models import User from django.test import TestCase, Client -from project.testapp.models import Zoo, WebPage +import json +from .testapp.models import Zoo, WebPage -class AnnotationTestCase(TestCase): +class HtmlFieldTestCase(TestCase): def setUp(self): super().setUp() @@ -18,9 +19,46 @@ def setUp(self): self.zoo = Zoo(name='Apenheul') self.zoo.save() - self.webpage = WebPage(zoo=self.zoo, content='') - + self.webpage = WebPage.objects.create(zoo=self.zoo, content='') def test_save_normal_text_ok(self): - self.webpage = WebPage(zoo=self.zoo, content='Artis') + response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps({'content': 'Artis'})) + self.assertEqual(response.status_code, 200) + + def test_simple_html_is_ok(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': '

Artis

Artis is a zoo in amsterdam'})) + self.assertEqual(response.status_code, 200) + + def test_wrong_attribute_not_ok(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': 'test'})) + self.assertEqual(response.status_code, 400) + + def test_simple_link_is_ok(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps( + {'content': 'Visit artis website'})) + self.assertEqual(response.status_code, 200) + + def test_javascript_link_is_not_ok(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({ + 'content': 'Visit artis website'})) + self.assertEqual(response.status_code, 400) + + + + def test_script_is_not_ok(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': ''})) + self.assertEqual(response.status_code, 400) + + def test_can_handle_reallife_data(self): + """ + This is the worst case that we could produce on the WYIWYG edittor + """ + content = '

normal text


HEADing 1


HEADING 2


HEADING 3


bold


italic


underlined


Link


  1. ol1
  2. ol2
  • ul1
  • ul2


subscripttgege

g

"' + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': content})) + self.assertEqual(response.status_code, 200) diff --git a/tests/testapp/views/__init__.py b/tests/testapp/views/__init__.py index 7dd1f4f0..40fcd3c9 100644 --- a/tests/testapp/views/__init__.py +++ b/tests/testapp/views/__init__.py @@ -19,3 +19,4 @@ from .user import UserView from .zoo import ZooView from .zoo_employee import ZooEmployeeView +from .web_page import WebPageView diff --git a/tests/testapp/views/web_page.py b/tests/testapp/views/web_page.py new file mode 100644 index 00000000..d7613031 --- /dev/null +++ b/tests/testapp/views/web_page.py @@ -0,0 +1,7 @@ +from binder.views import ModelView + +from ..models import WebPage + +# From the api docs +class WebPageView(ModelView): + model = WebPage From d47f0defce61304f92c9b8a7aed48f5db3ee1077 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Fri, 18 Feb 2022 12:18:55 +0100 Subject: [PATCH 25/34] Fix flake issues --- binder/plugins/models/__init__.py | 2 +- binder/plugins/models/html_field.py | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/binder/plugins/models/__init__.py b/binder/plugins/models/__init__.py index 5ad29194..7ac605e7 100644 --- a/binder/plugins/models/__init__.py +++ b/binder/plugins/models/__init__.py @@ -1 +1 @@ -from .html_field import HtmlField +from .html_field import HtmlField # noqa: F401 diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index 4c519e61..0409ac26 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -13,6 +13,7 @@ def link_validator(tag, attribute_name, attribute_value): }, ) + class HtmlValidator(HTMLParser): allowed_tags = [ # General setup @@ -42,8 +43,6 @@ class HtmlValidator(HTMLParser): 'invalid_attribute': 'Attribute %(attribute)s not allowed for tag %(tag)s' } - - def handle_starttag(self, tag: str, attrs: list) -> None: if tag not in self.allowed_tags: raise exceptions.ValidationError( @@ -54,7 +53,7 @@ def handle_starttag(self, tag: str, attrs: list) -> None: }, ) - allowed_attributes_for_tag = self.allowed_attributes.get(tag,[]) + allowed_attributes_for_tag = self.allowed_attributes.get(tag, []) for (attribute_name, attribute_content) in attrs: if attribute_name not in allowed_attributes_for_tag: @@ -70,7 +69,6 @@ def handle_starttag(self, tag: str, attrs: list) -> None: self.special_validators[(tag, attribute_name)](tag, attribute_name, attribute_content) - class HtmlField(TextField): """ Determine a safe way to save "secure" user provided HTML input, and prevent From c4d696e3a4fe4bc323edf095ecd46d4c6bb3f6c0 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Fri, 18 Feb 2022 12:21:24 +0100 Subject: [PATCH 26/34] Documentation --- docs/plugins/html_field.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/plugins/html_field.md diff --git a/docs/plugins/html_field.md b/docs/plugins/html_field.md new file mode 100644 index 00000000..3c830213 --- /dev/null +++ b/docs/plugins/html_field.md @@ -0,0 +1,5 @@ +# HTML Field + +The HTML field provides a django model field optimized for user posted HTML code. Its aim is to provide a safe +way to implement a CMS system, where the end user can create pages, but cannot do XSS injections. + From 4552f56d680f3d4ab66a0b38661d60bf9da2fc50 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Wed, 2 Mar 2022 10:46:23 +0100 Subject: [PATCH 27/34] Add gettext for html field --- binder/plugins/models/html_field.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index 0409ac26..dcac1cde 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -1,12 +1,13 @@ from django.db.models import TextField from html.parser import HTMLParser from django.core import exceptions +from django.utils.translation import gettext as _ def link_validator(tag, attribute_name, attribute_value): if not attribute_value.startswith('http://') and not attribute_value.startswith('https://'): raise exceptions.ValidationError( - 'Link is not valid', + _('Link is not valid'), code='invalid_tag', params={ 'tag': tag, @@ -39,8 +40,8 @@ class HtmlValidator(HTMLParser): } error_messages = { - 'invalid_tag': 'Tag %(tag)s is not allowed', - 'invalid_attribute': 'Attribute %(attribute)s not allowed for tag %(tag)s' + 'invalid_tag': _('Tag %(tag)s is not allowed'), + 'invalid_attribute': _('Attribute %(attribute)s not allowed for tag %(tag)s'), } def handle_starttag(self, tag: str, attrs: list) -> None: From 06b33d7a22f704d8fdaf8a3cdbb72cb1d171195c Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Wed, 2 Mar 2022 10:47:55 +0100 Subject: [PATCH 28/34] finish sentence --- binder/plugins/models/html_field.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index dcac1cde..26f1c287 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -72,7 +72,7 @@ def handle_starttag(self, tag: str, attrs: list) -> None: class HtmlField(TextField): """ - Determine a safe way to save "secure" user provided HTML input, and prevent + Determine a safe way to save "secure" user provided HTML input, and prevent XSS injections """ def validate(self, value, _): From 44cab36e7087372ae1f4cfd529b7d8a986b7e1dd Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Wed, 2 Mar 2022 10:57:13 +0100 Subject: [PATCH 29/34] Add test for nested attributes & also test the content of the error message --- binder/plugins/models/html_field.py | 4 ++-- tests/test_html_field.py | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index 26f1c287..c4365005 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -8,7 +8,7 @@ def link_validator(tag, attribute_name, attribute_value): if not attribute_value.startswith('http://') and not attribute_value.startswith('https://'): raise exceptions.ValidationError( _('Link is not valid'), - code='invalid_tag', + code='invalid_attribute', params={ 'tag': tag, }, @@ -60,7 +60,7 @@ def handle_starttag(self, tag: str, attrs: list) -> None: if attribute_name not in allowed_attributes_for_tag: raise exceptions.ValidationError( self.error_messages['invalid_attribute'], - code='invalid_tag', + code='invalid_attribute', params={ 'tag': tag, 'attribute': attribute_name diff --git a/tests/test_html_field.py b/tests/test_html_field.py index d0b2d283..49da290a 100644 --- a/tests/test_html_field.py +++ b/tests/test_html_field.py @@ -21,6 +21,8 @@ def setUp(self): self.webpage = WebPage.objects.create(zoo=self.zoo, content='') + + def test_save_normal_text_ok(self): response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps({'content': 'Artis'})) self.assertEqual(response.status_code, 200) @@ -35,6 +37,10 @@ def test_wrong_attribute_not_ok(self): data=json.dumps({'content': 'test'})) self.assertEqual(response.status_code, 400) + parsed_response = json.loads(response.content) + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_attribute', parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + def test_simple_link_is_ok(self): response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps( {'content': 'Visit artis website'})) @@ -46,13 +52,32 @@ def test_javascript_link_is_not_ok(self): 'content': 'Visit artis website'})) self.assertEqual(response.status_code, 400) + parsed_response = json.loads(response.content) + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_attribute', parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + def test_script_is_not_ok(self): response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps({'content': ''})) + self.assertEqual(response.status_code, 400) + parsed_response = json.loads(response.content) + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_tag', parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + + def test_script_is_not_ok_nested(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': ''})) + self.assertEqual(response.status_code, 400) + + parsed_response = json.loads(response.content) + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_tag', parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + + def test_can_handle_reallife_data(self): """ This is the worst case that we could produce on the WYIWYG edittor From 4373130ece50fdc4bf22801c83ff28497fbd0d71 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Thu, 3 Mar 2022 11:48:31 +0100 Subject: [PATCH 30/34] Merge multiple errors in HTMLField --- binder/plugins/models/html_field.py | 55 +++++++++++++++++++++-------- tests/test_html_field.py | 16 +++++++++ 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index c4365005..18c642a9 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -1,18 +1,29 @@ +from functools import reduce +from typing import List + from django.db.models import TextField from html.parser import HTMLParser -from django.core import exceptions +from django.core.exceptions import ValidationError from django.utils.translation import gettext as _ - -def link_validator(tag, attribute_name, attribute_value): - if not attribute_value.startswith('http://') and not attribute_value.startswith('https://'): - raise exceptions.ValidationError( +ALLOWED_LINK_PREFIXES = [ + 'http://', + 'https://', + 'mailto:' +] +def link_validator(tag, attribute_name, attribute_value) -> List[ValidationError]: + validation_errors = [] + if not any(map(lambda prefix: attribute_value.startswith(prefix), ALLOWED_LINK_PREFIXES)): + validation_errors.append(ValidationError( _('Link is not valid'), code='invalid_attribute', params={ 'tag': tag, }, - ) + )) + + + return validation_errors class HtmlValidator(HTMLParser): @@ -44,38 +55,54 @@ class HtmlValidator(HTMLParser): 'invalid_attribute': _('Attribute %(attribute)s not allowed for tag %(tag)s'), } + def validate(self, value: str) -> List[ValidationError]: + """ + Validates html, and gives a list of validation errors + """ + + self.errors = [] + + self.feed(value) + + return self.errors + def handle_starttag(self, tag: str, attrs: list) -> None: + tag_errors = [] if tag not in self.allowed_tags: - raise exceptions.ValidationError( + tag_errors.append(ValidationError( self.error_messages['invalid_tag'], code='invalid_tag', params={ 'tag': tag }, - ) + )) allowed_attributes_for_tag = self.allowed_attributes.get(tag, []) for (attribute_name, attribute_content) in attrs: if attribute_name not in allowed_attributes_for_tag: - raise exceptions.ValidationError( + tag_errors.append(ValidationError( self.error_messages['invalid_attribute'], code='invalid_attribute', params={ 'tag': tag, 'attribute': attribute_name }, - ) + )) if (tag, attribute_name) in self.special_validators: - self.special_validators[(tag, attribute_name)](tag, attribute_name, attribute_content) + tag_errors += self.special_validators[(tag, attribute_name)](tag, attribute_name, attribute_content) + + self.errors += tag_errors class HtmlField(TextField): """ Determine a safe way to save "secure" user provided HTML input, and prevent XSS injections """ - - def validate(self, value, _): + def validate(self, value: str, _): # Validate all html tags validator = HtmlValidator() - validator.feed(value) + errors = validator.validate(value) + + if errors: + raise ValidationError(errors) diff --git a/tests/test_html_field.py b/tests/test_html_field.py index 49da290a..34cc5531 100644 --- a/tests/test_html_field.py +++ b/tests/test_html_field.py @@ -87,3 +87,19 @@ def test_can_handle_reallife_data(self): data=json.dumps({'content': content})) self.assertEqual(response.status_code, 200) + + def test_multiple_errors(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({ + 'content': 'Visit artis website'})) + self.assertEqual(response.status_code, 400) + + parsed_response = json.loads(response.content) + self.assertEqual('ValidationError', parsed_response['code']) + + + self.assertEqual('invalid_tag', + parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + self.assertEqual('invalid_tag', + parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][1]['code']) + From 581926f16577cf0a6cc9301c5ca385fe71b8fd7a Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Thu, 3 Mar 2022 12:07:36 +0100 Subject: [PATCH 31/34] Add noreferrer noopener check for links --- binder/plugins/models/html_field.py | 56 +++++++++++++++++++++++++++-- tests/test_html_field.py | 41 +++++++++++++++++++-- 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index 18c642a9..acce5313 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -11,6 +11,36 @@ 'https://', 'mailto:' ] + + +def link_rel_validator(tag, attribute_name, attribute_value) -> List[ValidationError]: + validation_errors = [] + + rels = attribute_value.split(' ') + + if 'noopener' not in rels: + + validation_errors.append(ValidationError( + _('Link needs rel="noopener"'), + code='invalid_attribute', + params={ + 'tag': tag, + }, + )) + + if 'noreferrer' not in rels: + validation_errors.append(ValidationError( + _('Link needs rel="noreferer"'), + code='invalid_attribute', + params={ + 'tag': tag, + }, + )) + + + return validation_errors + + def link_validator(tag, attribute_name, attribute_value) -> List[ValidationError]: validation_errors = [] if not any(map(lambda prefix: attribute_value.startswith(prefix), ALLOWED_LINK_PREFIXES)): @@ -21,8 +51,6 @@ def link_validator(tag, attribute_name, attribute_value) -> List[ValidationError 'tag': tag, }, )) - - return validation_errors @@ -46,12 +74,18 @@ class HtmlValidator(HTMLParser): 'a': ['href', 'rel', 'target'] } + required_attributes = { + 'a': ['rel'], + } + special_validators = { - ('a', 'href'): link_validator + ('a', 'href'): link_validator, + ('a', 'rel'): link_rel_validator, } error_messages = { 'invalid_tag': _('Tag %(tag)s is not allowed'), + 'missing_attribute': _('Attribute %(attribute)s is required for tag %(tag)s'), 'invalid_attribute': _('Attribute %(attribute)s not allowed for tag %(tag)s'), } @@ -77,6 +111,21 @@ def handle_starttag(self, tag: str, attrs: list) -> None: }, )) + set_attributes = set(map(lambda attr: attr[0], attrs)) + required_attributes = set(self.required_attributes.get(tag, [])) + missing_attributes = required_attributes - set_attributes + for missing_attribute in missing_attributes: + tag_errors.append( + ValidationError( + self.error_messages['missing_attribute'], + code='missing_attribute', + params={ + 'tag': tag, + 'attribute': missing_attribute + }, + ) + ) + allowed_attributes_for_tag = self.allowed_attributes.get(tag, []) for (attribute_name, attribute_content) in attrs: @@ -99,6 +148,7 @@ class HtmlField(TextField): """ Determine a safe way to save "secure" user provided HTML input, and prevent XSS injections """ + def validate(self, value: str, _): # Validate all html tags validator = HtmlValidator() diff --git a/tests/test_html_field.py b/tests/test_html_field.py index 34cc5531..fa10ab7d 100644 --- a/tests/test_html_field.py +++ b/tests/test_html_field.py @@ -43,17 +43,21 @@ def test_wrong_attribute_not_ok(self): def test_simple_link_is_ok(self): response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps( - {'content': 'Visit artis website'})) + {'content': 'Visit artis website'})) + self.assertEqual(response.status_code, 200) + + def test_javascript_link_is_not_ok(self): response = self.client.put(f'/web_page/{self.webpage.id}/', data=json.dumps({ - 'content': 'Visit artis website'})) + 'content': 'Visit artis website'})) self.assertEqual(response.status_code, 400) parsed_response = json.loads(response.content) self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_attribute', parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) @@ -103,3 +107,36 @@ def test_multiple_errors(self): self.assertEqual('invalid_tag', parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][1]['code']) + + def test_link_no_rel_errors(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': 'bla'})) + self.assertEqual(response.status_code, 400) + + parsed_response = json.loads(response.content) + + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('missing_attribute', + parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + + def test_link_noopener_required(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': 'bla'})) + self.assertEqual(response.status_code, 400) + + parsed_response = json.loads(response.content) + + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_attribute', + parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) + + def test_link_noreferrer_required(self): + response = self.client.put(f'/web_page/{self.webpage.id}/', + data=json.dumps({'content': 'bla'})) + self.assertEqual(response.status_code, 400) + + parsed_response = json.loads(response.content) + + self.assertEqual('ValidationError', parsed_response['code']) + self.assertEqual('invalid_attribute', + parsed_response['errors']['web_page'][f'{self.webpage.id}']['content'][0]['code']) From 271a279baf413eed14344f08cd742276ee661dc0 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Tue, 8 Mar 2022 09:47:51 +0100 Subject: [PATCH 32/34] linting --- binder/plugins/models/html_field.py | 1 - 1 file changed, 1 deletion(-) diff --git a/binder/plugins/models/html_field.py b/binder/plugins/models/html_field.py index acce5313..d6cd5c65 100644 --- a/binder/plugins/models/html_field.py +++ b/binder/plugins/models/html_field.py @@ -1,4 +1,3 @@ -from functools import reduce from typing import List from django.db.models import TextField From 15e357f7c4bd731e3d0b6f766629ebccb138da64 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Thu, 10 Mar 2022 13:35:06 +0100 Subject: [PATCH 33/34] fix merge conflict --- binder/views.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/binder/views.py b/binder/views.py index 633a69e9..04922aa4 100644 --- a/binder/views.py +++ b/binder/views.py @@ -993,11 +993,7 @@ def _get_with_ids(self, pks, request, include_annotations, with_map, where_map): rev_field = f.remote_field.name query = ( view.model.objects -<<<<<<< HEAD - .filter(prefix_q_expression(q, rev_field, field), **{rev_field + '__in': pks}) -======= .filter(prefix_q_expression(q, rev_field, field, view.model), **{rev_field + '__in': pks}) ->>>>>>> master .values_list(rev_field + '__pk', 'pk') .distinct() ) From e96b054f93fef5a59f487c91cf766e6bbb6d8c03 Mon Sep 17 00:00:00 2001 From: Stefan Majoor Date: Fri, 6 Dec 2024 10:10:02 +0100 Subject: [PATCH 34/34] Fix crash on non gotten FKs --- binder/plugins/views/csvexport.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/binder/plugins/views/csvexport.py b/binder/plugins/views/csvexport.py index 8b092ea1..1741cbf6 100644 --- a/binder/plugins/views/csvexport.py +++ b/binder/plugins/views/csvexport.py @@ -288,14 +288,20 @@ def get_datum(data, key, prefix=''): if fk_ids is None: # This case happens if we have a nullable foreign key that is null. Treat this as a many - # to one relation with no values. + # to one relation with no values. fk_ids = [] elif type(fk_ids) != list: fk_ids = [fk_ids] # if head_key not in key_mapping: prefix_key = parent_data['with_mapping'][new_prefix[1:]] - datums = [str(get_datum(key_mapping[prefix_key][fk_id], subkey, new_prefix)) for fk_id in fk_ids] + datums = [] + for fk_id in fk_ids: + try: + datums.append(str(get_datum(key_mapping[prefix_key][fk_id], subkey, new_prefix))) + except KeyError: + pass + # datums = [str(get_datum(key_mapping[prefix_key][fk_id], subkey, new_prefix)) for fk_id in fk_ids] return self.csv_settings.multi_value_delimiter.join( datums )