Repo JSON Performance Improvements (#310)

* Fix Composer v2 package provider 404 > Any requested package which does not exist MUST return a 404 status code, which will indicate to Composer that this package does not exist in your repository. Make sure the 404 response is fast to avoid blocking Composer. Avoid redirects to alternative 404 pages. * Cache repo endpoints It needs to return the last modified date from the providers to use in the HTTP Response. Also fix minor performance improvement with array_merge * Add available-packages for Composer v2 This contains the full list of packages available on the repo. This stops Composer attempting to access the metadata-url for 'public' packages. * Fix Scanner Tests The mock wasn't updated to reflect the change in function. However, the tests weren't actually checking the scan was working. Co-authored-by: Arkadiusz Kondas <[email protected]>
repman-io · Oct 22, 2020 · ffe5080 · ffe5080
1 parent 62d7b9d
commit ffe5080
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 25 deletions.
diff --git a/src/Controller/RepoController.php b/src/Controller/RepoController.php
@@ -34,10 +34,14 @@ public function __construct(PackageQuery $packageQuery, PackageManager $packageM
      * @Route("/packages.json", host="{organization}.repo.{domain}", name="repo_packages", methods={"GET"}, defaults={"domain":"%domain%"}, requirements={"domain"="%domain%"})
      * @Cache(public=false)
      */
-    public function packages(Organization $organization): JsonResponse
+    public function packages(Request $request, Organization $organization): JsonResponse
     {
-        return new JsonResponse([
-            'packages' => $this->packageManager->findProviders($organization->alias(), $this->packageQuery->getAllNames($organization->id())),
+        $packageNames = $this->packageQuery->getAllNames($organization->id());
+        [$lastModified, $packages] = $this->packageManager->findProviders($organization->alias(), $packageNames);
+
+        $response = (new JsonResponse([
+            'packages' => $packages,
+            'available-packages' => array_map(static fn (PackageName $packageName) => $packageName->name(), $packageNames),
             'metadata-url' => '/p2/%package%.json',
             'notify-batch' => $this->generateUrl('repo_package_downloads', [
                 'organization' => $organization->alias(),
@@ -53,7 +57,13 @@ public function packages(Organization $organization): JsonResponse
                     'preferred' => true,
                 ],
             ],
-        ]);
+        ]))
+        ->setPrivate()
+        ->setLastModified($lastModified);
+
+        $response->isNotModified($request);
+
+        return $response;
     }
 
     /**
@@ -120,14 +130,24 @@ public function downloads(Request $request, Organization $organization): JsonRes
      *      requirements={"domain"="%domain%","package"="%package_name_pattern%"})
      * @Cache(public=false)
      */
-    public function providerV2(Organization $organization, string $package): JsonResponse
+    public function providerV2(Request $request, Organization $organization, string $package): JsonResponse
     {
-        $providerData = $this->packageManager->findProviders(
+        [$lastModified, $providerData] = $this->packageManager->findProviders(
             $organization->alias(),
             [new PackageName('', $package)]
         );
 
-        return new JsonResponse($providerData === [] ? new \stdClass() : $providerData);
+        if ($providerData === []) {
+            throw new NotFoundHttpException();
+        }
+
+        $response = (new JsonResponse($providerData))
+            ->setLastModified($lastModified)
+            ->setPrivate();
+
+        $response->isNotModified($request);
+
+        return $response;
     }
 
     /**

diff --git a/src/Service/Organization/PackageManager.php b/src/Service/Organization/PackageManager.php
@@ -30,22 +30,33 @@ public function __construct(Storage $distStorage, string $baseDir, Filesystem $f
     /**
      * @param PackageName[] $packages
      *
-     * @return mixed[]
+     * @return array{\DateTimeImmutable|null, mixed[]}
      */
     public function findProviders(string $organizationAlias, array $packages): array
     {
         $data = [];
+        $lastModified = null;
+
         foreach ($packages as $package) {
             $filepath = $this->filepath($organizationAlias, $package->name());
             if (!is_readable($filepath)) {
                 continue;
             }
 
-            $json = unserialize((string) file_get_contents($filepath));
-            $data = array_merge($data, $json['packages'] ?? []);
+            $fileModifyDate = (new \DateTimeImmutable())->setTimestamp((int) filemtime($filepath));
+
+            if ($fileModifyDate > $lastModified) {
+                $lastModified = $fileModifyDate;
+            }
+
+            $json = unserialize((string) file_get_contents($filepath), ['allowed_classes' => false]);
+            $data[] = $json['packages'] ?? [];
         }
 
-        return $data;
+        return [
+            $lastModified,
+            array_merge(...$data),
+        ];
     }
 
     /**

diff --git a/src/Service/Security/PackageScanner/SensioLabsPackageScanner.php b/src/Service/Security/PackageScanner/SensioLabsPackageScanner.php
@@ -102,7 +102,7 @@ private function findDistribution(Package $package): string
         $normalizedVersion = $latestReleasedVersion === 'no stable release' ?
             '9999999-dev' : $this->versionParser->normalize((string) $latestReleasedVersion);
 
-        $providerData = $this->packageManager->findProviders(
+        [,$providerData] = $this->packageManager->findProviders(
             $package->organizationAlias(),
             [new PackageName($package->id()->toString(), (string) $package->name())]
         );

diff --git a/tests/Functional/Controller/RepoControllerTest.php b/tests/Functional/Controller/RepoControllerTest.php
@@ -5,6 +5,7 @@
 namespace Buddy\Repman\Tests\Functional\Controller;
 
 use Buddy\Repman\Tests\Functional\FunctionalTestCase;
+use Ramsey\Uuid\Uuid;
 use Symfony\Component\HttpFoundation\BinaryFileResponse;
 use Symfony\Component\HttpFoundation\Response;
 
@@ -63,7 +64,11 @@ public function testOrganizationPackagesAction(): void
     {
         // create and login admin to check token last used date is changed
         $adminId = $this->createAndLoginAdmin('[email protected]', 'secret');
-        $this->fixtures->createToken($this->fixtures->createOrganization('buddy', $adminId), 'secret-org-token');
+        $organizationId = $this->fixtures->createOrganization('buddy', $adminId);
+        $this->fixtures->createToken($organizationId, 'secret-org-token');
+        $packageId = Uuid::uuid4()->toString();
+        $this->fixtures->createPackage($packageId, 'buddy', $organizationId);
+        $this->fixtures->syncPackageWithData($packageId, 'buddy-works/repman', 'Test', '1.1.1', new \DateTimeImmutable());
 
         // check token was never used
         $this->client->request('GET', $this->urlTo('organization_tokens', ['organization' => 'buddy']));
@@ -75,9 +80,24 @@ public function testOrganizationPackagesAction(): void
             'PHP_AUTH_PW' => 'secret-org-token',
         ]);
 
-        self::assertMatchesPattern('
+        self::assertJsonStringEqualsJsonString('
         {
-            "packages": [],
+            "packages": {
+               "buddy-works\/repman": {
+                    "1.2.3": {
+                        "version": "1.2.3",
+                        "version_normalized": "1.2.3.0",
+                        "dist": {
+                            "type": "zip",
+                            "url": "\/path\/to\/reference.zip",
+                            "reference": "ac7dcaf888af2324cd14200769362129c8dd8550"
+                        }
+                    }
+                }
+            },
+            "available-packages": [
+                "buddy-works/repman"
+            ],
             "metadata-url": "/p2/%package%.json",
             "notify-batch": "http://buddy.repo.repman.wip/downloads",
             "search": "https://packagist.org/search.json?q=%query%&type=%type%",
@@ -191,4 +211,37 @@ public function testProviderV2Action(): void
         }
         ', $this->client->getResponse()->getContent());
     }
+
+    public function testProviderV2ActionWithCache(): void
+    {
+        $adminId = $this->createAndLoginAdmin('[email protected]', 'secret');
+        $this->fixtures->createToken($this->fixtures->createOrganization('buddy', $adminId), 'secret-org-token');
+
+        $fileModifiedTime = (new \DateTimeImmutable())
+            ->setTimestamp((int) filemtime(__DIR__.'/../../Resources/p2/buddy-works/repman.json'));
+
+        $this->client->request('GET', '/p2/buddy-works/repman.json', [], [], [
+            'HTTP_HOST' => 'buddy.repo.repman.wip',
+            'PHP_AUTH_USER' => 'token',
+            'PHP_AUTH_PW' => 'secret-org-token',
+            'HTTP_IF_MODIFIED_SINCE' => $fileModifiedTime->format('D, d M Y H:i:s \G\M\T'),
+        ]);
+
+        self::assertEquals(304, $this->client->getResponse()->getStatusCode());
+        self::assertEmpty($this->client->getResponse()->getContent());
+    }
+
+    public function testProviderV2ForMissingPackage(): void
+    {
+        $adminId = $this->createAndLoginAdmin('[email protected]', 'secret');
+        $this->fixtures->createToken($this->fixtures->createOrganization('buddy', $adminId), 'secret-org-token');
+
+        $this->client->request('GET', '/p2/buddy-works/fake.json', [], [], [
+            'HTTP_HOST' => 'buddy.repo.repman.wip',
+            'PHP_AUTH_USER' => 'token',
+            'PHP_AUTH_PW' => 'secret-org-token',
+        ]);
+
+        self::assertTrue($this->client->getResponse()->isNotFound());
+    }
 }
diff --git a/tests/Unit/Service/Organization/PackageManagerTest.php b/tests/Unit/Service/Organization/PackageManagerTest.php
@@ -33,7 +33,7 @@ protected function setUp(): void
 
     public function testFindProvidersForPackage(): void
     {
-        $providers = $this->manager->findProviders('buddy', [
+        [,$providers] = $this->manager->findProviders('buddy', [
             new PackageName('id', 'buddy-works/repman'),
             new PackageName('id', 'not-exist/missing'),
         ]);

diff --git a/tests/Unit/Service/Security/SensioLabsPackageScannerTest.php b/tests/Unit/Service/Security/SensioLabsPackageScannerTest.php
@@ -4,6 +4,7 @@
 
 namespace Buddy\Repman\Tests\Unit\Service\Security;
 
+use Buddy\Repman\Entity\Organization\Package;
 use Buddy\Repman\Message\Security\SendScanResult;
 use Buddy\Repman\Repository\ScanResultRepository;
 use Buddy\Repman\Service\Organization\PackageManager;
@@ -52,6 +53,8 @@ public function testScanVersionNotFound(): void
         $package = PackageMother::synchronized('buddy-works/repman', '1.0.0', '');
 
         $this->scanner->scan($package);
+
+        $this->assertPackageSecurity('error', $package);
     }
 
     public function testSuccessfulScanNoAdvisories(): void
@@ -63,6 +66,8 @@ public function testSuccessfulScanNoAdvisories(): void
         $package = PackageMother::synchronized('buddy-works/repman', self::VERSION);
 
         $this->scanner->scan($package);
+
+        $this->assertPackageSecurity('ok', $package);
     }
 
     public function testSuccessfulScanWithAdvisories(): void
@@ -85,6 +90,8 @@ public function testSuccessfulScanWithAdvisories(): void
         $this->checkerMock = $this->createMock(SecurityChecker::class);
         $this->checkerMock->method('check')->willReturn($result);
         $this->prepareScanner()->scan($package);
+
+        $this->assertPackageSecurity('warning', $package);
     }
 
     public function testLockFileNotFound(): void
@@ -96,6 +103,8 @@ public function testLockFileNotFound(): void
         $package = PackageMother::synchronized('buddy-works/repman', self::VERSION);
 
         $this->prepareScanner('repman-no-lock')->scan($package);
+
+        $this->assertPackageSecurity('n/a', $package);
     }
 
     public function testBrokenZip(): void
@@ -107,21 +116,32 @@ public function testBrokenZip(): void
         $package = PackageMother::synchronized('buddy-works/repman', self::VERSION);
 
         $this->prepareScanner('repman-invalid-archive')->scan($package);
+
+        $this->assertPackageSecurity('error', $package);
     }
 
     private function prepareScanner(string $fixtureType = 'repman'): SensioLabsPackageScanner
     {
         $distFile = realpath(__DIR__.'/../../../Resources/fixtures/buddy/dist/buddy-works/'.$fixtureType.'/1.2.3.0_ac7dcaf888af2324cd14200769362129c8dd8550.zip');
         $packageManager = $this->createMock(PackageManager::class);
-        $packageManager->method('findProviders')->willReturn(['buddy-works/repman' => [self::VERSION => [
-            'version' => self::VERSION,
-            'dist' => [
-                'type' => 'zip',
-                'url' => $distFile,
-                'reference' => 'ac7dcaf888af2324cd14200769362129c8dd8550',
-            ],
-            'version_normalized' => '1.2.3.0',
-        ]]]);
+        $packageManager->method('findProviders')->willReturn(
+            [
+                new \DateTimeImmutable(),
+                [
+                    'buddy-works/repman' => [
+                        self::VERSION => [
+                            'version' => self::VERSION,
+                            'dist' => [
+                                'type' => 'zip',
+                                'url' => $distFile,
+                                'reference' => 'ac7dcaf888af2324cd14200769362129c8dd8550',
+                            ],
+                            'version_normalized' => '1.2.3.0',
+                        ],
+                    ],
+                ],
+            ]
+        );
 
         $packageManager->method('distFilename')->willReturn(Option::some($distFile));
 
@@ -137,4 +157,13 @@ private function prepareScanner(string $fixtureType = 'repman'): SensioLabsPacka
             $messageBusMock
         );
     }
+
+    private function assertPackageSecurity(string $expected, Package $package): void
+    {
+        $reflection = new \ReflectionObject($package);
+        $property = $reflection->getProperty('lastScanStatus');
+        $property->setAccessible(true);
+
+        self::assertEquals($expected, $property->getValue($package));
+    }
 }