BadRequestError is not being raised succesfully when sending garbage attributes in patch_resource #48
Description
Looking at the patch_resource function, we have
def patch_resource(self, session, json_data, api_type, obj_id):
"""
Replacement of resource values.
:param session: SQLAlchemy session
:param json_data: Request JSON Data
:param api_type: Type of the resource
:param obj_id: ID of the resource
"""
model = self._fetch_model(api_type)
resource = self._fetch_resource(session, api_type, obj_id,
Permissions.EDIT)
self._check_json_data(json_data)
orm_desc_keys = resource.__mapper__.all_orm_descriptors.keys()
if not ({'type', 'id'} <= set(json_data['data'].keys())):
raise BadRequestError('Missing type or id')
if str(json_data['data']['id']) != str(resource.id):
raise BadRequestError('IDs do not match')
if json_data['data']['type'] != resource.__jsonapi_type__:
raise BadRequestError('Type does not match')
json_data['data'].setdefault('relationships', {})
json_data['data'].setdefault('attributes', {})
missing_keys = set(json_data['data'].get('relationships', {}).keys()) \
- set(resource.__jsonapi_map_to_py__.keys())
if missing_keys:
raise BadRequestError(
'{} not relationships for {}.{}'.format(
', '.join(list(missing_keys)),
model.__jsonapi_type__, resource.id))
attrs_to_ignore = {'__mapper__', 'id'}
session.add(resource)
try:
for key, relationship in resource.__mapper__.relationships.items():
api_key = resource.__jsonapi_map_to_api__[key]
attrs_to_ignore |= set(relationship.local_columns) | {key}
if api_key not in json_data['data']['relationships'].keys():
continue
self.patch_relationship(
session, json_data['data']['relationships'][api_key],
model.__jsonapi_type__, resource.id, api_key)
data_keys = set(map((
lambda x: resource.__jsonapi_map_to_py__.get(x, None)),
json_data['data']['attributes'].keys()))
model_keys = set(orm_desc_keys) - attrs_to_ignore
if not data_keys <= model_keys:
raise BadRequestError(
'{} not attributes for {}.{}'.format(
', '.join(list(data_keys - model_keys)),
model.__jsonapi_type__, resource.id))
for key in data_keys & model_keys:
setter = get_attr_desc(resource, key, AttributeActions.SET)
setter(resource, json_data['data']['attributes'][resource.__jsonapi_map_to_api__[key]]) # NOQA
session.commit()
except IntegrityError as e:
session.rollback()
raise ValidationError(str(e.orig))
except AssertionError as e:
session.rollback()
raise ValidationError(e.msg)
except TypeError as e:
session.rollback()
raise ValidationError('Incompatible data type')
return self.get_resource(
session, {}, model.__jsonapi_type__, resource.id)
Looking at this code we could assume that if we had a test such as:
def test_patch_resource_unknown_attributes(self):
user = models.User(
first='Sally', last='Smith',
password='password', username='SallySmith1')
self.session.add(user)
blog_post = models.Post(
title='This Is A Title', content='This is the content',
author_id=user.id, author=user)
self.session.add(blog_post)
comment = models.Comment(
content='This is a comment', author_id=user.id,
post_id=blog_post.id, author=user, post=blog_post)
self.session.add(comment)
self.session.commit()
payload = {
'data': {
'type': 'posts',
'id': blog_post.id,
'attributes': {
'title': 'This is a new title',
'content': 'This is new content',
'author-id': 1,
'nonexistant': 'test'
},
'relationships': {
'author': {
'data': {
'type': 'users',
'id': user.id
}
}
}
}
}
with self.assertRaises(errors.BadRequestError) as error:
models.serializer.patch_resource(
self.session, payload, 'posts', blog_post.id)
expected_detail = 'nonexistant not attribute for posts.1'
self.assertEqual(error.exception.detail, expected_detail)
self.assertEqual(error.exception.status_code, 400)
That we would actually get a BadRequestError and the correct expected_detail.
However we do not get this because if we look at this specific section of patch_resource
data_keys = set(map((
lambda x: resource.__jsonapi_map_to_py__.get(x, None)),
json_data['data']['attributes'].keys()))
model_keys = set(orm_desc_keys) - attrs_to_ignore
if not data_keys <= model_keys:
raise BadRequestError(
'{} not attributes for {}.{}'.format(
', '.join(list(data_keys - model_keys)),
model.__jsonapi_type__, resource.id))
the data_keys actually becomes a set of {'title, 'None', 'author_id', 'content'} and the model_keys is {'title', 'author_id', 'content'}
When we try to raise the BadRequestError we then get aTypeError: 'sequence item 0: expected string, NoneType found'.
Thus if you look at the try/except, this is then caught by
except TypeError as e:
session.rollback()
raise ValidationError('Incompatible data type')
So although some sort of error is raised and it is gracefully handled. It is not the expected error since the TypeError is occuring when trying to raise that BadRequestError and is thus excepted and instead a ValidationError('Incompatible data type') is raised.
I feel that the actually BadRequestError should be raised correctly so that we have a more informed error as to what happened rather than a ValidationError.