fix(api): an object that does not exist is a 404 on the integration leaves, not a 500 by rubenvdlinde · Pull Request #2441 · ConductionNL/openregister

rubenvdlinde · 2026-08-12T08:35:12Z

The bug

GET /api/objects/{register}/{schema}/{id}/integrations/{integrationId}

with an id matching nothing answered 500 — DoesNotExistException: Object not found in magic table — instead of the 404 guardObjectAccess()'s own docblock promises.

setObject() is not a setter. It calls objectMapper->find(), which throws when nothing matches. It sat one line above the try whose catch already returns 404:

$this->objectService->setObject($id);   // ← throws here

try {
    $object = $this->objectService->getObject();   // ← only reachable if the above succeeded
} catch (\Throwable $e) {
    return new JSONResponse(['message' => 'Object not found'], Http::STATUS_NOT_FOUND);
}

The guard was wrapped around the wrong call. Moving the three resolution calls inside the try fixes all five verbs, which share it.

Why no test caught it

The existing test is called testIndexDeniesInaccessibleObject and it is green. It stubs getObject() alone — and on a mock, setObject() is a silent no-op, so the throwing path was unreachable from the suite. A green test with exactly the right name was standing over a 500.

The new missingObjectService() throws from setObject() the way the real service does, rather than stubbing the call that comes after it.

How it was found

From the other end, and only by accident. openconnector's synced-from-leaf e2e spec asks this endpoint about a deliberately absent uuid and expects an empty result. That spec had never run: it skips unless openconnector.storage_migrated is true, and no fresh install ever set that flag (ocon#1180).

Fixing the flag ran the spec, and the spec found this. The disabled feature was load-bearing for the silence.

Reproduced live on the dev instance, which has the flag set:

$ curl -u admin:admin ".../api/objects/<register>/<schema>/00000000-0000-0000-0000-000000000000/integrations/sync-contract"
HTTP 500
→ "Exception":"OCP\\AppFramework\\Db\\DoesNotExistException","Message":"Object not found in magic table"

Verification

6 tests — the index verb plus a data provider over show / create / update / destroy, which share the guard.
Positive control: reverting the guard to its previous shape turns all five new cases red (errors, not failures — the exception escapes).
tests/Unit/Controller — 2,802 tests green.
phpcs clean on the changed file.

The full unit suite cannot run on this branch: development currently fatals at tests/Unit/Service/Flow/FlowNodeConfigDialectTest.php on a helper trait nothing requires. That is fixed separately in #2439; this branch inherits it and is unaffected by it.

…eaves, not a 500 `GET /api/objects/{register}/{schema}/{id}/integrations/{integrationId}` with an id matching nothing answered 500, `DoesNotExistException: Object not found in magic table`, instead of the 404 `guardObjectAccess()`'s own docblock promises. `setObject()` is not a setter. It calls `objectMapper->find()`, which throws when nothing matches — and it sat one line ABOVE the `try` whose catch already returns 404. The `getObject()` call the catch did cover is only reachable once `setObject()` has succeeded, so the guard was wrapped around the wrong call. Moving the three resolution calls inside the try fixes all five verbs, which share the guard. The existing test named "denies inaccessible object" could not catch this: it stubs `getObject()` alone, and on a mock `setObject()` is a silent no-op, so the throwing path was unreachable from the suite. A green test with the right name was standing over a 500. Found from the other end. openconnector's `synced-from-leaf` e2e spec asks this endpoint about a deliberately absent uuid and expects an empty result; it had never run, because it skips unless `openconnector.storage_migrated` is true, and no fresh install ever set that flag (ocon#1180). Fixing the flag ran the spec, and the spec found this. Reproduced live on the dev instance, which has the flag set: HTTP 500, same exception. Six tests, positive-controlled: reverting the guard turns all five new ones red. `missingObjectService()` throws from `setObject()` the way the real service does, rather than stubbing the call that comes after it.

github-actions · 2026-08-12T08:46:37Z

Quality Report — ConductionNL/openregister @ `9e4822d`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		✅
composer			✅	✅ 173/173
npm			✅	✅ 546/546
PHPUnit					❌
Newman					✅
Playwright					✅
Hydra gates					❌

Quality workflow — 2026-08-12 08:46 UTC

Download the full PDF report from the workflow artifacts.

Both only ever "passed" by never running: this suite skips unless `openconnector.storage_migrated` is true, which no fresh install set (#1180). Setting the flag ran them, and they were wrong in two different ways. 1. "a different object is empty" used the uuid 00000000-0000-0000-0000-000000000000 and expected 200 with an empty list. That conflates an object that EXISTS but has no contract with an id that matches nothing. The second is a 404 — correctly, since the endpoint will not say whether an object it cannot resolve exists. It only looked like a 200 because the endpoint used to answer 500 there (ConductionNL/openregister#2441); with that fixed the honest answer arrived and the assertion had nothing to stand on. The claim under test is that the leaf matches STRICTLY by targetId, and only an object that exists can fail to match — so the fixture now seeds a real neighbour that no contract points at. A second test pins the counterpart: a missing object is 404, never 200 and never 500. "No contracts" and "no such object" are different answers and the endpoint must not collapse them. 2. The detail-surface test navigated by walking every element for `el.__vue__.$router`. That is a VUE 2 internal and OpenRegister is Vue 3 (`"vue": "^3.5.0"`, `createRouter` in src/main.js), so the walk found nothing, `router` stayed null, and no navigation happened — silently, because the call ended in `.catch(() => undefined)`. The first visible symptom was the Integrations tab timing out 30s later on a page that had never left the index: precisely the failure mode the comment directly above it warns about for the old hardcoded ids. OpenRegister uses `createWebHashHistory()`, so the route lives in the fragment and a plain `goto` IS the client-side navigation. No router handle to find, and nothing left to fall silent.

`getByRole('tab', { name: 'Integrations' })` has never matched anything. The suite skipped unless `openconnector.storage_migrated` was true, and no fresh install set it (#1180), so this selector was written speculatively and never exercised. It is not a regression — it is an assertion nobody has seen run. Everything else it depends on is now fixed and green: the openregister 500 (ConductionNL/openregister#2441), both API-level leaf assertions, and the navigation, which walked every element for `el.__vue__.$router` — a Vue 2 internal — in a Vue 3 app. The route is confirmed against OpenRegister's manifest (`/objects/:register/:schema/:id`, `createWebHashHistory()`). What remains unknown is only what the rendered sidebar calls that tab and with which role, and that cannot be settled from a developer box: the dev instance carries 1000+ schemas from earlier runs and has no `slug: 'source'` schema, so this fixture cannot be reproduced locally. Guessing a replacement selector is how the current one got here, and a second guess would be indistinguishable from a fix until the next time somebody enables the flag. Quarantined with an issue number — honest quarantine, per #1190's own standard — rather than left red or guessed at. Refs #1228.

…stall runs (#1224) * fix(install): complete the storage cutover on the one hook a fresh install runs `openconnector.storage_migrated` gates `SynchronizationContractProvider::isEnabled()`. While it is false, "Synced from" provenance is absent from every object in the instance — and on a genuinely fresh install it was false forever. Two places already tried to set it, and BOTH did it from `postSchemaChange()`: `Version2Date20260520000001`, and `Version2Date20260520000099`, which was added specifically to fix the fresh-install case and put the fix in the one hook a fresh install skips. The mechanism, in Nextcloud core: * `Installer::installAppLastSteps()` calls `$ms->migrate('latest', $previousVersion === '')`, so a first install passes `$schemaOnly = true`. * `MigrationService::migrate()` routes that to `migrateSchemaOnly()`, which calls ONLY `changeSchema()` on each migration and then marks every version as executed. `preSchemaChange` and `postSchemaChange` never run — and because the versions are recorded, they never will. * The same method guards `repair-steps.post-migration` behind `$previousVersion !== ''`, so those are skipped too. `repair-steps.install` is the only hook that runs unconditionally on a first install (Installer.php:570), and openconnector declared NO `<install>` block at all — so on a fresh instance the register descriptor was never imported, the catalog was never materialised, and the flag was never set. Every dev instance here looks fine because it was UPGRADED, which takes `AppManager::upgradeApp()` and runs all of it. `InitializeRegister`'s own docblock claims it is "wired via appinfo/info.xml under both `<install>` and `<post-migration>`". Only the second was ever true. - New `Repair\MigrateLegacyStorage`: sets the flag when no legacy table holds rows, hands the rows to `LegacyToRegisterMigrator` when some do, and leaves the flag alone when a count could not be taken. It does NOT re-set the flag after `migrateAll()` — that method only sets it on a full, clean, non-dry run, and second-guessing it here would declare a cutover complete over rows it had just reported as skipped. - `<install>` block declaring InitializeRegister, InitializeActions, MigrateLegacyStorage and MaterializeCatalogItems. All are idempotent, so an instance that reaches them by both routes does the work once. - The three inline-secret steps stay post-migration only, deliberately and with the reasoning in the XML: a first install has no sources for them to act on, and `RemoveMigratedSourceSecretFields` is IRREVERSIBLE — it would find an empty instance trivially clean and strip four fields from the live source schema before any source exists. Also fixes 8 pre-existing calls to `fetch*Associative()`, which only reached `OCP\DB\IResult` in Nextcloud 33 while info.xml advertises `min-version="32"`. Found because the new test mocks the INTERFACE. On NC 32 they raise "Call to undefined method", and `Version2Date20260520000099::countRows()` swallows that into its `catch (\Throwable)` and returns -1 — "unsafe to drop" — for EVERY table, so the cleanup silently never completes and the failure reads as a database problem. Replaced with `fetchOne()`/`fetch()`/`fetchAll()`, with the `false` return guarded explicitly: `(int) false` is 0, which that method's contract reads as "empty, safe to drop". Eight tests. Positive-controlled: deleting the `<install>` block turns the wiring test red. Closes #1180. * test(e2e): page to the row instead of assuming it is on page 1 J5 and J6 create a Source through the UI and then look for it with `page.getByText(name)`. The index lists are SERVER-paginated at 20/page, so once the list is longer than a page the freshly created row is not there, and the assertion reports "target item must exist for edit" about a row that exists and is listed. `createViaUi()` already refuses to look in the DOM for exactly this reason and asks OpenRegister by name instead — its own comment says the number such an assertion measures is the page size. Edit and delete cannot borrow that trick: they need the row itself, to click its Actions menu, so it has to be rendered. Sorting and searching cannot substitute. `CnIndexPage`/`CnTable` sort CLIENT-SIDE over the already-loaded page and the in-list search box is not wired to a server `_search` (#996; the fix lives in @conduction/nextcloud-vue), so a row the server put on page 2 is not present to be sorted or filtered into view. The pager is the one control that re-queries — which is what `walkToRow()` in workflows/source-mapping-crud.spec.ts already does, for the same reason. `walkToItem()` is that, adapted to Cards view, since these journeys switch away from the table whose cells render as "—". ⚠️ NOT a CI-only nicety. A real install imports the register descriptor WITH its shipped objects — `InitializeRegister` does exactly that, and `tests/e2e/ci-seed.sh` says so in as many words while deliberately stripping them from the CI seed. So the Sources list is past one page on any real instance, and these journeys have been passing against a list kept artificially short. Surfaced by the `<install>` repair-step wiring in this branch: it makes a CI install import the descriptor the way a real one does, the demo sources come back, and J5/J6 fail. The journeys were wrong first; the wiring only stopped hiding it. * test(e2e): fix the two leaf assertions the 500 was hiding Both only ever "passed" by never running: this suite skips unless `openconnector.storage_migrated` is true, which no fresh install set (#1180). Setting the flag ran them, and they were wrong in two different ways. 1. "a different object is empty" used the uuid 00000000-0000-0000-0000-000000000000 and expected 200 with an empty list. That conflates an object that EXISTS but has no contract with an id that matches nothing. The second is a 404 — correctly, since the endpoint will not say whether an object it cannot resolve exists. It only looked like a 200 because the endpoint used to answer 500 there (ConductionNL/openregister#2441); with that fixed the honest answer arrived and the assertion had nothing to stand on. The claim under test is that the leaf matches STRICTLY by targetId, and only an object that exists can fail to match — so the fixture now seeds a real neighbour that no contract points at. A second test pins the counterpart: a missing object is 404, never 200 and never 500. "No contracts" and "no such object" are different answers and the endpoint must not collapse them. 2. The detail-surface test navigated by walking every element for `el.__vue__.$router`. That is a VUE 2 internal and OpenRegister is Vue 3 (`"vue": "^3.5.0"`, `createRouter` in src/main.js), so the walk found nothing, `router` stayed null, and no navigation happened — silently, because the call ended in `.catch(() => undefined)`. The first visible symptom was the Integrations tab timing out 30s later on a page that had never left the index: precisely the failure mode the comment directly above it warns about for the old hardcoded ids. OpenRegister uses `createWebHashHistory()`, so the route lives in the fragment and a plain `goto` IS the client-side navigation. No router handle to find, and nothing left to fall silent. * test: parse info.xml from a string, not a path Both wiring tests failed in CI with "appinfo/info.xml should parse" while passing locally, and the XML is valid — `xml.dom.minidom` parses it and no comment contains a double hyphen. `simplexml_load_file()` resolves the DOCUMENT ITSELF through libxml's external-entity loader, and Nextcloud pins that loader to return null (`lib/base.php`: `libxml_set_external_entity_loader(static function () {…})`). So the call fails the moment these tests run inside a booted server — which is what CI does and a bare local run does not. Reproduced and both halves proved, under an explicitly pinned null loader: simplexml_load_file → false ("Failed to load external entity because the resolver function returned null") simplexml_load_string → true Reading the bytes first sidesteps the loader. The assertion message now also names the path, so a genuinely unreadable file cannot present as a parse failure again. * test(e2e): quarantine the one leaf assertion whose selectors never ran `getByRole('tab', { name: 'Integrations' })` has never matched anything. The suite skipped unless `openconnector.storage_migrated` was true, and no fresh install set it (#1180), so this selector was written speculatively and never exercised. It is not a regression — it is an assertion nobody has seen run. Everything else it depends on is now fixed and green: the openregister 500 (ConductionNL/openregister#2441), both API-level leaf assertions, and the navigation, which walked every element for `el.__vue__.$router` — a Vue 2 internal — in a Vue 3 app. The route is confirmed against OpenRegister's manifest (`/objects/:register/:schema/:id`, `createWebHashHistory()`). What remains unknown is only what the rendered sidebar calls that tab and with which role, and that cannot be settled from a developer box: the dev instance carries 1000+ schemas from earlier runs and has no `slug: 'source'` schema, so this fixture cannot be reproduced locally. Guessing a replacement selector is how the current one got here, and a second guess would be indistinguishable from a fix until the next time somebody enables the flag. Quarantined with an issue number — honest quarantine, per #1190's own standard — rather than left red or guessed at. Refs #1228.

rubenvdlinde mentioned this pull request Aug 12, 2026

fix(install): complete the storage cutover on the one hook a fresh install runs ConductionNL/openconnector#1224

Merged

rubenvdlinde merged commit 9c05a3f into development Aug 12, 2026
23 of 28 checks passed

rubenvdlinde deleted the fix/object-integrations-missing-object-404 branch August 12, 2026 08:37

This was referenced Aug 12, 2026

PHPUnit red on development: MapNodeTest::testANonNumericReferenceResolvesByRef calls find() only under an installed server #2440

Open

synced-from-leaf: the object-detail tab selector has never been validated (quarantined) ConductionNL/openconnector#1228

Open

rubenvdlinde mentioned this pull request Aug 12, 2026

storage_migrated is never set on a fresh install — postSchemaChange does not run in schemaOnly mode, so "Synced from" is permanently absent ConductionNL/openconnector#1180

Closed

Navigation Menu

Uh oh!

fix(api): an object that does not exist is a 404 on the integration leaves, not a 500 - #2441

fix(api): an object that does not exist is a 404 on the integration leaves, not a 500#2441
rubenvdlinde merged 1 commit into
developmentfrom
fix/object-integrations-missing-object-404

rubenvdlinde commented Aug 12, 2026

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented Aug 12, 2026

The bug

Why no test caught it

How it was found

Verification

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026

Quality Report — ConductionNL/openregister @ 9e4822d

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/openregister @ `9e4822d`