Quote (or don't quote) property values individually before joining

[darktrojan]

Instead of adding quotes to the delimiting comma and around the outside, quote the individual members separately if necessary. This prevents the whole string of multiple values being quoted because of the presence of a delimiting comma.

When quotes are required (multiValueSeparateDQuote is true) only URIs are valid, which will get quoted due to the colon. When not required only specific strings (WORK, VOICE, PARCEL, etc.) are valid, none of which need quotes.

[coveralls]

Pull Request Test Coverage Report for Build 3418295304

• 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.0007%) to 98.137%

---

Totals
Change from base Build 3275454147:	-0.0007%
Covered Lines:	9196
Relevant Lines:	9355

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 8427565049

Details

• 16 of 16 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 98.093%

---

Totals
Change from base Build 8419308194:	0.0%
Covered Lines:	9290
Relevant Lines:	9457

---

💛 - Coveralls

[darktrojan]

Aha, I thought I had asked about this before. I did, in #532.

[dilyanpalauzov]

In the provided test cases:

ADR;TYPE=dom,home,postal,parcel:;;123 Main Street;Any Town;CA;91921-1234
TEL;TYPE=home,voice:1234567

the property values are ;;123 Main Street;Any Town;CA;91921-1234 and 1234567. The property parameter values are dom,home,postal,parcel and home,voice. Just for clarification, can you state again, if the current case is about property values or property parameter values, as these terms can be easily confused.

Concerning the stringification of property parameter values and avoiding unnecessary quotes, I have a different approach — #535, for a different test-case. In A;X=Y:V it does the stringification of the property value parameters (Y) independent of the property-value (V) and thus irrespective on whether the value-type is multi-valued. I think this is the right approach.

I understand this is an open-source voluntary run project and I get what I paid for. Different users of ical.js submit corrections for inclusion upstream. Including the corrections upstream takes sometimes very, very long time. This results that different solutions to the same problem are submitted, while uploaded corrections are not merged upstream.

I will appreciate if some process is suggested, that makes merging submitted changes, which include test cases, upstream more fluent. This will be in the common interest.

I value the time invested in maintaining the project and personally I am not willing to be its maintainer. The only point I am raising is that it takes very long time between patch submission and its inclusion. So any speedup will be more than welcomed.

[dilyanpalauzov]

Ah, this is not about multivalued property values, but about multivalued parameter values. Can you add a test/example, which produces this stringification result:

TEL;TYPE=home,voice,x-a,"x-b,x-c":1234567

[darktrojan]

Yes, the names are confusing. I'd have changed the commit message with my recent push if only Github had notified me about your comments.

Ah, this is not about multivalued property values, but about multivalued parameter values. Can you add a test/example, which produces this stringification result:

TEL;TYPE=home,voice,x-a,"x-b,x-c":1234567

I'm not sure if that's valid. It's a question I've been wrestling with since I first tried to fix this bug. It looks like it should be valid, but I'm yet to find any concrete answer one way or the other. Maybe I'll just assume it is valid and move on.

[darktrojan]

There. Added the extra bit to the test, which unsurprisingly was already working. I also renamed the function in question to be less confusing.

[dilyanpalauzov]

https://www.rfc-editor.org/rfc/rfc6350#section-3.3 defines

any-param = (iana-token / x-name) "=" param-value *("," param-value)
param-value = *SAFE-CHAR / DQUOTE *QSAFE-CHAR DQUOTE
type-param = "TYPE=" type-value ("," type-value)
type-value = "work" / "home" / type-param-tel / type-param-related / iana-token / x-name
x-name = "x-" 1(ALPHA / DIGIT / "-")
; Names that begin with "x-" or "X-" are
; reserved for experimental use, not intended for released
; products, or for use in bilateral agreements.

QSAFE-CHAR = WSP / "!" / %x23-7E / NON-ASCII
; Any character except CTLs, DQUOTE

SAFE-CHAR = WSP / "!" / %x23-39 / %x3C-7E / NON-ASCII
; Any character except CTLs, DQUOTE, ";", ":"
x22 x3A x3B

Comma (ascii code dec 44, %x2C) is SAFE-CHAR and QSAFE-CHARE, as it is within %x23-39 and within %x23-7E.

I came now the conclusion that x-b,x-c as in TEL;TYPE=home,voice,x-a,\"x-b,x-c\":1234567\r\n is not permitted in TYPE as separate value: x-b,x-c is not an x-name, since it contains comma. The ABNF of type-parameter (TYPE=) does not permit a comma or a quote in its value, when the comma is not separator, it permits x-name.

I have created one more example:

diff --git a/test/stringify_test.js b/test/stringify_test.js
index bc375b6..1ebd7c4 100644
--- a/test/stringify_test.js
+++ b/test/stringify_test.js
@@ -193,6 +193,14 @@ suite('ICAL.stringify', function() {
             },
             "phone-number",
             "1234567"
+          ],
+          [
+            "x-a",
+            {
+                "x-B": "x-c^d,"
+            },
+            "text",
+            "Brum"
           ]
         ]
       ];
@@ -200,6 +208,7 @@ suite('ICAL.stringify', function() {
         "BEGIN:VCARD\r\n" +
         "ADR;TYPE=dom,home,postal,parcel:;;123 Main Street;Any Town;CA;91921-1234\r\n" +
         "TEL;TYPE=home,voice,x-a,\"x-b,x-c\":1234567\r\n" +
+        "X-A;X-B=x-c^^d,;VALUE=TEXT:Brum\r\n" +
         "END:VCARD";
       assert.equal(ICAL.stringify.component(subject), expected);
     });

This fails. The produced output is X-A;X-B="x-c^^d,";VALUE=TEXT:Brum but there is no need to quote, since x-c^^d, are all SAFE-CHAR*.

  931 passing (964ms)
  1 failing

  1) ICAL.stringify
       stringify component
         multiple types unquoted:

      AssertionError: expected 'BEGIN:VCARD\r\nADR;TYPE=dom,home,post…' to equal 'BEGIN:VCARD\r\nADR;TYPE=dom,home,post…'
      + expected - actual

       BEGIN:VCARD
       ADR;TYPE=dom,home,postal,parcel:;;123 Main Street;Any Town;CA;91921-1234
       TEL;TYPE=home,voice,x-a,"x-b,x-c":1234567
      -X-A;X-B="x-c^^d,";VALUE=TEXT:Brum
      +X-A;X-B=x-c^^d,;VALUE=TEXT:Brum
       END:VCARD
      
      at assert.equal (node_modules/chai/lib/chai/interface/assert.js:139:10)

[dilyanpalauzov]

#460 is a different approach to the same problem.

[darktrojan]

I took the tests from #460 since they're more comprehensive than mine, and fixed a failure they caught.

[dilyanpalauzov]

const multiValue = paramDesign?.multiValue;

[dilyanpalauzov]

For me it is unclear, why the above suggestion was skipped.

[dilyanpalauzov]

The backslash is not escaping character in property parameters. When I modify one of the tests as:

diff --git a/test/stringify_test.js b/test/stringify_test.js
index 40337b8..800fe19 100644
--- a/test/stringify_test.js
+++ b/test/stringify_test.js
@@ -84,9 +84,9 @@ suite('ICAL.stringify', function() {
       ICAL.design.defaultSet.property.custom = { defaultType: 'text' };
       ICAL.design.defaultSet.param.type = { multiValue: ',' };
       let subject = new ICAL.Property('custom');
-      subject.setParameter('type', ['ABC', 'XYZ']);
+      subject.setParameter('type', ['\\:ABC', 'XYZ']);
       subject.setValue('some value');
-      assert.equal(subject.toICALString(), 'CUSTOM;TYPE=ABC,XYZ:some value');
+      assert.equal(subject.toICALString(), 'CUSTOM;TYPE="\\:ABC",XYZ:some value');
       delete ICAL.design.defaultSet.property.custom;
       delete ICAL.design.defaultSet.param.type;
     });

it fails:

  1) ICAL.stringify                                                                                                                     
       stringify property                                                                                                               
         property with multiple parameter values:                                                                                       
                                                                                                                                        
      AssertionError: expected 'CUSTOM;TYPE=\:ABC,XYZ:some value' to equal 'CUSTOM;TYPE="\:ABC",XYZ:some value'                         
      + expected - actual                                                                                                               
                                                                                                                                        
      -CUSTOM;TYPE=\:ABC,XYZ:some value                                                                                                 
      +CUSTOM;TYPE="\:ABC",XYZ:some value                                                                                               

\:ABC must be quoted, since \ is not an escaping character and the property parameter value contains colon :.

#535 suggests to use value.indexOf() in stringify.propertyParameterValue instead of helpers.unescapedIndexOf().

[darktrojan]

Rebased with no further changes.

FWIW, we've been shipping these changes in Thunderbird since November 2022. It may not be perfect but I think we should merge as-is and open follow-up issues for outstanding problems.

[kewisch]

Sounds fine to me, thank you!

[dilyanpalauzov]

As I mentioned above, this change is suboptimal. #658 does further improve encoding (quoting) of property parameter values.