fix(core): cannot override properties with . in the name

[Chriscbr]

Closes #10109

While researching this, I saw some comments multiple years old saying that JavaScript doesn't support RegExp lookbehind assertions, but from the looks of https://node.green/ it is supported all the way back to 10.3.0, so that aligns with current CDK supported node versions.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[rix0rrr]

Can you also add a test for backslash escapes in other situations than preceding a .?

Like, what should things like:

r.addOverride('Properties.The\\Back\\Slashes', 42);
r.addOverride('Properties.The\\\\Back\\\\Slashes', 42);

Evaluate to?

Pull request has been modified.

[Chriscbr]

I've fixed the implementation and added some test cases to show the current range behaviors. To specify a property name that itself contains a period preceded by a backslash, you can do so by just adding extra backslashes in front.

It's worth mentioning if you want a property name to end with a backslash, e.g.

'Hello\\': { World: 42 },

that isn't doable, but I think something like this is inevitable with any escaping-based solution. The robust solution would be to change addOverride (or add a variation) that allows for passing in an array of strings. But I don't think there's currently need for that, so this fix should be good enough.

[rix0rrr]

Can you also add a test for backslash escapes in other situations than preceding a .?

What I meant here, which I didn't explain clearly enough (I'm sorry), is: usually, a \ escapes the next character regardless of what it is. Some of these combinations are special, and in cases where they aren't then the original character is left in place unchanged. Think of \n in a string literal: that makes newline, but if the \ is followed by a non-special character is it emitted as normal. For example, console.log('\s'); simply outputs 's', and console.log('\\n'); outputs \n. Also think of how you can escape whitespace in bash: cat filename\ with\ spaces.

I changed the processing logic a little to make it more general, to show you what I mean: the \ always means "the next character literally" and SPECIFICALLY in the case of \. it means: "a literal ., don't split on this.".

---

However, now having written that, I hate how the very obvious escape character is already escaped by the surrounding language, and so you end up having to write "hello\\.world" instead of "hello\.world".

So now I wonder whether we shouldn't use a different escape character.

"Microsoft escaping" (for lack of a better word) often doubles up the special character to escape it, so that would mean typing "hello..world", which is not great but at least not as bad as having to remember to escape the escape character in 99% of the cases...

Thoughts?

[Chriscbr]

Ah, I see what you mean - yeah there was some ambiguity, but I think what you are suggesting where all backslashes escape strings makes sense, and it fixes the problem I mentioned which is great.

I haven't seen the "Microsoft escaping" idea before, but I feel like that presents ambiguity because something like "hello...world" could feasibly be parsed in two different ways (["hello.", "world"] and ["hello", ".world"]) -- and whichever output you give a preference to, there's a question of how do you generate the opposite one. Mathematically, we could say that the function is not surjective, since not all strings could be output. At least with the traditional escaping approach, I believe we're still guaranteed to get surjectivity.

However, now having written that, I hate how the very obvious escape character is already escaped by the surrounding language, and so you end up having to write "hello\.world" instead of "hello.world".

Right, the leaning toothpick syndrome. To make it less verbose, we could write the tests with raw strings to make the behavior more clear (see below):

r.addOverride('Properties.Hello\\.World.Foo\\.Bar\\.Baz', 42);
// ... replaced with ...
r.addOverride(String.raw`Properties.Hello\.World.Foo\.Bar\.Baz', 42);

How does that sound?

[Side note: I was thinking of where else this problem is seen, and I think CSV is probably the most familiar (as this format depends on having a way to escape commas) -- but I personally dislike the solution used there, since it requires wrapping the whole string instead of localizing it to the problem character. So my preference would be towards using backslashes.]

[rix0rrr]

I was not aware of String.raw. That sounds like a good compromise.

[rix0rrr]

We ultimately needed to use a function to scan through the string after all. Using a regex with a backreference won't work because that regex won't be able to tell the difference between:

\.   <-- don't split
\\.  <-- split again

It depends on whether the number of backslashes preceding the . is even or odd. Even though I bet you can check for that using regex in some way, at this point a regular function to scan escapes seems more readable :)

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 27c32b1
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository