diff --git a/lib/Controller/SetupController.php b/lib/Controller/SetupController.php index 37e275524..9598ce739 100644 --- a/lib/Controller/SetupController.php +++ b/lib/Controller/SetupController.php @@ -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, ] diff --git a/lib/Service/DemoDataService.php b/lib/Service/DemoDataService.php index 3ed3fa681..5714cafb4 100644 --- a/lib/Service/DemoDataService.php +++ b/lib/Service/DemoDataService.php @@ -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 */ @@ -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( @@ -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() diff --git a/tests/Unit/Service/DemoDataServiceTest.php b/tests/Unit/Service/DemoDataServiceTest.php index c01be80f3..8d926f058 100644 --- a/tests/Unit/Service/DemoDataServiceTest.php +++ b/tests/Unit/Service/DemoDataServiceTest.php @@ -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` — 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 */ 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 $data Descriptor. @@ -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 $data Descriptor. + * @param string $version App version. + * @param boolean $force Whether the version gate is bypassed. + * + * @return array + */ + 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