Re: Last-Modified HTTP header in GET responses

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