fix(demo): stop the demo import forking the schema set and renaming the register - #1806
Merged
Merged
Conversation
…he register One call to `POST /api/setup/action/install-demo-data` handed OpenRegister's importer the whole mock descriptor under the configuration id `dossiq.demo`, with `force: true`. Two things followed, and neither said anything at the time. Schemas are matched by the PAIR (application, slug), so under `dossiq.demo` every slug missed and the importer's not-found branch is the CREATE branch. Measured on a clean rig: 139 duplicate schemas, 393 demo objects bound to them and invisible to an app that reads its schemas under `dossiq`. A register is matched by SLUG alone, so the descriptor's own register entry matched the live row and overwrote it: "Dossiq Case Management Register" became "Dossiq (demo)", its application became `dossiq.demo`, its version went 1.1.0 back to 1.0.0. `CONFIG_APP_ID` is kept. Its own namespace was never the bug: it keys the Configuration row and the `imported_config_<app>_version` / `_hash` pair, so the demo import and the real one cannot mask each other's version gate. The bug is that the same string also named the SCHEMA OWNER during the definitional pass. Handing those definitions over under `dossiq` instead would be worse: the mock file is a snapshot whose `case` is v1.9.0 with 50 properties where the shipped schema is v1.13.0 with 56, and a forced import writes the older one over the live one. So the definitional pass goes, not the namespace. The payload is narrowed to its objects, which resolve against the register and schemas the app already owns. RepairDemoDataSchemaFork repairs the instances that already ran it. It removes a schema only when all three hold: its application is exactly `dossiq.demo`, exactly one schema of that slug exists under `dossiq`, and no register lists its id. Anything failing any of the three is kept and named in the output. Removal goes through OpenRegister's audited cascade, so even what it does delete leaves a hash-chained snapshot. Also in here, all found by the same fresh-rig proof: - SeedVthWorkflowTemplates ran BEFORE VthSeedDataRepairStep, which provisions the case types it resolves, in both blocks. Reordered, and the pair is now asserted in RepairStepRegistrationTest. - Fixing the order alone left the count at zero. VthSeedLookup could not read a case type whose slug is metadata rather than a property, so both its probes missed rows that were there. A third probe reads `@self.slug`. - The skip messages named `base-register-seed-data`, which is not a repair step, not a command and not anything an operator can run. They now name what is actually missing. - LinkInFlightRemainingDecisionsRepair passed the register as a slug and the schema as its numeric config id, which sends the search bridge down its slug path with a number. Every surface warned and no object was examined. - docs/admin/flows.md claimed "the install says so". Only `maintenance:repair` and `upgrade` subscribe to what a repair step reports, so `occ app:enable` prints nothing. The doc says which path shows what, and the log line carries the adoption command. Verified locally: phpunit (2939 tests), phpcs, psalm, phpstan, phpmd per subdirectory, hydra gates. Proved on a throwaway rig on :8617. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
rubenvdlinde
requested review from
Rem-Dam,
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
September 4, 2026 15:03
Contributor
Quality Report — ConductionNL/dossiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue3-compile | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 106/106 | |||
| npm | ✅ | ✅ 541/541 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-04 15:10 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
pushed a commit
that referenced
this pull request
Sep 4, 2026
development gained #1806, which fixed the demo import from the OTHER end: it stops the import forking the schema set and renaming the register, which is what made 24 objects refuse as "ambiguous". That is the first-install half. This branch fixes the re-install half, so the two are complementary rather than competing, and both are needed for the spec to pass: #1806 makes the first install actually store its objects this branch stops "nothing needed storing" being reported as failure Resolved to keep both. The guard keeps development's much better diagnostic message (check the register import has run, a slug resolving to more than one row is refused as ambiguous) and gains the `unchanged === 0` condition, minus development's closing sentence about a re-import landing nothing: that case is now a success, so it no longer belongs in an error. The operator message keeps development's shape, which drops the schema count because a demo set defines no schemas, and keeps the "already present" count, which is what explains a zero. Also widened SetupControllerStatusTest's demo double, which returned an array without `unchanged` and made the controller read an undefined key. Same defect as the one that double already carries a comment about. PHPUnit 2944 green, phpcs, phpstan, psalm, lint, stylelint, prettier and vitest all exit 0.
This was referenced Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
One call to the app's own
POST /api/setup/action/install-demo-dataforked the schema set and defaced the live register. Reproduced on two independent clean installs; the numbers below are from a throwaway rig on :8617.DemoDataServicehanded OpenRegister's importer the whole mock descriptor under the configuration iddossiq.demo, withforce: true. The descriptor ships the register and the 139 schemas its objects were generated from, and two different matching rules turned that into damage:(application, slug). Underdossiq.demoevery slug missed, and the importer's not-found branch is the CREATE branch. Result: 139 duplicate schemas and 393 demo objects bound to them, invisible to an app that resolves its schemas underdossiq.dossiq.demo/caseheld three cases the case list never showed.updateFromArray()overwrote it:Dossiq Case Management RegisterbecameDossiq (demo), application becamedossiq.demo, version went 1.1.0 back to 1.0.0.dossiq#1784's counting fix was working correctly throughout. It honestly reported 417 of 444. The count was right; it was counting into a fork.
What
CONFIG_APP_IDwas for, and what changedIt is kept, and its own namespace was never the bug. OpenRegister keys a Configuration row and the
imported_config_<app>_version/_hashpair by this string, so a shared id would let the demo import and the real configuration import mask each other's version gate, and would retitle the app's own Configuration row todossiq demo data.The bug is that the same string also names the schema owner during the definitional pass. Handing the definitions over under
dossiqinstead is worse, not better: the mock file is a snapshot whosecaseis v1.9.0 with 50 properties where the shipped schema is v1.13.0 with 56, and a forced import writes the older one over the live one. Installing demo data must not be able to downgrade a schema.So the definitional pass goes, not the namespace.
objectsOnly()stripscomponents.registersandcomponents.schemas(and the top-level spellings) before the import. Objects need neither:ImportHandlerresolves a seed object's@selfslugs against the in-flight maps and falls back to a direct mapper lookup, which finds the rows the app's own configuration import already created.registersandschemasin the reply are now expected to be0, and that is asserted rather than left to read as a regression later. The setup message dropped its schema count with them: "stored across 139 schemas" was reporting the fork as a feature.The repair, and what it spares
RepairDemoDataSchemaFork(post-migration, reachable viaocc maintenance:repair) is what makes the fix usable on an instance that already ran the defective import: with two rows sharing a slug, the corrected object-only import resolves the schema ambiguously and refuses every object.A schema is removed only when all three hold:
applicationis exactlydossiq.demo, a string nothing but the defect has ever written;dossiq, so the real home demonstrably exists;Anything failing any of the three is kept and named in the output. A slug with no twin may be the only copy of something. A slug with several twins is a question about data, not a repair. A schema a register still links is reachable, and reachable is not unambiguously the fork. Unreadable registers are treated as "everything is linked", so a failed read spares rather than deletes.
Removal goes through OpenRegister's own
SchemaDeletionService, which snapshots every object into the hash-chained audit trail before deleting and only then drops the magic table. The register's identity is restored from the shippedinfoblock, and only when the title still reads exactlyDossiq (demo), so an operator who renamed it keeps their name. The demo Configuration row is kept as the record that this instance ran the import; only its dangling schema ids are pruned.Also fixed, all found by the same fresh-rig proof
SeedVthWorkflowTemplatesran before the case types it needs (both blocks). Reordered behindVthSeedDataRepairStep, andRepairStepRegistrationTestnow asserts the pair so it cannot regress silently.The ordering fix alone left the count at zero.
VthSeedLookupfiltered onidentifierandslugas object properties, butVthSeedDataRepairStepwrites a case type's slug as metadata. Both probes missed rows that were sitting there. A third probe reads@self.slug, and four of the five case types now resolve on the rig.The skip message named a step that does not exist. "run
base-register-seed-datafirst" is not a repair step, not a command and not anything an operator can run, and it was the only diagnosis on offer, which is how two real defects hid behind it. The messages now name what is actually missing.LinkInFlightRemainingDecisionsRepairpassed numeric schema ids where slugs belong. The bridge picks its path on "is either side non-numeric", so a slug register plus a numeric schema took the slug path and asked OpenRegister for a schema whose slug is a number. Every surface warned and not one object was examined, reported as "0 linked, 0 skipped, 0 errors".docs/admin/flows.mdclaimed "the install says so".AppManager::executeRepairStepsdispatches events that onlymaintenance:repair,upgradeand the web updater subscribe to, soocc app:enable dossiq(the docker path) prints nothing. The doc now says which path shows what, and the log line carries the adoption command so it is actionable on its own.Found and NOT fixed here
Nothing writes a VTH case type's
statusTypes.VthSeedDataRepairStep::seedCaseTypes()strips them on the stated grounds thatSeedVthWorkflowTemplatesowns them; that step only reads them and skips the whole template when it finds none. Measured on the rig: six VTH case types, 46 statusTypes on the instance, none attached to a VTH case type. Deciding which step writes them, and on what idempotency key, changes what an install provisions, so it is filed rather than guessed at. The false ownership comment is corrected and both the output and the log now name the missing thing.Rig evidence (:8617, own compose project, torn down with
down -v)dossiqdossiq.demodossiq/case(what the UI reads)occ maintenance:repairrun three more times: idempotent, and the second run reports "nodossiq.demoschemas; nothing to retire".Verified locally
CI is bottlenecked, so this was verified locally and each tool judged by its exit code:
phpunitfull suite: 2939 tests, 0 failures, exit 0phpcs --standard=phpcs.xml lib/: exit 0psalm: no errors, exit 0phpstan: no errors, exit 0phpmdper subdirectory (lib/Repair,lib/Service,lib/Controller) plus the unused-params ruleset: exit 0composer lint: exit 0^1.11.1) withHYDRA_GATE_BASE_REF=origin/development --scope-to-diff: 41 of 41 applicable gates green, on a non-empty diffFrontend checks were not run: this diff touches no JS, Vue, manifest or l10n file. One note on
StubApiDriftTest: it failed at first against a siblingopenregistercheckout that was 46 commits behind. The stub matches openregister'sorigin/developmentexactly (checked against the raw file), and the failure disappeared once the sibling was updated. CI clones openregister fresh, so it was never a real finding.🤖 Generated with Claude Code