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: LIKE expression with invalid string literals returns a parse error instead of panicking #1046

Merged

Conversation

Cali0707
Copy link
Contributor

@Cali0707 Cali0707 commented May 1, 2024

Fixes #931

When visiting the nodes of the AST while parsing the LIKE expression, it looks like we assumed that we would have a valid string literal. However, it is possible that there is no valid string literal, in which case we have to handle that and return an error rather than continuing to parse (and subsequently panicking)

@Cali0707 Cali0707 requested a review from a team as a code owner May 1, 2024 14:52
@Cali0707
Copy link
Contributor Author

Cali0707 commented May 1, 2024

cc @duglin @pierDipi @lionelvillard

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

LGTM

For reference, here's the spec for like-operation [ref]

like-operation ::= expression not-operator? like-operator string-literal

string-literal ::= ( "'" ( [^'] | "\'" )* "'" ) | ( '"' ( [^"] | '\"' )* '"')

@duglin
Copy link
Contributor

duglin commented May 3, 2024

Aside from my minor question, LGTM

@Cali0707
Copy link
Contributor Author

cc @embano1

@duglin
Copy link
Contributor

duglin commented May 14, 2024

do we have a testcase for this?

@Cali0707
Copy link
Contributor Author

do we have a testcase for this?

@duglin yes, the new tck test in this PR runs and passes with this change, but prior to the change it would panic

@lionelvillard
Copy link
Contributor

@duglin for final approval/merge

@duglin duglin force-pushed the fix-like-expression-panic-on-parse branch from 6f1b2c3 to 3e20125 Compare June 12, 2024 16:10
@duglin
Copy link
Contributor

duglin commented Jun 12, 2024

LGTM - it needed a rebase so kicked that off...

@duglin
Copy link
Contributor

duglin commented Jun 12, 2024

merging...

@duglin duglin merged commit e8fccd2 into cloudevents:main Jun 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic if LIKE pattern is not a quoted string
4 participants