You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/03/09 12:39:40 UTC
svn commit: r1665195 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
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);
Re: svn commit: r1665195 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 12.03.2015 14:19, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane@wandisco.com]
>> Sent: donderdag 12 maart 2015 12:06
>> To: dev@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@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
RE: svn commit: r1665195 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: donderdag 12 maart 2015 12:06
> To: dev@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@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)
Bert
>
> Are there other cases?
>
> Of the above 4 cases, we already handle the first and have no control
> over the third and fourth. The question is, can we differentiate the
> second from the third and fourth cases? It would be useful to be able to
> tell the user that they're committing to a read-only repository.
>
> In any case, I still maintain that "method not allowed" is not a very
> useful message for the user.
>
> -- Brane
Re: svn commit: r1665195 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 12.03.2015 05:36, Branko Čibej wrote:
> On 09.03.2015 12:39, rhuijben@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.
Are there other cases?
Of the above 4 cases, we already handle the first and have no control
over the third and fourth. The question is, can we differentiate the
second from the third and fourth cases? It would be useful to be able to
tell the user that they're committing to a read-only repository.
In any case, I still maintain that "method not allowed" is not a very
useful message for the user.
-- Brane
Re: svn commit: r1665195 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 12 March 2015 at 07:36, Branko Čibej <br...@wandisco.com> wrote:
> On 09.03.2015 12:39, rhuijben@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.
>>
[...]
>
>> 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.
I agree: SVN_ERR_RA_DAV_FORBIDDEN is wrong error code. New error code
is needed for proper fix and as I far I see it was added in r1666379.
> * 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
I find it useful when error message contains some generic explanation
of problem and some raw details from on second line for diagnostic
purpose.
> 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.
>
+1.
--
Ivan Zhakov
Re: svn commit: r1665195 - /subversion/trunk/subversion/libsvn_ra_serf/util.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.03.2015 12:39, rhuijben@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.
-- Brane