[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