Re: Last-Modified HTTP header in GET responses

Folks:

Can we get this fix back-ported into 1.10.x please? Breaking an LTS is
unfortunate as is waiting until the next LTS.

Cheers.

Doug

On Wed, Sep 4, 2019 at 8:03 PM Johan Corveleyn <jcorvel_at_gmail.com> wrote:

> 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
>

-- 
*DOUGLAS B ROBINSON* SENIOR PRODUCT MANAGER
T +1 925 396 1125
*E* doug.robinson_at_wandisco.com
-- 
* <http://wandisco.com/>*
**The *LiveData* Company
*Find out more 
*wandisco.com <http://wandisco.com/>*
 
<https://www.wandisco.com/liveanalytics>
THIS MESSAGE AND ANY ATTACHMENTS 
ARE CONFIDENTIAL, PROPRIETARY AND MAY BE PRIVILEGED
*
If this message was 
misdirected, WANdisco, Inc. and its subsidiaries, ("WANdisco") does not 
waive any confidentiality or privilege. If you are not the intended 
recipient, please notify us immediately and destroy the message without 
disclosing its contents to anyone. Any distribution, use or copying of this 
email or the information it contains by other than an intended recipient is 
unauthorized. The views and opinions expressed in this email message are 
the author's own and may not reflect the views and opinions of WANdisco, 
unless the author is authorized by WANdisco to express such views or 
opinions on its behalf. All email sent to or from this address is subject 
to electronic storage and review by WANdisco. Although WANdisco operates 
anti-virus programs, it does not accept responsibility for any damage 
whatsoever caused by viruses being passed.