Skip to content

M2m validation errors #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MLewiDev wants to merge 6 commits into
base: master
Choose a base branch
from
Open

M2m validation errors #174

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a345984
Added filed to gitignore file
Jun 28, 2021
433cf5d
Fix for test missing secret key
Jun 29, 2021
e91aa51
M2m validation errors
Jun 29, 2021
00929e7
Merge branch 'master' into m2m-validation-errors
Aug 23, 2021
12a0ca2
Fix linting error
Aug 23, 2021
38ed2ed
Modifications after code review
Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,6 @@ venv
.tox/
.coverage
.vscode/
build/**/*
foo/**/*
foobar*
7 changes: 6 additions & 1 deletion binder/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,7 +1405,8 @@ def store_m2m_field(obj, field, value, request):
ignored_fields.append(field)
except BinderValidationError as e:
validation_errors.append(e)

except BinderFieldTypeError:
raise
Comment thread
BBooijLiewes marked this conversation as resolved.
Outdated

if validation_errors:
raise sum(validation_errors, None)
Expand Down Expand Up @@ -2002,6 +2003,10 @@ def _multi_put_save_objects(self, ordered_objects, objects, request):
view._store(obj, values, request, pk=oid)
except BinderValidationError as e:
validation_errors.append(e)
except BinderFieldTypeError:
if not validation_errors:
raise

Comment on lines +2004 to +2007

@daanvdk daanvdk Oct 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I see 2 issues with this approach:

  • This now works for all BinderFieldTypeErrors, not just the ones that are caused by the save of another model not working. In the other cases a BinderFieldTypeError should be more important than a validation error.
  • If both the related model and the main model have validation errors this will now ignore the validation errors on the main model.

I would suggest to instead pass an extra argument to _store that provides which models have failed to save and then prevent the BinderFieldTypeError from being raised when the value refers to one of these models.

@MLewiDev MLewiDev Jan 31, 2022 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to create test for the second bullet, but I have a problem in generating a test for the first bullet point related to BinderFieldTypeError. Can you give some tips?

After writing failing tests i will start with refactor and try to use _store.

@daanvdk daanvdk Jan 31, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a BinderFieldTypeError should occur when the value that you are trying to save does not match the type of the field, so for example trying to save a string into an integer field. Do note that django does try to 'fix' values. So for example the other way around (an integer to a string field) it would just turn the integer into a string.

if oid < 0:
new_id_map[(model, oid)] = obj.id
for base in getmro(model)[1:]:
Expand Down
1 change: 1 addition & 0 deletions tests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
'DEBUG': True,
'SECRET_KEY': 'testy mctestface',
'ALLOWED_HOSTS': ['*'],
'SECRET_KEY': 'test_secret',
'DATABASES': {
'default': db_settings,
},
Expand Down
37 changes: 37 additions & 0 deletions tests/test_m2m_store_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,40 @@ def test_saving_o2o_with_validation_error(self):

response = self.client.put('/contact_person/', data=json.dumps(model_data), content_type='application/json')
self.assert_validation_error_as_response(response)

# Validation should be shown when contained in m2m relation
def test_saving_m2m_with_validation_error(self):
data = {
"id": -65,
# Name exceeds allowed 64 charecters, should show this error in response
'name': 'Scooby Dooooooooooooooooooooooooooooooooooooooooooooooooooooooooo',
}


zoo_data = {
"data": [
{
"id": -4,
'name': 'Test zoo',
'most_popular_animals': [-65],
"contacts": [],
'opening_time': '08:00'

}
],
"with": {
"animal": [data]
}
}

animal_data = {
"data": [data]
}

response_animal = self.client.put(
'/animal/', data=json.dumps(animal_data), content_type='application/json')
response_zoo = self.client.put(
'/zoo/', data=json.dumps(zoo_data), content_type='application/json')

self.assertEqual(response_zoo.status_code, 400)
self.assertIn(str(response_animal.content), str(response_zoo.content))