Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved parsing of single-quoted strings #2949

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Conversation

nvoxland
Copy link
Contributor

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

The sql parser did not correctly handle a string with \s before a closing ' when there were more 's in the line.

For example, this SQL failed to split on the #:

/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
#
/*!40101 SET NAMES utf8 */;
#
/*!50503 SET NAMES utf8mb4 */;
#
/*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 */;
#
/*!40101 SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO' */;
#

CREATE PROCEDURE test_procedure (
    in      p1      text
)
begin
    if p1 != '\\' then
select current_timestamp;
end if;
end
#

/*!40101 SET SQL_MODE=IFNULL(@OLD_SQL_MODE, '') */;
#
/*!40014 SET FOREIGN_KEY_CHECKS=IF(@OLD_FOREIGN_KEY_CHECKS IS NULL, 1, @OLD_FOREIGN_KEY_CHECKS) */;
#
/*!40101 SET CHARACTER_SET_CLIENT=@OLD_CHARACTER_SET_CLIENT */;
#

called from

   <sqlFile
                path="some_procedure_name.sql"
                relativeToChangelogFile="true"
                endDelimiter="#"
        />

Things to be aware of

Things to worry about

  • Changes all the single-quoted string handling, but should be a general improvement. Existing tests cover the main odd cases

@github-actions
Copy link

Unit Test Results

  4 548 files  ±    0    4 548 suites  ±0   32m 6s ⏱️ +15s
  4 519 tests +  11    4 305 ✔️ +  11     214 💤 ±0  0 ±0 
53 508 runs  +132  48 500 ✔️ +132  5 008 💤 ±0  0 ±0 

Results for commit dbf9bc2. ± Comparison against base commit ef4fc9f.

Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR updates the javacc-maven-plugin to v3.03. Maven Central reports this dependency has a CVE.

javacc-maven-plugin

REQUEST DEV REVIEW and/or CHANGES

CC @nvoxland, @kristyldatical , @kevin-atx

@wwillard7800 wwillard7800 self-requested a review June 21, 2022 15:39
@nvoxland
Copy link
Contributor Author

This PR updates the javacc-maven-plugin to v3.03. Maven Central reports this dependency has a CVE.

javacc-maven-plugin

This is not a vulnerability in javacc-maven-plugin, but in a test dependency it has (an older version of junit). We're not bringing in their test dependencies so that does not impact us.

Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fix addresses some issues parsing single-quoted strings.
  • The CVE in the javacc-maven-plugin is related to the plugin's build dependencies. We do not ship the javacc-maven-plugin test dependencies. This CVE can be ignored for the purposes of this PR.
  • New tests added to several test classes (SqlParserTest, StringUtilTest, SimpleSqlGrammarTest).

APPROVED

Passing Functional Tests
Passing Test Harness Execution

@nvoxland nvoxland merged commit 341e7a6 into master Jun 22, 2022
@nvoxland nvoxland deleted the fix-quote-parsing branch June 22, 2022 13:29
@kataggart kataggart added this to the NEXT milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants