Re: Last-Modified HTTP header in GET responses

On Wed, Sep 4, 2019 at 2:47 PM Johan Corveleyn <jcorvel_at_gmail.com> wrote:
>
> On Wed, Sep 4, 2019 at 2:01 PM Branko Čibej <brane_at_apache.org> wrote:
> >
> > On 04.09.2019 11:44, Johan Corveleyn wrote:
> > > On Tue, Sep 3, 2019 at 8:01 AM Branko Čibej <brane_at_apache.org> wrote:
> > [...]
> > >>> Anyway, how about bringing this feature back in some form?
> > >>> - Revert r1724790?
> > >> This is clearly the simplest solution, but I have no idea what the
> > >> performance impact would be. From looking at the diff, my best guess is
> > >> that svn_fs_node_created_rev() and svn_fs_revision_prop() dominate.
> > >>
> > >>> - or only for "external GET urls"?
> > >>> - or only if some Apache directive is set?
> > >>>
> > >>> Thoughts?
> > >> I would prefer not to add yet another configuration knob to the server.
> > >> I agree that versioned-resource URLs are only interesting for DAV-aware
> > >> clients, and those clients already know how to check for modifications
> > >> without looking at Last-Modified. That would imply that adding the
> > >> header for external URLs is the right solution.
> > >>
> > >> -- Brane
> > > Thanks for your response, Brane.
> > >
> > > I think the below patch would do it (set the Last-Modified header only
> > > for "external URLs").
> > > It's basically a revert of r1724790 (and adding a test), plus wrapping
> > > the actual setting of Last-Modified inside the following condition:
> > >
> > > if ((resource->type == DAV_RESOURCE_TYPE_REGULAR)
> > > && (resource->info->repos_path == resource->info->uri_path->data))
> >
> > Yes, all our resources are "regular", I think. The second part of the
> > condition just says that if something is called "/foo/bar" in the
> > repository, it's being accessed as
> > "http://example.org/repos-root/foo/bar" and not some alias. Which is
> > exactly what you want.
>
> Perfect.
>
> > > const char *
> > > dav_svn__getetag(const dav_resource *resource, apr_pool_t *pool)
> > > {
> > > @@ -3219,6 +3263,25 @@ set_headers(request_rec *r, const dav_resource *re
> > > if (!resource->exists)
> > > return NULL;
> > >
> > > + if ((resource->type == DAV_RESOURCE_TYPE_REGULAR)
> > > + && (resource->info->repos_path == resource->info->uri_path->data))
> > > + {
> > > + /* Include Last-Modified header for 'external' GET requests
> > > + (i.e. requests to URI's not under /!svn), to support usage of an
> > > + SVN server as a file server, where the client needs timestamps
> > > + for instance to use as "last modification time" of files on disk. */
> > > + apr_time_t last_modified;
> > > +
> > > + last_modified = get_last_modified(resource);
> >
> > "Declaration is initialisation" please, wherever possible. So:
> >
> > const apr_time_t last_modified = get_last_modified(resource);
> >
> >
> > This also prevents you from accidentally modifying last_modified later on.
>
> Ack, will fix.
>
> > > + if (last_modified != -1)
> > > + {
> > > + /* Note the modification time for the requested resource, and
> > > + include the Last-Modified header in the response. */
> > > + ap_update_mtime(r, last_modified);
> > > + ap_set_last_modified(r);
> > > + }
> > > + }
> > > +
> > > /* generate our etag and place it into the output */
> > > apr_table_setn(r->headers_out, "ETag",
> > > dav_svn__getetag(resource, resource->pool));
> > > Index: subversion/tests/cmdline/mod_dav_svn_tests.py
> > > ===================================================================
> > > --- subversion/tests/cmdline/mod_dav_svn_tests.py (revision 1866345)
> > > +++ subversion/tests/cmdline/mod_dav_svn_tests.py (working copy)
> > > @@ -640,6 +640,29 @@ def propfind_propname(sbox):
> > > actual_response = r.read()
> > > verify_xml_response(expected_response, actual_response)
> > >
> > > +@SkipUnless(svntest.main.is_ra_type_dav)
> > > +def last_modified_header(sbox):
> > > + "verify 'Last-Modified' header on 'external' GETs"
> > > +
> > > + sbox.build(create_wc=False, read_only=True)
> > > +
> > > + headers = {
> > > + 'Authorization': 'Basic ' +
> > > base64.b64encode(b'jconstant:rayjandom').decode(),
> > > + }
> > > +
> > > + h = svntest.main.create_http_connection(sbox.repo_url)
> > > +
> > > + # GET /repos/iota
> > > + # Expect to see a Last-Modified header.
> > > + h.request('GET', sbox.repo_url + '/iota', None, headers)
> > > + r = h.getresponse()
> > > + if r.status != httplib.OK:
> > > + raise svntest.Failure('Request failed: %d %s' % (r.status, r.reason))
> > > + svntest.verify.compare_and_display_lines(None, 'Last-Modified',
> > > + svntest.verify.RegexOutput('.+'),
> > > + r.getheader('Last-Modified'))
> > > + r.read()
> >
> > Interesting approach ... and sure, it should work.
> >
> > But I'd also add a test to check that Last-Modified is *not* set on
> > responses to versioned-resource URLs. And maybe also check that the HEAD
> > method returns this header as well -- it should, and checking for that
> > won't hurt.
>
> OK, I'll add those tests too, should be easy enough.
>
> (BTW, I copied the test from the cache_control_header() test in the
> same file :-))
>
> Thanks for the review!
> --
> Johan

Committed in r1866425.

-- 
Johan