Skip to content

Commit 85976da

Browse files
turegjorupturegjorup
committed
refactor: make the version factories stateless, reset LeantimeService
Changes - `PackageVersionFactory` and `ModuleVersionFactory` keep their deduplication buffers in locals threaded through the private helpers, not in properties - Key the version buffers on object identity rather than on the entity id, so they hold before Doctrine has assigned one - `LeantimeService` implements `ResetInterface`; autoconfigure tags it `kernel.reset` - Add tests for all three; the factories had none Why The contract advises statelessness over `ResetInterface` where it is possible, and for the factories it is: the buffers exist only to stand in for the repositories between `persist()` and `flush()` within one call, so their lifetime is exactly that call. Making them locals also fixes the reason they were flagged — they were cleared after `flush()` rather than in a `finally`, so a failing flush left entities from a closed EntityManager for the next call. That was a live bug in the messenger consumer, which already runs long. `LeantimeService` is the case the fallback is for. Its cache cannot become a local: `resolveUserName()` is called inside a loop over tickets, so dropping it would cost an API round trip per ticket. `reset()` restores the per-request lifetime `loadUsers()` already documents. The two `testAFailedFlushLeavesNothingForTheNextCall` tests fail against the previous implementations; the other thirteen pass either way and guard the deduplication behaviour, including a null-version quirk left deliberately intact.
1 parent aa1ad2f commit 85976da

7 files changed

Lines changed: 524 additions & 65 deletions

‎CHANGELOG.md‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3232
never happened
3333
- Leave worker mode off for now, but not for want of a runtime: `symfony/runtime`
3434
has shipped `FrankenPhpWorkerRunner` since 7.4, so enabling it is one line in
35-
`.docker/Caddyfile` and needs no package. Three stateful services want
36-
attention first
35+
`.docker/Caddyfile` and needs no package
36+
- Make `PackageVersionFactory` and `ModuleVersionFactory` stateless: their
37+
deduplication buffers are locals rather than properties, so a failing flush
38+
can no longer leave entities from a closed EntityManager for the next call.
39+
This was a live bug in the messenger consumer, which is already long-running
40+
- Implement `ResetInterface` on `LeantimeService`, whose memoised user
41+
directory has to stay a cache — `resolveUserName()` runs in a loop over
42+
tickets — but must not outlive the request
43+
- Cover all three with tests: the factories had none
3744
- [#96](https://github.com/itk-dev/devops_itksites/pull/96)
3845
Show the Service Agreements monthly price as Danish kroner,
3946
`12.500,50 kr.`, on index and detail

‎src/Service/LeantimeService.php‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
use Symfony\Contracts\HttpClient\Exception\ExceptionInterface;
88
use Symfony\Contracts\HttpClient\HttpClientInterface;
9+
use Symfony\Contracts\Service\ResetInterface;
910

1011
/**
1112
* Minimal JSON-RPC 2.0 client for the Leantime API.
@@ -16,7 +17,7 @@
1617
* the x-api-key header; this class only assembles the JSON-RPC body and
1718
* unwraps responses.
1819
*/
19-
class LeantimeService
20+
class LeantimeService implements ResetInterface
2021
{
2122
private const string JSONRPC_VERSION = '2.0';
2223
private const string API_PATH = '/api/jsonrpc/';
@@ -281,4 +282,28 @@ private function loadUsers(): void
281282
}
282283
}
283284
}
285+
286+
/**
287+
* Drop the memoised user directory.
288+
*
289+
* loadUsers() fetches the Leantime directory once and every later lookup
290+
* reads these two maps. Under php-fpm the instance died with the request,
291+
* so "once per service instance" also meant "once per request". In a
292+
* long-running worker the instance outlives the request and the directory
293+
* would never be refetched, leaving new users, renames and changed
294+
* addresses invisible until the worker recycled.
295+
*
296+
* Unlike the package and module factories, this cache cannot simply become
297+
* a local: resolveUserName() is called inside a loop over tickets, so
298+
* dropping it would cost one API round trip per ticket. Resetting it
299+
* restores the per-request lifetime loadUsers() already documents.
300+
*
301+
* autoconfigure tags this kernel.reset, and services_resetter calls it
302+
* between requests.
303+
*/
304+
public function reset(): void
305+
{
306+
$this->userNamesById = null;
307+
$this->userIdsByEmail = null;
308+
}
284309
}

‎src/Service/ModuleVersionFactory.php‎

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@
1414

1515
class ModuleVersionFactory
1616
{
17-
private array $createdModules = [];
18-
private array $createdModuleVersions = [];
19-
2017
public function __construct(
2118
private readonly EntityManagerInterface $entityManager,
2219
private readonly ModuleRepository $moduleRepository,
@@ -26,41 +23,42 @@ public function __construct(
2623

2724
public function setModuleVersions(Installation $installation, object $installedModules): void
2825
{
26+
// Locals, not properties: see PackageVersionFactory for why. Entities
27+
// persisted below are not flushed until the end of the call, so these
28+
// maps stand in for the repositories until then, and nothing outlives
29+
// the method.
30+
$createdModules = [];
31+
$createdModuleVersions = [];
32+
2933
$moduleVersions = new ArrayCollection();
3034
foreach ($installedModules as $name => $installed) {
31-
$module = $this->getModule($name, $installed->package);
35+
$module = $this->getModule($name, $installed->package, $createdModules);
3236

3337
if (isset($installed->display_name)) {
3438
$module->setDisplayName($installed->display_name);
3539
}
3640
$module->setEnabled('Enabled' === $installed->status);
3741

38-
$moduleVersion = $this->getModuleVersion($module, $installed->version);
42+
$moduleVersion = $this->getModuleVersion($module, $installed->version, $createdModuleVersions);
3943
$moduleVersions->add($moduleVersion);
4044
}
4145

4246
$installation->setModuleVersions($moduleVersions);
4347

4448
$this->entityManager->flush();
45-
$this->createdModules = [];
46-
$this->createdModuleVersions = [];
4749
}
4850

49-
private function getModule(string $name, string $package): Module
51+
/**
52+
* @param array<string, Module> $createdModules modules persisted in this call but not yet flushed, keyed by name and package
53+
*/
54+
private function getModule(string $name, string $package, array &$createdModules): Module
5055
{
56+
$key = $name."\0".$package;
57+
5158
$module = $this->moduleRepository->findOneBy([
5259
'name' => $name,
5360
'package' => $package,
54-
]);
55-
56-
if (null === $module) {
57-
/** @var Module $createdModule */
58-
foreach ($this->createdModules as $createdModule) {
59-
if ($name === $createdModule->getName() && $package === $createdModule->getPackage()) {
60-
$module = $createdModule;
61-
}
62-
}
63-
}
61+
]) ?? $createdModules[$key] ?? null;
6462

6563
if (null === $module) {
6664
$module = new Module();
@@ -69,13 +67,16 @@ private function getModule(string $name, string $package): Module
6967
$module->setName($name);
7068
$module->setPackage($package);
7169

72-
$this->createdModules[] = $module;
70+
$createdModules[$key] = $module;
7371
}
7472

7573
return $module;
7674
}
7775

78-
private function getModuleVersion(Module $module, string|int|float|null $version): ModuleVersion
76+
/**
77+
* @param array<string, ModuleVersion> $createdModuleVersions versions persisted in this call but not yet flushed, keyed by module identity and version
78+
*/
79+
private function getModuleVersion(Module $module, string|int|float|null $version, array &$createdModuleVersions): ModuleVersion
7980
{
8081
if (is_int($version) || is_float($version)) {
8182
$version = (string) $version;
@@ -86,13 +87,15 @@ private function getModuleVersion(Module $module, string|int|float|null $version
8687
'version' => $version,
8788
]);
8889

89-
if (null === $moduleVersion) {
90-
/** @var ModuleVersion $createdModuleVersion */
91-
foreach ($this->createdModuleVersions as $createdModuleVersion) {
92-
if ($module->getId() === $createdModuleVersion->getModule()->getId() && $version === $createdModuleVersion->getVersion()) {
93-
$moduleVersion = $createdModuleVersion;
94-
}
95-
}
90+
// A null version is deliberately left out of the buffer.
91+
// ModuleVersion::getVersion() reports 'Unknown' for null, so the scan
92+
// this replaced never matched a null-versioned module either. Keeping
93+
// that quirk keeps this change about state and nothing else; see the
94+
// note in the pull request.
95+
$key = null === $version ? null : spl_object_id($module)."\0".$version;
96+
97+
if (null === $moduleVersion && null !== $key) {
98+
$moduleVersion = $createdModuleVersions[$key] ?? null;
9699
}
97100

98101
if (null === $moduleVersion) {
@@ -102,7 +105,9 @@ private function getModuleVersion(Module $module, string|int|float|null $version
102105
$module->addModuleVersion($moduleVersion);
103106
$moduleVersion->setVersion($version);
104107

105-
$this->createdModuleVersions[] = $moduleVersion;
108+
if (null !== $key) {
109+
$createdModuleVersions[$key] = $moduleVersion;
110+
}
106111
}
107112

108113
return $moduleVersion;

‎src/Service/PackageVersionFactory.php‎

Lines changed: 32 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@
1414

1515
class PackageVersionFactory
1616
{
17-
private array $createdPackages = [];
18-
private array $createdPackageVersions = [];
19-
2017
public function __construct(
2118
private readonly EntityManagerInterface $entityManager,
2219
private readonly PackageRepository $packageRepository,
@@ -26,11 +23,22 @@ public function __construct(
2623

2724
public function setPackageVersions(Installation $installation, array $installedPackages): void
2825
{
26+
// Entities are persisted below but not flushed until the end of this
27+
// method, so the repositories cannot find them yet. These two maps
28+
// stand in for the repositories for the rest of the call, keeping the
29+
// same package from being created twice.
30+
//
31+
// They are locals, not properties. Nothing survives the method, so the
32+
// service holds no state between calls — which is what makes it safe in
33+
// a long-running process, the messenger consumer included.
34+
$createdPackages = [];
35+
$createdPackageVersions = [];
36+
2937
$packageVersions = new ArrayCollection();
3038
foreach ($installedPackages as $installed) {
3139
[$vendor, $name] = explode('/', (string) $installed->name);
3240

33-
$package = $this->getPackage($vendor, $name);
41+
$package = $this->getPackage($vendor, $name, $createdPackages);
3442

3543
$package->setDescription($installed->description);
3644
if (isset($installed->warning)) {
@@ -40,7 +48,7 @@ public function setPackageVersions(Installation $installation, array $installedP
4048
$package->setAbandoned($installed->abandoned);
4149
}
4250

43-
$packageVersion = $this->getPackageVersion($package, $installed->version);
51+
$packageVersion = $this->getPackageVersion($package, $installed->version, $createdPackageVersions);
4452
$installation->addPackageVersion($packageVersion);
4553

4654
$packageVersion->setVersion($installed->version);
@@ -50,35 +58,26 @@ public function setPackageVersions(Installation $installation, array $installedP
5058
if (isset($installed->{'latest-status'})) {
5159
$packageVersion->setLatestStatus($installed->{'latest-status'});
5260
}
53-
if (isset($installed->{'latest-status'})) {
54-
$packageVersion->setLatestStatus($installed->{'latest-status'});
55-
}
5661

5762
$packageVersions->add($packageVersion);
5863
}
5964

6065
$installation->setPackageVersions($packageVersions);
6166

6267
$this->entityManager->flush();
63-
$this->createdPackages = [];
64-
$this->createdPackageVersions = [];
6568
}
6669

67-
private function getPackage(string $vendor, string $name): Package
70+
/**
71+
* @param array<string, Package> $createdPackages packages persisted in this call but not yet flushed, keyed by vendor and name
72+
*/
73+
private function getPackage(string $vendor, string $name, array &$createdPackages): Package
6874
{
75+
$key = $vendor."\0".$name;
76+
6977
$package = $this->packageRepository->findOneBy([
7078
'vendor' => $vendor,
7179
'name' => $name,
72-
]);
73-
74-
if (null === $package) {
75-
/** @var Package $createdPackage */
76-
foreach ($this->createdPackages as $createdPackage) {
77-
if ($vendor === $createdPackage->getVendor() && $name === $createdPackage->getName()) {
78-
$package = $createdPackage;
79-
}
80-
}
81-
}
80+
]) ?? $createdPackages[$key] ?? null;
8281

8382
if (null === $package) {
8483
$package = new Package();
@@ -87,27 +86,26 @@ private function getPackage(string $vendor, string $name): Package
8786
$package->setVendor($vendor);
8887
$package->setName($name);
8988

90-
$this->createdPackages[] = $package;
89+
$createdPackages[$key] = $package;
9190
}
9291

9392
return $package;
9493
}
9594

96-
private function getPackageVersion(Package $package, string $version): PackageVersion
95+
/**
96+
* @param array<string, PackageVersion> $createdPackageVersions versions persisted in this call but not yet flushed, keyed by package identity and version
97+
*/
98+
private function getPackageVersion(Package $package, string $version, array &$createdPackageVersions): PackageVersion
9799
{
100+
// Keyed on object identity rather than on the id: within one call the
101+
// package may have been created moments ago, and object identity holds
102+
// whether or not Doctrine has assigned an id yet.
103+
$key = spl_object_id($package)."\0".$version;
104+
98105
$packageVersion = $this->packageVersionRepository->findOneBy([
99106
'package' => $package,
100107
'version' => $version,
101-
]);
102-
103-
if (null === $packageVersion) {
104-
/* @var PackageVersion $packageVersion */
105-
foreach ($this->createdPackageVersions as $createdPackageVersion) {
106-
if ($package->getId() === $createdPackageVersion->getPackage()->getId() && $version === $createdPackageVersion->getVersion()) {
107-
$packageVersion = $createdPackageVersion;
108-
}
109-
}
110-
}
108+
]) ?? $createdPackageVersions[$key] ?? null;
111109

112110
if (null === $packageVersion) {
113111
$packageVersion = new PackageVersion();
@@ -116,7 +114,7 @@ private function getPackageVersion(Package $package, string $version): PackageVe
116114
$package->addPackageVersion($packageVersion);
117115
$packageVersion->setVersion($version);
118116

119-
$this->createdPackageVersions[] = $packageVersion;
117+
$createdPackageVersions[$key] = $packageVersion;
120118
}
121119

122120
return $packageVersion;

0 commit comments

Comments
 (0)