Split multi-value TEXT on unescaped separator commas - #968
Conversation
maknapp
left a comment
maknapp
left a comment
There was a problem hiding this comment.
This does fix the issue, but I have some suggestions if you would like to work on this more.
| }; | ||
|
|
||
| var encodedValues = serializeAsList ? UnescapedCommas.Split(value) : new[] { value }; | ||
| var encodedValues = serializeAsList ? SplitOnUnescapedCommas(value) : new List<string> { value }; |
There was a problem hiding this comment.
SplitOnUnescapedCommas(...) could be changed to an iterator method and yield return each value directly to Decode(...) and Unescape(...). This would prevent several unnecessary lists from being created.
The multi-value splitter used a single-character negative lookbehind, (?<!\\), which treats any comma preceded by one backslash as escaped. A value ending in an escaped backslash serializes to \\ before the separator comma, so CATEGORIES:a\\,b (values a\ and b) was mis-split into a single value a\,b. The split was never updated to match the backslash escaping merged in ical-org#965, so a list containing backslashes no longer round-trips. Replace the lookbehind with an escape-aware left-to-right scan that counts backslash parity, mirroring Unescape: a comma separates only when preceded by an even number of backslashes.
e8e09ab to
985330e
Compare
|
Applied both — the split now uses |
|
| var encodedValues = serializeAsList ? UnescapedCommas.Split(value) : new[] { value }; | ||
| var escapedValues = encodedValues.Select(v => Decode(dt, v)).ToList(); | ||
| var values = escapedValues.Select(Unescape).ToList(); | ||
| var encodedValues = serializeAsList ? SplitOnUnescapedCommas(value) : Enumerable.Repeat(value, 1); |
There was a problem hiding this comment.
If we know value is not a list, it would be better to just return Unescape(Decode(dt, value)) directly.
| /// character, so a comma preceded by an odd number of backslashes belongs to a value | ||
| /// (RFC 5545 3.1.1). Mirrors the left-to-right consumption used by <see cref="Unescape"/>. | ||
| /// </summary> | ||
| internal static IEnumerable<string> SplitOnUnescapedCommas(string value) |
There was a problem hiding this comment.
Could make a span-based version using SearchValues for net8.0+
2 participants
Problem
Multi-valued TEXT properties (CATEGORIES, RESOURCES, ...) are split on separator commas with a single-character negative lookbehind:
The lookbehind only inspects the one character before the comma, so it cannot count backslash parity. A value that ends in a backslash is escaped to end in
\\(an escaped backslash), so the serialized stream contains...\\,where the comma is a genuine value separator. The lookbehind sees the trailing\and refuses to split.This is a direct sibling of the backslash-escaping fix merged in #965: the encoder now correctly emits
\\before a list separator, but the splitter was never updated to cope with it, so a list whose values contain backslashes no longer round-trips.Concretely,
[a\, b]serializes toCATEGORIES:a\\,b(correct) but deserializes back to the single valuea\,binstead of the two valuesa\andb.Truth table
A comma is a separator iff preceded by an even number (incl. 0) of backslashes; the old regex splits only when preceded by zero:
a,ba,ba\,ba,ba\\,ba\,ba\\\,ba\,ba\\\\,ba\\,bFix
Replace the lookbehind regex with an escape-aware left-to-right scan that counts backslash parity, mirroring the consumption already used by
Unescape: a backslash escapes the following character, so a comma is a separator only when it is not escaped.The single fix site governs every multi-valued TEXT property (
GetPropertyAllowsMultipleValues), so this repairs the whole class, not just CATEGORIES. The otherSplit(',')/Split(';')calls in the serializers operate on structured non-TEXT grammars with no backslash escaping and are unaffected; the lookbehind splitter was the only one of its kind.Tests
Added a round-trip test over CATEGORIES covering the full truth table above (plain separator, escaped comma inside a value, escaped backslash before a separator, escaped backslash + escaped comma, two escaped backslashes, trailing backslash on the last value, and several separators in one line). The three escaped-backslash cases fail on
mainand pass with the fix; the escaped-comma cases are the opposite-direction controls that must not over-split. Full suite green (2469 tests, net10.0).