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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
10 changes: 8 additions & 2 deletions kinto/views/__init__.py
Original file line number Diff line number Diff line change
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,9 @@ 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)
raise response
6 changes: 3 additions & 3 deletions tests/core/test_views_errors.py
Original file line number Diff line number Diff line change
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
6 changes: 6 additions & 0 deletions tests/test_views_buckets.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ def test_buckets_can_be_deleted_in_bulk(self):
self.app.get('/buckets/2', headers=self.headers, status=404)
self.app.get('/buckets/3', headers=self.headers, status=404)

def test_unknown_bucket_raises_404(self):
resp = self.app.get('/buckets/2', headers=self.headers, status=404)
self.assertEqual(resp.json['details']['id'], '2')
self.assertEqual(resp.json['details']['resource_name'], 'bucket')

def test_buckets_can_be_deleted(self):
self.app.get(self.bucket_url, headers=self.headers,
status=404)
Expand All @@ -228,6 +233,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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