Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
15 changes: 10 additions & 5 deletions lib/Controller/SetupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -285,18 +285,23 @@ private function installDemoData(): DataResponse {
}

// Recorded only after the import actually returned. Marking it first
// would let a failed install present as a finished step.
// would let a failed install present as a finished step, and an import
// that stored nothing now throws above rather than returning zeroes.
$this->appConfig->setValueString('dossiq', self::DEMO_DATA_DECIDED_KEY, 'installed');

// 🔴 THE COUNTS, ALWAYS. "Demo data installed" with no numbers cannot be
// told apart from an import that wrote nothing.
// 🔴 THE COUNTS, ALWAYS, AND BOTH OF THEM. "Demo data installed" with no
// numbers cannot be told apart from an import that wrote nothing — and
// one number cannot be told apart from a number read out of the file it
// was asked to import. The landing is stated against the ask.
return new DataResponse(
[
'success' => true,
'message' => sprintf(
'Demo data installed: %d objects across %d schemas.',
'Demo data installed: %d of %d objects stored across %d schemas (%d refused).',
$imported['objects'],
$imported['schemas']
$imported['requested'],
$imported['schemas'],
$imported['refused']
),
'detail' => $imported,
]
Expand Down
61 changes: 52 additions & 9 deletions lib/Service/DemoDataService.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,20 @@ public function isAvailable(): bool {
* outcome to an operator who just asked for this, so "nothing happened"
* must not be presentable as success.
*
* @return array{objects: integer, registers: integer, schemas: integer} What was imported.
* 🔴 AND THE COUNT IS WHAT LANDED, NOT WHAT WAS ASKED FOR. This method used
* to count `components.objects` in the shipped file and report that as the
* result, with a comment saying the number reported is "the number ASKED
* FOR". The ask is not an outcome: a descriptor of 456 objects reported
* "456 objects" whether the importer stored 456, three or none, so the ten
* demo keys no schema declared (#1782) were stripped on the way in under a
* green message that could not have said otherwise. `importFromJson()`
* answers with `objects` — the entities it created or updated — and
* `skipped.objects` — the ones it refused. Both are read here, and both are
* returned, so a caller can print the landing next to the ask.
*
* @throws RuntimeException When the descriptor is missing, unreadable, or OpenRegister is absent.
* @return array{objects: integer, requested: integer, refused: integer, registers: integer, schemas: integer} What was asked for and what landed.
*
* @throws RuntimeException When the descriptor is missing or unreadable, OpenRegister is absent, or the import stored nothing.
*
* @spec openspec/changes/first-time-setup/specs/first-time-setup/spec.md
*/
Expand All @@ -127,13 +138,12 @@ public function install(): array {
throw new RuntimeException('The demo dataset is not valid JSON: ' . $path);
}

// Counted from the file rather than from the importer's reply, so the
// number reported is the number ASKED FOR. A discrepancy between this
// and what lands is a real condition an operator should be able to see.
$objects = 0;
// The ASK: how many objects the shipped descriptor carries. Kept, but
// as one half of a comparison rather than as the answer.
$requested = 0;
$components = ($data['components'] ?? []);
if (is_array($components) === true && is_array(($components['objects'] ?? null)) === true) {
$objects = count($components['objects']);
$requested = count($components['objects']);
}

$result = $this->configurationService()->importFromApp(
Expand All @@ -143,20 +153,53 @@ public function install(): array {
force: true
);

// The LANDING. An importer reply with no `objects` key has said nothing
// about objects, and nothing is zero — never "as many as we asked for".
$skipped = (array)($result['skipped'] ?? []);
$imported = [
'objects' => $objects,
'objects' => count((array)($result['objects'] ?? [])),
'requested' => $requested,
'refused' => (int)($skipped['objects'] ?? 0),
'registers' => count((array)($result['registers'] ?? [])),
'schemas' => count((array)($result['schemas'] ?? [])),
];

// 🔴 AN IMPORT THAT STORED NOTHING IS NOT A SUCCESS, and this is the
// only place that can tell. Same shape as the seed steps of #1767 and
// #1769, which reported `success: true` with every counter at zero and
// recorded themselves as done. A descriptor that ships no objects at
// all is a different condition and stays a success: registers and
// schemas are a legitimate thing to ship on their own.
if ($requested > 0 && $imported['objects'] === 0) {
throw new RuntimeException(
'The demo import stored 0 of ' . $requested . ' object(s) ('
. $imported['refused'] . ' refused by OpenRegister). Nothing was written, so this is not '
. 'an install. Check the OpenRegister log for the refusals, and note that an object whose '
. 'version has not moved is left alone: re-importing an already-imported demo set lands nothing.'
);
}

$this->logger->info(
'[DemoDataService] imported demo data: '
. $imported['objects'] . ' object(s), '
. $imported['objects'] . ' of ' . $requested . ' object(s) stored, '
. $imported['refused'] . ' refused, '
. $imported['registers'] . ' register(s), '
. $imported['schemas'] . ' schema(s).',
['app' => Application::APP_ID]
);

if ($imported['objects'] < $requested) {
// Partial. Louder than info on purpose: some of what the app ships
// did not survive the import, and the message above is the only
// place the difference is visible.
$this->logger->warning(
'[DemoDataService] the demo import lost ' . ($requested - $imported['objects'])
. ' of ' . $requested . ' object(s) — ' . $imported['refused'] . ' refused, the rest were '
. 'left unchanged because an object of the same version already exists.',
['app' => Application::APP_ID]
);
}

return $imported;
}//end install()

Expand Down
130 changes: 126 additions & 4 deletions tests/Unit/Service/DemoDataServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,32 @@ private function shipDescriptor(int $objects = 2): void {
/**
* A stand-in for OpenRegister's importer that records how it was called.
*
* 🔴 IT ANSWERS IN THE IMPORTER'S REAL SHAPE, AND THAT IS THE POINT. The
* first version of this fake returned `registers` and `schemas` and NO
* `objects` key at all, while the assertion below expected five objects.
* A fake written from the call site encodes the caller's bug: the only
* reason those two agreed is that the service was reading its object count
* out of the FILE. `ImportHandler::importFromJson()` returns
* `objects: array<ObjectEntity>` — what it created or updated — alongside
* `skipped: array{objects: int, ...}` — what it refused.
*
* @param integer $landed Objects the importer says it created or updated.
* @param integer $refused Objects the importer says it refused.
*
* @return object The fake.
*/
private function importerSpy(): object {
return new class {
private function importerSpy(int $landed = 2, int $refused = 0): object {
return new class($landed, $refused) {
/** @var array<string, mixed> */
public array $seen = [];

/**
* @param integer $landed Objects created or updated.
* @param integer $refused Objects refused.
*/
public function __construct(private readonly int $landed, private readonly int $refused) {
}

/**
* @param string $appId Config identity.
* @param array<string, mixed> $data Descriptor.
Expand All @@ -103,23 +122,126 @@ private function importerSpy(): object {
*/
public function importFromApp(string $appId, array $data, string $version, bool $force): array {
$this->seen = ['appId' => $appId, 'version' => $version, 'force' => $force];
return ['registers' => ['dossiq'], 'schemas' => ['Thing']];
return [
'registers' => ['dossiq'],
'schemas' => ['Thing'],
'objects' => array_fill(0, $this->landed, 'entity'),
'skipped' => ['registers' => 0, 'schemas' => 0, 'objects' => $this->refused],
];
}
};
}

public function testItImportsTheDescriptorAndReportsTheCounts(): void {
$this->shipDescriptor(objects: 5);
$spy = $this->importerSpy();
$spy = $this->importerSpy(landed: 5);
$this->container->method('get')->willReturn($spy);

$result = $this->service->install();

$this->assertSame(5, $result['objects']);
$this->assertSame(5, $result['requested']);
$this->assertSame(0, $result['refused']);
$this->assertSame(1, $result['registers']);
$this->assertSame(1, $result['schemas']);
}

/**
* 🔴 THE COUNT IS WHAT LANDED, NOT WHAT WAS ASKED FOR.
*
* `install()` used to count `components.objects` in the shipped file and
* report that number as the import's result, with a comment saying the
* number reported is "the number ASKED FOR". So a descriptor carrying 456
* objects reported "456 objects" whether the importer stored 456, 3 or
* none — and the ten undeclared demo keys of #1782 were stripped on the
* way in under exactly that green message. The ask and the landing are two
* different numbers and the service must report both.
*/
public function testTheObjectCountComesFromTheImporterNotFromTheFile(): void {
$this->shipDescriptor(objects: 9);
// The file asks for nine; the importer says it stored three and refused two.
$this->container->method('get')->willReturn($this->importerSpy(landed: 3, refused: 2));

$result = $this->service->install();

$this->assertSame(3, $result['objects'], 'the count must be the importer\'s reply, not the file\'s length');
$this->assertSame(9, $result['requested']);
$this->assertSame(2, $result['refused'], 'a refusal is a real outcome and must be counted');
}

/**
* 🔴 AN IMPORT THAT STORES NOTHING IS NOT A SUCCESS.
*
* Same shape as the seed steps of #1767 and #1769: a step that touched
* nothing reported `success: true` with every counter at zero, was recorded
* as done, and was never offered again. With the count read from the file
* this state was unreachable — zero landed still printed the file's length
* — so there was nothing for a caller to test against.
*/
public function testAnImportThatStoresNothingThrowsRatherThanReportingSuccess(): void {
$this->shipDescriptor(objects: 4);
$this->container->method('get')->willReturn($this->importerSpy(landed: 0));

$this->expectException(RuntimeException::class);
$this->expectExceptionMessageMatches('/0 of 4/');
$this->service->install();
}

/**
* The refusal count is what tells an operator WHY nothing landed, so it has
* to survive into the message rather than being folded into a bare zero.
*/
public function testWhenEveryObjectIsRefusedTheMessageSaysSo(): void {
$this->shipDescriptor(objects: 4);
$this->container->method('get')->willReturn($this->importerSpy(landed: 0, refused: 4));

$this->expectException(RuntimeException::class);
$this->expectExceptionMessageMatches('/4 refused/');
$this->service->install();
}

/**
* An importer that answers without an `objects` key has told us nothing
* about objects. Reading that absence as "zero landed" is right; reading it
* as "as many as the file asked for" is the defect.
*/
public function testAnImporterReplyWithNoObjectsKeyCountsAsNothingLanded(): void {
$this->shipDescriptor(objects: 3);
$mute = new class {
/**
* @param string $appId Config identity.
* @param array<string, mixed> $data Descriptor.
* @param string $version App version.
* @param boolean $force Whether the version gate is bypassed.
*
* @return array<string, mixed>
*/
public function importFromApp(string $appId, array $data, string $version, bool $force): array {
return ['registers' => ['dossiq'], 'schemas' => ['Thing']];
}
};
$this->container->method('get')->willReturn($mute);

$this->expectException(RuntimeException::class);
$this->expectExceptionMessageMatches('/0 of 3/');
$this->service->install();
}

/**
* A descriptor with no objects at all is a different condition from an
* import that lost them, and must not be turned into a failure: registers
* and schemas are a legitimate thing to ship on their own.
*/
public function testADescriptorThatShipsNoObjectsIsNotTreatedAsALostImport(): void {
$this->shipDescriptor(objects: 0);
$this->container->method('get')->willReturn($this->importerSpy(landed: 0));

$result = $this->service->install();

$this->assertSame(0, $result['objects']);
$this->assertSame(0, $result['requested']);
}

/**
* 🔴 THE IMPORT IS FORCED. OpenRegister version-gates a non-forced import
* and SKIPS silently when the version has not moved. An operator who asks
Expand Down
Loading