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

env: fix errors on valid interpolation expressions #307

Merged
merged 1 commit into from Sep 23, 2022

Conversation

milas
Copy link
Member

@milas milas commented Sep 23, 2022

The parser was too strict here and would reject valid constructs that used an unescaped $ that was not part of a variable expression (and not ambiguous).

Now, only errors for unmatched braced expressions (e.g. ${FOO) are returned, but other valid cases are ignored and the $ will be treated literally, e.g. a $ string -> a $ string, which is the same as in POSIX.

@milas milas added the bug Something isn't working label Sep 23, 2022
@milas milas self-assigned this Sep 23, 2022
@milas milas requested a review from ndeloof as a code owner Sep 23, 2022
func TestInvalid(t *testing.T) {
invalidTemplates := []string{
"${",
"$}",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is valid and is in the new test cases above.

echo "$}"
$}

@glours
Copy link
Contributor

glours commented Sep 23, 2022

@milas can you sign-off your commit please 😂 ?

glours
glours approved these changes Sep 23, 2022
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,7 +31,7 @@ var substitutionNamed = "[_a-z][_a-z0-9]*"
var substitutionBraced = "[_a-z][_a-z0-9]*(?::?[-+?](.*}|[^}]*))?"

var patternString = fmt.Sprintf(
"%s(?i:(?P<escaped>%s)|(?P<named>%s)|{(?P<braced>%s)}|(?P<invalid>))",
"%s(?i:(?P<escaped>%s)|(?P<named>%s)|{(?:(?P<braced>%s)}|(?P<invalid>)))",
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a playground with the expanded regex and various cases: https://regex101.com/r/H5aQT8/1

Copy link
Member Author

Choose a reason for hiding this comment

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

The change here is to move the "invalid" named group (doesn't actually capture anything, used as a sentinel) to be part of the "braced" section so we can detect an unmatched brace, so there's an extra (non-capturing) group.

The parser was too strict here and would reject valid constructs
that used an unescaped `$` that was not part of a variable
expression (and not ambiguous).

Now, only errors for unmatched braced expressions (e.g. `${FOO`)
are returned, but other valid cases are ignored and the `$` will
be treated literally, e.g. `a $ string` -> `a $ string`, which is
the same as in POSIX.

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas milas force-pushed the env-parser-strictness branch from 74cfbe9 to 85f1221 Compare Sep 23, 2022
Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

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

Pretty awesome 4-character fix PR if I do say so myself!

@milas milas merged commit 6bf774c into compose-spec:master Sep 23, 2022
11 checks passed
@milas milas deleted the env-parser-strictness branch Sep 23, 2022
@laurazard
Copy link
Member

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants