Merge lp:~vexo/bzr-webdav/bzr-webdav into lp:bzr-webdav

Thanks for fixing the problems you noticed in bzr-webdav.

The only issue I see is that you forgot to add tests for these bugs that are resolved by your patches. tests/dav_server.py looks like the most likely place to add them. Vincent Ladeuil <email address hidden> would be the authority on the subject as he wrote this plugin.

Tests added via sub-classing DAVServer to exercise the different server behavior to make sure we handle it gracefully. QuirkyDAVServer/QuirkyTestingDAVRequestHandler should also serve decently as a place to stick any future tests for similar compatibility fixes without requiring a lot of work or exploding the number of tests we generate even further.

@Reagan: Sorry for the delay and thanks for working on that *and* adding tests.

I have almost nothing to comment, nice job ;)

I think my only question is: did you encounter issues with an existing DAV server (which one ?) or did you get there only by reading the RFC ?

Concretely, the only risk I can see with your patch is that by changing the client behavior we may now fail against servers which worked before... But IIUC, the only case where that could happen is if a server refuse to do a MOVE (with Overwrite: 'T') if the target does not exist. Thoughts ?

For the record:

39 +class QuirkyTestingDAVRequestHandler(TestingDAVRequestHandler):
40 + """
41 + A variant of TesttingDAVRequestHandler implementing various quirky/slightly
42 + off-spec behaviors to test how gracefully we handle them.
43 + """

PEP8 (or rather the variant we use in the bzr eco-system) asks for a single-line docstring so:
class QuirkyTestingDAVRequestHandler(TestingDAVRequestHandler):
    """Slightly off-spec behaviors.

    A variant of TesttingDAVRequestHandler implementing various quirky/slightly
    off-spec behaviors to test how gracefully we handle them.
    """

Or something, the idea is the first line should be a one-liner, more stuff could be added after a blank line.

78 - self._raise_curl_http_error(abspath, response,

Ouch !

106 - self._raise_curl_http_error(curl, 'unable to delete')

OUCH !

Damn, that's some lack of test coverage you fixed there :-/

Would you mind adding some to make sure we properly catch that kind of error ?

I guess that's how you discovered the bugs you filed though so 1) sorry
about that, 2) I'd really like to know which server you use ;)

If you can't address the above, just let know and I'll do it while landing, again, good work !

I initially stumbled on the issues testing https://secure.cloudsafe.com, who appear to use http://www.webdavsystem.com/server with a custom back-end. The MOVE issue might be specific to Cloudsafe in this case, but I think the 200 on DELETE "quirk" (still within spec, just unusual) should apply to anything built off of that IT HIT WebDAV frontend. (I've actually had to abandon that project for the moment, as they also don't currently support the PUT-with-content-range appending technique, and as I think you know, without that performance is just abysmal.)

With regard to the MOVE behavior, any RFC-compliant server should be totally okay with us sending the Overwrite header whether it's required or not. When it comes to the poorly-written servers, this is just my intuition, but it seems likely that it should be much more common to make the mistake of looking for that Overwrite: T header when it isn't actually required, than to go out of their way to ensure that it isn't set when it doesn't matter. (Looking at the IT HIT server changelog, their front-end apparently used to have exactly this bug, but they claim to have fixed it.)

Though, since the currently packaged version of bzr-webdav doesn't work with the current package of bzr on Quantal+, I get the feeling there might not be too many other users out there anyways, to worry about breaking some other Exotic WebDAV implementation ;). I've tested it against apache2 with mod_dav/davfs without issues. Unfortunately, I don't have an IIS server handy to test probably the most common configuration :/.

I'll look into adding some test-cases to attempt to exercise the various server-failure code paths, since I think that's what was lacking to let those slip through, though I don't have much time during the workweek so you might beat me to it ;).

@Reagan, thanks for all the work creating the tests for the bugs you fixed.

> Though, since the currently packaged version of bzr-webdav doesn't work with
> the current package of bzr on Quantal+
[...]

Which versions of bzr and bzr-webdav won't work together?

> I'll look into adding some test-cases to attempt to exercise the various
> server-failure code paths, since I think that's what was lacking to let those
> slip through, though I don't have much time during the workweek so you might
> beat me to it ;).

I'll look forward to the server-failure test-cases.

Thanks for the detailed answer.

>>>>> Reagan Sanders <email address hidden> writes:

    > I initially stumbled on the issues testing https://secure.cloudsafe.com, who
    > appear to use http://www.webdavsystem.com/server with a custom back-end. The
    > MOVE issue might be specific to Cloudsafe in this case, but I think the 200
    > on DELETE "quirk" (still within spec, just unusual) should apply to anything
    > built off of that IT HIT WebDAV frontend. (I've actually had to abandon that
    > project for the moment, as they also don't currently support the
    > PUT-with-content-range appending technique, and as I think you know, without
    > that performance is just abysmal.)

Yeah, I started bzr-webdav when it was the only way for me to host bzr
branches, I've since migrated to ssh which is more secure and performs
better.

    > With regard to the MOVE behavior, any RFC-compliant server should be totally
    > okay with us sending the Overwrite header whether it's required or not. When
    > it comes to the poorly-written servers, this is just my intuition, but it
    > seems likely that it should be much more common to make the mistake of
    > looking for that Overwrite: T header when it isn't actually required,

/me nods

    > Though, since the currently packaged version of bzr-webdav doesn't
    > work with the current package of bzr on Quantal+, I get the
    > feeling there might not be too many other users out there anyways,
    > to worry about breaking some other Exotic WebDAV implementation
    > ;).

Yeah, it's always hard to know...

    > I've tested it against apache2 with mod_dav/davfs without
    > issues.

Yup, using the lp:bzr-local-test-server plugin I did so too, injecting
an apache2 dav server into bzr test suite and running:

  bzr selftest -v -s bp.webdav -s bt.per_transport

which should be enough to cover all webdav plugin uses.

    > Unfortunately, I don't have an IIS server handy to test probably
    > the most common configuration :/.

Same here :-/

    > I'll look into adding some test-cases to attempt to exercise the
    > various server-failure code paths, since I think that's what was
    > lacking to let those slip through,

Exactly, thanks for uncovering them !

    > though I don't have much time during the workweek so you might
    > beat me to it ;).

Right, let's race :)

But to be honest, this shouldn't block landing your patch so I'll try to
do that first.

On second thought, I agree with Vincent that we should land this patch first and worry about more test cases in a parallel effort. Thanks for the great work!
+1

This should have landed now, just a few comments for the record:

I've dive in the code again and was able to further clean up various areas:

77 if code not in (200, 201, 204):
78 - self._raise_curl_http_error(abspath, response,
79 + self._raise_http_error(abspath, response,
80 'expected 200, 201 or 204.')

This was really dead code so I completely removed it (_perform() itself will raise the proper error so the _raise_curl_http_error couldn't be reached anyway).

I've added a test_delete_replies_202 with associated infrastructure to bypass the server code and just exercise the client one.

I removed some other dead code and useless imports.

Once again, thanks for your work.