refactor: clean code and move log logic to logger

ems-project · Dec 20, 2023 · 87fb859 · 87fb859
1 parent b0301d0
commit 87fb859
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 32 deletions.
diff --git a/EMS/common-bundle/src/Elasticsearch/Client.php b/EMS/common-bundle/src/Elasticsearch/Client.php
@@ -5,7 +5,6 @@
 namespace EMS\CommonBundle\Elasticsearch;
 
 use Elastica\Client as BaseClient;
-use Elastica\Connection;
 use Elastica\Exception\ClientException;
 use Elastica\Request;
 use Elastica\Response;
@@ -24,9 +23,7 @@ class Client extends BaseClient
      */
     public function request($path, $method = Request::GET, $data = [], array $query = [], $contentType = Request::DEFAULT_CONTENT_TYPE): Response
     {
-        if (null !== $this->stopwatch) {
-            $this->stopwatch->start('es_request', 'fos_elastica');
-        }
+        $this->stopwatch?->start('es_request', 'fos_elastica');
 
         $response = parent::request($path, $method, $data, $query, $contentType);
         $responseData = $response->getData();
@@ -44,11 +41,8 @@ public function request($path, $method = Request::GET, $data = [], array $query
             throw new ClientException($message);
         }
 
-        $this->logQuery($response, $connection, $path, $method, $query, $data);
-
-        if ($this->stopwatch) {
-            $this->stopwatch->stop('es_request');
-        }
+        $this->log($response, $lastRequest);
+        $this->stopwatch?->stop('es_request');
 
         return $response;
     }
@@ -58,30 +52,12 @@ public function setStopwatch(?Stopwatch $stopwatch = null): void
         $this->stopwatch = $stopwatch;
     }
 
-    /**
-     * @param array<mixed>|string $data
-     * @param array<mixed>        $query
-     */
-    private function logQuery(Response $elasticaResponse, Connection $connection, string $path, string $method, array $query, array|string $data): void
+    private function log(Response $response, Request $request): void
     {
         if (!$this->_logger instanceof ElasticaLogger || !$this->_logger->isEnabled()) {
             return;
         }
 
-        $connectionArray = [
-            'host' => $connection->getHost(),
-            'port' => $connection->getPort(),
-            'transport' => $connection->getTransport(),
-            'headers' => $connection->hasConfig('headers') ? $connection->getConfig('headers') : [],
-        ];
-
-        $responseData = $elasticaResponse->getData();
-
-        $queryTime = $elasticaResponse->getQueryTime();
-        $engineMS = $responseData['took'] ?? 0;
-
-        $itemCount = $responseData['hits']['total']['value'] ?? $responseData['hits']['total'] ?? 0;
-
-        $this->_logger->logQuery($path, $method, $data, $queryTime, $connectionArray, $query, $engineMS, $itemCount);
+        $this->_logger->logResponse($response, $request);
     }
 }
diff --git a/EMS/common-bundle/src/Elasticsearch/ElasticaLogger.php b/EMS/common-bundle/src/Elasticsearch/ElasticaLogger.php
@@ -4,6 +4,8 @@
 
 namespace EMS\CommonBundle\Elasticsearch;
 
+use Elastica\Request;
+use Elastica\Response;
 use EMS\CommonBundle\Contracts\Elasticsearch\QueryLoggerInterface;
 use EMS\Helpers\Standard\Json;
 use Psr\Log\AbstractLogger;
@@ -34,6 +36,55 @@ public function enable(): void
         $this->enabled = true;
     }
 
+    public function logResponse(Response $response, Request $request): void
+    {
+        $connection = $request->getConnection();
+
+        $responseData = $response->getData();
+        $queryTime = $response->getQueryTime();
+        $engineMS = $responseData['took'] ?? 0;
+        $itemCount = $responseData['hits']['total']['value'] ?? $responseData['hits']['total'] ?? 0;
+
+        $executionMS = $queryTime * 1000;
+        $data = $request->getData();
+
+        if ($this->debug) {
+            if (\is_string($data)) {
+                $jsonStrings = \explode("\n", $data);
+                $data = [];
+                foreach ($jsonStrings as $json) {
+                    if ('' != $json) {
+                        $data[] = Json::decode($json);
+                    }
+                }
+            } else {
+                $data = [$data];
+            }
+
+            $this->queries[] = [
+                'path' => $request->getPath(),
+                'method' => $request->getMethod(),
+                'data' => $data,
+                'executionMS' => $executionMS,
+                'engineMS' => $engineMS,
+                'connection' => [
+                    'host' => $connection->getHost(),
+                    'port' => $connection->getPort(),
+                    'transport' => $connection->getTransport(),
+                    'headers' => $connection->hasConfig('headers') ? $connection->getConfig('headers') : [],
+                ],
+                'queryString' => $request->getQuery(),
+                'itemCount' => $itemCount,
+                'backtrace' => (new \Exception())->getTraceAsString(),
+            ];
+        }
+
+        if (null !== $this->logger) {
+            $message = \sprintf('%s (%s) %0.2f ms', $request->getPath(), $request->getMethod(), $executionMS);
+            $this->logger->info($message, (array) $data);
+        }
+    }
+
     /**
      * @param array<mixed>|string $data       Arguments
      * @param array<mixed>        $connection Host, port, transport, and headers of the query

diff --git a/EMS/common-bundle/src/Elasticsearch/QueryStringEscaper.php b/EMS/common-bundle/src/Elasticsearch/QueryStringEscaper.php
@@ -0,0 +1,17 @@
+<?php
+
+declare(strict_types=1);
+
+namespace EMS\CommonBundle\Elasticsearch;
+
+class QueryStringEscaper
+{
+    private const REGEX_RESERVED_CHARACTERS = '/[\\+\\-\\=\\&\\|\\!\\(\\)\\{\\}\\[\\]\\^\\"\\~\\*\\<\\>\\?\\:\\\\\\/]/';
+
+    public static function escape(string $queryString): string
+    {
+        $test = \preg_replace(self::REGEX_RESERVED_CHARACTERS, \addslashes('\\\\$0'), $queryString);
+
+        return $test;
+    }
+}
diff --git a/EMS/common-bundle/tests/Unit/Elasticsearch/ElasticaLoggerAiTest.php b/EMS/common-bundle/tests/Unit/Elasticsearch/ElasticaLoggerAiTest.php
@@ -4,6 +4,9 @@
 
 namespace EMS\CommonBundle\Tests\Elasticsearch;
 
+use Elastica\Connection;
+use Elastica\Request;
+use Elastica\Response;
 use EMS\CommonBundle\Elasticsearch\ElasticaLogger;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -33,14 +36,18 @@ public function testLogQuery(): void
         $path = '/test_path';
         $method = 'GET';
         $data = ['key' => 'value'];
-        $queryTime = 0.5;
+
+        $response = new Response('');
+        $connection = new Connection();
+        $request = new Request($path, $method, $data);
+        $request->setConnection($connection);
 
         $this->logger->expects($this->once())->method('info')->with(
             $this->stringContains($path),
             $this->equalTo([$data])
         );
 
-        $this->elasticaLogger->logQuery($path, $method, $data, $queryTime);
+        $this->elasticaLogger->logResponse($response, $request);
 
         $this->assertSame(1, $this->elasticaLogger->getNbQueries());
         $queries = $this->elasticaLogger->getQueries();
@@ -51,7 +58,12 @@ public function testLogQuery(): void
 
     public function testReset(): void
     {
-        $this->elasticaLogger->logQuery('/test_path', 'GET', ['key' => 'value'], 0.5);
+        $response = new Response('');
+        $connection = new Connection();
+        $request = new Request('/test_path', 'GET', ['key' => 'value']);
+        $request->setConnection($connection);
+
+        $this->elasticaLogger->logResponse($response, $request);
         $this->assertSame(1, $this->elasticaLogger->getNbQueries());
 
         $this->elasticaLogger->reset();

diff --git a/EMS/common-bundle/tests/Unit/Elasticsearch/QueryStringEscaperTest.php b/EMS/common-bundle/tests/Unit/Elasticsearch/QueryStringEscaperTest.php
@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+namespace EMS\CommonBundle\Tests\Unit\Elasticsearch;
+
+use EMS\CommonBundle\Elasticsearch\QueryStringEscaper;
+use PHPUnit\Framework\TestCase;
+
+class QueryStringEscaperTest extends TestCase
+{
+    public function testEscape(): void
+    {
+        $this->assertSame('n° 01\\\\/070', QueryStringEscaper::escape('n° 01/070'));
+    }
+}