Skip to content

fix(apphost): stop MetricsController's extends from 500-ing every route - #1137

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/apphost-metrics-extends
Aug 4, 2026
Merged

fix(apphost): stop MetricsController's extends from 500-ing every route#1137
rubenvdlinde merged 1 commit into
developmentfrom
fix/apphost-metrics-extends

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The defect

MetricsController extends GenericMetricsController — a class owned by OpenRegister.

A parent class must be loaded before the child can be declared, and Nextcloud's router ReflectionClasses every controller while matching a route. So when OpenRegister is absent this did not degrade /api/metrics — it returned HTTP 500 for every route in openconnector. CI does not install OpenRegister.

Proven by declaring the class under an autoloader that refuses OCA\OpenRegister\*:

declaration router-style reflection OR autoload attempts index()
before FATAL Error: Class "…GenericMetricsController" not found 1 unreachable
after OK OK 0 503 + text/plain; version=0.0.4

The fix

The nullable-delegate guard that the sibling HealthController in this same file already uses. A nullable parameter type is never autoloaded — only extends/implements resolve at class-declaration time — and this service is built by an explicit factory, so the container never autowires that parameter. I verified that distinction directly rather than assuming it, since HealthController's docblock asserts it: a nullable typed param naming an absent class produces zero autoload attempts, while extends on the same class throws.

The factory's unconditional getRegisteredAppContainer('openregister') — which throws for an unregistered app — now sits behind the same isInstalled() guard.

Route name, URL, and the admin-only posture (deliberate absence of #[NoAdminRequired]) are unchanged.

Fleet context

Same defect and same fix as nldesign #208, pipelinq #683, docudesk #369, procest #720. I scanned all 21 repos for controllers whose class header (extends or implements) names an OpenRegister import — openconnector was the last one. The scanner was controlled in both directions against procest's pre-fix and post-fix trees; an earlier version of it returned a false zero on a known-bad tree and was discarded.

Two pre-existing issues fixed in the same files

  • OpenConnectorMetricsProviderTest was red — three tests asserted assertCount(3, $samples). fix(ci,observability): seed the OR register for Newman; revive 10 metrics dead since the OR cutover #1126 revived sources_total / calls_total / synchronization_runs_total, making it 6, and left the suite failing. Replaced with an assertion on the metric name set, so the next change fails with a diff that names the metric instead of 6 does not match 3. Negative-controlled: a wrong name in the expected set fails all three tests, so the assertion is not vacuous.
  • Application::boot() was missing its @spec tag.

Gates

PHP 8.4 container, against the worktree's own vendor/ — a real directory, not a symlink to the main checkout, which would silently test the wrong tree.

gate result
phpunit 2094 tests, 7552 assertions, 0 failures (was 3 failures)
phpcs 0 errors, 287 warnings (baseline 0 / 288)
phpmd 0 findings — positive-controlled, a probe file yields 3
psalm exit 0
phpstan No errors

No suppression was added. The single phpcs:ignore in the diff is the existing LegacyNamedAccessor note moved within the rewritten factory — 2 occurrences before, 2 after.

`MetricsController extends GenericMetricsController` — an OpenRegister class.
A parent class must be loaded before the child can be declared, and Nextcloud's
router `ReflectionClass`es EVERY controller while matching a route. So with
OpenRegister absent, this did not degrade `/api/metrics`: it returned HTTP 500
for every route in openconnector.

Replaced with the nullable-delegate guard the sibling HealthController in this
same file already uses. A nullable *parameter type* is never autoloaded (only
`extends`/`implements` resolve at class-declaration time), and the service is
built by an explicit factory, so the container never autowires that parameter.
The factory's unconditional `getRegisteredAppContainer('openregister')` — which
throws for an unregistered app — is now behind the same `isInstalled()` guard.

Route name, URL and the admin-only posture (no `#[NoAdminRequired]`) unchanged.

Verified by declaring the class under an autoloader that refuses
`OCA\OpenRegister\*`:
  before: FATAL Error "Class GenericMetricsController not found"
  after:  declares OK, router-style reflection OK, zero OpenRegister autoload
          attempts, and index() returns 503 with the Prometheus content type

Also fixes two pre-existing issues found in these files:

- OpenConnectorMetricsProviderTest asserted `assertCount(3, $samples)` in three
  tests. #1126 revived sources_total / calls_total / synchronization_runs_total,
  making it 6, and left the suite red (2094 tests, 3 failures). Replaced with an
  assertion on the metric NAME SET, so the next change fails with a diff that
  names the metric instead of "6 does not match 3". Negative-controlled: a wrong
  name in the expected set fails all three tests.
- Application::boot() was missing its @SPEC tag (phpcs warning).

Gates, in the PHP 8.4 container against the worktree's own vendor/ (a real
directory, not a symlink to the main checkout):
  phpunit  2094 tests, 7552 assertions, 0 failures  (was 3 failures)
  phpcs    0 errors, 287 warnings  (baseline: 0 errors, 288)
  phpmd    0 findings  (positive-controlled: a probe file yields 3)
  psalm    exit 0
  phpstan  No errors
No suppression was added; the one `phpcs:ignore` in the diff is the existing
LegacyNamedAccessor note moved within the rewritten factory (2 before, 2 after).
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openconnector @ 40679a6

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
build
composer ✅ 153/153
npm ✅ 692/692
PHPUnit ⏭️
Newman
Playwright

Quality workflow — 2026-08-04 07:51 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Merging. Verified at job level, not from the rollup — run 30888700167:

total jobs 26 (a startup_failure run reports 0 and emits no contexts at all)
success 19
skipped 7
failure / cancelled 0
E2E Tests (Playwright) success, 9 min

⚠️ One gap worth naming rather than glossing: PHPUnit is skipped in CI — this caller never switched it on (that is what #1130 exists to fix). So CI did not execute this change's tests. The coverage comes from the local run instead: 2094 tests, 7552 assertions, 0 failures, against the worktree's own vendor/ (a real directory — a symlinked or mounted vendor/ makes Composer resolve $baseDir through the link and silently test the main checkout's lib/). That run went from 3 failures to 0.

@rubenvdlinde
rubenvdlinde merged commit dd98e93 into development Aug 4, 2026
30 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/apphost-metrics-extends branch August 5, 2026 17:23
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.

2 participants