Skip to content
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.

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

Added details attribute to 404 errors. #861

Merged
merged 15 commits into from Oct 20, 2016
Merged
1 change: 1 addition & 0 deletions CHANGELOG.rst
Expand Up @@ -8,6 +8,7 @@ This document describes changes between each past release.

**New features**

- Added details attribute to 404 errors. (#818)
- Added a new built-in plugin ``kinto.plugins.admin`` to serve the kinto admin.


Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.rst
Expand Up @@ -31,6 +31,7 @@ Contributors
* Julien Bouquillon <contact@revolunet.com>
* Lavish Aggarwal <lucky.lavish@gmail.com>
* Maksym Shalenyi <supamaxy@gmail.com>
* Mansimar Kaur <mansimarkaur.mks@gmail.com>
* Masataka Takeuchi <masataka.takeuchi@l-is-b.com>
* Mathieu Agopian <mathieu@agopian.info>
* Mathieu Leplatre <mathieu@mozilla.com>
Expand Down
5 changes: 5 additions & 0 deletions docs/api/index.rst
Expand Up @@ -12,6 +12,11 @@ Changelog
---------


1.13 (2016-10-14)
'''''''''''''''''

- ``details`` attribute present in response of 404 errors.

1.12 (2016-10-11)
'''''''''''''''''

Expand Down
8 changes: 6 additions & 2 deletions kinto/core/resource/__init__.py
Expand Up @@ -680,8 +680,12 @@ def _get_record_or_404(self, record_id):
try:
return self.model.get_record(record_id)
except storage_exceptions.RecordNotFoundError:
response = http_error(HTTPNotFound(),
errno=ERRORS.INVALID_RESOURCE_ID)
details = {
"id": record_id,
"resource_name": self.request.current_resource_name
}
response = http_error(HTTPNotFound(), errno=ERRORS.INVALID_RESOURCE_ID,
details=details)
raise response

def _add_timestamp_header(self, response, timestamp=None):
Expand Down
1 change: 0 additions & 1 deletion kinto/core/testing.py
Expand Up @@ -79,7 +79,6 @@ def assertFormattedError(self, response, code, errno, error,
self.assertEqual(response.json['code'], code)
self.assertEqual(response.json['errno'], errno)
self.assertEqual(response.json['error'], error)

if message is not None:
self.assertIn(message, response.json['message'])
else: # pragma: no cover
Expand Down
11 changes: 9 additions & 2 deletions kinto/views/__init__.py
Expand Up @@ -2,7 +2,8 @@
import string

from kinto.core.storage import generators, exceptions
from pyramid import httpexceptions
from pyramid.httpexceptions import HTTPNotFound
from kinto.core.errors import http_error, ERRORS


class NameGenerator(generators.Generator):
Expand All @@ -29,4 +30,10 @@ def object_exists_or_404(request, collection_id, object_id, parent_id=''):
object_id=object_id)
except exceptions.RecordNotFoundError:
# XXX: We gave up putting details about parent id here (See #53).
raise httpexceptions.HTTPNotFound()
details = {
"id": object_id,
"resource_name": collection_id
}
response = http_error(HTTPNotFound(), errno=ERRORS.MISSING_RESOURCE,
details=details)
Copy link
Member

Choose a reason for hiding this comment

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

I think details can be on the same line.

raise response
6 changes: 3 additions & 3 deletions tests/core/test_views_errors.py
Expand Up @@ -44,15 +44,15 @@ def test_404_is_valid_formatted_error(self):
self.assertFormattedError(response, 404, ERRORS.MISSING_RESOURCE, "Not Found",
"The resource you are looking for could not be found.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this still be a normal 404? Why are we removing this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we want to add the details attribute to all 404 errors as in the normal 404 should also contain a details attribute in the response.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the test should be removed from here though ;)

As Ethan suggested, you can add more tests in tests/test_views_*.py instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't the valid format of a 404 error contain the details attribute as well, now? If yes, then I think this test should be modified to ensure that the error contains details. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well indeed, you could modify this test a bit, but not too much. It would be complementary with the new ones in test_views_*.py


def test_404_can_be_overridded(self):
def test_404_can_be_overridden(self):
custom_404 = http_error(httpexceptions.HTTPNotFound(),
errno=ERRORS.MISSING_RESOURCE,
message="Customized.")
with mock.patch('tests.core.testapp.views.Mushroom._extract_filters',
side_effect=custom_404):
response = self.app.get(self.sample_url, headers=self.headers, status=404)
self.assertFormattedError(
response, 404, ERRORS.MISSING_RESOURCE, "Not Found", "Customized.")
self.assertFormattedError(response, 404, ERRORS.MISSING_RESOURCE,
"Not Found", "Customized.")

def test_401_is_valid_formatted_error(self):
response = self.app.get(self.sample_url, status=401)
Expand Down
5 changes: 3 additions & 2 deletions tests/test_views_buckets.py
Expand Up @@ -197,8 +197,8 @@ def setUp(self):
r = self.app.post_json(self.collection_url + '/records',
MINIMALIST_RECORD,
headers=self.headers)
record_id = r.json['data']['id']
self.record_url = self.collection_url + '/records/%s' % record_id
self.record_id = r.json['data']['id']
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to add self here?

self.record_url = self.collection_url + '/records/%s' % self.record_id
# Delete the bucket.
self.app.delete(self.bucket_url, headers=self.headers)

Expand Down Expand Up @@ -228,6 +228,7 @@ def test_every_collections_are_deleted_too(self):
self.app.put_json(self.bucket_url, MINIMALIST_BUCKET,
headers=self.headers)
self.app.get(self.collection_url, headers=self.headers, status=404)

# Verify tombstones
resp = self.app.get('%s/collections?_since=0' % self.bucket_url,
headers=self.headers)
Expand Down
9 changes: 7 additions & 2 deletions tests/test_views_collections.py
Expand Up @@ -47,6 +47,12 @@ def test_unknown_bucket_raises_403(self):
other_bucket = self.collections_url.replace('beers', 'sodas')
self.app.get(other_bucket, headers=self.headers, status=403)

def test_unknown_collection_raises_404(self):
other_collection = self.collection_url.replace('barley', 'pills')
resp = self.app.get(other_collection, headers=self.headers, status=404)
self.assertEqual(resp.json['details']['id'], 'pills')
self.assertEqual(resp.json['details']['resource_name'], 'collection')

def test_collections_are_isolated_by_bucket(self):
other_bucket = self.collection_url.replace('beers', 'sodas')
self.app.put_json('/buckets/sodas',
Expand Down Expand Up @@ -117,8 +123,7 @@ def setUp(self):
self.app.delete(self.collection_url, headers=self.headers)

def test_collections_can_be_deleted(self):
self.app.get(self.collection_url, headers=self.headers,
status=404)
self.app.get(self.collection_url, headers=self.headers, status=404)

def test_collections_can_be_deleted_in_bulk(self):
alice_headers = get_user_headers('alice')
Expand Down
6 changes: 6 additions & 0 deletions tests/test_views_groups.py
Expand Up @@ -145,6 +145,12 @@ def test_groups_can_be_deleted(self):
self.app.get(self.group_url, headers=self.headers,
status=404)

def test_unknown_group_raises_404(self):
other_group = self.group_url.replace('moderators', 'blah')
resp = self.app.get(other_group, headers=self.headers, status=404)
self.assertEqual(resp.json['details']['id'], 'blah')
self.assertEqual(resp.json['details']['resource_name'], 'group')

def test_group_is_removed_from_users_principals_on_group_deletion(self):
self.app.put_json(self.group_url, MINIMALIST_GROUP,
headers=self.headers, status=201)
Expand Down
10 changes: 9 additions & 1 deletion tests/test_views_records.py
Expand Up @@ -38,7 +38,15 @@ def test_unknown_bucket_raises_403(self):

def test_unknown_collection_raises_404(self):
other_collection = self.collection_url.replace('barley', 'pills')
self.app.get(other_collection, headers=self.headers, status=404)
resp = self.app.get(other_collection, headers=self.headers, status=404)
self.assertEqual(resp.json['details']['id'], 'pills')
self.assertEqual(resp.json['details']['resource_name'], 'collection')

def test_unknown_record_raises_404(self):
other_record = self.record_url.replace(self.record['id'], self.record['id']+'blah')
response = self.app.get(other_record, headers=self.headers, status=404)
self.assertEqual(response.json['details']['id'], self.record['id']+'blah')
self.assertEqual(response.json['details']['resource_name'], 'record')

def test_unknown_collection_does_not_query_timestamp(self):
other_collection = self.collection_url.replace('barley', 'pills')
Expand Down