[MERGE] [Bug #129307] Better redirection handling
Vincent Ladeuil
v.ladeuil+lp at free.fr
Wed Aug 27 14:04:54 BST 2008
>>>>> "Christophe" == Christophe TROESTLER <Christophe.Troestler+bzr at umh.ac.be> writes:
Christophe> Hi,
Christophe> The following patch addresses bug #129307. In summary, the bug was
Christophe> that, when doing "bzr branch http://server/projet", the non-existing
Christophe> URL "http://server/projet/.bzr/smart" was redirected to an error page,
Christophe> say "http://server/error/" (and then returning 404) and this new
Christophe> (bogus) URL was used to check for ".bzr/branch-format". This resulted
Christophe> in requesting "http://server/error/.bzr/branch-format" instead of the
Christophe> correct "http://server/projet/.bzr/branch-format".
Christophe> The solution discussed with Vincent is to only memorize a redirected
Christophe> URL if the target is eventually found.
Christophe> The patch implements this and also treats subsequent redirections as
Christophe> missing targets (see the diff of workingtree.py)
Christophe> With these modifications, (as an example)
Christophe> bzr branch http://users.skynet.be/Pierre.Brukier/anum/projet
Christophe> merge
Christophe> co
Christophe> work as they should.
Well, not completely:
vila:~/src/bzr/reviews/129307-http-errors-redirected :) $ ./bzr branch http://users.skynet.be/Pierre.Brukier/anum/projet ~/tmp/pb-project
http://users.skynet.be/Pierre.Brukier/anum/projet/ is redirected to http://hostingerrors.isp.belgacom.be/Errors.cgi?url=http://users.skynet.be/Pierre.Brukier/anum/projet/
Branched 3 revision(s).
The warning is displayed while probing for the smart server. This
should not occur, the probing should just fail silently since
there is no smart server there.
This suggests that the tests are not complete. I suppose that you
use bzr *with* your patch applied in your day to day activities
so I'm pretty confident that the code is correct, at least for
your workflow.
Your approach sounded correct though since you test all the parts
you've been modified but I still think the error above is a
symptom and that a more complete approach is needed.
To achieve that you'll have to define an http test server that
redirects errors only (like users.skynet.be) and use that against
the http client implementations.
The later Roughly means adding a tuple (transport,
your_tests_server) in the get_test_permutations function in
_pycurl.py and _urllib.py (both in bzrlib/transport/http
directory).
As an intermediate step, you can configure an apache2 server to
match skynet.be with the local_test_server plugin
(http://launchpad.net/~vila/bzr/local-test-server/).
<snip/>
Christophe> === modified file 'bzrlib/tests/test_http.py'
Christophe> --- bzrlib/tests/test_http.py 2008-05-19 07:50:49 +0000
Christophe> +++ bzrlib/tests/test_http.py 2008-07-18 13:08:00 +0000
Christophe> @@ -42,6 +42,8 @@
Christophe> ui,
Christophe> urlutils,
Christophe> )
Christophe> +from bzrlib.bzrdir import BzrDir
Christophe> +from bzrlib.workingtree import WorkingTreeFormat
We are moving away from this style of import, we prefer:
from bzrlib import (
bzrdir,
workingtree,
)
and then using bzrdir.BzrDir and workintree.WorkingTreeFormat.
Christophe> from bzrlib.tests import (
Christophe> http_server,
Christophe> http_utils,
Christophe> @@ -125,6 +127,9 @@
Christophe> tp_adapter = TransportProtocolAdapter()
Christophe> tp_classes= (SmartHTTPTunnellingTest,
Christophe> TestDoCatchRedirections,
Christophe> + TestDoFollowRedirections,
Christophe> + TestOpenFormatRedirections,
Christophe> + TestFindFormatRedirections,
Christophe> TestHTTPConnections,
Christophe> TestHTTPRedirections,
Christophe> TestHTTPSilentRedirections,
Christophe> @@ -1423,6 +1428,108 @@
Christophe> transport.do_catching_redirections,
Christophe> self.get_a, self.old_transport, redirected)
Christophe> +class TestDoFollowRedirections(http_utils.TestCaseWithRedirectedWebserver):
Christophe> + """Test transport._do_following_redirections."""
Christophe> +
Christophe> + def setUp(self):
Christophe> + super(TestDoFollowRedirections, self).setUp()
Christophe> + self.build_tree_contents([('a', '0123456789')])
Christophe> +
Christophe> + self.old_transport = self._transport(self.old_server.get_url())
Christophe> +
Christophe> + def get_a(self, transport):
Christophe> + return transport.get('a')
Christophe> +
Christophe> + def test_no_redirection(self):
Christophe> + t = self._transport(self.new_server.get_url())
Christophe> + (a, t1) = transport._do_following_redirections(self.get_a, t)
Christophe> + self.assertEquals('0123456789', a.read())
Christophe> + # The transport should be the very same object.
Christophe> + self.assertEquals(t, t1)
Christophe> +
Christophe> + def test_one_redirection(self):
Christophe> + (a, t1) = transport._do_following_redirections(
Christophe> + self.get_a, self.old_transport)
Christophe> + self.assertEquals('0123456789', a.read())
Christophe> + self.assertEquals(self.new_server.get_url(), t1.base)
Christophe> +
Christophe> +
Christophe> + def __create_redirecting_server(self, server):
Christophe> + redirecting = http_utils.HTTPServerRedirecting()
Christophe> + redirecting.redirect_to(server.host, server.port)
Christophe> + return redirecting
Christophe> +
Christophe> + def test_redirection_loop(self):
Christophe> + # "Long" sequence of servers redirecting to the previous one
Christophe> + s = self.get_readonly_server()
Christophe> + for i in range(10):
This 10 looks rather arbitrary, what are you trying to accomplish
here ? Reaching MAX_REDIRECTIONS ? Then so say and use it.
Christophe> + s = self.__create_redirecting_server(s)
Christophe> + s.setUp()
You need a self.addCleanup(s.tearDown) here.
<snip/>
Christophe> +
Christophe> +class TestOpenFormatRedirections(http_utils.TestCaseWithRedirectedWebserver):
Christophe> + """Test BzrDir.open_from_transport in presence of redirections."""
Christophe> +
<snip/>
Christophe> +
Christophe> +class TestFindFormatRedirections(http_utils.TestCaseWithRedirectedWebserver):
Christophe> + """Test WorkingTreeFormat.find_format in presence of redirections."""
Christophe> +
<snip/>
This sounds good but by using a dedicated test server you'll get
them and much more for free.
Christophe> class TestAuth(http_utils.TestCaseWithWebserver):
Christophe> """Test authentication scheme"""
Christophe> === modified file 'bzrlib/transport/__init__.py'
Christophe> --- bzrlib/transport/__init__.py 2008-06-24 22:53:43 +0000
Christophe> +++ bzrlib/transport/__init__.py 2008-07-17 07:03:56 +0000
Christophe> @@ -53,6 +53,7 @@
Christophe> )
Christophe> from bzrlib.trace import (
Christophe> mutter,
Christophe> + note,
Christophe> )
Christophe> from bzrlib import registry
Christophe> @@ -1631,6 +1632,37 @@
Christophe> # occurred).
Christophe> raise errors.TooManyRedirections
Christophe> +def _do_following_redirections(action, transport):
Christophe> + """Execute an action with given transport following redirections.
Christophe> +
Christophe> + This is a common use of do_catching_redirections which is why it
Christophe> + is provided here.
Christophe> +
Christophe> + :return: a couple which first component is what 'action' returns
Christophe> + and second component is the possibly updated transport."""
Christophe> +
Christophe> + def do_action(transport):
Christophe> + return action(transport), transport
Christophe> +
Christophe> + def redirected(transport, e, redirection_notice):
Christophe> + qualified_source = e.get_source_url()
Christophe> + relpath = transport.relpath(qualified_source)
Christophe> + if not e.target.endswith(relpath):
Christophe> + # Not redirected to a branch-format, not a branch
Christophe> + raise errors.NotBranchError(path=e.target)
Christophe> + target = e.target[:-len(relpath)]
Christophe> + note('%s is%s redirected to %s', transport.base, e.permanently, target)
I think using note here (better use trace.note by the way since
you're here) is not appropriate as shown by the error above,
there are cases where you want to emit the message but most of
the time you don't.
Christophe> + # Let's try with a new transport
Christophe> + # FIXME: If 'transport' has a qualifier, this should
Christophe> + # be applied again to the new transport *iff* the
Christophe> + # schemes used are the same. Uncomment this code
Christophe> + # once the function (and tests) exist.
Christophe> + # -- vila20070212
Christophe> + #target = urlutils.copy_url_qualifiers(original, target)
Christophe> + return get_transport(target)
Christophe> +
Christophe> + return do_catching_redirections(do_action, transport, redirected)
Christophe> +
Christophe> class Server(object):
Christophe> """A Transport Server.
Christophe> === modified file 'bzrlib/workingtree.py'
Christophe> --- bzrlib/workingtree.py 2008-07-08 14:55:19 +0000
Christophe> +++ bzrlib/workingtree.py 2008-07-18 12:45:02 +0000
Christophe> @@ -77,7 +77,9 @@
Christophe> xml7,
Christophe> )
Christophe> import bzrlib.branch
Christophe> -from bzrlib.transport import get_transport
Christophe> +from bzrlib.transport import (
Christophe> + get_transport,
Christophe> + )
Christophe> import bzrlib.ui
Christophe> from bzrlib.workingtree_4 import WorkingTreeFormat4
Christophe> """)
Christophe> @@ -2656,7 +2658,10 @@
Christophe> transport = a_bzrdir.get_workingtree_transport(None)
Christophe> format_string = transport.get("format").read()
Christophe> return klass._formats[format_string]
Christophe> - except errors.NoSuchFile:
Christophe> + except (errors.NoSuchFile, errors.RedirectRequested):
Christophe> + # We treat a redirection like an absence of file since a
Christophe> + # sucessful redirection was going to be remembered when
Christophe> + # a_bzrdir was created.
Christophe> raise errors.NoWorkingTree(base=transport.base)
Christophe> except KeyError:
Christophe> raise errors.UnknownFormatError(format=format_string,
This sounds correct but I'm surprised that this is the single
modification to be done outside of bzrdir.py. I suspect that
using a dedicated test server will reveal cases outside your
usual workflow.
BB:resubmit
Vincent
More information about the bazaar
mailing list