-
Notifications
You must be signed in to change notification settings - Fork 422
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this bug! This is a good first step, but I don't think it's complete yet.
@@ -40,19 +40,16 @@ def test_backoff_header_is_present_on_error_responses(self): | |||
self.assertEquals(response.headers['Backoff'], '10') | |||
|
|||
def test_404_is_valid_formatted_error(self): | |||
response = self.app.get('/unknown', status=404) | |||
self.assertFormattedError(response, 404, ERRORS.MISSING_RESOURCE, "Not Found", | |||
"The resource you are looking for could not be found.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -70,7 +70,7 @@ class FormattedErrorMixin(object): | |||
""" | |||
|
|||
def assertFormattedError(self, response, code, errno, error, | |||
message=None, info=None): | |||
message=None, info=None, details=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we don't use this details
variable anywhere here. Am I missing something?
detail_dict = { | ||
"id": collection_id, | ||
"resource_name": "collection" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where we define the custom 404 for collections, and for records, but I don't see any place where we define one for buckets. Where is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here "id"
should have value of object_id
and resource_name
value of collection_id
(ref #710)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glasserc Even i couldn't find anything for buckets.
@leplatrem id is to object_id I understood from #710 but what exactly does a resource_name define?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resource name is one of bucket
, collection
, group
, record
. Here by using collection_id
it would work as expected. The name of this parameter is very bad I know, because it seems related to collections only, but it actually represent the resource id/name. In #710 we would fix that naming problem.
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.") | ||
response=response, code=404, errno=ERRORS.MISSING_RESOURCE, error="Not Found", | ||
message="Customized.", details={"id": "abc", "resource_name": "collection"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this test is relevant to what you want to change. This looks like a test for the underlying kinto.core
resource code and specifically like it's trying to verify that if a resource has an _extract_filters
method that raises an exception, that exception gets passed through correctly.
I think you need tests to verify that getting a non-existent bucket, collection, or record has the correct details attached. You might look for those tests in tests/test_views_*.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good start!
detail_dict = { | ||
"id": collection_id, | ||
"resource_name": "collection" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here "id"
should have value of object_id
and resource_name
value of collection_id
(ref #710)
@@ -40,19 +40,16 @@ def test_backoff_header_is_present_on_error_responses(self): | |||
self.assertEquals(response.headers['Backoff'], '10') | |||
|
|||
def test_404_is_valid_formatted_error(self): | |||
response = self.app.get('/unknown', status=404) | |||
self.assertFormattedError(response, 404, ERRORS.MISSING_RESOURCE, "Not Found", | |||
"The resource you are looking for could not be found.") |
There was a problem hiding this comment.
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
@@ -8,6 +8,7 @@ This document describes changes between each past release. | |||
|
|||
**New features** | |||
|
|||
- Added details attribute to 404 errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: add reference to original issue.
Also, this change belongs to the protocol section of changes, and requires a line in the API changelog (docs/api/index.rst)
@@ -674,8 +674,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) | |||
detail_dict = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: you can name this variable just details
errno=ERRORS.INVALID_RESOURCE_ID) | ||
detail_dict = { | ||
"id": record_id, | ||
"resource_name": "record" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here resource_name
should have value of self.request.current_resource_name
09489b2
to
e4fdefc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you found test_views_records
and test_views_collections
, but I don't see any tests added to test_views_buckets
. I think you could add some there too.
self.app.get(other_collection, headers=self.headers, status=404) | ||
resp = self.app.get(other_collection, headers=self.headers, status=404) | ||
self.assertIn('id', resp.json['details']) | ||
self.assertIn('resource_name', resp.json['details']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're only checking that the attributes are present in the error message, not that they are correct. I think we can add stronger tests that also check that resp.json['details']['id']
is equal to "barley"
, and similarly that the resource_name
is "collection"
.
I'm not sure every single test needs to be updated. Personally, I think it's OK if we leave these tests alone -- they check that we got a 404, and that's good enough. Instead I would add new tests that check that when we make a request that we expect to get a 404, that that 404 comes back with these details.
Since we expect all 404s to have a certain structure (with id
and resource_name
having certain attributes), it might be worthwhile to add a method somewhere called something like check404
that enforces this structure. Then, the tests that use this method will look more declarative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes more sense to add another test to check the 404 format. Thanks!
@@ -2,7 +2,8 @@ | |||
import string | |||
|
|||
from kinto.core.storage import generators, exceptions | |||
from pyramid import httpexceptions | |||
from pyramid.httpexceptions import (HTTPNotFound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: superfluous parenthesis
@@ -89,8 +88,7 @@ def assertFormattedError(self, response, code, errno, error, | |||
self.assertIn(info, response.json['info']) | |||
else: # pragma: no cover | |||
self.assertNotIn('info', response.json) | |||
|
|||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: leave blank lines ;)
details = { | ||
"id": object_id, | ||
"resource_name": collection_id | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: something looks wrong with the indentation here
self.app.get(other_bucket, headers=self.headers, status=404) | ||
resp = self.app.get(other_bucket, headers=self.headers, status=404) | ||
self.assertIn('id', resp.json['details']) | ||
self.assertIn('resource_name', resp.json['details']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we only keep the assertions that are relevant to the specification (ie. the test title).
For example, the test on the values of id and details is not properly relevant for the spec «collections are isolated by bucket».
I suggest that you only keep the assertions regarding details
in dedicated tests similar to test_unknown_collection_raises_404
from test_views_records.py
, but for each of bucket, group, collection and record in the other test_views_
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is correct and could be merged!
But we're very picky with regards to the quality of tests as well :)
Basically, each feature should have its dedicated spec (c.f test as specs).
The one for collection is good! But I suggest that you add:
test_unknown_group_raises_404
in
test_views_group.py
test_unknown_record_raises_404
in
test_views_record.py
Good luck ;) We're almost there!
self.app.get(other, headers=self.headers, status=404) | ||
response = self.app.get(other, headers=self.headers, status=404) | ||
self.assertEqual(response.json['details']['id'], self.record['id']) | ||
self.assertEqual(response.json['details']['resource_name'], 'record') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we don't need the assertion on the details
fields. It's unrelated to the specs of this test: «test_unknown_collection_does_not_query_timestamp»
self.app.get(self.collection_url, headers=self.headers, status=404) | ||
response = self.app.get(self.collection_url, headers=self.headers, status=404) | ||
self.assertEqual(response.json['details']['id'], 'barley') | ||
self.assertEqual(response.json['details']['resource_name'], 'collection') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already covered this in test_unknown_collection_raises_404
from test_views_collections
. The assertion is not necessary here.
self.app.get(self.record_url, headers=self.headers, status=404) | ||
response = self.app.get(self.record_url, headers=self.headers, status=404) | ||
self.assertEqual(response.json['details']['id'], self.record_id) | ||
self.assertEqual(response.json['details']['resource_name'], 'record') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assertions should be moved to a specific test in test_views_record.py
that says test_unknown_record_raises_404
details = { | ||
"id": object_id, | ||
"resource_name": collection_id | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation should be like the one you did in kinto/core/resource/__init__.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits.
@@ -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'] |
There was a problem hiding this comment.
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?
"resource_name": collection_id | ||
} | ||
response = http_error(HTTPNotFound(), errno=ERRORS.MISSING_RESOURCE, | ||
details=details) |
There was a problem hiding this comment.
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.
This is good and ready to be merged! The failing test is unrelated to this PR:
|
Yes let me fix the docs build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty close! I don't see any tests for missing buckets.
Great! Congratz! |
Fixes #818
r? @Natim @glasserc @leplatrem
Please tell me if I'm wrong somewhere.