From c2b8dd28d8856aa76b5bd21b9c4c92cb2337072c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Feb 2016 04:06:13 -0800 Subject: [PATCH] Support ID-based repository URIs, and canonicalize repository URIs Summary: Ref T4245. Make `/diffusion/123/` work, but redirect the user to `/diffusion/XYZ/` if the repository has a callsign. (Right now, every repository has a callsign, so this always redirects.) Also redirect `/R123:abcdef` if the repository has a callsign. Also also, move the Pull garbage collector somewhere more sensible. Test Plan: - Added test coverage. - Visited `/diffusion/1/`, was redirected. - Visited `/diffusion/R1:abcdef`, was redirected. - Browsed Diffusion normally. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4245 Differential Revision: https://secure.phabricator.com/D15301 --- src/__phutil_library_map__.php | 2 +- .../PhabricatorDiffusionApplication.php | 11 +++-- .../controller/DiffusionController.php | 14 ++++++ .../DiffusionPullEventGarbageCollector.php | 0 .../storage/PhabricatorRepository.php | 49 +++++++++++++++++++ .../PhabricatorRepositoryURITestCase.php | 41 ++++++++++++++++ 6 files changed, 113 insertions(+), 4 deletions(-) rename src/applications/diffusion/{ => garbagecollector}/DiffusionPullEventGarbageCollector.php (100%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d277b62547..ae699acf07 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -689,7 +689,7 @@ phutil_register_library_map(array( 'DiffusionPreCommitRefRepositoryHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefRepositoryHeraldField.php', 'DiffusionPreCommitRefRepositoryProjectsHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefRepositoryProjectsHeraldField.php', 'DiffusionPreCommitRefTypeHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefTypeHeraldField.php', - 'DiffusionPullEventGarbageCollector' => 'applications/diffusion/DiffusionPullEventGarbageCollector.php', + 'DiffusionPullEventGarbageCollector' => 'applications/diffusion/garbagecollector/DiffusionPullEventGarbageCollector.php', 'DiffusionPushCapability' => 'applications/diffusion/capability/DiffusionPushCapability.php', 'DiffusionPushEventViewController' => 'applications/diffusion/controller/DiffusionPushEventViewController.php', 'DiffusionPushLogController' => 'applications/diffusion/controller/DiffusionPushLogController.php', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index fa82b4e80f..b8766920fc 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -64,7 +64,11 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { '(?:query/(?P[^/]+)/)?' => 'DiffusionPushLogListController', 'view/(?P\d+)/' => 'DiffusionPushEventViewController', ), - '(?P[A-Z]+)/' => array( + '(?:'. + '(?P[A-Z]+)'. + '|'. + '(?P[1-9]\d*)'. + ')/' => array( '' => 'DiffusionRepositoryController', 'repository/(?P.*)' => 'DiffusionRepositoryController', @@ -115,8 +119,9 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { // catch-all for serving repositories over HTTP. We must accept // requests without the trailing "/" because SVN commands don't // necessarily include it. - '(?P[A-Z]+)(?:/.*)?' => - 'DiffusionRepositoryDefaultController', + '(?:(?P[A-Z]+)|(?P[1-9]\d*))'. + '(?:/.*)?' + => 'DiffusionRepositoryDefaultController', 'inline/' => array( 'edit/(?P[^/]+)/' => 'DiffusionInlineCommentController', diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index 00bb0ce4d4..c83990cc15 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -64,6 +64,20 @@ abstract class DiffusionController extends PhabricatorController { return new Aphront404Response(); } + // If the client is making a request like "/diffusion/1/...", but the + // repository has a different canonical path like "/diffusion/XYZ/...", + // redirect them to the canonical path. + + $request_path = $request->getPath(); + $repository = $drequest->getRepository(); + + $canonical_path = $repository->getCanonicalPath($request_path); + if ($canonical_path !== null) { + if ($canonical_path != $request_path) { + return id(new AphrontRedirectResponse())->setURI($canonical_path); + } + } + $this->diffusionRequest = $drequest; return null; diff --git a/src/applications/diffusion/DiffusionPullEventGarbageCollector.php b/src/applications/diffusion/garbagecollector/DiffusionPullEventGarbageCollector.php similarity index 100% rename from src/applications/diffusion/DiffusionPullEventGarbageCollector.php rename to src/applications/diffusion/garbagecollector/DiffusionPullEventGarbageCollector.php diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 9b38f911d4..461f495c55 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -687,6 +687,55 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return "/r{$callsign}{$identifier}"; } + public function getCanonicalPath($request_path) { + $standard_pattern = + '(^'. + '(?P/diffusion/)'. + '(?P[^/]+)'. + '(?P(?:/.*)?)'. + '\z)'; + + $matches = null; + if (preg_match($standard_pattern, $request_path, $matches)) { + $prefix = $matches['prefix']; + + $callsign = $this->getCallsign(); + if ($callsign) { + $identifier = $callsign; + } else { + $identifier = $this->getID(); + } + + $suffix = $matches['suffix']; + if (!strlen($suffix)) { + $suffix = '/'; + } + + return $prefix.$identifier.$suffix; + } + + $commit_pattern = + '(^'. + '(?P/)'. + '(?P'. + '(?:'. + 'r(?P[A-Z]+)'. + '|'. + 'R(?P[1-9]\d*):'. + ')'. + '(?P[a-f0-9]+)'. + ')'. + '\z)'; + + $matches = null; + if (preg_match($commit_pattern, $request_path, $matches)) { + $commit = $matches['commit']; + return $this->getCommitURI($commit); + } + + return null; + } + public function generateURI(array $params) { $req_branch = false; $req_commit = false; diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php index 0d13cf0e08..3994567225 100644 --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php +++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php @@ -9,6 +9,47 @@ final class PhabricatorRepositoryURITestCase ); } + public function testRepositoryURICanonicalization() { + $repo = id(new PhabricatorRepository()) + ->makeEphemeral() + ->setID(123); + + $tests = array( + '/diffusion/123' => '/diffusion/123/', + '/diffusion/123/' => '/diffusion/123/', + '/diffusion/123/browse/master/' => '/diffusion/123/browse/master/', + '/kangaroo/' => null, + ); + + foreach ($tests as $input => $expect) { + $this->assertEqual( + $expect, + $repo->getCanonicalPath($input), + pht('Canonical Path (ID, No Callsign): %s', $input)); + } + + $repo->setCallsign('XYZ'); + + $tests = array( + '/diffusion/123' => '/diffusion/XYZ/', + '/diffusion/123/' => '/diffusion/XYZ/', + '/diffusion/123/browse/master/' => '/diffusion/XYZ/browse/master/', + '/diffusion/XYZ' => '/diffusion/XYZ/', + '/diffusion/XYZ/' => '/diffusion/XYZ/', + '/diffusion/XYZ/browse/master/' => '/diffusion/XYZ/browse/master/', + '/diffusion/ABC/' => '/diffusion/XYZ/', + '/kangaroo/' => null, + '/R1:abcdef' => '/rXYZabcdef', + ); + + foreach ($tests as $input => $expect) { + $this->assertEqual( + $expect, + $repo->getCanonicalPath($input), + pht('Canonical Path (ID, Callsign): %s', $input)); + } + } + public function testURIGeneration() { $svn = PhabricatorRepositoryType::REPOSITORY_TYPE_SVN; $git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;