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

Validate C# DefineConstants input #9612

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@ namespace Microsoft.VisualStudio.ProjectSystem.Debug;

internal static class KeyValuePairListEncoding
{
public static IEnumerable<(string Name, string Value)> Parse(string input)
public static IEnumerable<(string Name, string Value)> Parse(string input, char separator = ',')
{
if (string.IsNullOrWhiteSpace(input))
{
yield break;
}

foreach (var entry in ReadEntries(input))
foreach (var entry in ReadEntries(input, separator))
{
var (entryKey, entryValue) = SplitEntry(entry);
var decodedEntryKey = Decode(entryKey);
Expand All @@ -23,13 +23,13 @@ internal static class KeyValuePairListEncoding
}
}

static IEnumerable<string> ReadEntries(string rawText)
static IEnumerable<string> ReadEntries(string rawText, char separator)
{
bool escaped = false;
int entryStart = 0;
for (int i = 0; i < rawText.Length; i++)
{
if (rawText[i] == ',' && !escaped)
if (separator == rawText[i] && !escaped)
adamint marked this conversation as resolved.
Show resolved Hide resolved
{
yield return rawText.Substring(entryStart, i - entryStart);
entryStart = i + 1;
Expand Down Expand Up @@ -67,7 +67,7 @@ static IEnumerable<string> ReadEntries(string rawText)
}
}

return (string.Empty, string.Empty);
return (entry, string.Empty);
}

static string Decode(string value)
Expand All @@ -76,12 +76,13 @@ static string Decode(string value)
}
}

public static string Format(IEnumerable<(string Name, string Value)> pairs)
public static string Format(IEnumerable<(string Name, string Value)> pairs, string separator = ",")
{
// Copied from ActiveLaunchProfileEnvironmentVariableValueProvider in the .NET Project System.
// In future, EnvironmentVariablesNameValueListEncoding should be exported from that code base and imported here.
Comment on lines 88 to 89
Copy link
Member

Choose a reason for hiding this comment

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

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


return string.Join(",", pairs.Select(kvp => $"{Encode(kvp.Name)}={Encode(kvp.Value)}"));
return string.Join(separator, pairs.Select(kvp => string.IsNullOrEmpty(kvp.Value)
? Encode(kvp.Name)
: $"{Encode(kvp.Name)}={Encode(kvp.Value)}"));
adamint marked this conversation as resolved.
Show resolved Hide resolved

static string Encode(string value)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public override async Task<string> OnGetUnevaluatedPropertyValueAsync(string pro

return KeyValuePairListEncoding.Format(
ParseDefinedConstantsFromUnevaluatedValue(unevaluatedDefineConstantsValue)
.Select(symbol => (Key: symbol, Value: bool.FalseString))
.Select(symbol => (symbol, bool.FalseString))
);
}

Expand Down Expand Up @@ -76,9 +76,18 @@ public override async Task<bool> IsValueDefinedInContextAsync(string propertyNam
IEnumerable<string> innerConstants =
ParseDefinedConstantsFromUnevaluatedValue(await defaultProperties.GetUnevaluatedPropertyValueAsync(ConfiguredBrowseObject.DefineConstantsProperty) ?? string.Empty);

IEnumerable<string> constantsToWrite = KeyValuePairListEncoding.Parse(unevaluatedPropertyValue)
// we receive a comma-separated list, we should convert it to a semicolon-separated list
// because each item may have multiple values separated by semicolons
// ie "A,B;C" should be converted to "A;B;C"
unevaluatedPropertyValue = KeyValuePairListEncoding.Format(
KeyValuePairListEncoding.Parse(unevaluatedPropertyValue, separator: ','),
separator: ";");
adamint marked this conversation as resolved.
Show resolved Hide resolved

IEnumerable<string> constantsToWrite = KeyValuePairListEncoding.Parse(unevaluatedPropertyValue, separator: ';')
.Select(pair => pair.Name)
.Where(x => !innerConstants.Contains(x))
.Select(constant => constant.Trim(';')) // trim any leading or trailing semicolons, because we will add our own separating semicolons
.Where(constant => !innerConstants.Contains(constant))
.Where(constant => !string.IsNullOrEmpty(constant)) // you aren't allowed to add a semicolon as a constant
.Distinct()
.ToList();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE.md file in the project root for more information.

using Microsoft.VisualStudio.ProjectSystem.Debug;

namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties;

public class KeyValuePairListEncodingTests
{
[Theory]
[InlineData("key1=value1;key2=value2", new[] { "key1", "value1", "key2", "value2" })]
[InlineData("key1=value1;;key2=value2", new[] { "key1", "value1", "key2", "value2" })]
[InlineData("key1=value1;;;key2=value2", new[] { "key1", "value1", "key2", "value2" })]
[InlineData("key1=value1;key2=value2;key3=value3", new[] { "key1", "value1", "key2", "value2", "key3", "value3" })]
[InlineData("key1;key2=value2", new[] { "key1", "", "key2", "value2" })]
[InlineData("key1;key2;key3=value3", new[] { "key1", "", "key2", "", "key3", "value3" })]
[InlineData("key1;;;key3;;", new[] { "key1", "", "key3", "" })]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@adamint adamint Dec 6, 2024

Choose a reason for hiding this comment

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

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 =.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 =

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

public void Parse_ValidInput_ReturnsExpectedPairs(string input, string[] expectedPairs)
{
var result = KeyValuePairListEncoding.Parse(input, ';').SelectMany(pair => new[] { pair.Name, pair.Value }).ToArray();
Assert.Equal(expectedPairs, result);
}

[Theory]
[InlineData(new[] { "key1", "value1", "key2", "value2" }, "key1=value1;key2=value2")]
[InlineData(new[] { "key1", "value1", "key2", "value2", "key3", "value3" }, "key1=value1;key2=value2;key3=value3")]
[InlineData(new[] { "key1", "", "key2", "value2" }, "key1;key2=value2")]
[InlineData(new[] { "key1", "", "key2", "", "key3", "value3" }, "key1;key2;key3=value3")]
adamint marked this conversation as resolved.
Show resolved Hide resolved
public void Format_ValidPairs_ReturnsExpectedString(string[] pairs, string expectedString)
{
var nameValuePairs = ToNameValues(pairs);
var result = KeyValuePairListEncoding.Format(nameValuePairs, ";");
Assert.Equal(expectedString, result);
return;

static IEnumerable<(string Name, string Value)> ToNameValues(IEnumerable<string> pairs)
{
using var e = pairs.GetEnumerator();
while (e.MoveNext())
{
var name = e.Current;
Assert.True(e.MoveNext());
var value = e.Current;
yield return (name, value);
}
}
}
}
Loading