views: If-Modified-Since bug

[nharraud]

Problem
There seems to be a test failing because of ContentNegotiatedMethodView. check_if_modified_since. An error was spotted on Travis (see below), but it appears randomly. The test fails because the request returns 200 instead of 304 when the resource has not changed.

Solution
This needs some checking but it seems to me that I suspect that this has something to do with dt = dt.replace(microsecond=0).

The method removes the milliseconds correctly as a service that sends a time stamp with milliseconds is not HTTP-compliant. Last-Modified MUST be sent as a HTTP-date which §3.3.1 specifies very clearly. (source)

However the test client is sending a date containing microseconds via Last-Modified
and reuses it in another request.

We could truncate the microseconds from the Last-Modified header in the test. We could also reject the request if the If-Modified-Since contains microseconds, or truncate the microseconds.

Travis error:

=================================== FAILURES ===================================
___________ test_content_negotiation_method_view_global_serializers ____________
app = <Flask 'testapp'>
    def test_content_negotiation_method_view_global_serializers(app):
        """Test ContentNegotiationMethodView. Test default serializers."""
        _subtest_content_negotiation_method_view(app, _ObjectItem, params={
            'serializers': {
>               'application/json': _obj_to_json_serializer,
            }
        })
tests/test_invenio_rest.py:203: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
app = <Flask 'testapp'>
content_negotiated_class = <class 'test_invenio_rest._ObjectItem'>
params = {'serializers': {'application/json': <function _obj_to_json_serializer at 0x7fb1622830d0>}}
    def _subtest_content_negotiation_method_view(app, content_negotiated_class,
                                                 params):
        """Test a given ContentNegotiatedMethodView subclass."""
        app.add_url_rule('/objects/<int:id>',
                         view_func=content_negotiated_class
                         .as_view(content_negotiated_class.view_name,
                                  **params))
    
        with app.test_client() as client:
            read_methods = [client.get, client.head]
            write_methods = [client.patch, client.put, client.post, client.delete]
            all_methods = read_methods + write_methods
            method_names = {
                'GET': client.get,
                'POST': client.post,
                'PUT': client.put,
                'PATCH': client.patch,
                'DELETE': client.delete,
            }
    
            def check_normal_response(res, method):
                if method != client.head:
                    parsed = json.loads(res.get_data(as_text=True))
                    expected = {'id': 1, 'method': parsed['method']}
                    assert parsed == expected
                    # check that the right method was called
                    assert method_names[parsed['method']] == method
                    assert res.content_type == 'application/json'
                assert res.status_code == 200
                # check that the ETag is correct
                assert unquote_etag(res.headers['ETag']) == \
                    unquote_etag(quote_etag('abc'))
    
            def check_304_response(res):
                assert res.status_code == 304
                # check that the ETag is correct
                assert unquote_etag(res.headers['ETag']) == \
                    unquote_etag(quote_etag('abc'))
    
            # check valid call with condition
            headers = [('Accept', 'application/json')]
            for method in all_methods:
                res = method('/objects/1', headers=headers)
                check_normal_response(res, method)
    
            # check valid call without condition
            for method in all_methods:
                res = method('/objects/1')
                check_normal_response(res, method)
    
            # check that non accepted mime types are not accepted
            headers = [('Accept', 'application/xml')]
            for method in all_methods:
                res = method('/objects/1', headers=headers)
                assert res.status_code == 406
    
            # check that errors are forwarded properly
            headers = [('Accept', 'application/json')]
            for method in all_methods:
                res = method('/objects/42', headers=headers)
                assert res.status_code == 404
    
            # check Matching If-None-Match
            headers_nonmatch_match = [('Accept', 'application/json'),
                                      ('If-None-Match', '"xyz", "abc"')]
            headers_nonmatch_star = [('Accept', 'application/json'),
                                     ('If-None-Match', '"xyz", "*"')]
            for method in read_methods:
                res = method('/objects/1', headers=headers_nonmatch_match)
                check_304_response(res)
                res = method('/objects/1', headers=headers_nonmatch_star)
                check_304_response(res)
    
            for method in write_methods:
                res = method('/objects/1', headers=headers_nonmatch_match)
                assert res.status_code == 412
                res = method('/objects/1', headers=headers_nonmatch_star)
                assert res.status_code == 412
    
            # check Matching If-None-Match with weak ETags
            headers_nonmatch_match = [('Accept', 'application/json'),
                                      ('If-None-Match', 'W/"xyz", W/"abc"')]
            headers_nonmatch_star = [('Accept', 'application/json'),
                                     ('If-None-Match', 'W/"xyz", "*"')]
            query_string_weak_etags = {'weak_etags': True}
1m            for method in read_methods:
                res = method('/objects/1',
                             headers=headers_nonmatch_match,
                             query_string=query_string_weak_etags)
                check_304_response(res)
                res = method('/objects/1',
                             headers=headers_nonmatch_star,
                             query_string=query_string_weak_etags)
                check_304_response(res)
    
            for method in write_methods:
                res = method('/objects/1',
                             headers=headers_nonmatch_match,
                             query_string=query_string_weak_etags)
                assert res.status_code == 412
                res = method('/objects/1',
                             headers=headers_nonmatch_star,
                             query_string=query_string_weak_etags)
                assert res.status_code == 412
    
            # check non matching If-None-Match
            headers = [('Accept', 'application/json'),
                       ('If-None-Match', '"xyz", "def"')]
            for method in all_methods:
                res = method('/objects/1', headers=headers)
                check_normal_response(res, method)
    
            # check non matching If-None-Match with weak ETags
            headers = [('Accept', 'application/json'),
                       ('If-None-Match', 'W/"xyz", W/"def"')]
            query_string_weak_etags = {'weak_etags': True}
            for method in all_methods:
                res = method('/objects/1',
                             headers=headers,
                             query_string=query_string_weak_etags)
                check_normal_response(res, method)
    
            # check matching If-Match
            headers = [('Accept', 'application/json'),
                       ('If-Match', '"abc", "def"')]
            for method in all_methods:
                res = method('/objects/1', headers=headers)
                check_normal_response(res, method)
    
            # check matching If-Match with weak ETags
            headers = [('Accept', 'application/json'),
                       ('If-Match', 'W/"abc", W/"def"')]
            query_string_weak_etags = {'weak_etags': True}
            for method in all_methods:
                res = method('/objects/1',
                             headers=headers,
                             query_string=query_string_weak_etags)
                check_normal_response(res, method)
    
            # check non matching If-Match
            headers = [('Accept', 'application/json'),
                       ('If-Match', '"xyz", "def"')]
            for method in all_methods:
                res = method('/objects/1', headers=headers)
[1m                assert res.status_code == 412
    
            # check non matching If-Match with weak ETags
            headers = [('Accept', 'application/json'),
                       ('If-Match', 'W/"xyz", W/"def"')]
            query_string_weak_etags = {'weak_etags': True}
            for method in all_methods:
                res = method('/objects/1',
                             headers=headers,
                             query_string=query_string_weak_etags)
                assert res.status_code == 412
    
            # check If-Modified-Since
            res = client.get('/objects/1')
            last_modified = res.headers['Last-Modified']
            res = client.get(
                '/objects/1', headers={'If-Modified-Since': last_modified})
>           assert res.status_code == 304
E           assert 200 == 304
E            +  where 200 = <Response streamed [200 OK]>.status_code
tests/test_invenio_rest.py:713: AssertionError

[lnielsen]

I'm I understand correctly that this is a bug in the test code?

[nharraud]

I didn't have time to check everything and couldn't reproduce the issue, but yes, from preliminary investigation it looks like a bug in the test code.