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

Fix bugs in SQLScriptScanner with big String literals and PostgreSQL identifiers (as introduced by #7646) #7818

Merged
merged 5 commits into from
Nov 17, 2023

Conversation

inponomarev
Copy link
Contributor

"This never happened before, and here we go again": unfortunately #7646 introduced not only proper bug fixes, but some new bugs as well, reproducible on some of our test SQL scripts.

This PR introduces 3 changes:

  1. (⚠️ The biggest problem). It seems that Java's regexp "(\\"|[^"])*", though working well with strings of a reasonable size, tends to yield StackOverflowError when literals are big (10k simbols and more). The workaround is the same as with comments: use finite-state machine to match the literal, not the regexp.
  2. According to PostgreSQL docs, dollar sign can be a part of an identifier (non-standard behaviour for SQL, as they themselves admit), and it must be handled so that this$is$a$valid$postgreSQL$identifier is not mixed with dollar-quoted strings
  3. m.find(offset) && m.start() == offset was suboptimal (we are only interested in cases when the regexp matches exactly starting from the offset and there is no reason in finding it somewhere far away in the script).

Existing tests are not modified, new tests added.

@inponomarev inponomarev requested a review from a team as a code owner November 16, 2023 11:40
@kiview kiview changed the title fix bugs introduced by #7646 Fix bugs in SQLScriptScanner with big String literals and PostgreSQL identifiers (as introduced by #7646) Nov 16, 2023
@kiview kiview added this to the next milestone Nov 16, 2023
@kiview
Copy link
Member

kiview commented Nov 16, 2023

The checkstyle issue breaks CI.

I am not sure, why it is not correctly displayed as failing CI, I'd say our find_gradle_jobs task is not correctly being recognized as failing.

You can run the checkstyle checks locally with ./gradlew :database-commons:spotlessCheck.

@eddumelendez eddumelendez merged commit b59888a into testcontainers:main Nov 17, 2023
90 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @inponomarev !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants