Refactor and improve media type negotiation #14

Changaco · 2016-07-20T12:11:06Z

The original idea was to move the negotiation to an algorithm function, but in the end I put it in Dynamic.render() instead. Either way moving it out of the simplates module should help us with AspenWeb/salon#13.

This PR adds a few tests and fixes a few issues, it should notably make https://github.com/liberapay/liberapay.com/blob/2c03dbfd75e9a747b309bfad64590a0e2aceca05/liberapay/__init__.py#L67-L74 obsolete.

Changaco · 2016-07-20T12:14:25Z

Here's the output of git rebase master -x 'rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test' so you can see the failing tests (sorry, no color):

Executing: rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test
d59c2d9
====================================================================================== test session starts ======================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/changaco/projets/aspen/aspen.py, inifile: 
plugins: cov-2.2.1
collected 709 items 

tests/dispatch_table_test.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_configuration.py ......................
tests/test_dispatcher.py ..........................................................................
tests/test_dynamic_resource.py ................................
tests/test_json_resource.py .....................
tests/test_mappings.py ...............
tests/test_renderers.py .
tests/test_request_processor.py ....
tests/test_resources.py ................
tests/test_resources_generics.py .......................
tests/test_simplates.py .........................
tests/test_unicode.py ...
tests/test_utils.py .......
tests/test_weird.py .

================================================================================== 709 passed in 4.17 seconds ===================================================================================
Executing: rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test
153742c
====================================================================================== test session starts ======================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/changaco/projets/aspen/aspen.py, inifile: 
plugins: cov-2.2.1
collected 709 items 

tests/dispatch_table_test.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_configuration.py ......................
tests/test_dispatcher.py ..........................................................................
tests/test_dynamic_resource.py ................................
tests/test_json_resource.py .....................
tests/test_mappings.py ...............
tests/test_renderers.py .
tests/test_request_processor.py ....
tests/test_resources.py ................
tests/test_resources_generics.py .......................
tests/test_simplates.py .........................
tests/test_unicode.py ...
tests/test_utils.py .......
tests/test_weird.py .

================================================================================== 709 passed in 4.11 seconds ===================================================================================
Executing: rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test
3cd0639
====================================================================================== test session starts ======================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/changaco/projets/aspen/aspen.py, inifile: 
plugins: cov-2.2.1
collected 710 items 

tests/dispatch_table_test.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_configuration.py ......................
tests/test_dispatcher.py ..........................................................................
tests/test_dynamic_resource.py .......................F.........
tests/test_json_resource.py .....................
tests/test_mappings.py ...............
tests/test_renderers.py .
tests/test_request_processor.py ....
tests/test_resources.py ................
tests/test_resources_generics.py .......................
tests/test_simplates.py .........................
tests/test_unicode.py ...
tests/test_utils.py .......
tests/test_weird.py .

=========================================================================================== FAILURES ============================================================================================
_________________________________________________________________________ test_treat_media_type_variants_as_equivalent __________________________________________________________________________

harness = <aspen.testing.Harness object at 0x7fce79bb8090>

    def test_treat_media_type_variants_as_equivalent(harness):
        _guess_type = mimetypes.guess_type
        mimetypes.guess_type = lambda url, **kw: ('application/x-javascript' if url.endswith('.js') else '', None)
        try:
            output = harness.simple(
                filepath='foobar.spt',
                contents="[---]\n[---] application/javascript\n[---] text/plain\n",
>               uripath='/foobar.js',
            )

tests/test_dynamic_resource.py:166: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
aspen/testing.py:85: in simple
    return self._hit('GET', uripath, querystring, **kw)
aspen/testing.py:94: in _hit
    , return_after=return_after
aspen/request_processor/__init__.py:62: in process
    , **kw
env/lib/python2.7/site-packages/algorithm.py:288: in run
    new_state = function(**deps.as_kwargs)
aspen/request_processor/algorithm.py:74: in render_resource
    return {'output': resource.render(state)}
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <aspen.http.resource.Dynamic object at 0x7fce79bb8990>
state = {'accept_header': None, 'algorithm': <algorithm.Algorithm object at 0x7fce79bb8390>, 'dispatch_result': DispatchResult..., wild...u'Found.', extra={u'accept': u'application/x-javascript'}, constrain_path=True), 'exception': NotFound(), ...}

    def render(self, state):
        """Render the resource with the given state as context, return Output.

            Before rendering we need to determine what type of content we're going
            to send back, by trying to find a match between the media types the
            client wants and the ones provided by the resource.

            The two sources for what the client wants are the extension in the
            request URL, and the Accept header. If the former fails to match we
            raise NotFound (404), if the latter fails we raise NegotiationFailure
            (406).

            Note that we don't always respect the `Accept` header (the spec says
            we can ignore it: <https://tools.ietf.org/html/rfc7231#section-5.3.2>).
            """
        available = self.available_types
        # When there is an extension in the path, the dispatcher gives us the
        # corresponding media type (or an empty string if unknown)
        accept = dispatch_accept = state['dispatch_result'].extra.get('accept')
        if accept is not None:
            # If the extension is unknown, raise NotFound
            if accept == '':
                raise NotFound()
            # Accept custom JSON media type
            if accept == 'application/json':
                accept += ',' + self.request_processor.media_type_json
        elif len(available) == 1:
            # If there's only one available type and no extension in the path,
            # then we ignore the Accept header
            return super(Dynamic, self).render(available[0], state)
        else:
            accept = state.get('accept_header')
        if accept:
            try:
                best_match = mimeparse.best_match(available, accept)
            except ValueError:
                # Unparseable accept header
                best_match = None
            if best_match:
                return super(Dynamic, self).render(best_match, state)
            elif best_match == '':
                if dispatch_accept is not None:
                    # e.g. client requested `/foo.json` but `/foo.spt` has no JSON page
>                   raise NotFound()
E                   NotFound

aspen/http/resource.py:87: NotFound
============================================================================= 1 failed, 709 passed in 4.12 seconds ==============================================================================
Executing: rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test
4f03188
====================================================================================== test session starts ======================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/changaco/projets/aspen/aspen.py, inifile: 
plugins: cov-2.2.1
collected 710 items 

tests/dispatch_table_test.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_configuration.py ......................
tests/test_dispatcher.py ..........................................................................
tests/test_dynamic_resource.py .................................
tests/test_json_resource.py .....................
tests/test_mappings.py ...............
tests/test_renderers.py .
tests/test_request_processor.py ....
tests/test_resources.py ................
tests/test_resources_generics.py .......................
tests/test_simplates.py .........................
tests/test_unicode.py ...
tests/test_utils.py .......
tests/test_weird.py .

================================================================================== 710 passed in 3.99 seconds ===================================================================================
Executing: rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test
048fce1
====================================================================================== test session starts ======================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/changaco/projets/aspen/aspen.py, inifile: 
plugins: cov-2.2.1
collected 712 items 

tests/dispatch_table_test.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_configuration.py ......................
tests/test_dispatcher.py ...........................................................................F
tests/test_dynamic_resource.py .................................
tests/test_json_resource.py .....................
tests/test_mappings.py ...............
tests/test_renderers.py .
tests/test_request_processor.py ....
tests/test_resources.py ................
tests/test_resources_generics.py .......................
tests/test_simplates.py .........................
tests/test_unicode.py ...
tests/test_utils.py .......
tests/test_weird.py .

=========================================================================================== FAILURES ============================================================================================
_____________________________________________________________________ test_extra_accept_header_is_empty_string_when_unknown _____________________________________________________________________

harness = <aspen.testing.Harness object at 0x7f086c558990>

    def test_extra_accept_header_is_empty_string_when_unknown(harness):
        dispatch_result = harness.simple(
            filepath='foo.spt',
            uripath='/foo.unknown-extension',
            return_after='dispatch_path_to_filesystem',
            want='dispatch_result',
        )
>       assert dispatch_result.extra['accept'] == ''
E       assert 'text/plain' == ''
E         - text/plain

tests/test_dispatcher.py:495: AssertionError
============================================================================= 1 failed, 711 passed in 4.11 seconds ==============================================================================
Executing: rm aspen/**/*.pyc tests/*.pyc; git describe --always; python2 build.py test
ea491e6
====================================================================================== test session starts ======================================================================================
platform linux2 -- Python 2.7.12, pytest-2.9.1, py-1.4.31, pluggy-0.3.1
rootdir: /home/changaco/projets/aspen/aspen.py, inifile: 
plugins: cov-2.2.1
collected 712 items 

tests/dispatch_table_test.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_configuration.py ......................
tests/test_dispatcher.py ............................................................................
tests/test_dynamic_resource.py .................................
tests/test_json_resource.py .....................
tests/test_mappings.py ...............
tests/test_renderers.py .
tests/test_request_processor.py ....
tests/test_resources.py ................
tests/test_resources_generics.py .......................
tests/test_simplates.py .........................
tests/test_unicode.py ...
tests/test_utils.py .......
tests/test_weird.py .

================================================================================== 712 passed in 4.25 seconds ===================================================================================
Successfully rebased and updated refs/heads/negotiation.

chadwhitacre · 2016-07-20T16:13:27Z

aspen/http/resource.py

-                # e.g. client requested `/foo.json` but `/foo.spt` has no JSON page
+
+        if dispatch_accept is not None:
+            # If the extension is unknown, raise NotFound


Was this our previous behavior? I might think that /foo.random-blah should get served by the default type rather than 404ing.

Hmm, I think the previous behavior was inconsistent, as dispatch_accept would be None, so accept would be set to accept_header, and thus it could: return a 406, or choose a type from the Accept header which that doesn't match the extension in the URL path, or fall back to the first available type.

I think we do want to return a 404. If the developer wants to use a custom extension they can register it in the mimetypes module.

I was thinking of the case where there's a dot in the final path part that's not intended to indicate a file extension. @kaguillera is questioning whether "you'd really have that situation." :)

E.g., if instead of https://tools.ietf.org/html/rfc7231#section-5.3.2 it were https://tools.ietf.org/html/rfc7231/5.3.2.

I guess it's possible to hit this corner case, but only if you're parsing request.path.raw yourself instead of using the features that are meant to allow capturing variable data from the path: wildcards and "rfc2396 params".

I'm thinking of law browsers such as http://www.ecode360.com/AM2259 or http://www.ecfr.gov/cgi-bin/text-idx?SID=a457db49113d5f8ca45f5252dd7dced0&mc=true&node=se31.3.1010_1100&rgn=div8. You might have on the filesystem:

www/%section.spt

Maybe you are parsing request.path['section'] further, or maybe you're passing it through to a database (SELECT * FROM sections WHERE section=%s).

I agree it's an edge case, and I'm okay with merging this PR as-is and revisiting if/when we hit this edge case in the future.

A wildcard like %section.spt shouldn't be affected, the dispatcher isn't supposed to set extra['accept'] in that case (maybe we need a test for that?).

Changaco · 2016-07-20T16:38:33Z

I'm not convinced that a single return statement is cleaner/clearer.

chadwhitacre · 2016-07-20T17:12:39Z

I'm not convinced that a single return statement is cleaner/clearer.

Are you convinced that it's at least as clear? :-)

chadwhitacre · 2016-07-20T18:10:19Z

I'm ready to merge this if you can live with the single-return version of render.

Changaco · 2016-07-20T18:15:39Z

Are you convinced that it's at least as clear? :-)

It seems to me that not returning immediately makes it less clear that the negotiation is finished. You have to read the rest of the code to figure out that it's not doing anything else. This is probably more evident for the first return that was removed than for the second.

I can live with the single return, I just don't see how it's better.

chadwhitacre · 2016-07-20T19:04:50Z

Here's a statement of the conventional wisdom that minimizing returns makes code easier to understand, because it reduces a source of variability (I think I used to own Code Complete, but I can't find it now to see their full argument). I sometimes use short-cut returns at the beginning of a long function, but I tend to think that if a function is large enough to suggest returning in the middle, then it's large enough to be refactored into multiple functions.

How about 5ab64d7?

chadwhitacre · 2016-07-20T19:14:06Z

(... and d374ce9? :)

Changaco · 2016-07-20T19:49:57Z

That actually looks worse to me. /o\ I found bugs by putting all the negotiation code together, I'm wary of splitting it back into multiple functions. Also, the negotiate_content_type you created doesn't actually use self, that's a sign that it should be either inlined or decorated with staticmethod or moved somewhere else (again, I'm wary of that).

The SO answer you linked to says that "In certain routines, once you know the answer, you want to return it to the calling routine immediately". I think that applies here, but I'd rather have the single return than the multiple functions.

chadwhitacre · 2016-07-20T20:24:06Z

Okay, then! :-)

I've pushed the last four commits off the branch. Ready to go?

Changaco · 2016-07-20T20:39:21Z

Oh, okay.

Changaco added 6 commits July 18, 2016 13:43

rationalize media type attributes of resources

d59c2d9

refactor media type negotiation

153742c

failing test for handling of media type variants

3cd0639

treat media/type and media/x-type as equivalents

4f03188

failing test for dispatch_result.extra['accept']

048fce1

stop using media_type_default in dispatcher

ea491e6

Changaco added the Review label Jul 20, 2016

chadwhitacre force-pushed the negotiation branch from 8d8be42 to b09cbac Compare July 20, 2016 15:36

Add a test for the case of the missing extension

5865e3e

chadwhitacre force-pushed the negotiation branch from b09cbac to 5865e3e Compare July 20, 2016 15:46

chadwhitacre reviewed Jul 20, 2016
View reviewed changes

chadwhitacre force-pushed the negotiation branch from d374ce9 to 5865e3e Compare July 20, 2016 20:24

Changaco merged commit b77037f into master Jul 20, 2016

Changaco deleted the negotiation branch July 20, 2016 20:39

chadwhitacre mentioned this pull request Jul 22, 2016

update some python APIs gratipay/gratipay.com#4099

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and improve media type negotiation #14

Refactor and improve media type negotiation #14

Changaco commented Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre Jul 20, 2016

Changaco Jul 20, 2016

chadwhitacre Jul 20, 2016

chadwhitacre Jul 20, 2016

Changaco Jul 20, 2016

chadwhitacre Jul 20, 2016

Changaco Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

Changaco commented Jul 20, 2016

Refactor and improve media type negotiation #14

Refactor and improve media type negotiation #14

Conversation

Changaco commented Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre Jul 20, 2016

Choose a reason for hiding this comment

Changaco Jul 20, 2016

Choose a reason for hiding this comment

chadwhitacre Jul 20, 2016

Choose a reason for hiding this comment

chadwhitacre Jul 20, 2016

Choose a reason for hiding this comment

Changaco Jul 20, 2016

Choose a reason for hiding this comment

chadwhitacre Jul 20, 2016

Choose a reason for hiding this comment

Changaco Jul 20, 2016

Choose a reason for hiding this comment

Changaco commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

Changaco commented Jul 20, 2016

chadwhitacre commented Jul 20, 2016

Changaco commented Jul 20, 2016