[MERGE][1.0][Bug 124089] Add root_client_path parameter to SmartWSGIApp, and use it to translate paths received from clients to local paths.
John Arbash Meinel
john at arbash-meinel.com
Thu Dec 20 21:31:19 GMT 2007
John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
Let me summarize to make sure I'm understanding.
Clients are going to see paths based on the url they connect to. So
something like:
http://host/path/to/repo
The client will be passing paths like:
GET, /path/to/repo/.bzr/branch-format
This patch changes it so that all smart server operations strip off
whatever "root_client_path" is set to.
Is that the case, or is it if the client connects to:
http://host/path/to/repo/.bzr/smart
then they will send requests for
GET, .bzr/branch-format
but if they connected to
http://host/path/to/repo/branch/.bzr/smart
then they would be sending
GET, .bzr/branch-format
and you need to add in the
'branch/' portion.
Which means that if you want to serve only things under '/path/to', you
would create a SmartWSGIApp that has a backing transport of
get_transport('/srv/bzr/public/real/path/on/disk')
root_client_path = '/path/to'
And the whole point is that the URL namespace before what is exposed
doesn't have to match what is on disk.
+class
TestSmartServerRequestFindRepository(tests.TestCaseWithMemoryTransport):
"""Tests for BzrDir.find_repository."""
def test_no_repository(self):
@@ -75,7 +138,7 @@
request =
smart.bzrdir.SmartServerRequestFindRepository(backing)
self.make_bzrdir('.')
self.assertEqual(SmartServerResponse(('norepository', )),
- request.execute(backing.local_abspath('')))
+ request.execute('/'))
def test_nonshared_repository(self):
# nonshared repositorys only allow 'find' to return a handle
when the
@@ -84,10 +147,10 @@
backing = self.get_transport()
request =
smart.bzrdir.SmartServerRequestFindRepository(backing)
result = self._make_repository_and_result()
- self.assertEqual(result,
request.execute(backing.local_abspath('')))
+ self.assertEqual(result, request.execute(''))
self.make_bzrdir('subdir')
self.assertEqual(SmartServerResponse(('norepository', )),
- request.execute(backing.local_abspath('subdir')))
+ request.execute('subdir'))
Why do you use "execute('/')" in the first one and "execute('')" in the
second? You seem to normally use "execute('')" in the other tests.
I agree with what someone else said, which is to make "wsgi.make_app()"
have "path_var='REQUEST_URI'" be default. That way to configure a
read-only smart server is just:
smart_server_app = wsgi.make_app('/path/to/root', '/root/')
I'm a little curious why RelpathSetter now takes '' as its prefix to
strip, while SmartWSGIApp takes '/root/'.
It also seems odd that RelpathSetter always tries to strip off slashes,
while SmartWSGIApp.__call__ is always making sure they are present.
I get the feeling that RelpathSetter should just be removed, as it
doesn't seem to add much. And that would also mean that 3rd-party
implementations don't need to use 'bzrlib.relpath'
# The relpath traverses all of the mandatory root client
path.
# Remove the root_client_path from the relpath, and set
# adjusted_tcp to None to tell the request handler that no
further
# path translation is required.
adjusted_rcp = None
adjusted_relpath = relpath[len(self.root_client_path):]
^- typo "adjusted_rcp to None"
Overall, it seems like the SmartWSGIApp is handling the
'root_client_path' and then passing some hybrid of it into
make_request(..., adjusted_rcp).
It all feels a lot like double handling. You validate the path, and then
you pass that down into the lower levels to validate it again.
I *think* it might just be that one of the adjustments is based on the
URL that the client posts to, and the other is based on the path inside
the actual request. Do we just need better variable names to make it
clear what is happening when? (When are we dealing with an Apache URL,
when are we doing with a Smart path, is it in the client namespace, or
the server namespace.)
Anyway, I'm happy to see the functionality merged, and I don't think
this is a step backwards in complexity. I don't think it is a clear
step-forward in clarity.
It might just be a documentation/comment issue. (As Vincent said about
my sftp_readv comment making the code a lot easier to understand, even
if it was still too complex :)
Interestingly enough, to get "bzr log bzr+http://" to work (with my old
configuration), all I needed was your client patch.
For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C20071214015112.GC14963%40steerpike.home.puzzling.org%3E
More information about the bazaar
mailing list