Skip to content

Commit 1b6cf76

Browse files
SeldaekSeldaek
committed
Go through UoW vs direct SQL for url updates
1 parent ac5a16e commit 1b6cf76

2 files changed

Lines changed: 124 additions & 14 deletions

File tree

‎src/Package/Updater.php‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
6363
use Symfony\Contracts\EventDispatcher\Event;
6464
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
65+
use Webmozart\Assert\Assert;
6566

6667
final readonly class VersionUpdatedResult
6768
{
@@ -458,7 +459,7 @@ public function update(IOInterface $io, Config $config, Package $package, VcsRep
458459
*
459460
* @param array{id: int, version: string, normalizedVersion: string, development: int, source: array{type: string|null, url: string|null, reference: string|null}|null, dist: array{type: string|null, url: string|null, reference: string|null, shasum: string|null}|null, softDeletedAt: string|null, deletionReason: string|null, lastBlockedReference: string|null, defaultBranch: int} $existingVersion
460461
*/
461-
private function applyStableImmutabilityGate(IOInterface $io, \Doctrine\ORM\EntityManagerInterface $em, Package $package, array $existingVersion, CompletePackageInterface $data, int $flags, VcsDriverInterface $driver, ?string $newEffectiveRef): VersionSkippedResult
462+
private function applyStableImmutabilityGate(IOInterface $io, \Doctrine\ORM\EntityManagerInterface $em, VersionRepository $versionRepo, Package $package, array $existingVersion, CompletePackageInterface $data, int $flags, VcsDriverInterface $driver, ?string $newEffectiveRef): VersionSkippedResult
462463
{
463464
$normVersion = $data->getVersion();
464465
$prettyVersion = $data->getPrettyVersion();
@@ -484,7 +485,7 @@ private function applyStableImmutabilityGate(IOInterface $io, \Doctrine\ORM\Enti
484485
}
485486

486487
if ($flags & self::UPDATE_SOURCE_DIST_URL) {
487-
$this->applySourceDistUrlRewrite($io, $em, $existingVersion, $data, $driver);
488+
$this->applySourceDistUrlRewrite($io, $versionRepo, $existingVersion, $data, $driver);
488489
}
489490

490491
return new VersionSkippedResult(id: $existingVersion['id'], version: strtolower($normVersion));
@@ -516,7 +517,7 @@ private function applyStableImmutabilityGate(IOInterface $io, \Doctrine\ORM\Enti
516517
*
517518
* @param array{id: int, source: array{type: string|null, url: string|null, reference: string|null}|null, dist: array{type: string|null, url: string|null, reference: string|null, shasum: string|null}|null} $existingVersion
518519
*/
519-
private function applySourceDistUrlRewrite(IOInterface $io, \Doctrine\ORM\EntityManagerInterface $em, array $existingVersion, CompletePackageInterface $data, VcsDriverInterface $driver): void
520+
private function applySourceDistUrlRewrite(IOInterface $io, VersionRepository $versionRepo, array $existingVersion, CompletePackageInterface $data, VcsDriverInterface $driver): void
520521
{
521522
$prettyVersion = $data->getPrettyVersion();
522523
$skip = function (string $reason) use ($io, $prettyVersion, $data, $existingVersion): void {
@@ -573,19 +574,36 @@ private function applySourceDistUrlRewrite(IOInterface $io, \Doctrine\ORM\Entity
573574
return;
574575
}
575576

576-
$newSource = ($oldSource ?? []) + [];
577+
$version = $versionRepo->find($existingVersion['id']);
578+
if (null === $version) {
579+
$skip('version not found by id');
580+
581+
return;
582+
}
583+
584+
Assert::notNull($oldSource);
585+
Assert::notNull($oldDist);
586+
$oldSourceUrl = $oldSource['url'] ?? null;
587+
$oldDistUrlStored = $oldDist['url'] ?? null;
588+
589+
$newSource = $oldSource;
577590
$newSource['url'] = $newSourceUrl;
578-
$newDist = ($oldDist ?? []) + [];
591+
$newDist = $oldDist;
579592
$newDist['url'] = $newDistUrl;
580593

581-
$em->getConnection()->executeStatement(
582-
'UPDATE package_version SET source = :source, dist = :dist WHERE id = :id',
583-
[
584-
'source' => json_encode($newSource, \JSON_UNESCAPED_SLASHES | \JSON_UNESCAPED_UNICODE),
585-
'dist' => json_encode($newDist, \JSON_UNESCAPED_SLASHES | \JSON_UNESCAPED_UNICODE),
586-
'id' => $existingVersion['id'],
587-
]
588-
);
594+
$version->setSource($newSource);
595+
$version->setDist($newDist);
596+
597+
$this->logger->info('Rewrote source/dist URL for stable version', [
598+
'package' => $data->getName(),
599+
'version' => $prettyVersion,
600+
'version_id' => $existingVersion['id'],
601+
'source_url_from' => $oldSourceUrl,
602+
'source_url_to' => $newSourceUrl,
603+
'dist_url_from' => $oldDistUrlStored,
604+
'dist_url_to' => $newDistUrl,
605+
'reference' => $newSourceRef,
606+
]);
589607
}
590608

591609
/**
@@ -625,7 +643,7 @@ private function updateInformation(IOInterface $io, VersionRepository $versionRe
625643

626644
// Stable-version immutability gate: any existing stable version is frozen.
627645
if (!$data->isDev()) {
628-
return $this->applyStableImmutabilityGate($io, $em, $package, $existingVersion, $data, $flags, $driver, $newEffectiveRef);
646+
return $this->applyStableImmutabilityGate($io, $em, $versionRepo, $package, $existingVersion, $data, $flags, $driver, $newEffectiveRef);
629647
}
630648
} elseif ($newEffectiveRef === null) {
631649
// Brand-new version with no usable identity: refuse to create.

‎tests/Package/UpdaterTest.php‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,98 @@ public function testDeleteBeforeWipesDevRowsButPreservesStableAndSoftDeletedStab
537537
self::assertSame(0, $this->countAudits(AuditRecordType::VersionReferenceChangeBlocked));
538538
}
539539

540+
public function testUpdateSourceDistUrlRewritesUrlsViaEntityAndKeepsRefsAndShasumIntact(): void
541+
{
542+
$em = self::getEM();
543+
$ref = str_repeat('a', 40);
544+
$shasum = str_repeat('b', 64);
545+
546+
$version = $this->seedStableVersion($this->package, '1.0.0', '1.0.0.0', $ref);
547+
$version->setSource(['type' => 'git', 'url' => 'https://old.example.com/test/pkg.git', 'reference' => $ref]);
548+
$version->setDist(['type' => 'zip', 'url' => 'https://old.example.com/dist/'.$ref, 'reference' => $ref, 'shasum' => $shasum]);
549+
$em->persist($version);
550+
$em->flush();
551+
$versionId = $version->getId();
552+
553+
$upstream = new CompletePackage('test/pkg', '1.0.0.0', '1.0.0');
554+
$upstream->setSourceType('git');
555+
$upstream->setSourceUrl('https://new.example.com/test/pkg.git');
556+
$upstream->setSourceReference($ref);
557+
$upstream->setDistType('zip');
558+
$upstream->setDistUrl('https://new.example.com/dist/'.$ref);
559+
$upstream->setDistReference($ref);
560+
// upstream shasum intentionally differs to prove the rewrite does NOT touch shasum
561+
$upstream->setDistSha1Checksum(str_repeat('c', 64));
562+
563+
$this->repositoryMock = $this->createStub(VcsRepository::class);
564+
$this->repositoryMock->method('getPackages')->willReturn([$upstream]);
565+
566+
$driver = $this->createStub(GitDriver::class);
567+
$driver->method('getRootIdentifier')->willReturn('master');
568+
$driver->method('getComposerInformation')->willReturn([]);
569+
$driver->method('getDist')->willReturn(['type' => 'zip', 'url' => 'https://new.example.com/dist/'.$ref, 'reference' => $ref, 'shasum' => $shasum]);
570+
$this->repositoryMock->method('getDriver')->willReturn($driver);
571+
572+
$packageManagerMock = $this->createMock(PackageManager::class);
573+
$packageManagerMock->expects($this->never())->method('notifyVersionReferenceChangeBlocked');
574+
$this->rebuildUpdater($packageManagerMock);
575+
576+
$this->updater->update($this->ioMock, $this->config, $this->package, $this->repositoryMock, Updater::UPDATE_SOURCE_DIST_URL);
577+
578+
$em->clear();
579+
$reloaded = $em->getRepository(Version::class)->find($versionId);
580+
self::assertNotNull($reloaded);
581+
self::assertSame('https://new.example.com/test/pkg.git', $reloaded->getSource()['url'] ?? null);
582+
self::assertSame($ref, $reloaded->getSource()['reference'] ?? null, 'source.reference must not change');
583+
self::assertSame('git', $reloaded->getSource()['type'] ?? null);
584+
self::assertSame('https://new.example.com/dist/'.$ref, $reloaded->getDist()['url'] ?? null);
585+
self::assertSame($ref, $reloaded->getDist()['reference'] ?? null, 'dist.reference must not change');
586+
self::assertSame($shasum, $reloaded->getDist()['shasum'] ?? null, 'dist.shasum must not be overwritten');
587+
588+
self::assertSame(0, $this->countAudits(AuditRecordType::VersionReferenceChanged));
589+
self::assertSame(0, $this->countAudits(AuditRecordType::VersionReferenceChangeBlocked));
590+
}
591+
592+
public function testUpdateSourceDistUrlSkipsWhenDriverDistUrlDoesNotMatch(): void
593+
{
594+
$em = self::getEM();
595+
$ref = str_repeat('a', 40);
596+
$shasum = str_repeat('b', 64);
597+
598+
$version = $this->seedStableVersion($this->package, '1.0.0', '1.0.0.0', $ref);
599+
$version->setSource(['type' => 'git', 'url' => 'https://old.example.com/test/pkg.git', 'reference' => $ref]);
600+
$version->setDist(['type' => 'zip', 'url' => 'https://old.example.com/dist/'.$ref, 'reference' => $ref, 'shasum' => $shasum]);
601+
$em->persist($version);
602+
$em->flush();
603+
$versionId = $version->getId();
604+
605+
$upstream = new CompletePackage('test/pkg', '1.0.0.0', '1.0.0');
606+
$upstream->setSourceType('git');
607+
$upstream->setSourceUrl('https://attacker.example.com/test/pkg.git');
608+
$upstream->setSourceReference($ref);
609+
$upstream->setDistType('zip');
610+
$upstream->setDistUrl('https://attacker.example.com/dist/'.$ref);
611+
$upstream->setDistReference($ref);
612+
613+
$this->repositoryMock = $this->createStub(VcsRepository::class);
614+
$this->repositoryMock->method('getPackages')->willReturn([$upstream]);
615+
616+
$driver = $this->createStub(GitDriver::class);
617+
$driver->method('getRootIdentifier')->willReturn('master');
618+
$driver->method('getComposerInformation')->willReturn([]);
619+
// driver-confirmed dist URL points elsewhere — the incoming claim must be rejected
620+
$driver->method('getDist')->willReturn(['type' => 'zip', 'url' => 'https://different.example.com/dist/'.$ref, 'reference' => $ref, 'shasum' => $shasum]);
621+
$this->repositoryMock->method('getDriver')->willReturn($driver);
622+
623+
$this->updater->update($this->ioMock, $this->config, $this->package, $this->repositoryMock, Updater::UPDATE_SOURCE_DIST_URL);
624+
625+
$em->clear();
626+
$reloaded = $em->getRepository(Version::class)->find($versionId);
627+
self::assertNotNull($reloaded);
628+
self::assertSame('https://old.example.com/test/pkg.git', $reloaded->getSource()['url'] ?? null, 'URL must not be rewritten when driver disagrees');
629+
self::assertSame('https://old.example.com/dist/'.$ref, $reloaded->getDist()['url'] ?? null);
630+
}
631+
540632
public function testAutoSoftDeletedDevVersionIsRecoveredWhenBranchReappearsWithUnchangedRef(): void
541633
{
542634
$existing = $this->seedDevVersion($this->package, 'dev-main', 'dev-main', 'sameref1234567890');

0 commit comments

Comments
 (0)