Validate C# DefineConstants input

[adamint]

As can be seen in this bug, you can add arbitrary semicolons in the DefineConstants control and these will be saved. This PR

1. Adds tests for KeyValuePairListEncoding
2. Allows for different separators
3. Since the CPS controls hands the property interceptor a comma-separated list of values, but each individual value may itself contain a semicolon-separated list of constants, the property interceptor is changed to first convert the comma-separated list to a semicolon-separated list, then parse it again and trim semicolons from each value

Microsoft Reviewers: Open in CodeFlow

[drewnoakes]

IIRC VB handles these properties differently to C#, so we should make sure to test both languages. @melytc do you recall the details? I think VB allows you to have values (i.e. A=1) for define constants, rather than just names (i.e. A).

[drewnoakes]

Would be good to cover some more inputs:

• ""
• " "
• "="
• ";"

If there are invalid inputs, a test that ensures that Parse throws would be good. For example, "==", or null.

[adamint]

I added test cases on those inputs except null, as the method accepts a non-nullable input string. But I wouldn't necessarily agree that there are invalid inputs possible here, since == should parse to an empty key, value =.

[drewnoakes]

Thanks for adding more tests.

since == should parse to an empty key, value =

I'm less confident about that than you. I feel like == should be an invalid and ambiguous input that throws. If values are expected to contain delimiters, then they should be escaped.

[adamint]

I feel like == should be an invalid and ambiguous input that throws.

I'm not entirely in agreement; the difference between = and the other delimiters is that = has no meaning other than as part of a value after already encountering an =

[drewnoakes]

I mean that it's ambiguous whether the name or value is =. Values also cannot contain commas without escaping.

What happens if the Format method is given names/values that contain = or ,? My guess is that these values would not round-trip correctly.

For robustness, we should either throw on invalid inputs, or implement escaping so that these values are handled correctly.

[drewnoakes]

What happens if you enter strings in the UI containing = and ,?

[adamint]

@drewnoakes I've updated this now and paired with CPS changes to bring this control to a state I feel happy with.

Specifically, the additional change I made here, other than adding more tests, is to in OnGetUnevaluatedPropertyValueAsync also return values that have been removed (ie, in Constant1;Constant2, we will individually show the values Constant1 and Constant2 in the control, but not Constant1;Constant2, so we want to remove that value). I've made two gifs to show the before and after of the control. You can see how messed-up the state can get in the before gif.

before:

after:

[melytc]

IIRC VB handles these properties differently to C#, so we should make sure to test both languages. @melytc do you recall the details? I think VB allows you to have values (i.e. A=1) for define constants, rather than just names (i.e. A).

that's right! VB uses this syntax: symbol1 = value1, symbol2 = value2.

In the project properties UI, we have a different control for VB:

@adamint are these changes that you are proposing only for the C# control?

[adamint]

@adamint are these changes that you are proposing only for the C# control?

yes the vb property has a different control and property interceptor! this is only for c#

[drewnoakes]

I'm not sure it's safe to store this state here. These providers are generally safe to call concurrently. By tracking this state here, that's no longer the case. It might work most of the time, but it makes me a little nervous. I'll spend some more time here to see if I can come up with any suggestions.

[adamint]

I've removed this state and instead handle collapsing multiple values down into one inside the CPS control, as provided by a regex rule in editor metadata. Thanks, your comment prompted me to rethink how this worked.

[haileymck]

@adamint are these changes that you are proposing only for the C# control?

yes the vb property has a different control and property interceptor! this is only for c#

this is a blast from the past!

[drewnoakes]

Is this right?

Logically, (A && B) || B is the same as just B.

Where A is allowsEmptyKey and B is !string.IsNullOrEmpty(decodedEntryValue).

If I'm not mistaken, allowsEmptyKey does nothing here.

I think a ! should be removed:

Suggested change

	if ((allowsEmptyKey && !string.IsNullOrEmpty(decodedEntryValue))
	|| !string.IsNullOrEmpty(decodedEntryKey) || !string.IsNullOrEmpty(decodedEntryValue))
	if ((allowsEmptyKey && string.IsNullOrEmpty(decodedEntryValue))
	|| !string.IsNullOrEmpty(decodedEntryKey) || !string.IsNullOrEmpty(decodedEntryValue))

But the fact that this passed the unit tests makes me wonder if the tests need improving or if the check can just be removed here altogether.

[drewnoakes]

Is this comment still correct? With the changes here, these implementations have diverged.

[drewnoakes]

Rather than introducing a new interface, we can use Moq's built-in support for this.

https://github.com/devlooped/moq/wiki/Quickstart#advanced-features

From memory I think it's something like (those docs aren't super clear):

var mock = new Mock<ConfiguredProject>();
mock.Setup(...);
...
var mock2 = mock.As<ConfiguredProject2>();
mock2.Setup(c => c.EnsureProjectEvaluatedAsync()).Returns(Task.CompletedTask);

return mock.Object;

[drewnoakes]

Though creating a new interface was a pretty clever way to achieve the same thing. Use whichever you prefer. I don't think there's much material difference.