You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2011/03/01 16:09:07 UTC
[PATCH] HTTPv2 allow client to control transaction name in protocol
The V2 HTTP protocol replaces MKACTIVITY and the client supplied UUID
with POST and a server supplied name that corresponds to the
underlying FS transaction name. This means that FS transaction names
are now exposed in the HTTP protocol. For ordinary client-server
operation this is not a problem but it does cause difficulties for any
sort of proxy that tries to multiplex or replay HTTP requests.
The root of the problem is that transaction names depend on the
internal state of the repository, via the transaction sequence number.
Consider two nominally identical repository states, say two
repositories where one is a dump/load or svnsync copy of another, or a
single repository before and after a failed commit. These nominally
identical repository states may have different sequence numbers and
so will provide different responses to POST, and that will in turn
require different URLs in subsequent requests that make up the commit.
This is a change from the V1 protocol.
There are ways that proxies could work around this problem but they
involve building more knowledge of the Subversion protocol into the
proxy. An alternative is to change the V2 protocol to disconnect the
internal transaction name from the "visible transaction name". This
patch allows the client to send a UUID with POST, makes the server use
the UUID to construct the "visible transaction name" and makes the
server detect and map client-supplied visible names to the underlying FS
transaction names. The new behaviour is optional, if the client doesn't
send a UUID the server continues to respond with the FS transaction
name. The server uses the existind activities database to store the new
mapping; although these are not strictly DAV activities they are
essentially the same information.
The patch includes changes to make the client send the new header, but
that part is compile-time disabled by default as I don't expect normal
clients to use it. The "client" that sends the new UUID will usually
be the proxy.
[Disclaimer: I work for WANdisco and the WANdisco replicator is a proxy
that will have to handle the V2 protocol.]
Log and patch:
Allow HTTPv2 clients to specify that the FS transaction name should
not be returned by a "create-txn" POST request.
If the client sends an SVN-Txn-Name header it defines part of the
SVN-Txn-Name transaction name to be returned by the server, if no
header is sent the server will use the FS transaction name as before.
The server will identify client supplied "visible transaction names"
in subsequent requests and map them to the underlying FS transaction
name.
* subversion/include/svn_dav.h
(SVN_DAV_TXN_NAME_HEADER): Document new behaviour.
* notes/http-and-webdav/http-protocol-v2.txt: Update.
* subversion/mod_dav_svn/dav_svn.h
(DAV_SVN__VISIBLE_TXN_PREFIX): New.
(struct dav_svn_root): Add visible_txn_name member.
* subversion/mod_dav_svn/posts/create_txn.c
(dav_svn__post_create_txn): Get visible_txn_name from header if sent,
otherwise use the FS txn_name, store visible_txn_name:txn_name mapping
in the activity database if required.
* subversion/mod_dav_svn/version.c
(merge): Delete visible_txn_name:txn_name mapping after commit.
* subversion/mod_dav_svn/repos.c
(parse_txnstub_uri, parse_txnroot_uri): Map visible_txn_name to txn_name.
(remove_resource): Delete mapping when aborting the transaction.
* subversion/libsvn_ra_serf/commit.c
(setup_post_headers): New.
(open_root): Set header_delegate.
Index: notes/http-and-webdav/http-protocol-v2.txt
===================================================================
--- notes/http-and-webdav/http-protocol-v2.txt (revision 1075718)
+++ notes/http-and-webdav/http-protocol-v2.txt (working copy)
@@ -226,6 +226,13 @@
which can then be appended to the transaction stub and
transaction root stub as necessary.
+ - Optionally, the client may send a UUID with the POST request
+ and the the server response will base the "visible name" of the
+ transaction on that UUID rather than on FS transaction nmae.
+ The client still uses the returned transaction name in
+ subsequent requests, and the server maps it to the underlying
+ FS transaction name.
+
- Once the commit transaction is created, the client is free to
send write requests against transaction resources it constructs
itself. This eliminates the CHECKOUT requests, and also
Index: subversion/mod_dav_svn/dav_svn.h
===================================================================
--- subversion/mod_dav_svn/dav_svn.h (revision 1075718)
+++ subversion/mod_dav_svn/dav_svn.h (working copy)
@@ -52,6 +52,9 @@
/* a pool-key for the shared dav_svn_root used by autoversioning */
#define DAV_SVN__AUTOVERSIONING_ACTIVITY "svn-autoversioning-activity"
+/* used to distinguish client supplied TXN-NAME from FS supplied, this
+ is a string that is NEVER the start of an FS transaction name */
+#define DAV_SVN__VISIBLE_TXN_PREFIX "$"
/* dav_svn_repos
*
@@ -202,6 +205,17 @@
*/
const char *txn_name;
+ /* The visible transaction name returned to an HTTPv2 client and
+ used in subsequent requests. This may be equal to the FS
+ txn_name, or it may be derived from a name supplied by the client
+ in which case the activity database is used to map to the FS
+ transaction name.
+
+ PRIVATE resources that directly represent either a txn or
+ txn-root use this field.
+ */
+ const char *visible_txn_name;
+
/* If the root is part of a transaction, this contains the FS's transaction
handle. It may be NULL if this root corresponds to a specific revision.
It may also be NULL if we have not opened the transaction yet.
Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 1075718)
+++ subversion/mod_dav_svn/version.c (working copy)
@@ -1427,6 +1427,16 @@
svn_error_clear(serr);
serr = SVN_NO_ERROR;
}
+
+ /* HTTPv2 doesn't send DELETE after a successful MERGE so if
+ using the optional visible_txn_name mapping then delete it
+ here. */
+ if (source->info->root.visible_txn_name
+ && !strncmp(DAV_SVN__VISIBLE_TXN_PREFIX,
+ source->info->root.visible_txn_name,
+ sizeof(DAV_SVN__VISIBLE_TXN_PREFIX) - 1))
+ dav_svn__delete_activity(source->info->repos,
+ source->info->root.visible_txn_name);
}
else
{
Index: subversion/mod_dav_svn/posts/create_txn.c
===================================================================
--- subversion/mod_dav_svn/posts/create_txn.c (revision 1075718)
+++ subversion/mod_dav_svn/posts/create_txn.c (working copy)
@@ -37,6 +37,7 @@
ap_filter_t *output)
{
const char *txn_name;
+ const char *visible_txn_name;
dav_error *derr;
request_rec *r = resource->info->r;
@@ -45,9 +46,24 @@
resource->pool)))
return derr;
+ /* If the client supplied a transaction name then add the special
+ prefix and store a mapping from the prefixed client name to the
+ FS transaction name in the activity database. */
+ visible_txn_name = apr_table_get(r->headers_in, SVN_DAV_TXN_NAME_HEADER);
+ if (!visible_txn_name || !visible_txn_name[0])
+ visible_txn_name = txn_name;
+ else
+ visible_txn_name = apr_pstrcat(resource->pool, DAV_SVN__VISIBLE_TXN_PREFIX,
+ visible_txn_name, NULL);
+ if (!strncmp(DAV_SVN__VISIBLE_TXN_PREFIX, visible_txn_name,
+ sizeof(DAV_SVN__VISIBLE_TXN_PREFIX) - 1))
+ if ((derr = dav_svn__store_activity(resource->info->repos,
+ visible_txn_name, txn_name)))
+ return derr;
+
/* Build a "201 Created" response with header that tells the
client our new transaction's name. */
- apr_table_set(r->headers_out, SVN_DAV_TXN_NAME_HEADER, txn_name);
+ apr_table_set(r->headers_out, SVN_DAV_TXN_NAME_HEADER, visible_txn_name);
r->status = HTTP_CREATED;
return NULL;
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 1075718)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -486,7 +486,13 @@
comb->res.type = DAV_RESOURCE_TYPE_PRIVATE;
comb->priv.restype = DAV_SVN_RESTYPE_TXN_COLLECTION;
- comb->priv.root.txn_name = apr_pstrdup(comb->res.pool, path);
+ comb->priv.root.visible_txn_name = apr_pstrdup(comb->res.pool, path);
+ if (strncmp(DAV_SVN__VISIBLE_TXN_PREFIX, comb->priv.root.visible_txn_name,
+ sizeof(DAV_SVN__VISIBLE_TXN_PREFIX) - 1))
+ comb->priv.root.txn_name = comb->priv.root.visible_txn_name;
+ else
+ comb->priv.root.txn_name
+ = dav_svn__get_txn(comb->priv.repos, comb->priv.root.visible_txn_name);
return FALSE;
}
@@ -528,16 +534,24 @@
/* There's no slash character in our path. Assume it's just an
TXN_NAME pointing to the root path. That should be cool.
We'll just drop through to the normal case handling below. */
- comb->priv.root.txn_name = apr_pstrdup(comb->res.pool, path);
+ comb->priv.root.visible_txn_name = apr_pstrdup(comb->res.pool, path);
comb->priv.repos_path = "/";
}
else
{
- comb->priv.root.txn_name = apr_pstrndup(comb->res.pool, path,
- slash - path);
+ comb->priv.root.visible_txn_name = apr_pstrndup(comb->res.pool, path,
+ slash - path);
comb->priv.repos_path = slash;
}
+ if (strncmp(DAV_SVN__VISIBLE_TXN_PREFIX, comb->priv.root.visible_txn_name,
+ sizeof(DAV_SVN__VISIBLE_TXN_PREFIX) - 1))
+ comb->priv.root.txn_name = comb->priv.root.visible_txn_name;
+ else
+ comb->priv.root.txn_name
+ = dav_svn__get_txn(comb->priv.repos,
+ comb->priv.root.visible_txn_name);
+
return FALSE;
}
@@ -3783,11 +3797,15 @@
if (resource->type == DAV_RESOURCE_TYPE_PRIVATE
&& resource->info->restype == DAV_SVN_RESTYPE_TXN_COLLECTION)
{
- /* We'll assume that no activity was created to map to this
- transaction. */
- return dav_svn__abort_txn(resource->info->repos,
- resource->info->root.txn_name,
- resource->pool);
+ if (strncmp(DAV_SVN__VISIBLE_TXN_PREFIX,
+ resource->info->root.visible_txn_name,
+ sizeof(DAV_SVN__VISIBLE_TXN_PREFIX) - 1))
+ return dav_svn__abort_txn(resource->info->repos,
+ resource->info->root.txn_name,
+ resource->pool);
+ else
+ return dav_svn__delete_activity(resource->info->repos,
+ resource->info->root.visible_txn_name);
}
/* ### note that the parent was checked out at some point, and this
Index: subversion/include/svn_dav.h
===================================================================
--- subversion/include/svn_dav.h (revision 1075718)
+++ subversion/include/svn_dav.h (working copy)
@@ -145,7 +145,9 @@
* name of the Subversion transaction created by the request. It can
* then be appended to the transaction stub and transaction root stub
* for access to the properties and paths, respectively, of the named
- * transaction. (HTTP protocol v2 only) */
+ * transaction. Optionally, the client can send this header in the
+ * POST request to cause the response to be based on the client-supplied
+ * UUID. (HTTP protocol v2 only) */
#define SVN_DAV_TXN_NAME_HEADER "SVN-Txn-Name"
/**
Index: subversion/libsvn_ra_serf/commit.c
===================================================================
--- subversion/libsvn_ra_serf/commit.c (revision 1075718)
+++ subversion/libsvn_ra_serf/commit.c (working copy)
@@ -1141,6 +1141,19 @@
}
static svn_error_t *
+setup_post_headers(serf_bucket_t *headers,
+ void *baton,
+ apr_pool_t *pool)
+{
+#ifdef SVN_SERF_SEND_TXN_NAME
+ serf_bucket_headers_set(headers, SVN_DAV_TXN_NAME_HEADER,
+ svn_uuid_generate(pool));
+#endif
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
setup_delete_headers(serf_bucket_t *headers,
void *baton,
apr_pool_t *pool)
@@ -1308,6 +1321,8 @@
handler->body_type = SVN_SKEL_MIME_TYPE;
handler->body_delegate = create_txn_post_body;
handler->body_delegate_baton = NULL;
+ handler->header_delegate = setup_post_headers;
+ handler->header_delegate_baton = NULL;
handler->path = ctx->session->me_resource;
handler->conn = ctx->session->conns[0];
handler->session = ctx->session;
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> Wait a minute. The svn client won't ever send this. So I'm not sure
> what you mean by "client send path" here, or "requiring ... the use of
> a proxy".
>
> Your request for this feature is entirely predicated on a proxy. So of
> course you need a proxy. And the client is never sending this header.
By default a standard Subversion client won't send the header, but any
client could send it -- it's part of the protocol. The patch includes
some conditional code that causes the svn client to send the header.
That code is not normally compiled, but if it is compiled it allows
testing both the server behaviour to VTXN-NAME, and the svn client
behaviour when it receives VTXN-NAME, without requiring a proxy that
adds VTXN-NAME.
The patch has also been tested within WANdisco using their proxy.
>> And it may be more convenient for the proxy as well, but I've not looked
>> at any proxy implementations so I can't say for certain.
>
> What?! You must be looking at a proxy implementation. That is why you
> want this feature.
Not me, other people do that. People who work on WANdisco's proxy
described the problem in terms of the HTTP protocol.
As far as I am concerned it is more convenient for the server to return
the header, as I can test using a small amount of extra code to send
VTXN-NAME. If the server doesn't return the header then I need more
test code.
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Mar 9, 2011 at 06:43, Philip Martin <ph...@wandisco.com> wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Greg Stein <gs...@gmail.com> writes:
>>
>>> The proxy is the one altering the request/response flow. From the
>>> server's standpoint, the client (in this case, the proxy) already
>>> knows the name. Why should the server tell it what it already knows?
>>> Why should it put that useless information onto the wire?
>>
>> It's convenient to implement it that way, a very simple change in the
>> client send path allows testing the client recieve path. If the server
>> doesn't send the header then testing will require a more complicated
>> client change, or the use of a proxy that sends the header.
Wait a minute. The svn client won't ever send this. So I'm not sure
what you mean by "client send path" here, or "requiring ... the use of
a proxy".
Your request for this feature is entirely predicated on a proxy. So of
course you need a proxy. And the client is never sending this header.
> And it may be more convenient for the proxy as well, but I've not looked
> at any proxy implementations so I can't say for certain.
What?! You must be looking at a proxy implementation. That is why you
want this feature.
I don't get it. First you mention "a proxy wants to do this, so I need
<this>", and then you say "but I haven't looked at proxies".
This seems inconsistent.
-g
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Greg Stein <gs...@gmail.com> writes:
>
>> The proxy is the one altering the request/response flow. From the
>> server's standpoint, the client (in this case, the proxy) already
>> knows the name. Why should the server tell it what it already knows?
>> Why should it put that useless information onto the wire?
>
> It's convenient to implement it that way, a very simple change in the
> client send path allows testing the client recieve path. If the server
> doesn't send the header then testing will require a more complicated
> client change, or the use of a proxy that sends the header.
And it may be more convenient for the proxy as well, but I've not looked
at any proxy implementations so I can't say for certain.
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> The proxy is the one altering the request/response flow. From the
> server's standpoint, the client (in this case, the proxy) already
> knows the name. Why should the server tell it what it already knows?
> Why should it put that useless information onto the wire?
It's convenient to implement it that way, a very simple change in the
client send path allows testing the client recieve path. If the server
doesn't send the header then testing will require a more complicated
client change, or the use of a proxy that sends the header.
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Mar 9, 2011 at 04:40, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>...
>> Basically, the server is responding with somebody the requestor
>> already knows. So I wonder which approach is "best". It seems to be
>> kinda six-of-one/half-dozen-of-another. I suspect the server just
>> needs an if/else, so that might not be nearly the burden relative
>> modifying the proxy to add that header.
>
> The server needs an "if" in order to avoid sending TXN-NAME when it
> receives VTXN-NAME (and not sending TXN-NAME is the main reason for this
> change). If the server doesn't send VTXN-NAME then the response is not
> complete and the server is relying on the the proxy to make it complete.
> I think it is better if the server generates a complete response.
The proxy is the one altering the request/response flow. From the
server's standpoint, the client (in this case, the proxy) already
knows the name. Why should the server tell it what it already knows?
Why should it put that useless information onto the wire?
Cheers,
-g
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> On Tue, Mar 8, 2011 at 11:08, Philip Martin <ph...@wandisco.com> wrote:
>> VTXN operation:
>>
>> - client sends POST
>> - proxy adds SVN-VTxn-Name:UUID
>> - server creates transaction called TXN-NAME
>> - server replies SVN-VTxn-Name:UUID
>> - proxy passes
>
> or:
>
> - proxy adds: SVN-VTxn-Name:UUID
>
>> - client sends !svn/vtxn/UUID
>> - proxy passes
>> - server extracts UUID and maps to TXN-NAME
>
> So my question is whether to have the server do it, or have the proxy do it.
>
> Basically, the server is responding with somebody the requestor
> already knows. So I wonder which approach is "best". It seems to be
> kinda six-of-one/half-dozen-of-another. I suspect the server just
> needs an if/else, so that might not be nearly the burden relative
> modifying the proxy to add that header.
The server needs an "if" in order to avoid sending TXN-NAME when it
receives VTXN-NAME (and not sending TXN-NAME is the main reason for this
change). If the server doesn't send VTXN-NAME then the response is not
complete and the server is relying on the the proxy to make it complete.
I think it is better if the server generates a complete response.
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Greg Stein <gs...@gmail.com>.
On Tue, Mar 8, 2011 at 11:08, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>> And to be clear: the server *could* just remain silent, and the proxy
>> would insert the SVN-VTxn-Name header in the response back to the
>> client, right? Would that be an improvement/simplification?
>
> I don't think so.
>
> Normal operation:
>
> - client sends POST
> - server creates transaction called TXN-NAME
> - server replies SVN-Txn-Name:TXN-NAME
> - client send !svn/txn/TXN-NAME
> - server extracts TXN-NAME
>
> VTXN operation:
>
> - client sends POST
> - proxy adds SVN-VTxn-Name:UUID
> - server creates transaction called TXN-NAME
> - server replies SVN-VTxn-Name:UUID
> - proxy passes
or:
- proxy adds: SVN-VTxn-Name:UUID
> - client sends !svn/vtxn/UUID
> - proxy passes
> - server extracts UUID and maps to TXN-NAME
So my question is whether to have the server do it, or have the proxy do it.
Basically, the server is responding with somebody the requestor
already knows. So I wonder which approach is "best". It seems to be
kinda six-of-one/half-dozen-of-another. I suspect the server just
needs an if/else, so that might not be nearly the burden relative
modifying the proxy to add that header.
Cheers,
-g
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> And to be clear: the server *could* just remain silent, and the proxy
> would insert the SVN-VTxn-Name header in the response back to the
> client, right? Would that be an improvement/simplification?
I don't think so.
Normal operation:
- client sends POST
- server creates transaction called TXN-NAME
- server replies SVN-Txn-Name:TXN-NAME
- client send !svn/txn/TXN-NAME
- server extracts TXN-NAME
VTXN operation:
- client sends POST
- proxy adds SVN-VTxn-Name:UUID
- server creates transaction called TXN-NAME
- server replies SVN-VTxn-Name:UUID
- proxy passes
- client sends !svn/vtxn/UUID
- proxy passes
- server extracts UUID and maps to TXN-NAME
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Greg Stein <gs...@gmail.com>.
On Sat, Mar 5, 2011 at 13:16, Philip Martin <ph...@wandisco.com> wrote:
> Greg Stein <gs...@gmail.com> writes:
>
>>> If the client sends, or a proxy injects, an SVN-VTxn-Name header with
>>> the POST request it defines the transaction name to be returned to the
>>> client in the POST response. If the client recieves the new
>>> SVN-VTxn-Name header it uses that name in the new URIs in the requests
>>> that make up the commit.
>>
>> I don't understand why the *client* needs to read that header. The
>> base URI that the server returns already has the proper txn name,
>> right?
>>
>> Sending *to* the server creates the "client-provided" feature that you
>> want to retain. I just don't understand the other direction.
>
> By default the client doesn't send the header, it receives SVN-Txn-Name
> containing the server generated name and uses that in the txn/txr URIs.
> A client could send SVN-VTxn-Name but most clients will not. If a proxy
> inserts an SVN-VTxn-Name header into the request, the server replies
> with an SVN-VTxn-Name header containing the proxy generated name. The
> client then receives SVN-VTxn-Name containing the proxy generated name
> and uses that with the vtxn/vtxr URIs.
Ah. Gotcha. Thanks.
And to be clear: the server *could* just remain silent, and the proxy
would insert the SVN-VTxn-Name header in the response back to the
client, right? Would that be an improvement/simplification?
Cheers,
-g
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
>> If the client sends, or a proxy injects, an SVN-VTxn-Name header with
>> the POST request it defines the transaction name to be returned to the
>> client in the POST response. If the client recieves the new
>> SVN-VTxn-Name header it uses that name in the new URIs in the requests
>> that make up the commit.
>
> I don't understand why the *client* needs to read that header. The
> base URI that the server returns already has the proper txn name,
> right?
>
> Sending *to* the server creates the "client-provided" feature that you
> want to retain. I just don't understand the other direction.
By default the client doesn't send the header, it receives SVN-Txn-Name
containing the server generated name and uses that in the txn/txr URIs.
A client could send SVN-VTxn-Name but most clients will not. If a proxy
inserts an SVN-VTxn-Name header into the request, the server replies
with an SVN-VTxn-Name header containing the proxy generated name. The
client then receives SVN-VTxn-Name containing the proxy generated name
and uses that with the vtxn/vtxr URIs.
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Mar 4, 2011 at 07:29, Philip Martin <ph...@wandisco.com> wrote:
>...
> Extend Subversion's v2 HTTP protocol to include URIs that allow the
> client to define the transaction name visible in on the wire.
>
> If the client sends, or a proxy injects, an SVN-VTxn-Name header with
> the POST request it defines the transaction name to be returned to the
> client in the POST response. If the client recieves the new
> SVN-VTxn-Name header it uses that name in the new URIs in the requests
> that make up the commit.
I don't understand why the *client* needs to read that header. The
base URI that the server returns already has the proper txn name,
right?
Sending *to* the server creates the "client-provided" feature that you
want to retain. I just don't understand the other direction.
Cheers,
-g
Re: [PATCH v2] HTTPv2 allow client to control transaction name in
protocol
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/04/2011 07:29 AM, Philip Martin wrote:
> "C. Michael Pilato" <cm...@collab.net> writes:
>
>> Just a thought: Have you considered expanding the scope of the private
>> resource space rather than using the magic prefix hack? You could add
>> ".../!svn/vtxn/UUID" and ".../!svn/vtxr/UUID/..." to be alternate ways to
>> address transactions and transaction roots (the "v" there being a shortcut
>> for "virtual"). This is *effectively* the same approach as yours -- there's
>> a different prefix here. But the prefix is a clearly defined piece of the
>> protocol, not just some magic bit buried in mod_dav_svn's codebase.
>
> This looks promising, it's quite straightforward to implement. New
> log/patch follows:
Oh, that's *so* much more satisfying. Most of this kind of work is
copy-n-paste-n-tweak, I know, but I gave it a once-over anyway. Looks good
to me!
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH v2] HTTPv2 allow client to control transaction name in
protocol
Posted by Julian Foad <ju...@wandisco.com>.
Philip Martin wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > One thing that's not 100% clear from the protocol doc update is whether
> > the server sends *both* txn names in response, or just the "V" version.
> > If it sends both, then we need to specify whether the client has to use
> > the "V" version or can choose to use either one, or can mix accesses
> > arbitrarily using either. I can't think of any reason the server would
> > need to send both, so can we keep things simple by specifying that it
> > doesn't?
>
> The server sends one or the other, not both.
Good.
> > Additionally, this response will contain some new URL stub values:
> >
> > SVN-Rev-Stub: /REPOS-ROOT/!svn/rev
> > SVN-Rev-Root-Stub: /REPOS-ROOT/!svn/rvr
> > SVN-Txn-Stub: /REPOS-ROOT/!svn/txn
> > SVN-Txn-Root-Stub: /REPOS-ROOT/!svn/txr
> >
> > Should it send "vtxn" and "vtxr" stubs too, or instead?
>
> Those are not in the POST response.
Yes - that's the OPTIONS response.
> The server already sends SVN-Txn-Stub and SVN-Txn-Root-Stub during
> capabilities negotiation, the patch makes it send SVN-VTxn-Stub and
> SVN-VTxn-Root stub as well.
OK, good. Just doc tweaks then.
Thanks.
- Julian
> > (I don't
> > understand why the protocol needs to send these stubs explicitly at all:
> > is there some reason why these cannot just be constructed by the
> > client?)
>
Re: [PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
Julian Foad <ju...@wandisco.com> writes:
> One thing that's not 100% clear from the protocol doc update is whether
> the server sends *both* txn names in response, or just the "V" version.
> If it sends both, then we need to specify whether the client has to use
> the "V" version or can choose to use either one, or can mix accesses
> arbitrarily using either. I can't think of any reason the server would
> need to send both, so can we keep things simple by specifying that it
> doesn't?
The server sends one or the other, not both.
> Additionally, this response will contain some new URL stub values:
>
> SVN-Rev-Stub: /REPOS-ROOT/!svn/rev
> SVN-Rev-Root-Stub: /REPOS-ROOT/!svn/rvr
> SVN-Txn-Stub: /REPOS-ROOT/!svn/txn
> SVN-Txn-Root-Stub: /REPOS-ROOT/!svn/txr
>
> Should it send "vtxn" and "vtxr" stubs too, or instead?
Those are not in the POST response.
The server already sends SVN-Txn-Stub and SVN-Txn-Root-Stub during
capabilities negotiation, the patch makes it send SVN-VTxn-Stub and
SVN-VTxn-Root stub as well.
> (I don't
> understand why the protocol needs to send these stubs explicitly at all:
> is there some reason why these cannot just be constructed by the
> client?)
--
Philip
Re: [PATCH v2] HTTPv2 allow client to control transaction name in
protocol
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2011-03-04, Philip Martin wrote:
> Extend Subversion's v2 HTTP protocol to include URIs that allow the
> client to define the transaction name visible in on the wire.
>
> If the client sends, or a proxy injects, an SVN-VTxn-Name header with
> the POST request it defines the transaction name to be returned to the
> client in the POST response. If the client recieves the new
> SVN-VTxn-Name header it uses that name in the new URIs in the requests
> that make up the commit.
>
> By default the client will not send the new header.
>
> * subversion/mod_dav_svn/dav_svn.h
> (struct dav_svn_root): Add vtxn_name member.
> (dav_svn__get_vtxn_stub, dav_svn__get_vtxn_root_stub): New.
>
> * subversion/mod_dav_svn/mod_dav_svn.c
> (dav_svn__get_vtxn_stub, dav_svn__get_vtxn_root_stub): New.
>
> * subversion/mod_dav_svn/posts/create_txn.c
> (dav_svn__post_create_txn): Get vtxn_name from header, if sent, and
> store vtxn_name:txn_name mapping in the activity database.
>
> * subversion/mod_dav_svn/version.c
> (get_option): Add vtxn stub and vtxn root stub headers.
> (merge): Delete visible_txn_name:txn_name mapping after commit.
>
> * subversion/mod_dav_svn/repos.c
> (parse_vtxnstub_uri, parse_vtxnroot_uri): New.
> (special_subdirs): Add vtxn and vtxr.
> (remove_resource): Delete mapping when aborting the transaction.
>
> * subversion/libsvn_ra_serf/ra_serf.h
> (struct svn_ra_serf__session_t): Add vtxn_stub and vtxn_root_stub.
>
> * subversion/libsvn_ra_serf/commit.c
> (setup_post_headers): New.
> (post_headers_iterator_callback): Handle vtxn name header.
> (open_root): Set header_delegate.
>
> * subversion/libsvn_ra_serf/options.c
> (capabilities_headers_iterator_callback): Support vtxn stub and
> vtxn root stub headers.
>
> * subversion/libsvn_ra_neon/options.c
> (parse_capabilities): Support vtxn stub and vtxn root stub headers.
>
> * subversion/include/svn_dav.h
> (SVN_DAV_VTXN_STUB_HEADER, SVN_DAV_VTXN_ROOT_STUB_HEADER,
> SVN_DAV_VTXN_NAME_HEADER): New.
>
> * notes/http-and-webdav/http-protocol-v2.txt: Update.
>
> Index: notes/http-and-webdav/http-protocol-v2.txt
> ===================================================================
> --- notes/http-and-webdav/http-protocol-v2.txt (revision 1077861)
> +++ notes/http-and-webdav/http-protocol-v2.txt (working copy)
> @@ -128,6 +128,14 @@
> Various read- and write-type requests can be issued against
> these resources (MKCOL, PUT, PROPFIND, PROPPATCH, GET, etc.).
>
> + - alternate transaction resource (!svn/vtxn/VTXN-NAME)
> + alternate transaction root resource (!svn/vtxr/VTXN-NAME/[PATH])
> +
> + Alternative names for the transaction based on a virtual, or
> + visible, name supplied by the client when the transaction
> + was created. The client supplied name is optional, if not
> + supplied these resource names are not valid.
> +
> * Opening an RA session:
>
> ra_serf will send an OPTIONS request when creating a new
> @@ -220,12 +228,17 @@
>
> - no more need to "discover" the activity URI; !svn/act/ is gone.
>
> - - client no longer creates an activity UUID itself.
> + - client no longer needs to create an activity UUID itself.
>
> - instead, POST returns the name of the transaction it created,
> - which can then be appended to the transaction stub and
> - transaction root stub as necessary.
> + as TXN-NAME, which can then be appended to the transaction
> + stub and transaction root stub as necessary.
>
> + - if the client does choose to supply a UUID with the POST
> + request then the POST returns that UUID as VTXN-NAME, and the
> + client then uses that with the alternate transaction stub and
> + transaction root stub in subsequent requests.
One thing that's not 100% clear from the protocol doc update is whether
the server sends *both* txn names in response, or just the "V" version.
If it sends both, then we need to specify whether the client has to use
the "V" version or can choose to use either one, or can mix accesses
arbitrarily using either. I can't think of any reason the server would
need to send both, so can we keep things simple by specifying that it
doesn't?
Other parts of this doc may need updating:
* Opening an RA session:
[...]
Additionally, this response will contain some new URL stub values:
SVN-Rev-Stub: /REPOS-ROOT/!svn/rev
SVN-Rev-Root-Stub: /REPOS-ROOT/!svn/rvr
SVN-Txn-Stub: /REPOS-ROOT/!svn/txn
SVN-Txn-Root-Stub: /REPOS-ROOT/!svn/txr
Should it send "vtxn" and "vtxr" stubs too, or instead? (I don't
understand why the protocol needs to send these stubs explicitly at all:
is there some reason why these cannot just be constructed by the
client?)
* Commits
[...]
Specific new changes:
- The activity-UUID-to-Subversion-txn-name abstraction is gone.
We now expose the Subversion txn names explicitly through the
protocol.
[...]
- client no longer creates an activity UUID itself.
- Julian
[PATCH v2] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> Just a thought: Have you considered expanding the scope of the private
> resource space rather than using the magic prefix hack? You could add
> ".../!svn/vtxn/UUID" and ".../!svn/vtxr/UUID/..." to be alternate ways to
> address transactions and transaction roots (the "v" there being a shortcut
> for "virtual"). This is *effectively* the same approach as yours -- there's
> a different prefix here. But the prefix is a clearly defined piece of the
> protocol, not just some magic bit buried in mod_dav_svn's codebase.
This looks promising, it's quite straightforward to implement. New
log/patch follows:
Extend Subversion's v2 HTTP protocol to include URIs that allow the
client to define the transaction name visible in on the wire.
If the client sends, or a proxy injects, an SVN-VTxn-Name header with
the POST request it defines the transaction name to be returned to the
client in the POST response. If the client recieves the new
SVN-VTxn-Name header it uses that name in the new URIs in the requests
that make up the commit.
By default the client will not send the new header.
* subversion/mod_dav_svn/dav_svn.h
(struct dav_svn_root): Add vtxn_name member.
(dav_svn__get_vtxn_stub, dav_svn__get_vtxn_root_stub): New.
* subversion/mod_dav_svn/mod_dav_svn.c
(dav_svn__get_vtxn_stub, dav_svn__get_vtxn_root_stub): New.
* subversion/mod_dav_svn/posts/create_txn.c
(dav_svn__post_create_txn): Get vtxn_name from header, if sent, and
store vtxn_name:txn_name mapping in the activity database.
* subversion/mod_dav_svn/version.c
(get_option): Add vtxn stub and vtxn root stub headers.
(merge): Delete visible_txn_name:txn_name mapping after commit.
* subversion/mod_dav_svn/repos.c
(parse_vtxnstub_uri, parse_vtxnroot_uri): New.
(special_subdirs): Add vtxn and vtxr.
(remove_resource): Delete mapping when aborting the transaction.
* subversion/libsvn_ra_serf/ra_serf.h
(struct svn_ra_serf__session_t): Add vtxn_stub and vtxn_root_stub.
* subversion/libsvn_ra_serf/commit.c
(setup_post_headers): New.
(post_headers_iterator_callback): Handle vtxn name header.
(open_root): Set header_delegate.
* subversion/libsvn_ra_serf/options.c
(capabilities_headers_iterator_callback): Support vtxn stub and
vtxn root stub headers.
* subversion/libsvn_ra_neon/options.c
(parse_capabilities): Support vtxn stub and vtxn root stub headers.
* subversion/include/svn_dav.h
(SVN_DAV_VTXN_STUB_HEADER, SVN_DAV_VTXN_ROOT_STUB_HEADER,
SVN_DAV_VTXN_NAME_HEADER): New.
* notes/http-and-webdav/http-protocol-v2.txt: Update.
Index: notes/http-and-webdav/http-protocol-v2.txt
===================================================================
--- notes/http-and-webdav/http-protocol-v2.txt (revision 1077861)
+++ notes/http-and-webdav/http-protocol-v2.txt (working copy)
@@ -128,6 +128,14 @@
Various read- and write-type requests can be issued against
these resources (MKCOL, PUT, PROPFIND, PROPPATCH, GET, etc.).
+ - alternate transaction resource (!svn/vtxn/VTXN-NAME)
+ alternate transaction root resource (!svn/vtxr/VTXN-NAME/[PATH])
+
+ Alternative names for the transaction based on a virtual, or
+ visible, name supplied by the client when the transaction
+ was created. The client supplied name is optional, if not
+ supplied these resource names are not valid.
+
* Opening an RA session:
ra_serf will send an OPTIONS request when creating a new
@@ -220,12 +228,17 @@
- no more need to "discover" the activity URI; !svn/act/ is gone.
- - client no longer creates an activity UUID itself.
+ - client no longer needs to create an activity UUID itself.
- instead, POST returns the name of the transaction it created,
- which can then be appended to the transaction stub and
- transaction root stub as necessary.
+ as TXN-NAME, which can then be appended to the transaction
+ stub and transaction root stub as necessary.
+ - if the client does choose to supply a UUID with the POST
+ request then the POST returns that UUID as VTXN-NAME, and the
+ client then uses that with the alternate transaction stub and
+ transaction root stub in subsequent requests.
+
- Once the commit transaction is created, the client is free to
send write requests against transaction resources it constructs
itself. This eliminates the CHECKOUT requests, and also
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 1077861)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -626,6 +626,22 @@
}
+const char *
+dav_svn__get_vtxn_stub(request_rec *r)
+{
+ return apr_pstrcat(r->pool, dav_svn__get_special_uri(r), "/vtxn",
+ (char *)NULL);
+}
+
+
+const char *
+dav_svn__get_vtxn_root_stub(request_rec *r)
+{
+ return apr_pstrcat(r->pool, dav_svn__get_special_uri(r), "/vtxr",
+ (char *)NULL);
+}
+
+
svn_boolean_t
dav_svn__get_autoversioning_flag(request_rec *r)
{
Index: subversion/mod_dav_svn/dav_svn.h
===================================================================
--- subversion/mod_dav_svn/dav_svn.h (revision 1077861)
+++ subversion/mod_dav_svn/dav_svn.h (working copy)
@@ -202,6 +202,15 @@
*/
const char *txn_name;
+ /* The optional vtxn name supplied by an HTTPv2 client and
+ used in subsequent requests. This may be NULL if the client
+ is not using a vtxn name.
+
+ PRIVATE resources that directly represent either a txn or
+ txn-root use this field.
+ */
+ const char *vtxn_name;
+
/* If the root is part of a transaction, this contains the FS's transaction
handle. It may be NULL if this root corresponds to a specific revision.
It may also be NULL if we have not opened the transaction yet.
@@ -392,7 +401,13 @@
/* For accessing transaction properties (typically "!svn/txr") */
const char *dav_svn__get_txn_root_stub(request_rec *r);
+/* For accessing transaction resources (typically "!svn/vtxn") */
+const char *dav_svn__get_vtxn_stub(request_rec *r);
+/* For accessing transaction properties (typically "!svn/vtxr") */
+const char *dav_svn__get_vtxn_root_stub(request_rec *r);
+
+
/*** activity.c ***/
/* Create a new transaction based on HEAD in REPOS, setting *PTXN_NAME
Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 1077861)
+++ subversion/mod_dav_svn/version.c (working copy)
@@ -250,6 +250,12 @@
apr_table_set(r->headers_out, SVN_DAV_TXN_STUB_HEADER,
apr_pstrcat(resource->pool, repos_root_uri, "/",
dav_svn__get_txn_stub(r), (char *)NULL));
+ apr_table_set(r->headers_out, SVN_DAV_VTXN_ROOT_STUB_HEADER,
+ apr_pstrcat(resource->pool, repos_root_uri, "/",
+ dav_svn__get_vtxn_root_stub(r), (char *)NULL));
+ apr_table_set(r->headers_out, SVN_DAV_VTXN_STUB_HEADER,
+ apr_pstrcat(resource->pool, repos_root_uri, "/",
+ dav_svn__get_vtxn_stub(r), (char *)NULL));
}
return NULL;
@@ -1427,6 +1433,12 @@
svn_error_clear(serr);
serr = SVN_NO_ERROR;
}
+
+ /* HTTPv2 doesn't send DELETE after a successful MERGE so if
+ using the optional vtxn name mapping then delete it here. */
+ if (source->info->root.vtxn_name)
+ dav_svn__delete_activity(source->info->repos,
+ source->info->root.vtxn_name);
}
else
{
Index: subversion/mod_dav_svn/posts/create_txn.c
===================================================================
--- subversion/mod_dav_svn/posts/create_txn.c (revision 1077861)
+++ subversion/mod_dav_svn/posts/create_txn.c (working copy)
@@ -37,6 +37,7 @@
ap_filter_t *output)
{
const char *txn_name;
+ const char *vtxn_name;
dav_error *derr;
request_rec *r = resource->info->r;
@@ -47,7 +48,20 @@
/* Build a "201 Created" response with header that tells the
client our new transaction's name. */
- apr_table_set(r->headers_out, SVN_DAV_TXN_NAME_HEADER, txn_name);
+ vtxn_name = apr_table_get(r->headers_in, SVN_DAV_VTXN_NAME_HEADER);
+ if (vtxn_name && vtxn_name[0])
+ {
+ /* If the client supplied a vtxn name then store a mapping from
+ the client name to the FS transaction name in the activity
+ database. */
+ if ((derr = dav_svn__store_activity(resource->info->repos,
+ vtxn_name, txn_name)))
+ return derr;
+ apr_table_set(r->headers_out, SVN_DAV_VTXN_NAME_HEADER, vtxn_name);
+ }
+ else
+ apr_table_set(r->headers_out, SVN_DAV_TXN_NAME_HEADER, txn_name);
+
r->status = HTTP_CREATED;
return NULL;
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 1077861)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -491,7 +491,25 @@
return FALSE;
}
+static int
+parse_vtxnstub_uri(dav_resource_combined *comb,
+ const char *path,
+ const char *label,
+ int use_checked_in)
+{
+ /* format: !svn/vtxn/TXN_NAME */
+ if (parse_txnstub_uri(comb, path, label, use_checked_in))
+ return TRUE;
+
+ comb->priv.root.vtxn_name = comb->priv.root.txn_name;
+ comb->priv.root.txn_name = dav_svn__get_txn(comb->priv.repos,
+ comb->priv.root.vtxn_name);
+
+ return FALSE;
+}
+
+
static int
parse_txnroot_uri(dav_resource_combined *comb,
const char *path,
@@ -541,7 +559,25 @@
return FALSE;
}
+static int
+parse_vtxnroot_uri(dav_resource_combined *comb,
+ const char *path,
+ const char *label,
+ int use_checked_in)
+{
+ /* format: !svn/vtxr/TXN_NAME/[PATH] */
+ if (parse_txnroot_uri(comb, path, label, use_checked_in))
+ return TRUE;
+
+ comb->priv.root.vtxn_name = comb->priv.root.txn_name;
+ comb->priv.root.txn_name = dav_svn__get_txn(comb->priv.repos,
+ comb->priv.root.vtxn_name);
+
+ return FALSE;
+}
+
+
static int
parse_wrk_baseline_uri(dav_resource_combined *comb,
const char *path,
@@ -621,6 +657,8 @@
{ "rvr", parse_revroot_uri, 1, TRUE, DAV_SVN_RESTYPE_REVROOT_COLLECTION },
{ "txn", parse_txnstub_uri, 1, FALSE, DAV_SVN_RESTYPE_TXN_COLLECTION},
{ "txr", parse_txnroot_uri, 1, TRUE, DAV_SVN_RESTYPE_TXNROOT_COLLECTION},
+ { "vtxn", parse_vtxnstub_uri, 1, FALSE, DAV_SVN_RESTYPE_TXN_COLLECTION},
+ { "vtxr", parse_vtxnroot_uri, 1, TRUE, DAV_SVN_RESTYPE_TXNROOT_COLLECTION},
{ NULL } /* sentinel */
};
@@ -3783,11 +3821,13 @@
if (resource->type == DAV_RESOURCE_TYPE_PRIVATE
&& resource->info->restype == DAV_SVN_RESTYPE_TXN_COLLECTION)
{
- /* We'll assume that no activity was created to map to this
- transaction. */
- return dav_svn__abort_txn(resource->info->repos,
- resource->info->root.txn_name,
- resource->pool);
+ if (resource->info->root.vtxn_name)
+ return dav_svn__delete_activity(resource->info->repos,
+ resource->info->root.vtxn_name);
+ else
+ return dav_svn__abort_txn(resource->info->repos,
+ resource->info->root.txn_name,
+ resource->pool);
}
/* ### note that the parent was checked out at some point, and this
Index: subversion/include/svn_dav.h
===================================================================
--- subversion/include/svn_dav.h (revision 1077861)
+++ subversion/include/svn_dav.h (working copy)
@@ -133,6 +133,11 @@
* from a "txn root"). (HTTP protocol v2 only) */
#define SVN_DAV_TXN_STUB_HEADER "SVN-Txn-Stub"
+/** Companion to @c SVN_DAV_TXN_STUB_HEADER, used when a POST request
+ * returns @c SVN_DAV_VTXN_NAME_HEADER in response to a client
+ * supplied name. (HTTP protocol v2 only) */
+#define SVN_DAV_VTXN_STUB_HEADER "SVN-VTxn-Stub"
+
/** This header provides an opaque URI which represents the root
* directory of a Subversion transaction (revision-in-progress),
* similar to the concept of a "txn root" in the libsvn_fs API. The
@@ -141,6 +146,11 @@
* protocol v2 only) */
#define SVN_DAV_TXN_ROOT_STUB_HEADER "SVN-Txn-Root-Stub"
+/** Companion to @c SVN_DAV_TXN_ROOT_STUB_HEADER, used when a POST
+ * request returns @c SVN_DAV_VTXN_NAME_HEADER in response to a
+ * client supplied name. (HTTP protocol v2 only) */
+#define SVN_DAV_VTXN_ROOT_STUB_HEADER "SVN-VTxn-Root-Stub"
+
/** This header is used in the POST response to tell the client the
* name of the Subversion transaction created by the request. It can
* then be appended to the transaction stub and transaction root stub
@@ -148,6 +158,12 @@
* transaction. (HTTP protocol v2 only) */
#define SVN_DAV_TXN_NAME_HEADER "SVN-Txn-Name"
+/** This header is used in the POST request, to pass a client supplied
+ * alternative transaction name to the server, and in the the POST
+ * response, to tell the client that the alternative transaction
+ * resource names should be used. (HTTP protocol v2 only) */
+#define SVN_DAV_VTXN_NAME_HEADER "SVN-VTxn-Name"
+
/**
* @name Fulltext MD5 headers
*
Index: subversion/libsvn_ra_neon/ra_neon.h
===================================================================
--- subversion/libsvn_ra_neon/ra_neon.h (revision 1077861)
+++ subversion/libsvn_ra_neon/ra_neon.h (working copy)
@@ -146,6 +146,8 @@
const char *rev_root_stub; /* for accessing REV/PATH pairs */
const char *txn_stub; /* for accessing transactions (i.e. txnprops) */
const char *txn_root_stub; /* for accessing TXN/PATH pairs */
+ const char *vtxn_stub; /* for accessing transactions (i.e. txnprops) */
+ const char *vtxn_root_stub; /* for accessing TXN/PATH pairs */
/*** End HTTP v2 stuff ***/
Index: subversion/libsvn_ra_neon/options.c
===================================================================
--- subversion/libsvn_ra_neon/options.c (revision 1077861)
+++ subversion/libsvn_ra_neon/options.c (working copy)
@@ -253,6 +253,14 @@
{
ras->txn_stub = apr_pstrdup(ras->pool, val);
}
+ if ((val = ne_get_response_header(req, SVN_DAV_VTXN_ROOT_STUB_HEADER)))
+ {
+ ras->vtxn_root_stub = apr_pstrdup(ras->pool, val);
+ }
+ if ((val = ne_get_response_header(req, SVN_DAV_VTXN_STUB_HEADER)))
+ {
+ ras->vtxn_stub = apr_pstrdup(ras->pool, val);
+ }
}
Index: subversion/libsvn_ra_serf/ra_serf.h
===================================================================
--- subversion/libsvn_ra_serf/ra_serf.h (revision 1077861)
+++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
@@ -195,6 +195,8 @@
const char *rev_root_stub; /* for accessing REV/PATH pairs */
const char *txn_stub; /* for accessing transactions (i.e. txnprops) */
const char *txn_root_stub; /* for accessing TXN/PATH pairs */
+ const char *vtxn_stub; /* for accessing transactions (i.e. txnprops) */
+ const char *vtxn_root_stub; /* for accessing TXN/PATH pairs */
/*** End HTTP v2 stuff ***/
Index: subversion/libsvn_ra_serf/commit.c
===================================================================
--- subversion/libsvn_ra_serf/commit.c (revision 1077861)
+++ subversion/libsvn_ra_serf/commit.c (working copy)
@@ -1141,6 +1141,21 @@
}
static svn_error_t *
+setup_post_headers(serf_bucket_t *headers,
+ void *baton,
+ apr_pool_t *pool)
+{
+#ifdef SVN_SERF_SEND_VTXN_NAME
+ /* Enable this to exercise the VTXN-NAME code based on a client
+ supplied transaction name. */
+ serf_bucket_headers_set(headers, SVN_DAV_VTXN_NAME_HEADER,
+ svn_uuid_generate(pool));
+#endif
+
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
setup_delete_headers(serf_bucket_t *headers,
void *baton,
apr_pool_t *pool)
@@ -1254,6 +1269,18 @@
prc_cc->txn_root_url =
svn_path_url_add_component2(sess->txn_root_stub, val, prc_cc->pool);
}
+
+ if (svn_cstring_casecmp(key, SVN_DAV_VTXN_NAME_HEADER) == 0)
+ {
+ /* Build out vtxn and vtxn-root URLs using the vtxn name we're
+ given, and store the whole lot of it in the commit context. */
+ prc_cc->txn_name = apr_pstrdup(prc_cc->pool, val);
+ prc_cc->txn_url =
+ svn_path_url_add_component2(sess->vtxn_stub, val, prc_cc->pool);
+ prc_cc->txn_root_url =
+ svn_path_url_add_component2(sess->vtxn_root_stub, val, prc_cc->pool);
+ }
+
return 0;
}
@@ -1308,6 +1335,8 @@
handler->body_type = SVN_SKEL_MIME_TYPE;
handler->body_delegate = create_txn_post_body;
handler->body_delegate_baton = NULL;
+ handler->header_delegate = setup_post_headers;
+ handler->header_delegate_baton = NULL;
handler->path = ctx->session->me_resource;
handler->conn = ctx->session->conns[0];
handler->session = ctx->session;
Index: subversion/libsvn_ra_serf/options.c
===================================================================
--- subversion/libsvn_ra_serf/options.c (revision 1077861)
+++ subversion/libsvn_ra_serf/options.c (working copy)
@@ -373,6 +373,14 @@
{
orc->session->txn_root_stub = apr_pstrdup(orc->session->pool, val);
}
+ else if (svn_cstring_casecmp(key, SVN_DAV_VTXN_STUB_HEADER) == 0)
+ {
+ orc->session->vtxn_stub = apr_pstrdup(orc->session->pool, val);
+ }
+ else if (svn_cstring_casecmp(key, SVN_DAV_VTXN_ROOT_STUB_HEADER) == 0)
+ {
+ orc->session->vtxn_root_stub = apr_pstrdup(orc->session->pool, val);
+ }
else if (svn_cstring_casecmp(key, SVN_DAV_REPOS_UUID_HEADER) == 0)
{
orc->session->uuid = apr_pstrdup(orc->session->pool, val);
--
Philip
Re: [PATCH] HTTPv2 allow client to control transaction name in protocol
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/01/2011 01:56 PM, C. Michael Pilato wrote:
> On 03/01/2011 01:12 PM, Philip Martin wrote:
>> "C. Michael Pilato" <cm...@collab.net> writes:
>>
>>> Just a thought: Have you considered expanding the scope of the private
>>> resource space rather than using the magic prefix hack? You could add
>>> ".../!svn/vtxn/UUID" and ".../!svn/vtxr/UUID/..." to be alternate ways to
>>> address transactions and transaction roots (the "v" there being a shortcut
>>> for "virtual"). This is *effectively* the same approach as yours -- there's
>>> a different prefix here. But the prefix is a clearly defined piece of the
>>> protocol, not just some magic bit buried in mod_dav_svn's codebase.
>>
>> I'll have a think about that. One aim is that the proxy can be as dumb
>> as possible about the Subversion protocol, so that it doesn't have to
>> rewrite all commit requests. If the client doesn't send the vtxn/vtxr
>> URLs the proxy has to do more work.
>>
>> Another thing about exposing the transaction name in the protocol is
>> that it is much more predictable than a UUID. Temporary files with
>> predictable names can be a security issue, are predictable transaction
>> names a security issue?
>
> I want to say that we've had this discussion on-list before, but I might be
> remembering something else. I'll see if I can find any prior chatter about
> this.
Found it:
http://svn.haxx.se/dev/archive-2009-02/0097.shtml
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] HTTPv2 allow client to control transaction name in protocol
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/01/2011 01:12 PM, Philip Martin wrote:
> "C. Michael Pilato" <cm...@collab.net> writes:
>
>> Just a thought: Have you considered expanding the scope of the private
>> resource space rather than using the magic prefix hack? You could add
>> ".../!svn/vtxn/UUID" and ".../!svn/vtxr/UUID/..." to be alternate ways to
>> address transactions and transaction roots (the "v" there being a shortcut
>> for "virtual"). This is *effectively* the same approach as yours -- there's
>> a different prefix here. But the prefix is a clearly defined piece of the
>> protocol, not just some magic bit buried in mod_dav_svn's codebase.
>
> I'll have a think about that. One aim is that the proxy can be as dumb
> as possible about the Subversion protocol, so that it doesn't have to
> rewrite all commit requests. If the client doesn't send the vtxn/vtxr
> URLs the proxy has to do more work.
>
> Another thing about exposing the transaction name in the protocol is
> that it is much more predictable than a UUID. Temporary files with
> predictable names can be a security issue, are predictable transaction
> names a security issue?
I want to say that we've had this discussion on-list before, but I might be
remembering something else. I'll see if I can find any prior chatter about
this.
If there's reason to believe that the current approach is dangerous, then by
all means let's dispose of it, fall back to client-provided transaction
aliases, and move along! I don't have a particularly strong opinion on the
matter save for one: don't make the protocol unnecessarily complex!
Necessary complexity? Now that's a different matter altogether.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> Just a thought: Have you considered expanding the scope of the private
> resource space rather than using the magic prefix hack? You could add
> ".../!svn/vtxn/UUID" and ".../!svn/vtxr/UUID/..." to be alternate ways to
> address transactions and transaction roots (the "v" there being a shortcut
> for "virtual"). This is *effectively* the same approach as yours -- there's
> a different prefix here. But the prefix is a clearly defined piece of the
> protocol, not just some magic bit buried in mod_dav_svn's codebase.
I'll have a think about that. One aim is that the proxy can be as dumb
as possible about the Subversion protocol, so that it doesn't have to
rewrite all commit requests. If the client doesn't send the vtxn/vtxr
URLs the proxy has to do more work.
Another thing about exposing the transaction name in the protocol is
that it is much more predictable than a UUID. Temporary files with
predictable names can be a security issue, are predictable transaction
names a security issue?
Could a malicious client guess a transaction name and make changes that
would subsequently be committed by the transaction "owner"? I think
auth checks happen when writing to the transaction, so the malicious
client can only make changes that would be allowed by auth.
However the pre/post-commit hooks only run at commit (http MERGE), so
the malicious clients changes would go through these with the
transaction owners credentials.
--
Philip
Re: [PATCH] HTTPv2 allow client to control transaction name in protocol
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/01/2011 11:50 AM, Philip Martin wrote:
> "C. Michael Pilato" <cm...@collab.net> writes:
>
>> On 03/01/2011 10:09 AM, Philip Martin wrote:
>>> Index: subversion/mod_dav_svn/dav_svn.h
>>> ===================================================================
>>> --- subversion/mod_dav_svn/dav_svn.h (revision 1075718)
>>> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
>>> @@ -52,6 +52,9 @@
>>> /* a pool-key for the shared dav_svn_root used by autoversioning */
>>> #define DAV_SVN__AUTOVERSIONING_ACTIVITY "svn-autoversioning-activity"
>>>
>>> +/* used to distinguish client supplied TXN-NAME from FS supplied, this
>>> + is a string that is NEVER the start of an FS transaction name */
>>> +#define DAV_SVN__VISIBLE_TXN_PREFIX "$"
>>
>> Whoops! Technically, mod_dav_svn hasn't access to the knowledge required to
>> make this assertion. Who says a transaction's name can't start with a
>> dollar sign?
>
> I suppose I should make the comment more accurate. The prefix
> determines whether the visible:internal mapping will apply. If a
> internal transaction's name starts with the prefix then an identity
> mapping will get applied. That still works but the overhead of going
> through the activity database makes it marginally less efficient.
My point was merely that we don't document anywhere the limitations of a
Subversion filesystem transaction's naming scheme, therefore it is not
possible for a dependent library to declare that any given string falls
outside that naming scheme.
BUT. I was wrong. I just found in svn_fs.h the following:
* Transaction names are guaranteed to contain only letters (upper-
* and lower-case), digits, `-', and `.', from the ASCII character
* set.
Given this, it's perfectly fine for mod_dav_svn to make such declarations.
> There is a demand for WANdisco's replicator. It may be the only product
> at the moment but it's possible that somebody will write an open source
> product that also does multiplexing and/or replay. They will then face
> the same problem that WANdisco does now.
I think you've missed the point, Philip. This isn't about WANdisco or its
(arguably rather useful) product. It's about Subversion. It's about
reversing steps we intentionally took to de-obfuscate the way that
repository transactions are referred to by the Subversion client. It's
about taking a clean protocol definition:
".../!svn/txn/TXN-NAME/" and ".../!svn/txr/TXN-NAME/...", where TXN-NAME
is a Subversion filesystem transaction name.
and muddying it up:
".../!svn/txn/SOME-STRING/" and ".../!svn/txr/SOME-STRING/...", where
SOME-STRING is either a magic token (identified by a prefix consisting
of characters not legal in a Subversion filesystem transaction name)
which the server knows how to map to a transaction name, or is just a
transaction name itself.
Now, maybe we were short-sighted to take these steps in the first place.
Maybe it's better if the client always refers to transactions through some
layer of indirection such as the activity database subsystem. I don't know.
I just don't want to grumble every time I think about the HTTPv2 spec.
Just a thought: Have you considered expanding the scope of the private
resource space rather than using the magic prefix hack? You could add
".../!svn/vtxn/UUID" and ".../!svn/vtxr/UUID/..." to be alternate ways to
address transactions and transaction roots (the "v" there being a shortcut
for "virtual"). This is *effectively* the same approach as yours -- there's
a different prefix here. But the prefix is a clearly defined piece of the
protocol, not just some magic bit buried in mod_dav_svn's codebase.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] HTTPv2 allow client to control transaction name in protocol
Posted by Philip Martin <ph...@wandisco.com>.
"C. Michael Pilato" <cm...@collab.net> writes:
> On 03/01/2011 10:09 AM, Philip Martin wrote:
>> Index: subversion/mod_dav_svn/dav_svn.h
>> ===================================================================
>> --- subversion/mod_dav_svn/dav_svn.h (revision 1075718)
>> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
>> @@ -52,6 +52,9 @@
>> /* a pool-key for the shared dav_svn_root used by autoversioning */
>> #define DAV_SVN__AUTOVERSIONING_ACTIVITY "svn-autoversioning-activity"
>>
>> +/* used to distinguish client supplied TXN-NAME from FS supplied, this
>> + is a string that is NEVER the start of an FS transaction name */
>> +#define DAV_SVN__VISIBLE_TXN_PREFIX "$"
>
> Whoops! Technically, mod_dav_svn hasn't access to the knowledge required to
> make this assertion. Who says a transaction's name can't start with a
> dollar sign?
I suppose I should make the comment more accurate. The prefix
determines whether the visible:internal mapping will apply. If a
internal transaction's name starts with the prefix then an identity
mapping will get applied. That still works but the overhead of going
through the activity database makes it marginally less efficient.
> I'm still at odds with myself about this patch in general. HTTPv2 offers us
> a chance to clean up and simplify our protocol, and before it even sees its
> first release you want to add a bit of obfuscation and indirection required
> solely by one commercial product? Come on. (If you know me at all, you
> realize my antagonism is aimed not at said product, but at the proposed
> protocol hack.) So I'm trying to convince myself that there's some general
> utility to this change -- that somewhere, sometime, somebody else will thank
> us for adding it.
There is a demand for WANdisco's replicator. It may be the only product
at the moment but it's possible that somebody will write an open source
product that also does multiplexing and/or replay. They will then face
the same problem that WANdisco does now.
--
Philip
Re: [PATCH] HTTPv2 allow client to control transaction name in protocol
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/01/2011 10:09 AM, Philip Martin wrote:
> Index: subversion/mod_dav_svn/dav_svn.h
> ===================================================================
> --- subversion/mod_dav_svn/dav_svn.h (revision 1075718)
> +++ subversion/mod_dav_svn/dav_svn.h (working copy)
> @@ -52,6 +52,9 @@
> /* a pool-key for the shared dav_svn_root used by autoversioning */
> #define DAV_SVN__AUTOVERSIONING_ACTIVITY "svn-autoversioning-activity"
>
> +/* used to distinguish client supplied TXN-NAME from FS supplied, this
> + is a string that is NEVER the start of an FS transaction name */
> +#define DAV_SVN__VISIBLE_TXN_PREFIX "$"
Whoops! Technically, mod_dav_svn hasn't access to the knowledge required to
make this assertion. Who says a transaction's name can't start with a
dollar sign?
I'm still at odds with myself about this patch in general. HTTPv2 offers us
a chance to clean up and simplify our protocol, and before it even sees its
first release you want to add a bit of obfuscation and indirection required
solely by one commercial product? Come on. (If you know me at all, you
realize my antagonism is aimed not at said product, but at the proposed
protocol hack.) So I'm trying to convince myself that there's some general
utility to this change -- that somewhere, sometime, somebody else will thank
us for adding it.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand