Fix codacy.generic.sql.lookup type without language or apps fnd#23
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Codacy rules in docs/codacy-rules.yaml by replacing the previous regex patterns for SQL files with a pattern-either block containing more complex regular expressions. A critical catastrophic backtracking (ReDoS) vulnerability was identified in one of the new regex patterns, which could cause the analysis process to hang on large SQL files. A fix was suggested to exclude newlines from the character class.
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull Request Overview
While this PR aims to refine SQL lookup rules, there are critical gaps between the implementation and the intended acceptance criteria that should prevent merging in its current state. Specifically, the regex for lookup_type fails to exclude SQL comments, which will lead to false positives. Conversely, the pattern for apps.fnd_lookup_values is overly restrictive and will fail to detect valid code when SQL hints or block comments (e.g., /*+ hint */) precede the keyword.
Additionally, there is a systemic risk regarding the use of the \K escape sequence; this requires a PCRE-compatible engine. If the execution environment does not support PCRE, these rules will fail to execute entirely. No sample SQL files or test cases were provided to verify these patterns against the stated requirements.
About this PR
- The use of
\K(Reset Start of Match) requires a PCRE-compatible regex engine. Please verify that the environment where these rules are executed supports PCRE, otherwise the rules may fail to function or throw errors. - The PR lacks sample SQL files or test cases. Given the complexity of the regex patterns being introduced to handle block delimiters and comments, including valid and invalid SQL samples is necessary to ensure the rules behave as expected.
Test suggestions
- Missing recommended test scenario: SQL statement containing 'lookup_type' without a 'language' clause should trigger a violation.
- Missing recommended test scenario: SQL statement containing 'lookup_type' with a 'language' clause should not trigger a violation.
- Missing recommended test scenario: Any usage of 'apps.fnd_lookup_values' should trigger a violation.
- Missing recommended test scenario: Keywords located within '--' or '/* */' comments should be ignored.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: SQL statement containing 'lookup_type' without a 'language' clause should trigger a violation.
2. Missing recommended test scenario: SQL statement containing 'lookup_type' with a 'language' clause should not trigger a violation.
3. Missing recommended test scenario: Any usage of 'apps.fnd_lookup_values' should trigger a violation.
4. Missing recommended test scenario: Keywords located within '--' or '/* */' comments should be ignored.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
No description provided.