On 12.03.2015 14:19, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane_at_wandisco.com]
>> Sent: donderdag 12 maart 2015 12:06
>> To: dev_at_subversion.apache.org
>> Subject: Re: svn commit: r1665195 -
>> /subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> On 12.03.2015 05:36, Branko Čibej wrote:
>>> On 09.03.2015 12:39, rhuijben_at_apache.org wrote:
>>>> Author: rhuijben
>>>> Date: Mon Mar 9 11:39:39 2015
>>>> New Revision: 1665195
>>>>
>>>> URL: http://svn.apache.org/r1665195
>>>> Log:
>>>> In ra-serf: stop handling HTTP status 405 'Method forbidden' as a generic
>>>> out of date error. When locking this is the error we get when the resource
>>>> does not exist in HEAD, but in general it tells us that there is an
>>>> authorization problem.
>>>>
>>>> As the lock code has its own http status handling now, we can change
>>>> the error reported from the generic error handling code path.
>>>>
>>>> This should give users that accidentally use anonymous 'http' on a
>>>> server that uses 'https' for authorized operations a much better response,
>>>> than a simple out of date error (with the recommendation to update added
>>>> by their client).
>>>>
>>>> Note that in most cases the detailed error response from the server
>>>> is used instead of this generic error code for just the HTTP status.
>>>>
>>>> * subversion/libsvn_ra_serf/util.c
>>>> (svn_ra_serf__error_on_status): Tweak generic error for http status 405.
>>>>
>>>> Modified:
>>>> subversion/trunk/subversion/libsvn_ra_serf/util.c
>>>>
>>>> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
>>>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c
>> ?rev=1665195&r1=1665194&r2=1665195&view=diff
>> ================================================================
>> ==============
>>>> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
>>>> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Mon Mar 9 11:39:39
>> 2015
>>>> @@ -1762,8 +1762,8 @@ svn_ra_serf__error_on_status(serf_status
>>>> return svn_error_createf(SVN_ERR_FS_NOT_FOUND, NULL,
>>>> _("'%s' path not found"), path);
>>>> case 405:
>>>> - return svn_error_createf(SVN_ERR_FS_OUT_OF_DATE, NULL,
>>>> - _("'%s' is out of date"), path);
>>>> + return svn_error_createf(SVN_ERR_RA_DAV_FORBIDDEN, NULL,
>>>> + _("HTTP method is not allowed on '%s'"), path);
>>>> case 409:
>>>> return svn_error_createf(SVN_ERR_FS_CONFLICT, NULL,
>>>> _("'%s' conflicts"), path);
>>> -1, please revert this.
>>>
>>> * SVN_ERR_RA_DAV_FORBIDDEN is the wrong error code to use here.
>>> "Forbidden" is 403, not 405; the difference is significant.
>>> * Far from being a "better response," the new error message is
>>> arguably more confusing to users than the old one. Most Subversion
>>> users understand "out of date," but I bet most don't know what an
>>> HTTP method is.
>>> * Even if we decided that it's OK to confuse users with the details of
>>> the HTTP protocol, you'd have to tell them *which* method is not
>>> allowed.
>> I'm trying to understand how r1666096 makes things better. We're now
>> using SVN_ERR_RA_NOT_IMPLEMENTED to flag a 405 error from the server,
>> which, IMO, is even worse than before: this error code is used by the RA
>> loader when an URL scheme doesn't map to an RA implementation, and in
>> libsvn_client when a particular RA implementation doesn't support an RA
>> API (which should never happen anyway); neither of these have anything
>> whatsoever to do with the 405 HTTP status.
>>
>> Effectively, that commit only addresses my third point.
>>
>> Can we please start by understanding when, exactly, we can receive a 405
>> from the server? I can imagine several scenarios:
>>
>> * LOCK something not at HEAD (IIUC, this is handled separately);
>> * Write (PUT, MKCOL, etc.) to a repository on a read-only connection;
>> * Invalid server configuration (e.g., a missing method in a <Limit>
>> block);
>> * Non-conformant proxy between client and server.
> Yes...
>
> And when we receive a 405 from mod_dav, we actually receive a detailed server error, which contains the actual apr code of the backend.
>
> The original error handling was added in the initial development of ra_serf, when the developers were mostly interested in getting the tests running. Instead of fixing that they actually ignored the server generated error (as was very common then), they just handled error 405.
>
> Since then the problem of not detecting the real errors has been fixed, so the only case where we see it now is on servers and proxies that really don't implement this (or other) features... Returning 'out of date' (which is generally only found in detailed server reports), makes libsvn_client's commit processing trigger specific behavior that we never test for, because our server never returns this error.
>
>
> We should at least return something else than out of date... Preferably something that a user reading the error would understand (and doesn't make 'svn' recommend to simply update when you commit to a URL that isn't configured to allow commits, such as currently happens on http://serf.googlecode.com/svn/trunk when you try to commit without rebasing to https)
Is there any reason not to define a new error code for this condition?
Neither ...DAV_FORBIDDEN nor ...RA_NOT_IMPLEMENTED are appropriate.
-- Brane
Received on 2015-03-12 14:33:53 CET