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(template-strings): do not apply helper functions on unresolved string #4692

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

shumailxyz
Copy link
Contributor

@shumailxyz shumailxyz commented Jun 21, 2023

What this PR does / why we need it:
Before this fix, helper functions were being applied even on resolved string and the unsubstituted string was considered the string itself for helper function.

Basically, in the first resolveTemplateStrings of module, when only inputs are available and keys like environment are missing, unresolved strings are fine as allowPartial is set to true. However, the helper functions were still being applied.

For example, for a template string like

TEST:  "base64Encode(\"${environment.namespace}\")"`

base64Encode was being called for environment.namespace instead of the actual value and resulted in wrong values. So before the 2nd resolveTemplateStrings of module, when the module context is available, there was no base64Encode(\"${environment.namespace}\") left to be resolved.

The workaround mentioned in original issue #4614
TEST: "${environment && base64Encode(\"${environment.namespace}\")}"
works because of the environment &&. Even though the base64Encode is applied on environment.namespace, the overall expression fails due to environment missing in first pass.

I added a check before helper functions are applied to not apply helper on unresolved string. Had to wrap text in ${} so it's returned as an expression to be resolved later. In case, allowPartial is set to false, it returns error.

Which issue(s) this PR fixes:

Fixes #4614

Special notes for your reviewer:
I am not sure if we can avoid this check altogether and instead extend the actual parser to address this issue. 🤔

@shumailxyz shumailxyz marked this pull request as ready for review June 21, 2023 20:55
@shumailxyz shumailxyz requested review from edvald and a team June 21, 2023 20:56
@vvagaytsev
Copy link
Collaborator

LGTM, but I'd like @edvald to take a look too. Can we add some test cases? The repro examples from the original issue can be used as a base.

@edvald
Copy link
Collaborator

edvald commented Jun 23, 2023

Fix looks great (and very well caught @shumailxyz!) but agree that a test case would be helpful.

@shumailxyz shumailxyz force-pushed the fix/template-str-unresolved branch from 5ab82e3 to 8dbff94 Compare June 26, 2023 10:53
@shumailxyz shumailxyz merged commit 0b47ccc into main Jun 26, 2023
@shumailxyz shumailxyz deleted the fix/template-str-unresolved branch June 26, 2023 12:54
ShankyJS pushed a commit that referenced this pull request Jul 10, 2023
…ring (#4692)

* fix(template-strings): do not apply helper functions on unresolved string

* test: add unit test for helper on unresolved template string
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.

Template string variables unresolved in some circumstances
3 participants