DXCDT-56: Fixing @@ array replacement to work with and without wrapped quotes #421
Conversation
This file contains 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
added 2 commits
February 24, 2022 11:27
…target regex pattern with or without wrapped strings, added tests
…dFile to loadFileAndReplaceKeywords
willvedd
commented
Feb 24, 2022
| @@ -4,18 +4,32 @@ import dotProp from 'dot-prop'; | |||
| import _ from 'lodash'; | |||
| import log from './logger'; | |||
|
|
|||
| export function keywordArrayReplace(input, mappings) { | |||
There was a problem hiding this comment.
Separated out and exported to enable unit testing.
willvedd
commented
Feb 24, 2022
Comment on lines
+10
to
+11
| const pattern = `@@${key}@@`; | ||
| const patternWithQuotes = `"${pattern}"`; |
There was a problem hiding this comment.
The key functional change in this PR is using regex to replace the @@ pattern with and without wrapping quotes.
willvedd
commented
Feb 24, 2022
| const patternWithQuotes = `"${pattern}"`; | ||
|
|
||
| const regex = new RegExp(`${patternWithQuotes}|${pattern}`, 'g'); | ||
| input = input.replace(regex, JSON.stringify(mappings[key])); |
There was a problem hiding this comment.
The key difference between this function and the below keywordStringReplace function is the JSON.stringify call.
willvedd
commented
Feb 24, 2022
| stringReplaceNoQuotes: ##STRING_REPLACEMENT## | ||
| stringReplaceWithQuotes: "##STRING_REPLACEMENT##" | ||
| arrayReplace: @@ARRAY_REPLACEMENT@@ | ||
| arrayReplaceWithQuotes: "@@ARRAY_REPLACEMENT@@" |
There was a problem hiding this comment.
Unlikely that someone would wrap this value in quotes when using YAML, but at least it's technically supported. I don't believe this behavior will interfere with any current customer workflows.
willvedd
commented
Feb 24, 2022
| const inputJSON = '{ "arrayReplaceNoQuotes": @@ARRAY_REPLACEMENT@@, "arrayReplaceWithQuotes": "@@ARRAY_REPLACEMENT@@", "stringReplace": "##STRING_REPLACEMENT##", "noReplace": "OTHER_REPLACEMENT" }'; | ||
| const output = utils.keywordReplace(inputJSON, mapping); | ||
|
|
||
| expect(() => JSON.parse(output)).to.not.throw(); |
There was a problem hiding this comment.
This is the big test, just making sure the output JSON is validated.
…nction Distinguishing between two loadFile functions
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.
✏️ Changes
The way that the
@@FOO@@array variable replacement worked has caused a bit of confusion from users. Firstly, it's not obvious if these markers need to be wrapped in quotes or not. Without them, it invalidates the JSON file that they're used in, but if they do use quotes, the replacement won't work as expected. This can be seen in #415 and #184.For example, as it currently is implemented, developers would need to have the following configurations:
{ "web_origins": @@WEB_ORIGINS@@ }This is invalid JSON and a bit of a wonky thing to force onto the developer. With this change, they can now use the array replacements with or without wrapped quotes:
{ "grant_types": @@GRANT_TYPES@@, "web_origins": "@@WEB_ORIGINS@@" }The tricky part here is that this needs to work both for JSON and YAML input files. This fix is primarily for JSON configurations to maintain the validity of the file. However, YAML is a bit less strict about having the @@ array markers wrapped in quotes, however that is still supported, albeit not with a desired outcome. Examples of this can be seen in the test YAML test cases below.
A big bulk of this PR is adding tests to confirm and enforce the behavior that exists currently.
🎯 Testing
Separated out
keywordArrayReplaceandkeywordStringReplaceinto their own functions so that they could have their own unit tests applied. Also added higher-level unit tests to thekeywordReplacefunction to confirm expected results for both YAML and JSON inputs.✅This change has unit test coverage