Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repo JSON Performance Improvements #310

Merged
merged 6 commits into from
Oct 22, 2020
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions src/Controller/RepoController.php
Original file line number Diff line number Diff line change
@@ -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;
}

/**
19 changes: 15 additions & 4 deletions src/Service/Organization/PackageManager.php
Original file line number Diff line number Diff line change
@@ -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),
];
}

/**
Original file line number Diff line number Diff line change
@@ -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())]
);
59 changes: 56 additions & 3 deletions tests/Functional/Controller/RepoControllerTest.php
Original file line number Diff line number Diff line change
@@ -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('test@buddy.works', '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('test@buddy.works', '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('test@buddy.works', '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());
}
}
2 changes: 1 addition & 1 deletion tests/Unit/Service/Organization/PackageManagerTest.php
Original file line number Diff line number Diff line change
@@ -30,7 +30,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'),
]);
47 changes: 38 additions & 9 deletions tests/Unit/Service/Security/SensioLabsPackageScannerTest.php
Original file line number Diff line number Diff line change
@@ -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));
}
}