Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100:
> Hi,
>
> For atomic-revprop, the client needs to know if the error was
> SVN_ERR_BAD_OLD_VALUE or not. For ra_local and ra_svn this is
> already done; this patch does it for DAV (with Neon).
>
>
> With mod_dav, we only have 2 ways to affect the 207 multistatus
> response from a failed PROPPATCH:
>
> - We can set the text that appears in <D:responsedescription>,
> including injecting arbitrary XML.
> - We can set the <D:status> value to a (numeric) HTTP error code.
There is also dav_svn__error_response_tag().
>
> The approach that's been discussed on-list and on IRC was to
> inject the error as XML inside <D:responsedescription>. I did
> actually implement that, only to discover that Neon completely
> rejects the XML in that case[1]. While we would obviously fix
> this in the 1.7 client code, it could break compatibility between
> the 1.7 server and 1.6 and older clients. The only way to handle
In other words, changing that table means changing the protocol, which
isn't backwards-compatible. Good call.
> this would be to introduce a new client capability, so the server
> could fall back to the old code for 1.6 clients. This means we'd
> have two code paths for error-handling... this gets very complex.
>
>
> However, there is a simpler way! The <D:status> element contains
> the HTTP error code, usually "500 Internal Server Error". So we
> could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE.
> I think of old_value_p as a precondition for the operation, a bit
> like "If-Modified-Since:", so I'd suggest "412 Precondition Failed".
> Note that generic DAV clients won't ever see this message, because
> they won't be sending old_value_p.
>
Fair enough. Are there currently any circumstances where
HTTP_PRECONDITION_FAILED would be raised? (e.g., by mod_dav)
As far as I can tell (by greping httpd sources), the answer is "No".
> This patch only does mod_dav_svn and Neon. I don't want to waste
> time doing both HTTP libraries if this patch is rejected. (Also,
> Serf is slightly harder because it doesn't parse <D:status> yet).
>
> The patch is against the atomic-revprop branch, and requires
> Patch 1 to be applied first. (It does apply to trunk, too).
>
> [[[
> Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
> code inside a 207 multi-status response.
>
> * subversion/mod_dav_svn/util.c:
> (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412.
>
> * subversion/libsvn_ra_neon/util.c:
> (multistatus_baton_t): New member contains_precondition_error.
> (end_207_element): Check for HTTP 412 status code and create
> a SVN_ERR_BAD_OLD_VALUE error if found.
>
> Patch by: Jon Foster <jon.foster_at_cabot.co.uk>
> ]]]
>
Tested it and it works fine:
[[[
CMD: atomic-ra-revprop-change http://localhost:8081/svn-test-work/repositories/prop_tests-34 0 flower "( 11 old_value_p 0 5 value 6 violet )" neon
CMD: /home/daniel/src/svn/atomic-revprop/subversion/tests/cmdline/atomic-ra-revprop-change http://localhost:8081/svn-test-work/repositories/prop_tests-34 0 flower ( 11 old_value_p 0 5 value 6 violet ) neon exited with 1
<TIME = 0.305840>
subversion/tests/cmdline/atomic-ra-revprop-change.c:156: (apr_err=175002)
subversion/libsvn_ra_neon/fetch.c:1210: (apr_err=175002)
atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent
subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008)
atomic-ra-revprop-change: At least one property change failed; repository is unchanged
subversion/libsvn_ra_neon/util.c:1550: (apr_err=125014)
subversion/libsvn_ra_neon/util.c:236: (apr_err=125014)
atomic-ra-revprop-change: Error setting property 'flower':
revprop 'flower' has unexpected value in filesystem
]]]
where 125014 is SVN_ERR_BAD_OLD_VALUE. :-)
I'll commit this once I have an ra_serf version too. Would you like to
write the ra_serf part of the patch, too?
Thanks!
Daniel
> Kind regards,
>
> Jon
>
>
> [1] More precisely, libsvn_ra_neon/util.c:wrapper_startelm_cb()
> raises a SVN_ERR_XML_MALFORMED "XML data was not well-formed"
> error if <D:responsedescription> contains any XML tags. See
> the ELEM_responsedescription row in multistatus_nesting_table[]
> in the same file - it explicitly states that if there's any
> XML tags inside <D:responsedescription> then the XML is invalid.
>
> Index: subversion/mod_dav_svn/util.c
> ===================================================================
> --- subversion/mod_dav_svn/util.c (revision 998620)
> +++ subversion/mod_dav_svn/util.c (working copy)
> @@ -107,6 +107,9 @@
> case SVN_ERR_FS_PATH_ALREADY_LOCKED:
> status = HTTP_LOCKED;
> break;
> + case SVN_ERR_BAD_OLD_VALUE:
> + status = HTTP_PRECONDITION_FAILED;
> + break;
> /* add other mappings here */
> }
>
> Index: subversion/libsvn_ra_neon/util.c
> ===================================================================
> --- subversion/libsvn_ra_neon/util.c (revision 998620)
> +++ subversion/libsvn_ra_neon/util.c (working copy)
> @@ -167,6 +167,7 @@
> svn_ra_neon__request_t *req;
> svn_stringbuf_t *description;
> svn_boolean_t contains_error;
> + svn_boolean_t contains_precondition_error;
> } multistatus_baton_t;
>
> /* Implements svn_ra_neon__startelm_cb_t. */
> @@ -231,9 +232,12 @@
> return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
> _("The request response contained at least "
> "one error"));
> + else if (b->contains_precondition_error)
> + return svn_error_create(SVN_ERR_BAD_OLD_VALUE, NULL,
> + b->description->data);
> else
> return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
> - b->description->data);
> + *b->description->data);
^^^
Unintentional change? (triggers compiler warning)
> }
> break;
>
> @@ -260,6 +264,10 @@
> else
> b->propstat_has_error = (status.klass != 2);
>
> + /* Handle "412 Precondition Failed" specially */
> + if (status.code == 412)
> + b->contains_precondition_error = TRUE;
> +
> free(status.reason_phrase);
> }
> else
Received on 2010-09-20 13:40:54 CEST