BadRequestError is not being raised succesfully when sending garbage attributes in patch_resource

[kaitj91]

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.

[Anderycks]

I'm not sure I can work out how this bug is different from bug #44. Can you summarize the difference for me?

[kaitj91]

It really is not very different. The differences is that it occurs in a different function and that in this case it is being caught and handled with ValidationError whereas in the bug #44 it is not.