From 78381749c04c5c51994f217e0aa10173c3b929c9 Mon Sep 17 00:00:00 2001 From: Charles Sprayberry Date: Sat, 17 Feb 2024 11:08:14 -0500 Subject: [PATCH] Simplify configuration, logging creation (#67) * Simplify configuration, logging creation * Change test to use logging test handler * Check for exception output correctly --- dummy_app/DummyApplicationFeatures.php | 2 +- dummy_app/DummyLoggerFactory.php | 25 ------- dummy_app/DummyMonologInitializer.php | 24 ++++++ known-issues.xml | 37 ++++++++-- src/Logging/ApplicationLoggerProvider.php | 15 ++++ src/Logging/MonologLoggerFactory.php | 1 - .../StdoutMonologLoggerInitializer.php | 26 ------- src/Util/ReflectionCache.php | 17 ++++- src/Web/Application/AmpApplication.php | 9 --- src/Web/Application/ApplicationFeatures.php | 4 - src/Web/Application/NoApplicationFeatures.php | 26 ------- src/Web/Application/StaticAssetSettings.php | 2 +- test/Helper/StubApplicationFeatures.php | 18 +++++ test/Integration/BootstrapTest.php | 41 ++++++++-- test/Integration/HttpServerTest.php | 47 ++++++------ .../Web/Application/AmpApplicationTest.php | 74 ++----------------- test/Unit/Web/Logging/LoggerFactoryTest.php | 14 ---- .../StdoutMonologLoggerInitializerTest.php | 61 --------------- 18 files changed, 172 insertions(+), 271 deletions(-) delete mode 100644 dummy_app/DummyLoggerFactory.php create mode 100644 dummy_app/DummyMonologInitializer.php create mode 100644 src/Logging/ApplicationLoggerProvider.php delete mode 100644 src/Logging/StdoutMonologLoggerInitializer.php delete mode 100644 src/Web/Application/NoApplicationFeatures.php create mode 100644 test/Helper/StubApplicationFeatures.php delete mode 100644 test/Unit/Web/Logging/StdoutMonologLoggerInitializerTest.php diff --git a/dummy_app/DummyApplicationFeatures.php b/dummy_app/DummyApplicationFeatures.php index 83020c1..d668330 100644 --- a/dummy_app/DummyApplicationFeatures.php +++ b/dummy_app/DummyApplicationFeatures.php @@ -10,7 +10,7 @@ use Labrador\Web\Application\ApplicationFeatures; use Labrador\Web\Application\StaticAssetSettings; -#[Service(primary: true)] +#[Service] final class DummyApplicationFeatures implements ApplicationFeatures { public function getSessionMiddleware() : ?SessionMiddleware { diff --git a/dummy_app/DummyLoggerFactory.php b/dummy_app/DummyLoggerFactory.php deleted file mode 100644 index 2511b5d..0000000 --- a/dummy_app/DummyLoggerFactory.php +++ /dev/null @@ -1,25 +0,0 @@ -pushProcessor(new PsrLogMessageProcessor()); + $logger->pushHandler($this->testHandler); + } + +} \ No newline at end of file diff --git a/known-issues.xml b/known-issues.xml index 3394b2c..a95c3f5 100644 --- a/known-issues.xml +++ b/known-issues.xml @@ -1,5 +1,27 @@ - + + + + self::$cache[$class] + + + [] + self::$cache + + + + + $errors + + + $value + + + + + $map + + middlewareCompleted]]> @@ -14,6 +36,11 @@ profiles]]> + + + + + getAttribute('path')]]> @@ -25,6 +52,10 @@ + + get('csrfTokens')]]> + get('csrfTokens')]]> + $tokens[$index] @@ -40,9 +71,5 @@ $tokens $tokens - - get('csrfTokens')]]> - get('csrfTokens')]]> - diff --git a/src/Logging/ApplicationLoggerProvider.php b/src/Logging/ApplicationLoggerProvider.php new file mode 100644 index 0000000..86d882f --- /dev/null +++ b/src/Logging/ApplicationLoggerProvider.php @@ -0,0 +1,15 @@ +createLogger(LoggerType::Application); + } + +} diff --git a/src/Logging/MonologLoggerFactory.php b/src/Logging/MonologLoggerFactory.php index 6c13ccf..b631b56 100644 --- a/src/Logging/MonologLoggerFactory.php +++ b/src/Logging/MonologLoggerFactory.php @@ -22,7 +22,6 @@ public function __construct( public function createLogger(LoggerType $loggerType) : LoggerInterface { if (!array_key_exists($loggerType->value, $this->cache)) { $logger = new Logger($loggerType->value); - $logger->pushProcessor(new PsrLogMessageProcessor()); $this->loggerInitializer->initialize($logger, $loggerType); diff --git a/src/Logging/StdoutMonologLoggerInitializer.php b/src/Logging/StdoutMonologLoggerInitializer.php deleted file mode 100644 index 5681ce6..0000000 --- a/src/Logging/StdoutMonologLoggerInitializer.php +++ /dev/null @@ -1,26 +0,0 @@ -setFormatter(new ConsoleFormatter()); - - $logger->pushHandler($handler); - } - -} diff --git a/src/Util/ReflectionCache.php b/src/Util/ReflectionCache.php index 9841481..5c8cb37 100644 --- a/src/Util/ReflectionCache.php +++ b/src/Util/ReflectionCache.php @@ -2,16 +2,27 @@ namespace Labrador\Util; +use ReflectionClass; +use ReflectionException; + final class ReflectionCache { + /** + * @var array{class-string, ReflectionClass} + */ private static array $cache = []; /** * @param class-string $class - * @return \ReflectionClass + * @return ReflectionClass + * @throws ReflectionException */ - public static function fromClass(string $class) : \ReflectionClass { - return self::$cache[$class] ??= new \ReflectionClass($class); + public static function fromClass(string $class) : ReflectionClass { + $reflection = self::$cache[$class] ??= new ReflectionClass($class); + + assert($reflection instanceof ReflectionClass); + + return $reflection; } } diff --git a/src/Web/Application/AmpApplication.php b/src/Web/Application/AmpApplication.php index 5458498..58878f0 100644 --- a/src/Web/Application/AmpApplication.php +++ b/src/Web/Application/AmpApplication.php @@ -116,15 +116,6 @@ public function stop() : void { } public function handleRequest(Request $request) : Response { - if ($this->features->autoRedirectHttpToHttps() && $request->getUri()->getScheme() === 'http') { - $uri = $request->getUri()->withScheme('https'); - $uri = $uri->withPort($this->features->getHttpsRedirectPort()); - return new Response( - status: HttpStatus::MOVED_PERMANENTLY, - headers: ['Location' => (string) $uri] - ); - } - $benchmark = RequestBenchmark::requestReceived($request, $this->preciseTime); try { diff --git a/src/Web/Application/ApplicationFeatures.php b/src/Web/Application/ApplicationFeatures.php index 86b0107..b9a54e0 100644 --- a/src/Web/Application/ApplicationFeatures.php +++ b/src/Web/Application/ApplicationFeatures.php @@ -10,10 +10,6 @@ interface ApplicationFeatures { public function getSessionMiddleware() : ?SessionMiddleware; - public function autoRedirectHttpToHttps() : bool; - - public function getHttpsRedirectPort() : ?int; - public function getStaticAssetSettings() : ?StaticAssetSettings; } \ No newline at end of file diff --git a/src/Web/Application/NoApplicationFeatures.php b/src/Web/Application/NoApplicationFeatures.php deleted file mode 100644 index a4dc6d1..0000000 --- a/src/Web/Application/NoApplicationFeatures.php +++ /dev/null @@ -1,26 +0,0 @@ -info('This is a test message'); - self::assertStringContainsString('This is a test message', StreamBuffer::output($this->stdout)); + + $handler = $this->container->get(DummyMonologInitializer::class)->testHandler; + self::assertInstanceOf(TestHandler::class, $handler); + self::assertTrue( + $handler->hasInfoThatContains('This is a test message') + ); } public function testCorrectlyConfiguredAnnotatedContainerReturnsRouter() : void { @@ -155,17 +163,36 @@ public function testApplicationAutowiringControllersLogged() : void { $bootstrap->bootstrapApplication(); - self::assertStringContainsString('Autowiring route GET /hello/world to HelloWorld controller.', StreamBuffer::output($this->stdout)); + $handler = $this->container->get(DummyMonologInitializer::class)->testHandler; + self::assertInstanceOf(TestHandler::class, $handler); + + self::assertTrue( + $handler->hasInfoThatContains('Autowiring route GET /hello/world to HelloWorld controller.') + ); + } + + public static function expectedMiddlewareProvider() : array { + return [ + BarMiddleware::class => [BarMiddleware::class, Priority::Low], + BazMiddleware::class => [BazMiddleware::class, Priority::Medium], + FooMiddleware::class => [FooMiddleware::class, Priority::High], + QuxMiddleware::class => [QuxMiddleware::class, Priority::Critical] + ]; } - public function testApplicationAutowiringApplicationMiddlewareLogged() : void { + /** + * @dataProvider expectedMiddlewareProvider + */ + public function testApplicationAutowiringApplicationMiddlewareLogged(string $middlewareClass, Priority $priority) : void { $bootstrap = new Bootstrap($this->containerBootstrap, profiles: ['default', 'integration-test']); $bootstrap->bootstrapApplication(); - self::assertStringContainsString('Adding ' . BarMiddleware::class . ' to application with Low priority.', StreamBuffer::output($this->stdout)); - self::assertStringContainsString('Adding ' . BazMiddleware::class . ' to application with Medium priority.', StreamBuffer::output($this->stdout)); - self::assertStringContainsString('Adding ' . FooMiddleware::class . ' to application with High priority.', StreamBuffer::output($this->stdout)); - self::assertStringContainsString('Adding ' . QuxMiddleware::class . ' to application with Critical priority.', StreamBuffer::output($this->stdout)); + $handler = $this->container->get(DummyMonologInitializer::class)->testHandler; + self::assertInstanceOf(TestHandler::class, $handler); + + self::assertTrue( + $handler->hasInfoThatContains('Adding ' . $middlewareClass . ' to application with ' . $priority->name . ' priority.') + ); } } diff --git a/test/Integration/HttpServerTest.php b/test/Integration/HttpServerTest.php index b563e42..0dcb133 100644 --- a/test/Integration/HttpServerTest.php +++ b/test/Integration/HttpServerTest.php @@ -4,14 +4,12 @@ use Amp\Http\Client\HttpClientBuilder; use Amp\Http\Client\Request; -use Amp\Http\Cookie\ResponseCookie; use Amp\Http\HttpStatus; use Amp\PHPUnit\AsyncTestCase; use Cspray\AnnotatedContainer\AnnotatedContainer; use Cspray\StreamBufferIntercept\BufferIdentifier; use Cspray\StreamBufferIntercept\StreamBuffer; -use Labrador\DummyApp\Controller\SessionDtoController; -use Labrador\DummyApp\CountingService; +use Labrador\DummyApp\DummyMonologInitializer; use Labrador\DummyApp\Middleware\BarMiddleware; use Labrador\DummyApp\Middleware\BazMiddleware; use Labrador\DummyApp\Middleware\FooMiddleware; @@ -20,9 +18,12 @@ use Labrador\Test\BootstrapAwareTestTrait; use Labrador\Test\Helper\VfsDirectoryResolver; use Labrador\Web\Application\Application; +use Monolog\Handler\TestHandler; +use Monolog\LogRecord; use org\bovigo\vfs\vfsStream as VirtualFilesystem; use org\bovigo\vfs\vfsStreamDirectory as VirtualDirectory; use org\bovigo\vfs\vfsStreamWrapper as VirtualStream; +use PHPUnit\Framework\Constraint\StringMatchesFormatDescription; use Ramsey\Uuid\Uuid; class HttpServerTest extends AsyncTestCase { @@ -105,8 +106,6 @@ public function testRouteMiddlewareRespected() : void { $request = new Request('http://localhost:4200/hello/middleware', 'GET'); $response = $client->request($request); - $output = StreamBuffer::output(self::$stdout); - self::assertSame(HttpStatus::OK, $response->getStatus()); self::assertSame('Hello, Universe!', $response->getBody()->buffer()); } @@ -126,16 +125,19 @@ public function testCorrectAccessLogOutputSendToStdout() : void { $request = new Request('http://localhost:4200/hello/world'); $client->request($request); - $expected = <<get(DummyMonologInitializer::class)->testHandler; + self::assertInstanceOf(TestHandler::class, $handler); + + self::assertTrue($handler->hasInfoThatPasses(static function (LogRecord $record) { + $expected = <<evaluate( + other: $record->formatted, + returnResult: true + ); + })); } public function testExceptionThrowHasCorrectLogOutputSentToStdout() : void { @@ -144,15 +146,18 @@ public function testExceptionThrowHasCorrectLogOutputSentToStdout() : void { $request = new Request('http://localhost:4200/exception'); $client->request($request); - $expectedContext = '{"client_address":"127.0.0.1:%d","method":"GET","path":"/exception","exception_class":"RuntimeException","file":"%a/RouterListener.php","line_number":28,"exception_message":"A message detailing what went wrong that should show up in logs.","stack_trace":%a}'; - $expected = <<get(DummyMonologInitializer::class)->testHandler; + self::assertInstanceOf(TestHandler::class, $handler); + + self::assertTrue($handler->hasErrorThatPasses(static function (LogRecord $record) { + $expectedContext = '{"client_address":"127.0.0.1:%d","method":"GET","path":"/exception","exception_class":"RuntimeException","file":"%a/RouterListener.php","line_number":28,"exception_message":"A message detailing what went wrong that should show up in logs.","stack_trace":%a}'; + $expected = <<evaluate( + other: $record->formatted, + returnResult: true + ); + })); } } \ No newline at end of file diff --git a/test/Unit/Web/Application/AmpApplicationTest.php b/test/Unit/Web/Application/AmpApplicationTest.php index d929160..e30a9c2 100644 --- a/test/Unit/Web/Application/AmpApplicationTest.php +++ b/test/Unit/Web/Application/AmpApplicationTest.php @@ -23,6 +23,7 @@ use FastRoute\RouteParser\Std as StdRouteParser; use Labrador\DummyApp\Middleware\BarMiddleware; use Labrador\DummyApp\MiddlewareCallRegistry; +use Labrador\Test\Helper\StubApplicationFeatures; use Labrador\Test\Unit\Web\Stub\ErrorHandlerFactoryStub; use Labrador\Test\Unit\Web\Stub\ErrorThrowingController; use Labrador\Test\Unit\Web\Stub\ErrorThrowingMiddleware; @@ -95,7 +96,7 @@ function($data) { return new GcbDispatcher($data); } $this->router, $this->emitter, new Logger('labrador-http-test', [$this->testHandler], [new PsrLogMessageProcessor()]), - new NoApplicationFeatures(), + new StubApplicationFeatures(), $this->analyticsQueue, $this->preciseTime ); @@ -341,7 +342,10 @@ public function autoRedirectHttpToHttps() : bool { } public function getStaticAssetSettings() : ?StaticAssetSettings { - return new StaticAssetSettings(dirname(__DIR__, 3) . '/Helper/assets'); + return new StaticAssetSettings( + dirname(__DIR__, 3) . '/Helper/assets', + 'assets' + ); } public function getHttpsRedirectPort() : ?int { @@ -427,70 +431,6 @@ public function handleRequest(Request $request, RequestHandler $requestHandler) self::assertNotNull($controller->getSession()); } - public function testApplicationFeaturesRedirectHttpToHttpsWithNullPort() : void { - $request = new Request( - $this->getMockBuilder(Client::class)->getMock(), - HttpMethod::Get->value, - Http::createFromString('http://example.com/tls-test?foo=bar') - ); - - $subject = new AmpApplication( - $this->httpServer, - new ErrorHandlerFactoryStub($this->errorHandler), - $this->router, - $this->emitter, - new Logger('labrador-http-test', [$this->testHandler], [new PsrLogMessageProcessor()]), - $this->getApplicationFeaturesWithHttpToHttpsRedirectAndNoHttpsPort(), - $this->analyticsQueue, - $this->preciseTime, - ); - - $subject->start(); - - $response = $subject->handleRequest($request); - - self::assertSame( - HttpStatus::MOVED_PERMANENTLY, - $response->getStatus() - ); - self::assertSame( - ['location' => ['https://example.com/tls-test?foo=bar']], - $response->getHeaders() - ); - } - - public function testApplicationFeaturesRedirectHttpToHttpsWithExplicitPort() : void { - $request = new Request( - $this->getMockBuilder(Client::class)->getMock(), - HttpMethod::Get->value, - Http::createFromString('http://example.com/tls-test?foo=bar') - ); - - $subject = new AmpApplication( - $this->httpServer, - new ErrorHandlerFactoryStub($this->errorHandler), - $this->router, - $this->emitter, - new Logger('labrador-http-test', [$this->testHandler], [new PsrLogMessageProcessor()]), - $this->getApplicationFeaturesWithHttpToHttpsRedirectAndExplicitHttpsPort(), - $this->analyticsQueue, - $this->preciseTime, - ); - - $subject->start(); - - $response = $subject->handleRequest($request); - - self::assertSame( - HttpStatus::MOVED_PERMANENTLY, - $response->getStatus() - ); - self::assertSame( - ['location' => ['https://example.com:9001/tls-test?foo=bar']], - $response->getHeaders() - ); - } - public function testNormalProcessingHasCorrectRequestAnalyticsQueued() : void { $request = new Request( $this->getMockBuilder(Client::class)->getMock(), @@ -541,7 +481,7 @@ public function testExceptionThrownInRouterHasCorrectRequestAnalytics() : void { new ErrorThrowingRouter($exception = new RuntimeException()), $this->emitter, new NullLogger(), - new NoApplicationFeatures(), + new StubApplicationFeatures(), $this->analyticsQueue, $this->preciseTime ); diff --git a/test/Unit/Web/Logging/LoggerFactoryTest.php b/test/Unit/Web/Logging/LoggerFactoryTest.php index 0eab823..ef54724 100644 --- a/test/Unit/Web/Logging/LoggerFactoryTest.php +++ b/test/Unit/Web/Logging/LoggerFactoryTest.php @@ -42,23 +42,9 @@ public function testLoggerReturnedWithCorrectName(LoggerType $loggerType) : void $logger = $subject->createLogger($loggerType); self::assertInstanceOf(Logger::class, $logger); - self::assertSame($loggerType->value, $logger->getName()); } - /** - * @dataProvider loggerTypeProvider - */ - public function testLoggerReturnedWithPsrLogMessageProcessor(LoggerType $loggerType) : void { - $subject = new MonologLoggerFactory($this->nullInitializer); - $logger = $subject->createLogger($loggerType); - - self::assertInstanceOf(Logger::class, $logger); - - self::assertCount(1, $logger->getProcessors()); - self::assertInstanceOf(PsrLogMessageProcessor::class, $logger->getProcessors()[0]); - } - /** * @dataProvider loggerTypeProvider */ diff --git a/test/Unit/Web/Logging/StdoutMonologLoggerInitializerTest.php b/test/Unit/Web/Logging/StdoutMonologLoggerInitializerTest.php deleted file mode 100644 index 38b992a..0000000 --- a/test/Unit/Web/Logging/StdoutMonologLoggerInitializerTest.php +++ /dev/null @@ -1,61 +0,0 @@ -logger = new Logger('stdout-initializer-test'); - $this->subject = new StdoutMonologLoggerInitializer(); - $this->bufferIdentifier = StreamBuffer::intercept(STDOUT); - StreamBuffer::reset($this->bufferIdentifier); - } - - protected function tearDown() : void { - StreamBuffer::stopIntercepting($this->bufferIdentifier); - } - - public function testInitializedLoggerHasCorrectHandler() : void { - self::assertCount(0, $this->logger->getHandlers()); - - $this->subject->initialize($this->logger, LoggerType::Application); - - self::assertCount(1, $this->logger->getHandlers()); - - $handler = $this->logger->getHandlers()[0]; - self::assertInstanceOf(StreamHandler::class, $handler); - self::assertInstanceOf(ConsoleFormatter::class, $handler->getFormatter()); - } - - public function testLoggerSendsMessagesToStdout() : void { - $this->subject->initialize($this->logger, LoggerType::Worker); - - $this->logger->info('Message sent to STDOUT'); - - self::assertStringContainsString( - 'Message sent to STDOUT', - StreamBuffer::output($this->bufferIdentifier) - ); - } - - - -} \ No newline at end of file