Skip to content

fix(repair): move OpenRegister schemas from softwarecatalog onto the stackiq application id - #723

Merged
rubenvdlinde merged 4 commits into
developmentfrom
feat/migrate-schema-application-id
Aug 24, 2026
Merged

fix(repair): move OpenRegister schemas from softwarecatalog onto the stackiq application id#723
rubenvdlinde merged 4 commits into
developmentfrom
feat/migrate-schema-application-id

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What

Adds MigrateSchemaApplicationId, a repair step that moves this app's OpenRegister
schemas from application = 'softwarecatalog' onto application = 'stackiq'.
Registered in appinfo/info.xml under both <post-migration> and <install>.

It is a different column from RenameDutchSchemaSlugs — that step rewrites
openregister_schemas.slug, this one rewrites openregister_schemas.application
and neither covers the other. It is also the third store keyed by app id:
oc_appconfig and oc_preferences are Nextcloud's and are already carried by
MigrateAppConfigKeys / MigrateUserPreferences; this one is OpenRegister's.

The two lookups

OpenRegister resolves the two halves of a configuration by different keys:

resolved by
register RegisterMapper::find(slug) — slug alone
schema SchemaMapper::findByApplicationAndSlug(slug, application) — the pair

SettingsService passes appId: Application::APP_ID, which is now stackiq, while
every schema the app already owns still carries application = 'softwarecatalog'.
The pair matches nothing — and ImportHandler's not-found branch is not an error
path, it is the create a new one path.

So the next import builds a second, empty set of schemas under stackiq while
every stored object stays bound to the old rows. Nothing errors. The app renders
empty collections.

Position is load-bearing in both directions

  • After RenameDutchSchemaSlugs, so the collision check is judged against the
    slugs the rows will actually carry.
  • Before InitializeSettings, because that is what triggers the register import
    via SettingsService::initialize().

In <install> it sits immediately before InitializeSettings for the same reason.
An app-id rename presents to Nextcloud as a fresh install, so <install> is the
block that actually fires on the very upgrade the step exists for.

Measured, not assumed

Schema counts still on the old application id, read from a live install:

old application schemas slugs that would collide
docudesk 231 0
openconnector 200 200
procest 180 0
decidesk 98 0
softwarecatalog 21 0
openbuild 17 0
planix 2 2
larpingapp 2 2
hrmq 2 0

All 21 softwarecatalog schemas move cleanly — none of their slugs has a twin under
stackiq. On an instance carrying no softwarecatalog rows the step reports
nothing to do and writes nothing.

It refuses rather than merges

Where a slug already has a twin under the new application id, the fork has
already happened. Re-pointing would leave two rows sharing (application, slug), and
findByApplicationAndSlug() caps at one row — so one silently wins every lookup and
the other's objects become unreachable.

Choosing between them is a decision about data, not a migration. The step logs
and leaves both alone.

A failed read is not an empty result

This is the whole safety of the step. An empty list says every move is safe; a
failed read says nothing at all. Conflating them would move every schema on top of
an existing twin. There is a test for exactly that case.

It also never deletes a schema, and never throws — it runs under <install>, where
an escaping exception aborts the install and the app never enables.

Verification — and what could not run locally

  • php vendor/bin/phpunit --no-coverage (the default phpunit.xml) cannot run standalone in this repo, and that is pre-existing: the bootstrap dies with Class "OC_App" not found. This app's suite needs a real Nextcloud; CI runs it in-container (server/apps/stackiq).
  • The no-server config (phpunit-unit.xml) runs 752 tests. Standalone, 8 of them error on Class "Doctrine\DBAL\ParameterType" not found — 7 new ones plus the pre-existing PortfolioReportControllerTest::testCsvFormatReturnsDownloadResponse. doctrine/dbal is not in this app's vendor tree and OCP\DB\QueryBuilder\IQueryBuilder references it; the Nextcloud server bundles it (3rdparty/doctrine/dbal), so an in-container run has it.
  • With the server's bundled 3rdparty/autoload.php prepended to that bootstrap — exactly the in-container condition, no source change — all 752 pass: OK (752 tests, 3055 assertions, 24 skipped, 1 deprecation).
  • php-cs-fixer clean on the new files; composer phpcs, composer phpmd — clean.
  • composer psalm — No errors found. composer phpstan — No errors.
  • Hydra gate-16 spec-coverage: # count=0. Hydra spec-anchors: empty log.
  • appinfo/info.xml parses.

Coordination

Branched from origin/development and built in a separate git worktree so the
in-flight register-slug rename branch (feat/rename-register-slug-to-stackiq) was
never touched. Only lib/Repair/, tests/Unit/Repair/ and appinfo/info.xml are
changed here. If that branch lands first, the two <step> insertions in
appinfo/info.xml are in different positions and should merge; this step belongs
after MigrateRegisterSlug wherever both are present.

…stackiq application id

OpenRegister resolves a REGISTER by slug alone, but a SCHEMA by the PAIR
(application, slug) via SchemaMapper::findByApplicationAndSlug(). This app passes
appId: Application::APP_ID = 'stackiq', while every schema it already owns still
carries application = 'softwarecatalog'. The pair matches nothing, and ImportHandler's
not-found branch is not an error path — it is the create-a-new-one path. The next
import therefore builds a second, EMPTY schema set under the new application id while
every stored object stays bound to the old rows. Nothing errors; the app renders
empty collections.

Measured on a live install: 21 schemas under softwarecatalog, zero under stackiq, and
zero slug collisions — all 21 move cleanly.

A DIFFERENT COLUMN from RenameDutchSchemaSlugs (that step rewrites
openregister_schemas.slug, this one openregister_schemas.application), and the third
store keyed by app id after oc_appconfig and oc_preferences. Registered after
RenameDutchSchemaSlugs so the collision check sees the final slugs, and before
InitializeSettings because that triggers the register import.

The step refuses rather than merges where a slug already has a twin under the new
application id, distinguishes a FAILED READ from an EMPTY RESULT, never deletes a
schema and never throws — it runs under <install>, where an escaping exception aborts
the install.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 8cd098e

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
check-schema-l10n
composer ✅ 130/130
npm ✅ 720/720
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-24 09:42 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 8cd098e

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
check-schema-l10n
composer ✅ 130/130
npm ✅ 720/720
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-24 10:02 UTC

Download the full PDF report from the workflow artifacts.

Both sides added an install repair step for the same rename: this branch adds
MigrateSchemaApplicationId, development added MigrateRegisterSlug. Neither
supersedes the other, so both stay.

The order is load-bearing. MigrateSchemaApplicationId resolves each schema by
the (application, slug) PAIR, and the slug it matches on is the one
MigrateRegisterSlug has just corrected. Run the schema step first and it
matches against the old slug, silently doing nothing on the upgrade it exists
for. Register first, schema second, both ahead of InitializeSettings.
… install

E2E failed on one test: the SBOM component table showed "lodash" twice where
the second import should have replaced the first. Development is 95/0; this
branch was 94/1, and the pre-merge commit on this branch was green — so the
merge introduced it.

Cause is a missing prerequisite in <install>. MigrateSchemaApplicationId says
of its own position:

  POSITION IS LOAD-BEARING IN BOTH DIRECTIONS. After RenameDutchSchemaSlugs,
  so the collision check is judged against the slugs the rows will actually
  carry; before InitializeSettings, which is what triggers the register import.

<post-migration> honours that — MigrateRegisterSlug, RenameDutchSchemaSlugs,
then MigrateSchemaApplicationId. <install> ran the first and third and skipped
the second, so on the fresh-install path the collision check was judged
against pre-rename slugs. That path is not an edge case: an app-id rename
presents to Nextcloud as a fresh install, which is the whole reason this block
repeats the steps.

The failure mode follows from the step's own contract — it refuses rather than
merges, leaving two rows where it sees a twin. Two schema rows is two
component rows, which is the duplicate "lodash" the test caught.

I had also claimed in the merge commit that the ordering constraint was
against MigrateRegisterSlug. It is not; it is against RenameDutchSchemaSlugs,
a different class rewriting a different column.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 4c0b956

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
check-schema-l10n
composer ✅ 130/130
npm ✅ 720/720
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-24 17:11 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 2f3d2de

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
check-schema-l10n
composer ✅ 130/130
npm ✅ 720/720
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-24 18:39 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Holding this one. The E2E failure is real and reproducible, and I could not pin the mechanism well enough to merge past it.

What fails

sbom-import.spec.ts:157 fails on the first import:

Locator: getByTestId('sbom-component-table').getByText('lodash')
Error: strict mode violation: resolved to 2 elements

Branch: 94 passed / 1 failed. Development: 95 passed / 0 failed (run 32708760314). So this PR causes it.

Why that is worth taking seriously

This PR's diff is three files, and appinfo/info.xml registers two repair steps that never ran beforeRenameDutchSchemaSlugs and MigrateSchemaApplicationId.

RenameDutchSchemaSlugs::SLUG_MAP contains:

'moduleVersie' => 'moduleVersion',

and src/components/sbom/SbomComponentsPanel.vue carries this warning, from the last time that exact rename was applied:

⚠️ THE NAME IS A CONTRACT. When the schema slug moduleVersie was translated to moduleVersion the consumer was renamed and this producer was not, so the consumer resolved undefined, returned an empty data bag, and every derived value went quietly empty — no error, no warning, and the provenance line simply never rendered.

MigrateSchemaApplicationId also documents that it refuses rather than merges on a slug collision, which by design leaves two schemas standing. Two sbomComponent schemas is a shape that would produce exactly the duplicate row the test sees.

What I checked and could not confirm

RenameDutchSchemaSlugDecisions::plan() already refuses a rename when the target slug exists, so the naive collision is guarded. I could not find the repair-step output in the E2E job log to confirm which step fired, or whether either refused. Without that the mechanism stays a hypothesis.

Why not merge anyway

The duplication is user-visible data, and both steps are install-time migrations. A repair step that can leave a forked schema is the kind of change that reports success while the UI quietly doubles rows — the failure mode this app has already been bitten by once.

Next step for whoever picks this up: run the two repair steps against the E2E fixture and check how many sbomComponent schemas exist afterwards, and under which application id.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 28a7986

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
check-schema-l10n
composer ✅ 130/130
npm ✅ 720/720
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-24 21:20 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit ff2abf1 into development Aug 24, 2026
86 checks passed
@rubenvdlinde
rubenvdlinde deleted the feat/migrate-schema-application-id branch August 24, 2026 21:23
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Correction to my earlier comment. This merged with a fully green run — 20 checks pass, 0 fail, and E2E at 95 passed / 0 failed, matching development exactly.

The sbom-import failure I held this PR on was flaky, not the schema fork I described.

My earlier analysis chained real facts into a conclusion they did not support: this PR does activate RenameDutchSchemaSlugs, its map does contain 'moduleVersie' => 'moduleVersion', and SbomComponentsPanel.vue does carry a warning about that exact rename having broken things once. All true, and none of it was evidence that it was happening here.

I noted at the time that I could not find any repair-step output in the E2E log and that the mechanism was therefore a hypothesis. I should have weighted that absence more heavily — no trace of the steps firing is closer to evidence against the theory than for it. Instead I kept citing the hypothesis as the reason to block.

Nothing was shipped on the strength of the wrong analysis: the merge happened on a green run, not on my reasoning. But the earlier comment reads as a diagnosis and it is not one — hence this correction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant