Added details attribute to 404 errors. #861

mansimarkaur · 2016-10-13T18:38:52Z

Fixes #818

Add documentation.
Add tests.
Add a changelog entry.
Add your name in the contributors file.

r? @Natim @glasserc @leplatrem
Please tell me if I'm wrong somewhere.

glasserc

Thanks for tackling this bug! This is a good first step, but I don't think it's complete yet.

glasserc · 2016-10-13T19:15:49Z

tests/core/test_views_errors.py

@@ -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.")


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

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.

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

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?

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

glasserc · 2016-10-13T19:18:39Z

kinto/core/testing.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):


It seems like we don't use this details variable anywhere here. Am I missing something?

glasserc · 2016-10-13T19:23:30Z

kinto/views/__init__.py

+        detail_dict = {
+                "id": collection_id,
+                "resource_name": "collection"
+            }


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?

Here "id" should have value of object_id and resource_name value of collection_id (ref #710)

@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?

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.

glasserc · 2016-10-13T19:30:30Z

tests/core/test_views_errors.py

        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"})


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.

leplatrem

Very good start!

leplatrem · 2016-10-14T10:24:51Z

kinto/views/__init__.py

+        detail_dict = {
+                "id": collection_id,
+                "resource_name": "collection"
+            }


Here "id" should have value of object_id and resource_name value of collection_id (ref #710)

leplatrem · 2016-10-14T10:25:46Z

tests/core/test_views_errors.py

@@ -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.")


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

leplatrem · 2016-10-14T10:27:42Z

CHANGELOG.rst

@@ -8,6 +8,7 @@ This document describes changes between each past release.

 **New features**

+- Added details attribute to 404 errors.


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)

leplatrem · 2016-10-14T10:28:42Z

kinto/core/resource/__init__.py

@@ -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 = {


nitpick: you can name this variable just details

leplatrem · 2016-10-14T10:30:57Z

kinto/core/resource/__init__.py

-                                  errno=ERRORS.INVALID_RESOURCE_ID)
+            detail_dict = {
+                "id": record_id,
+                "resource_name": "record"


Here resource_name should have value of self.request.current_resource_name

glasserc

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.

glasserc · 2016-10-14T20:46:19Z

tests/test_views_records.py

-        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'])


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.

Yes, makes more sense to add another test to check the 404 format. Thanks!

leplatrem · 2016-10-14T21:29:15Z

kinto/views/__init__.py

@@ -2,7 +2,8 @@
 import string

 from kinto.core.storage import generators, exceptions
-from pyramid import httpexceptions
+from pyramid.httpexceptions import (HTTPNotFound)


nitpick: superfluous parenthesis

leplatrem · 2016-10-14T21:29:29Z

kinto/core/testing.py

@@ -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)
-
-
+


nitpick: leave blank lines ;)

leplatrem · 2016-10-14T21:29:59Z

kinto/views/__init__.py

+        details = {
+                "id": object_id,
+                "resource_name": collection_id
+            }


nitpick: something looks wrong with the indentation here

leplatrem · 2016-10-14T21:35:44Z

tests/test_views_collections.py

-        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'])


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.

leplatrem

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!

leplatrem · 2016-10-17T10:33:43Z

tests/test_views_records.py

-        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')


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»

leplatrem · 2016-10-17T10:36:13Z

tests/test_views_buckets.py

-        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')


We already covered this in test_unknown_collection_raises_404 from test_views_collections. The assertion is not necessary here.

leplatrem · 2016-10-17T10:37:09Z

tests/test_views_buckets.py

-        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')


These assertions should be moved to a specific test in test_views_record.py that says test_unknown_record_raises_404

leplatrem · 2016-10-17T10:37:49Z

kinto/views/__init__.py

+        details = {
+                    "id": object_id,
+                    "resource_name": collection_id
+                }


The indentation should be like the one you did in kinto/core/resource/__init__.py

Natim

Few nits.

Natim · 2016-10-19T08:01:41Z

tests/test_views_buckets.py

@@ -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']


Why do you need to add self here?

Natim · 2016-10-19T08:02:02Z

kinto/views/__init__.py

+            "resource_name": collection_id
+        }
+        response = http_error(HTTPNotFound(), errno=ERRORS.MISSING_RESOURCE,
+                              details=details)


I think details can be on the same line.

leplatrem · 2016-10-19T08:02:42Z

This is good and ready to be merged!

The failing test is unrelated to this PR:

generating indices... genindex http-routingtable py-modindex

writing additional pages... index

Exception occurred:

  File "/home/travis/build/Kinto/kinto/.tox/docs/lib/python2.7/site-packages/sphinx/jinja2glue.py", line 179, in get_source

    raise TemplateNotFound(template)

TemplateNotFound: defindex.html

The full traceback has been saved in /tmp/sphinx-err-F2OoKS.log, if you want to report the issue to the developers.

Please also report this if it was a user error, so that a better error message can be provided next time.

A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!

Natim · 2016-10-19T08:03:44Z

Yes let me fix the docs build.

glasserc

Looks pretty close! I don't see any tests for missing buckets.

leplatrem · 2016-10-20T13:11:50Z

Great! Congratz!

almet added the in progress label Oct 13, 2016

glasserc requested changes Oct 13, 2016

View reviewed changes

leplatrem requested changes Oct 14, 2016

View reviewed changes

mansimarkaur added 5 commits October 14, 2016 20:06

added the details attribute to 404 errors

708cb9b

added details attribute to assertFormattedError

4190c44

modifies tests for format of 404 errors

7d9631a

updated changelog

f68d999

Corrected resource_name and restored 404 tests

e4fdefc

mansimarkaur force-pushed the details_to_404 branch from 09489b2 to e4fdefc Compare October 14, 2016 14:41

mansimarkaur added 5 commits October 14, 2016 22:12

Restored the tests and assertFormattedError method

3a1dc00

Added tests for details attribute

3394b9f

Updates changelog and docs

23e8ef1

Fixed flake8 errors

c732448

Fixed flake8 errors

bd3c5a6

glasserc requested changes Oct 14, 2016

View reviewed changes

leplatrem requested changes Oct 14, 2016

View reviewed changes

mansimarkaur added 3 commits October 15, 2016 04:29

fixed flake8

714fd27

Added stronger tests for details

a5e9291

More strong tests added

fb79170

leplatrem requested changes Oct 17, 2016

View reviewed changes

Modified and added tests

9e15b99

leplatrem approved these changes Oct 19, 2016

View reviewed changes

Natim reviewed Oct 19, 2016

View reviewed changes

glasserc requested changes Oct 19, 2016

View reviewed changes

Test for unknown bucket added

a47bba5

leplatrem merged commit f7cd8d0 into Kinto:master Oct 20, 2016

		@@ -8,6 +8,7 @@ This document describes changes between each past release.

		New features

		- Added details attribute to 404 errors.

Added details attribute to 404 errors. #861

Added details attribute to 404 errors. #861

Conversation

mansimarkaur commented Oct 13, 2016

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem commented Oct 19, 2016

Natim commented Oct 19, 2016

glasserc left a comment

Choose a reason for hiding this comment

leplatrem commented Oct 20, 2016