Report patterns that fail to match with a warning

src/Analyser/PatternAnalyser.php

@@ -59,7 +59,7 @@ public function addPossibleInsightClass(string $insightClass): static
      */
     protected function findPossibleInsightClass(string $insightClass): int
     {
-        $index = array_search($insightClass, $this->possibleInsightClasses);
+        $index = array_search($insightClass, $this->possibleInsightClasses, true);
         if ($index === false) {
             throw new InvalidArgumentException("Class " . $insightClass . " not found in possible insight classes.");
         }
@@ -96,7 +96,25 @@ public function overridePossibleInsightClass(string $parentInsightClass, string
     }
 
     /**
-     * Analyse a log and return an Analysis
+     * Get the patterns of all possible insight classes
+     *
+     * getPatterns() often builds its patterns from string fragments, so calling it
+     * for every entry of a log adds up. The result is only reused within one analysis,
+     * so changes to the possible insight classes or their patterns still take effect.
+     *
+     * @return array<class-string<PatternInsightInterface>, string[]> The patterns of each insight class, keyed by class name
+     */
+    protected function resolvePatterns(): array
+    {
+        $patternsByClass = [];
+        foreach ($this->possibleInsightClasses as $possibleInsightClass) {
+            $patternsByClass[$possibleInsightClass] = $possibleInsightClass::getPatterns();
+        }
+        return $patternsByClass;
+    }
+
+    /**
+     * Analyze a log and return an Analysis
      *
      * @return AnalysisInterface
      */
@@ -105,50 +123,80 @@ public function analyse(): AnalysisInterface
         $analysis = new Analysis();
         $analysis->setLog($this->log);
 
+        $patternsByClass = $this->resolvePatterns();
+
         foreach ($this->log as $entry) {
-            foreach ($this->possibleInsightClasses as $possibleInsightClass) {
-                /** @var PatternInsightInterface $possibleInsightClass */
-                $patterns = $possibleInsightClass::getPatterns();
-                foreach ($patterns as $patternKey => $pattern) {
-                    $insights = $this->analyseEntry($entry, $possibleInsightClass, $patternKey, $pattern);
-                    if ($insights) {
-                        foreach ($insights as $insight) {
-                            $analysis->addInsight($insight);
-                        }
-                    }
-                }
+            foreach ($this->analyseEntry($entry, $patternsByClass) as $insight) {
+                $analysis->addInsight($insight);
             }
         }
 
         return $analysis;
     }
 
     /**
-     * Compare the entry against the given pattern and create an insight object if it matches
+     * Compare an entry against all patterns and create an insight for every match
      *
      * @param EntryInterface $entry
-     * @param string $possibleInsightClass
-     * @param mixed $patternKey
-     * @param string $pattern
-     * @return null|PatternInsightInterface[]
+     * @param array<class-string<PatternInsightInterface>, string[]> $patternsByClass The patterns from resolvePatterns()
+     * @return PatternInsightInterface[]
      */
-    protected function analyseEntry(EntryInterface $entry, string $possibleInsightClass, mixed $patternKey, string $pattern): ?array
+    protected function analyseEntry(EntryInterface $entry, array $patternsByClass): array
     {
-        $result = preg_match_all($pattern, $entry, $matches, PREG_SET_ORDER);
-        if ($result === false || $result === 0) {
-            return null;
+        // Converting an entry to a string joins all of its lines, so it is done once here
+        // and not for every pattern, which used to be most of the time spent analysing a log.
+        $text = (string)$entry;
+
+        $insights = [];
+        foreach ($patternsByClass as $insightClass => $patterns) {
+            foreach ($patterns as $patternKey => $pattern) {
+                foreach ($this->findMatches($pattern, $text) as $matches) {
+                    $insights[] = $this->createInsight($insightClass, $entry, $matches, $patternKey);
+                }
+            }
         }
 
-        $return = [];
-        foreach ($matches as $match) {
-            /** @var PatternInsightInterface $insight */
-            $insight = new $possibleInsightClass();
-            $insight->setMatches($match, $patternKey);
-            $insight->setEntry($entry);
+        return $insights;
+    }
 
-            $return[] = $insight;
+    /**
+     * Find all matches of a pattern in a text
+     *
+     * A pattern that fails, for example because it exceeds the backtrack limit,
+     * triggers a warning and is treated as not matching, so the rest of the log is still analysed.
+     * Without the warning, such a pattern would silently never find its problem.
+     *
+     * @param string $pattern
+     * @param string $text
+     * @return array<array<array-key, string>> One array of groups for every match, empty if the pattern does not match
+     */
+    protected function findMatches(string $pattern, string $text): array
+    {
+        $result = preg_match_all($pattern, $text, $matches, PREG_SET_ORDER);
+        if ($result === false) {
+            // $matches can contain the matches found before the error, they are discarded
+            trigger_error("preg_match_all() failed for pattern " . $pattern . ": " . preg_last_error_msg(), E_USER_WARNING);
+            return [];
         }
 
-        return $return;
+        return $matches;
+    }
+
+    /**
+     * Create an insight for a match of one of its patterns in an entry
+     *
+     * @param class-string<PatternInsightInterface> $insightClass
+     * @param EntryInterface $entry The entry the pattern matched in
+     * @param array<array-key, string> $matches The groups of the match
+     * @param mixed $patternKey The key of the matching pattern in getPatterns()
+     * @return PatternInsightInterface
+     */
+    protected function createInsight(string $insightClass, EntryInterface $entry, array $matches, mixed $patternKey): PatternInsightInterface
+    {
+        $insight = new $insightClass();
+        $insight->setMatches($matches, $patternKey);
+        $insight->setEntry($entry);
+
+        return $insight;
     }
-}
+}

test/tests/Analyser/PatternAnalyserTest.php

@@ -4,6 +4,8 @@
 
 use Aternos\Codex\Analyser\PatternAnalyser;
 use Aternos\Codex\Analysis\Analysis;
+use Aternos\Codex\Analysis\AnalysisInterface;
+use Aternos\Codex\Analysis\InsightInterface;
 use Aternos\Codex\Analysis\PatternInsightInterface;
 use Aternos\Codex\Log\Entry;
 use Aternos\Codex\Log\File\PathLogFile;
@@ -85,6 +87,228 @@ public function testAnalyseWithPossibleInsightClasses(): void
         $this->assertJsonStringEqualsJsonString(json_encode($this->getExpectedAnalysis()->getInsights()), json_encode($analysis->getInsights()));
     }
 
+    public function testAnalyseResolvesPatternsOnce(): void
+    {
+        $logFile = new PathLogFile(__DIR__ . '/../../data/problem.log');
+        $log = (new TestPatternLog())->setLogFile($logFile);
+        $log->parse();
+
+        $insightClass = new class extends TestPatternProblem {
+            public static int $getPatternsCalls = 0;
+
+            /**
+             * @inheritDoc
+             */
+            public static function getPatterns(): array
+            {
+                static::$getPatternsCalls++;
+                return parent::getPatterns();
+            }
+        };
+
+        $analyser = (new PatternAnalyser())->addPossibleInsightClass($insightClass::class);
+        $analysis = $log->analyse($analyser);
+
+        $this->assertGreaterThan(1, count($log));
+        $this->assertSame(1, $insightClass::$getPatternsCalls);
+        $this->assertCount(4, $analysis->getInsights());
+    }
+
+    public function testAnalyseConvertsEveryEntryToStringOnce(): void
+    {
+        $entry = new class extends Entry {
+            public int $stringConversions = 0;
+
+            /**
+             * @inheritDoc
+             */
+            public function __toString(): string
+            {
+                $this->stringConversions++;
+                return parent::__toString();
+            }
+        };
+        $entry->addLine(new Line(1, "I have a problem with ABC"));
+        $entry->addLine(new Line(2, "This log was generated by software v1.2.3"));
+
+        $log = (new TestPatternLog())->addEntry($entry);
+        $analysis = $log->analyse();
+
+        $this->assertSame(1, $entry->stringConversions);
+        $this->assertCount(2, $analysis->getInsights());
+    }
+
+    public function testAnalysePassesMatchesAndPatternKeyToInsight(): void
+    {
+        $insightClass = new class extends TestPatternProblem {
+            /**
+             * @inheritDoc
+             */
+            public static function getPatterns(): array
+            {
+                return [
+                    'problem' => '/I have a problem with (\w+)/',
+                    'version' => '/generated by software (v[0-9.]+)/'
+                ];
+            }
+
+            /**
+             * @inheritDoc
+             */
+            public function setMatches(array $matches, mixed $patternKey): void
+            {
+                $this->cause = $patternKey . " " . $matches[1];
+            }
+        };
+
+        $entry = (new Entry())
+            ->addLine(new Line(1, "I have a problem with ABC"))
+            ->addLine(new Line(2, "This log was generated by software v1.2.3"))
+            ->addLine(new Line(3, "I have a problem with DEF"));
+        $log = (new TestPatternLog())->addEntry($entry);
+
+        $analyser = (new PatternAnalyser())->addPossibleInsightClass($insightClass::class);
+        $analysis = $log->analyse($analyser);
+
+        $this->assertSame($log, $analysis->getLog());
+        $this->assertSame([
+            "There is a problem with problem ABC",
+            "There is a problem with problem DEF",
+            "There is a problem with version v1.2.3"
+        ], array_map(fn(InsightInterface $insight) => $insight->getMessage(), $analysis->getInsights()));
+        foreach ($analysis->getInsights() as $insight) {
+            $this->assertSame($entry, $insight->getEntry());
+        }
+    }
+
+    public function testAnalyseUsesInsightClassAddedTwiceOnlyOnce(): void
+    {
+        $logFile = new PathLogFile(__DIR__ . '/../../data/problem.log');
+        $log = (new TestPatternLog())->setLogFile($logFile);
+        $log->parse();
+
+        $analyser = (new PatternAnalyser())
+            ->setPossibleInsightClasses([
+                TestPatternProblem::class,
+                TestPatternProblem::class,
+                TestPatternInformation::class
+            ]);
+
+        $analysis = $log->analyse($analyser);
+        $this->assertJsonStringEqualsJsonString(json_encode($this->getExpectedAnalysis()->getInsights()), json_encode($analysis->getInsights()));
+    }
+
+    public function testAnalyseIgnoresRemovedInsightClass(): void
+    {
+        $logFile = new PathLogFile(__DIR__ . '/../../data/problem.log');
+        $log = (new TestPatternLog())->setLogFile($logFile);
+        $log->parse();
+
+        $analyser = (new PatternAnalyser())
+            ->setPossibleInsightClasses([
+                TestPatternProblem::class,
+                TestPatternInformation::class
+            ]);
+        $analyser->removePossibleInsightClass(TestPatternProblem::class);
+
+        $analysis = $log->analyse($analyser);
+        $this->assertCount(0, $analysis->getProblems());
+        $this->assertCount(1, $analysis->getInformation());
+    }
+
+    /**
+     * Analyse a log and collect the messages of the warnings triggered by the analyser
+     *
+     * @param TestPatternLog $log
+     * @param PatternAnalyser $analyser
+     * @param string[] $warnings
+     * @return AnalysisInterface
+     */
+    protected function analyseAndCollectWarnings(TestPatternLog $log, PatternAnalyser $analyser, array &$warnings): AnalysisInterface
+    {
+        set_error_handler(function (int $level, string $message) use (&$warnings): bool {
+            $warnings[] = $message;
+            return true;
+        }, E_USER_WARNING);
+
+        try {
+            return $log->analyse($analyser);
+        } finally {
+            restore_error_handler();
+        }
+    }
+
+    public function testAnalyseReportsFailingPatternAndContinues(): void
+    {
+        $insightClass = new class extends TestPatternProblem {
+            /**
+             * @inheritDoc
+             */
+            public static function getPatterns(): array
+            {
+                return [
+                    '/I have a problem with (\w+)/u',
+                    '/I have an issue with (\w+)/'
+                ];
+            }
+        };
+
+        // The invalid UTF-8 byte makes preg_match_all() return false for patterns with the u modifier
+        $entry = (new Entry())
+            ->addLine(new Line(1, "I have a problem with ABC \xFF"))
+            ->addLine(new Line(2, "I have an issue with DEF"));
+        $log = (new TestPatternLog())
+            ->addEntry($entry)
+            ->addEntry((new Entry())->addLine(new Line(3, "I have a problem with GHI")));
+        $analyser = (new PatternAnalyser())->addPossibleInsightClass($insightClass::class);
+
+        $warnings = [];
+        $analysis = $this->analyseAndCollectWarnings($log, $analyser, $warnings);
+
+        $this->assertSame(
+            ['preg_match_all() failed for pattern /I have a problem with (\w+)/u: Malformed UTF-8 characters, possibly incorrectly encoded'],
+            $warnings
+        );
+        $this->assertSame(
+            ["There is a problem with DEF", "There is a problem with GHI"],
+            array_map(fn(InsightInterface $insight) => $insight->getMessage(), $analysis->getInsights())
+        );
+    }
+
+    public function testAnalyseDiscardsMatchesFoundBeforePatternFailed(): void
+    {
+        $insightClass = new class extends TestPatternProblem {
+            /**
+             * @inheritDoc
+             */
+            public static function getPatterns(): array
+            {
+                return ['/I have a problem with (\w+)|(?:a+)+b/'];
+            }
+        };
+
+        // The pattern matches in the first line and then exceeds the backtrack limit in the second line
+        $entry = (new Entry())
+            ->addLine(new Line(1, "I have a problem with ABC"))
+            ->addLine(new Line(2, str_repeat("a", 40)));
+        $log = (new TestPatternLog())->addEntry($entry);
+        $analyser = (new PatternAnalyser())->addPossibleInsightClass($insightClass::class);
+
+        $warnings = [];
+        $backtrackLimit = ini_set('pcre.backtrack_limit', '1000');
+        try {
+            $analysis = $this->analyseAndCollectWarnings($log, $analyser, $warnings);
+        } finally {
+            ini_set('pcre.backtrack_limit', $backtrackLimit);
+        }
+
+        $this->assertSame(
+            ['preg_match_all() failed for pattern /I have a problem with (\w+)|(?:a+)+b/: Backtrack limit exhausted'],
+            $warnings
+        );
+        $this->assertSame([], $analysis->getInsights());
+    }
+
     public function testAddPossibleInsightClassThrowsExceptionIfPossibleInsightClassDoesNotImplementPatternInsightInterface(): void
     {
         $this->expectException(\InvalidArgumentException::class);