Merge pull request #36298 from nextcloud/perf/app-framework/lazy-midd…

…leware-registration

perf(app-framework): Make app middleware registration even lazier

lib/private/AppFramework/Bootstrap/Coordinator.php

@@ -153,7 +153,6 @@ private function registerApps(array $appIds): void {
 		$this->registrationContext->delegateDashboardPanelRegistrations($this->dashboardManager);
 		$this->registrationContext->delegateEventListenerRegistrations($this->eventDispatcher);
 		$this->registrationContext->delegateContainerRegistrations($apps);
-		$this->registrationContext->delegateMiddlewareRegistrations($apps);
 	}
 
 	public function getRegistrationContext(): ?RegistrationContext {

lib/private/AppFramework/Bootstrap/RegistrationContext.php

@@ -617,29 +617,10 @@ public function delegateContainerRegistrations(array $apps): void {
 	}
 
 	/**
-	 * @param App[] $apps
+	 * @return ServiceRegistration<Middleware>[]
 	 */
-	public function delegateMiddlewareRegistrations(array $apps): void {
-		while (($middleware = array_shift($this->middlewares)) !== null) {
-			$appId = $middleware->getAppId();
-			if (!isset($apps[$appId])) {
-				// If we land here something really isn't right. But at least we caught the
-				// notice that is otherwise emitted for the undefined index
-				$this->logger->error("App $appId not loaded for the container middleware registration");
-
-				continue;
-			}
-
-			try {
-				$apps[$appId]
-					->getContainer()
-					->registerMiddleWare($middleware->getService());
-			} catch (Throwable $e) {
-				$this->logger->error("Error during capability registration of $appId: " . $e->getMessage(), [
-					'exception' => $e,
-				]);
-			}
-		}
+	public function getMiddlewareRegistrations(): array {
+		return $this->middlewares;
 	}
 
 	/**

lib/private/AppFramework/DependencyInjection/DIContainer.php

@@ -315,6 +315,17 @@ public function __construct(string $appName, array $urlParams = [], ServerContai
 				$c->get(\OC\AppFramework\Middleware\AdditionalScriptsMiddleware::class)
 			);
 
+			/** @var \OC\AppFramework\Bootstrap\Coordinator $coordinator */
+			$coordinator = $c->get(\OC\AppFramework\Bootstrap\Coordinator::class);
+			$registrationContext = $coordinator->getRegistrationContext();
+			if ($registrationContext !== null) {
+				$appId = $this->getAppName();
+				foreach ($registrationContext->getMiddlewareRegistrations() as $middlewareRegistration) {
+					if ($middlewareRegistration->getAppId() === $appId) {
+						$dispatcher->registerMiddleware($c->get($middlewareRegistration->getService()));
+					}
+				}
+			}
 			foreach ($this->middleWares as $middleWare) {
 				$dispatcher->registerMiddleware($c->get($middleWare));
 			}

tests/lib/AppFramework/Bootstrap/RegistrationContextTest.php

@@ -27,6 +27,7 @@
 
 use OC\AppFramework\Bootstrap\RegistrationContext;
 use OC\AppFramework\Bootstrap\ServiceRegistration;
+use OC\Core\Middleware\TwoFactorMiddleware;
 use OCP\AppFramework\App;
 use OCP\AppFramework\IAppContainer;
 use OCP\EventDispatcher\IEventDispatcher;
@@ -145,24 +146,6 @@ public function testRegisterParameter(): void {
 		]);
 	}
 
-	public function testRegisterMiddleware(): void {
-		$app = $this->createMock(App::class);
-		$name = 'abc';
-		$container = $this->createMock(IAppContainer::class);
-		$app->method('getContainer')
-			->willReturn($container);
-		$container->expects($this->once())
-			->method('registerMiddleware')
-			->with($name);
-		$this->logger->expects($this->never())
-			->method('error');
-
-		$this->context->for('myapp')->registerMiddleware($name);
-		$this->context->delegateMiddlewareRegistrations([
-			'myapp' => $app,
-		]);
-	}
-
 	public function testRegisterUserMigrator(): void {
 		$appIdA = 'myapp';
 		$migratorClassA = 'OCA\App\UserMigration\AppMigrator';
@@ -195,4 +178,14 @@ public function dataProvider_TrueFalse() {
 			[false]
 		];
 	}
+
+	public function testGetMiddlewareRegistrations(): void {
+		$this->context->registerMiddleware('core', TwoFactorMiddleware::class);
+
+		$registrations = $this->context->getMiddlewareRegistrations();
+
+		self::assertNotEmpty($registrations);
+		self::assertSame('core', $registrations[0]->getAppId());
+		self::assertSame(TwoFactorMiddleware::class, $registrations[0]->getService());
+	}
 }

tests/lib/AppFramework/DependencyInjection/DIContainerTest.php

@@ -25,9 +25,13 @@
 
 namespace Test\AppFramework\DependencyInjection;
 
+use OC\AppFramework\Bootstrap\Coordinator;
+use OC\AppFramework\Bootstrap\RegistrationContext;
+use OC\AppFramework\Bootstrap\ServiceRegistration;
 use OC\AppFramework\DependencyInjection\DIContainer;
 use OC\AppFramework\Http\Request;
 use OC\AppFramework\Middleware\Security\SecurityMiddleware;
+use OCP\AppFramework\Middleware;
 use OCP\AppFramework\QueryException;
 use OCP\IConfig;
 use OCP\IRequestId;
@@ -84,6 +88,38 @@ public function testMiddlewareDispatcherIncludesSecurityMiddleware() {
 		$this->assertTrue($found);
 	}
 
+	public function testMiddlewareDispatcherIncludesBootstrapMiddlewares(): void {
+		$coordinator = $this->createMock(Coordinator::class);
+		$this->container[Coordinator::class] = $coordinator;
+		$this->container['Request'] = $this->createMock(Request::class);
+		$registrationContext = $this->createMock(RegistrationContext::class);
+		$registrationContext->method('getMiddlewareRegistrations')
+			->willReturn([
+				new ServiceRegistration($this->container['appName'], 'foo'),
+				new ServiceRegistration('otherapp', 'bar'),
+			]);
+		$this->container['foo'] = new class extends Middleware {
+		};
+		$this->container['bar'] = new class extends Middleware {
+		};
+		$coordinator->method('getRegistrationContext')->willReturn($registrationContext);
+
+		$dispatcher = $this->container['MiddlewareDispatcher'];
+
+		$middlewares = $dispatcher->getMiddlewares();
+		self::assertNotEmpty($middlewares);
+		foreach ($middlewares as $middleware) {
+			if ($middleware === $this->container['bar']) {
+				$this->fail('Container must not register this middleware');
+			}
+			if ($middleware === $this->container['foo']) {
+				// It is done
+				return;
+			}
+		}
+		$this->fail('Bootstrap registered middleware not found');
+	}
+
 	public function testInvalidAppClass() {
 		$this->expectException(QueryException::class);
 		$this->container->query('\OCA\Name\Foo');