Allow asking for JSON using a query parameter #216

ashokdelphia · 2019-04-15T16:09:15Z

Thanks for this useful package. It made adding a richer health-check endpoint very straightforward.

I'm using it in a context where a number of different agents are hitting the endpoint, and not all are easily configurable to set arbitrary headers for their requests. For us, the HTML response is about 1000 bytes, but the JSON response is about 50, so I would like to make the default response JSON instead of HTML.

~~I tried to add reasonable test coverage, and simple docs, but please let me know if you'd rather handle those differently.~~

~~I stopped short of fully parsing the Accept header and interpreting the q-values, as that felt like it was likely overkill for this purpose.~~

This PR adds an alternative way to ask for a JSON response, by passing a json query parameter.

(See the comments for the change in direction here.)

…t format. As in the existing code, this doesn't try to fully parse the Accept header, but just uses simple substring matching within the header. As such, it doesn't take account of any q-values. If both text/html and application/json are present, it ignores the request's preferences and uses the configured preferred format.

It seems likely that this was the original intention.

codecov-io · 2019-04-15T16:12:17Z

Codecov Report

Merging #216 into master will increase coverage by 1.67%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   73.85%   75.52%   +1.67%     
==========================================
  Files          32       32              
  Lines         348      380      +32     
==========================================
+ Hits          257      287      +30     
- Misses         91       93       +2

Impacted Files	Coverage Δ
health_check/views.py	`96.87% <94.11%> (-3.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a69b6a...866a113. Read the comment docs.

codingjoe · 2019-04-23T10:50:21Z

hi @ashokdelphia

thanks for the contribution. You are not the first one, to approach us with that idea. Please read through #208 to see my concerns about this topic.
I am not willing to add a new setting for this feature. However I am willing to add support for a new GET attribute ?json=1, similar to how DRF does this.

It would be great to get this one the road, just give it a little more work.

Best
-Joe

…ON response.

ashokdelphia · 2019-04-23T14:04:53Z

Hi @codingjoe,

Sorry I didn't see the previous PR on a similar topic before making this.

I understand wanting to keep the number of configurable options down. I'd prefer having something that doesn't need a parameter, but it's a reasonable middle ground that will still make it possible for consumers of the endpoint where I can't set arbitrary headers.

Hopefully the latest changes are in line with what you're expecting. Please let me know if they need any adjustment.

@codingjoe

This follows @codingjoe's suggested approach, which I diverged from earlier.

codingjoe

@ashokdelphia I made some changes, mainly I renamed the GET attribute back to format=json, sorry for the back and forth.
Furthermore I change the code to actually respect the weight of the accept header media types as well as using the GET parameter as override, no matter what has been defined in the header.

Let me know if you see any problems. Otherwise, I will merge and release this feature. Thanks for all the hard work, much appreciated.

ashokdelphia · 2019-04-25T15:45:07Z

health_check/views.py

-        return self.render_to_response(context, status=status_code)
+        accept_header = request.META.get('HTTP_ACCEPT', '*/*')
+        for media in MediaType.parse_header(accept_header):
+            if media.mime_type in ['text/html', '	application/xhtml+xml', '*/*']:


It looks like there's a spurious tab in the second element here.

ashokdelphia

The new changes generally look good to me.

The spurious tab character is the only thing that looks clearly wonky to me.

Cheers.

ashokdelphia · 2019-04-25T15:58:10Z

tests/test_views.py

+    def test_from_string(self):
+        assert MediaType.from_string('*/*') == MediaType('*/*')
+        assert MediaType.from_string('*/*;0.9') == MediaType('*/*', 0.9)
+        assert MediaType.from_string('*/*;0.9') == MediaType('*/*', 0.9)


I don't think the format here is following how accept headers should be formatted.

I think it should be of the form foo/bar;q=0.9, where the quality a parameter q, not a bare value.

https://tools.ietf.org/html/rfc7231#section-5.3.2

It may also be worth having explicit tests for the various places you're allowed to have optional whitespace, starting with directly after the ;, which I think is reasonably common.

good lord you are right. This was wrong. I fixed it and added a bunch more tests. Please double check, before I release it.

The code tries to return text/html in this case, but there's a slip in how it allows it.

ashokdelphia · 2019-04-26T20:52:13Z

I adjusted the spurious whitespace, but while adding a failing test, I noticed that passing an accept header with an unrecognised type seems to cause a problem:

    def test_success_accept_unknown(self, client):
        class SuccessBackend(BaseHealthCheckBackend):
            def run_check(self):
                pass

        plugin_dir.reset()
        plugin_dir.register(SuccessBackend)
        response = client.get(self.url, HTTP_ACCEPT='application/octet-stream')
        assert response['content-type'] == 'text/html; charset=utf-8'

Gives:

===================================================== FAILURES =====================================================
_____________________________________ TestMainView.test_success_accept_unknown _____________________________________
tests/test_views.py:215: in test_success_accept_unknown
    response = client.get(self.url, HTTP_ACCEPT='application/octet-stream')
venv/lib/python3.7/site-packages/django/test/client.py:535: in get
    response = super().get(path, data=data, secure=secure, **extra)
venv/lib/python3.7/site-packages/django/test/client.py:347: in get
    **extra,
venv/lib/python3.7/site-packages/django/test/client.py:422: in generic
    return self.request(**r)
venv/lib/python3.7/site-packages/django/test/client.py:503: in request
    raise exc_value
venv/lib/python3.7/site-packages/django/core/handlers/exception.py:34: in inner
    response = get_response(request)
venv/lib/python3.7/site-packages/django/core/handlers/base.py:115: in _get_response
    response = self.process_exception_by_middleware(e, request)
venv/lib/python3.7/site-packages/django/core/handlers/base.py:113: in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
venv/lib/python3.7/site-packages/django/views/generic/base.py:71: in view
    return self.dispatch(request, *args, **kwargs)
venv/lib/python3.7/site-packages/django/views/generic/base.py:97: in dispatch
    return handler(request, *args, **kwargs)
venv/lib/python3.7/site-packages/django/views/decorators/cache.py:45: in _wrapped_view_func
    add_never_cache_headers(response)
venv/lib/python3.7/site-packages/django/utils/cache.py:252: in add_never_cache_headers
    patch_response_headers(response, cache_timeout=-1)
venv/lib/python3.7/site-packages/django/utils/cache.py:243: in patch_response_headers
    if not response.has_header('Expires'):
E   AttributeError: 'NoneType' object has no attribute 'has_header'

A bisect suggests it breaks in 317ad1c, but I haven't had time to dig in to why yet. I can take a look after the weekend, if it looks like a real problem and you don't have time before then.

codingjoe · 2019-04-27T10:28:29Z

@ashokdelphia there was no fallback. I added it and a test. I also added another commit to support an empty accept header.

codingjoe · 2019-04-27T11:48:59Z

Released in 3.10.0

Thx @ashokdelphia, really good work!

ashokdelphia added 3 commits April 15, 2019 11:31

Add assertions for the content-type in existing tests.

aa36878

Fix typo/grammar.

6d35afc

It seems likely that this was the original intention.

codingjoe assigned codingjoe and ashokdelphia Apr 23, 2019

ashokdelphia added 2 commits April 23, 2019 09:45

Remove ability to set a preferred response format.

105f3f4

Allow passing a query parameter as an alternative way to ask for a JS…

2fa45b5

…ON response.

ashokdelphia changed the title ~~Allow configuring default response format~~ Allow asking for JSON using a query parameter Apr 23, 2019

Fix lint.

8961aab

Pass a parameter called 'json' instead of 'format'.

a964002

This follows @codingjoe's suggested approach, which I diverged from earlier.

codingjoe force-pushed the allow-changing-default-response-format branch from bca85e1 to b880d53 Compare April 25, 2019 12:37

Rename json=1 to format=json and respect accept header order

317ad1c

codingjoe force-pushed the allow-changing-default-response-format branch from b880d53 to 317ad1c Compare April 25, 2019 12:39

codingjoe approved these changes Apr 25, 2019

View reviewed changes

ashokdelphia commented Apr 25, 2019

View reviewed changes

codingjoe and others added 4 commits April 25, 2019 23:40

Fix media type parsing, thx @ashokdelphia

e6445c9

Add failing test for accepting XHTML.

0f3c9e7

The code tries to return text/html in this case, but there's a slip in how it allows it.

Remove spurious tab within XHTML type.

bf7a8df

Fix lint.

b6b8554

codingjoe added 2 commits April 27, 2019 12:24

Add fallback response type, if none or unsupported specified

16952ed

Support empty accept header

866a113

codingjoe merged commit 575f811 into revsys:master Apr 27, 2019

ashokdelphia deleted the allow-changing-default-response-format branch April 29, 2019 12:27

ashokdelphia mentioned this pull request Apr 29, 2019

Accept header parsing rejects valid headers #220

Closed

ashokdelphia mentioned this pull request May 15, 2019

Improve accept header handling for unknown types #222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow asking for JSON using a query parameter #216

Allow asking for JSON using a query parameter #216

ashokdelphia commented Apr 15, 2019 •

edited

Loading

codecov-io commented Apr 15, 2019 •

edited

Loading

codingjoe commented Apr 23, 2019

ashokdelphia commented Apr 23, 2019

codingjoe left a comment

ashokdelphia Apr 25, 2019

ashokdelphia left a comment

ashokdelphia Apr 25, 2019

ashokdelphia Apr 25, 2019

codingjoe Apr 25, 2019

ashokdelphia commented Apr 26, 2019

codingjoe commented Apr 27, 2019

codingjoe commented Apr 27, 2019

Allow asking for JSON using a query parameter #216

Allow asking for JSON using a query parameter #216

Conversation

ashokdelphia commented Apr 15, 2019 • edited Loading

codecov-io commented Apr 15, 2019 • edited Loading

Codecov Report

codingjoe commented Apr 23, 2019

ashokdelphia commented Apr 23, 2019

codingjoe left a comment

Choose a reason for hiding this comment

ashokdelphia Apr 25, 2019

Choose a reason for hiding this comment

ashokdelphia left a comment

Choose a reason for hiding this comment

ashokdelphia Apr 25, 2019

Choose a reason for hiding this comment

ashokdelphia Apr 25, 2019

Choose a reason for hiding this comment

codingjoe Apr 25, 2019

Choose a reason for hiding this comment

ashokdelphia commented Apr 26, 2019

codingjoe commented Apr 27, 2019

codingjoe commented Apr 27, 2019

ashokdelphia commented Apr 15, 2019 •

edited

Loading

codecov-io commented Apr 15, 2019 •

edited

Loading