fix($conditionalTest): preserve commas inside SQL_if (quotes + \, escape)#903
Open
oplehto wants to merge 1 commit into
Open
fix($conditionalTest): preserve commas inside SQL_if (quotes + \, escape)#903oplehto wants to merge 1 commit into
oplehto wants to merge 1 commit into
Conversation
…ape)
The 2- vs 3-parameter form of $conditionalTest was decided purely by counting
top-level commas, and the comma scan only skipped commas inside parentheses.
Any 2-parameter macro whose SQL_if contained a comma was therefore misparsed
and the comma dropped, producing broken SQL. Affected cases:
- leading-comma idiom: $conditionalTest(, expr, $var)
- trailing-comma idiom: $conditionalTest(expr,, $var)
- a comma inside a string literal, e.g. AND t = 'a,b', was treated as an
argument separator and truncated the query.
Fix:
- Treat a 2-comma macro as 3-parameter only when BOTH branches are non-empty;
an empty first/middle segment means the comma belongs to SQL_if.
- Make the comma scan quote-aware ('...', "...", `...`) and backslash-aware,
so commas inside string literals are never treated as separators.
- Add `\,` as an explicit escape for a literal comma in SQL_if.
- Document the `\,` escape in the macro autocomplete tooltip.
Adds regression tests for the leading/trailing-comma idioms, the 3-parameter
form, quoted commas (single/double/backtick), escaped quotes, and `\,` escaping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The new conditionalTest 3-parameter format caused some existing queries to break where a comma was a part of the first parameter value. These are somewhat common if we extend GROUP BY or ORDER BY lists with a
conditionalTestmacro: Often there is a need to have a leading comma in the string that conditionalTest produces.Furthermore it would be good IMO to disambiguate if a comma is a delimiter or part of a string properly so there should also be a "proper" form where the commas are escaped.
The 2- vs 3-parameter form of $conditionalTest was decided purely by counting top-level commas, and the comma scan only skipped commas inside parentheses. Any 2-parameter macro whose SQL_if contained a comma was therefore misparsed and the comma dropped, producing broken SQL. Affected cases:
argument separator and truncated the query.
Fix which maintains the compatibility with the existing style and also provides a better, escaped format:
...) and backslash-aware, so commas inside string literals are never treated as separators.\,as an explicit escape for a literal comma in SQL_if.\,escape in the macro autocomplete tooltip.Adds regression tests for the leading/trailing-comma idioms, the 3-parameter form, quoted commas (single/double/backtick), escaped quotes, and
\,escaping.