launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03194
[Merge] lp:~thumper/launchpad/anon-http-branch-id-access into lp:launchpad
Tim Penhey has proposed merging lp:~thumper/launchpad/anon-http-branch-id-access into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #377519 in Launchpad itself: "Stacked on location breaks if the stacked upon branch is renamed"
https://bugs.launchpad.net/launchpad/+bug/377519
For more details, see:
https://code.launchpad.net/~thumper/launchpad/anon-http-branch-id-access/+merge/56287
This branch is the start of a pipeline to fix the problem
where branches that are stacked on another branch that changes
any part of the branch unique_name get broken.
The solution is instead to stack on an ID based url.
This branch adds support for a +branch-id/%(id)s branch alias
for public branches served over HTTP using the rewrite script.
--
https://code.launchpad.net/~thumper/launchpad/anon-http-branch-id-access/+merge/56287
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/anon-http-branch-id-access into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/codehosting.py'
--- lib/lp/code/interfaces/codehosting.py 2011-02-24 15:30:54 +0000
+++ lib/lp/code/interfaces/codehosting.py 2011-04-05 04:44:21 +0000
@@ -8,6 +8,7 @@
__metaclass__ = type
__all__ = [
'BRANCH_ALIAS_PREFIX',
+ 'BRANCH_ID_ALIAS_PREFIX',
'BRANCH_TRANSPORT',
'compose_public_url',
'CONTROL_TRANSPORT',
@@ -55,6 +56,9 @@
# The path prefix for getting at branches via their short name.
BRANCH_ALIAS_PREFIX = '+branch'
+# The path prefix for getting at branches via their id.
+BRANCH_ID_ALIAS_PREFIX = '+branch-id'
+
# The scheme types that are supported for codehosting.
SUPPORTED_SCHEMES = 'bzr+ssh', 'http'
=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py 2011-02-24 15:30:54 +0000
+++ lib/lp/code/model/branchlookup.py 2011-04-05 04:44:21 +0000
@@ -46,6 +46,7 @@
ILinkedBranchTraverser,
)
from lp.code.interfaces.branchnamespace import IBranchNamespaceSet
+from lp.code.interfaces.codehosting import BRANCH_ID_ALIAS_PREFIX
from lp.code.interfaces.linkedbranch import get_linked_to_branch
from lp.code.model.branch import Branch
from lp.registry.errors import (
@@ -288,14 +289,29 @@
return None
return self._getBranchInNamespace(namespace_data, branch_name)
- def getIdAndTrailingPath(self, path, from_slave=False):
- """See `IBranchLookup`. """
- if from_slave:
- store = ISlaveStore(Branch)
+ def _getIdAndTrailingPathByIdAlias(self, store, path):
+ """Query by the integer id."""
+ parts = path.split('/', 2)
+ try:
+ branch_id = int(parts[1])
+ except (ValueError, IndexError):
+ return None, None
+ result = store.find(
+ (Branch.id),
+ Branch.id == branch_id,
+ Branch.private == False).one()
+ if result is None:
+ return None, None
else:
- store = IMasterStore(Branch)
+ try:
+ return branch_id, '/' + parts[2]
+ except IndexError:
+ return branch_id, ''
+
+ def _getIdAndTrailingPathByUniqueName(self, store, path):
+ """Query based on the unique name."""
prefixes = []
- for first, second in iter_split(path[1:], '/'):
+ for first, second in iter_split(path, '/'):
prefixes.append(first)
result = store.find(
(Branch.id, Branch.unique_name),
@@ -304,9 +320,21 @@
return None, None
else:
branch_id, unique_name = result
- trailing = path[len(unique_name) + 1:]
+ trailing = path[len(unique_name):]
return branch_id, trailing
+ def getIdAndTrailingPath(self, path, from_slave=False):
+ """See `IBranchLookup`. """
+ if from_slave:
+ store = ISlaveStore(Branch)
+ else:
+ store = IMasterStore(Branch)
+ path = path.lstrip('/')
+ if path.startswith(BRANCH_ID_ALIAS_PREFIX):
+ return self._getIdAndTrailingPathByIdAlias(store, path)
+ else:
+ return self._getIdAndTrailingPathByUniqueName(store, path)
+
def _getBranchInNamespace(self, namespace_data, branch_name):
if namespace_data['product'] == '+junk':
return self._getPersonalBranch(
=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py 2010-10-24 21:00:11 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py 2011-04-05 04:44:21 +0000
@@ -25,6 +25,7 @@
ILinkedBranchTraverser,
)
from lp.code.interfaces.branchnamespace import get_branch_namespace
+from lp.code.interfaces.codehosting import BRANCH_ID_ALIAS_PREFIX
from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
from lp.registry.errors import (
NoSuchDistroSeries,
@@ -38,6 +39,7 @@
)
from lp.registry.interfaces.productseries import NoSuchProductSeries
from lp.testing import (
+ person_logged_in,
run_with_login,
TestCaseWithFactory,
)
@@ -118,6 +120,42 @@
'/' + branch.unique_name + '/' + path)
self.assertEqual((branch.id, '/' + path), result)
+ def test_branch_id_alias(self):
+ path = BRANCH_ID_ALIAS_PREFIX
+ result = self.branch_set.getIdAndTrailingPath('/' + path)
+ self.assertEqual((None, None), result)
+
+ def test_branch_id_alias_not_int(self):
+ path = BRANCH_ID_ALIAS_PREFIX + '/foo'
+ result = self.branch_set.getIdAndTrailingPath('/' + path)
+ self.assertEqual((None, None), result)
+
+ def test_branch_id_alias_private(self):
+ owner = self.factory.makePerson()
+ branch = self.factory.makeAnyBranch(owner=owner, private=True)
+ with person_logged_in(owner):
+ path = '/%s/%s' % (BRANCH_ID_ALIAS_PREFIX, branch.id)
+ result = self.branch_set.getIdAndTrailingPath(path)
+ self.assertEqual((None, None), result)
+
+ def test_branch_id_alias_public(self):
+ branch = self.factory.makeAnyBranch()
+ path = '/%s/%s' % (BRANCH_ID_ALIAS_PREFIX, branch.id)
+ result = self.branch_set.getIdAndTrailingPath(path)
+ self.assertEqual((branch.id, ''), result)
+
+ def test_branch_id_alias_public_slash(self):
+ branch = self.factory.makeAnyBranch()
+ path = '/%s/%s/' % (BRANCH_ID_ALIAS_PREFIX, branch.id)
+ result = self.branch_set.getIdAndTrailingPath(path)
+ self.assertEqual((branch.id, '/'), result)
+
+ def test_branch_id_alias_public_with_path(self):
+ branch = self.factory.makeAnyBranch()
+ path = '/%s/%s/foo' % (BRANCH_ID_ALIAS_PREFIX, branch.id)
+ result = self.branch_set.getIdAndTrailingPath(path)
+ self.assertEqual((branch.id, '/foo'), result)
+
class TestGetByPath(TestCaseWithFactory):
"""Test `IBranchLookup.getByLPPath`."""
=== modified file 'lib/lp/codehosting/rewrite.py'
--- lib/lp/codehosting/rewrite.py 2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/rewrite.py 2011-04-05 04:44:21 +0000
@@ -11,6 +11,7 @@
from canonical.config import config
from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.interfaces.codehosting import BRANCH_ID_ALIAS_PREFIX
from lp.codehosting.vfs import branch_id_to_path
from lp.services.utils import iter_split
@@ -99,7 +100,10 @@
branch_id, trailing, cached = self._getBranchIdAndTrailingPath(
resource_location)
if branch_id is None:
- r = self._codebrowse_url(resource_location)
+ if resource_location.startswith('/' + BRANCH_ID_ALIAS_PREFIX):
+ r = 'NULL'
+ else:
+ r = self._codebrowse_url(resource_location)
else:
if trailing.startswith('/.bzr'):
r = urlutils.join(
=== modified file 'lib/lp/codehosting/tests/test_rewrite.py'
--- lib/lp/codehosting/tests/test_rewrite.py 2010-12-23 01:02:00 +0000
+++ lib/lp/codehosting/tests/test_rewrite.py 2011-04-05 04:44:21 +0000
@@ -16,6 +16,7 @@
from canonical.config import config
from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.code.interfaces.codehosting import BRANCH_ID_ALIAS_PREFIX
from lp.codehosting.rewrite import BranchRewriter
from lp.codehosting.vfs import branch_id_to_path
from lp.services.log.logger import BufferLogger
@@ -91,6 +92,56 @@
'http://localhost:8080/%s/.bzr' % unique_name],
output)
+ def test_rewriteLine_id_alias_found_dot_bzr(self):
+ # Requests for /+branch-id/$id/.bzr/... are redirected to where the
+ # branches are served from by ID if they are public.
+ rewriter = self.makeRewriter()
+ branches = [
+ self.factory.makeProductBranch(private=False),
+ self.factory.makePersonalBranch(private=False),
+ self.factory.makePackageBranch(private=False)]
+ transaction.commit()
+ output = [
+ rewriter.rewriteLine("/%s/%s/.bzr/README" % (
+ BRANCH_ID_ALIAS_PREFIX, branch.id))
+ for branch in branches]
+ expected = [
+ 'file:///var/tmp/bazaar.launchpad.dev/mirrors/%s/.bzr/README'
+ % branch_id_to_path(branch.id)
+ for branch in branches]
+ self.assertEqual(expected, output)
+
+ def test_rewriteLine_id_alias_private(self):
+ # All requests for /+branch-id/$id/... for private branches return
+ # 'NULL'. This is translated by apache to a 404.
+ rewriter = self.makeRewriter()
+ branch = self.factory.makeAnyBranch(private=True)
+ path = '/%s/%s' % (
+ BRANCH_ID_ALIAS_PREFIX, removeSecurityProxy(branch).id)
+ transaction.commit()
+ output = [
+ rewriter.rewriteLine("%s/changes" % path),
+ rewriter.rewriteLine("%s/.bzr" % path)
+ ]
+ self.assertEqual(['NULL', 'NULL'], output)
+
+ def test_rewriteLine_id_alias_logs_cache_hit(self):
+ # The second request for a branch using the alias hits the cache.
+ rewriter = self.makeRewriter()
+ branch = self.factory.makeAnyBranch()
+ transaction.commit()
+ path = "/%s/%s/.bzr/README" % (BRANCH_ID_ALIAS_PREFIX, branch.id)
+ rewriter.rewriteLine(path)
+ rewriter.rewriteLine(path)
+ logging_output_lines = self.getLoggerOutput(
+ rewriter).strip().split('\n')
+ self.assertEqual(2, len(logging_output_lines))
+ self.assertIsNot(
+ None,
+ re.match("INFO .* -> .* (.*s, cache: HIT)",
+ logging_output_lines[-1]),
+ "No hit found in %r" % logging_output_lines[-1])
+
def test_rewriteLine_static(self):
# Requests to /static are rewritten to codebrowse urls.
rewriter = self.makeRewriter()
