[PATCH] conv_xmlformat_to_vcard invents empty property components

[henrik]

Given

<contact>
  <EMail Preferred="true">
    <Content>email1@email1.com</Content>
  </EMail>
  <FormattedName>
    <Content>First1 Last1</Content>
  </FormattedName>
  <Name>
    <LastName>Last1</LastName>
    <FirstName>First1</FirstName>
  </Name>
  <Nickname>
    <Content>Nick1</Content>
  </Nickname>
  <Telephone Location="Work">
    <Content>11111111</Content>
  </Telephone>
</contact>

conv_xmlformat_to_vcard produces

BEGIN:VCARD
VERSION:2.1
EMAIL;PREF:email1@email1.com
FN:First1 Last1
N:Last1;First1;;;
NICKNAME:Nick1
TEL;WORK:11111111
END:VCARD

Notice the three extra semi-colons in N.

I believe this is a bug.

 http://www.imc.org/pdi/vcard-21.txt is not very clear on whether the properties are required or optional, but since it gives an example with only first and last name, I believe the properties must be optional.

Quote

Name
This property specifies a structured representation of the name of the person, place or thing associated with the vCard object.
This property is identified by the property name N.
This property is defined to encapsulate the individual components of an object's name.
The property value consists of the components of the name specified as positional fields separated by the Field Delimiter character (ASCII decimal 59).
The property value is a concatenation of the Family Name (first field), Given Name (second field), Additional Names (third field),
Name Prefix (fourth field), and Name Suffix (fifth field) strings.
The following is an example of the Name property for a person:
N:Public;John;Quinlan;Mr.;Esq.
The following is an example of the Name property for a resource or place:
N:Veni, Vidi, Vici;The Restaurant.

Whereas this may seem like a detail, it has important consequences: If the created VCARD is compared to the original XML in a slow sync, the names will differ, and the entries will not map

[henrik]

It seems that in xmlformat-vcard.c handle_name_attribute we always get 5 components. So, we cannot distinguish between missing and empty components. This patch is a workaround that seems to work:

Index: src/xmlformat-vcard.c
===================================================================
--- src/xmlformat-vcard.c	(revision 5870)
+++ src/xmlformat-vcard.c	(working copy)
@@ -502,6 +502,35 @@
 	return xmlfield;
 }
 
+void handle_property_components(OSyncXMLField *xmlfield, VFormatAttribute *attr, OSyncError **error, ...) {
+	GList *values = vformat_attribute_get_values_decoded(attr);
+	GList *reverse = g_list_last(values);
+	while ( (((GString*)reverse->data)->len==0) && (reverse->prev!=NULL) ) {
+		reverse=reverse->prev;
+	}
+	GList *g = g_list_first(values);
+	GString *s=g->data;
+	while ( (g!=NULL) && (s!=NULL) ) {
+		g = g_list_next(g);
+		if (g) s=g->data;
+	}
+
+	GList *forward = g_list_first(values);
+		va_list args;
+		va_start(args, error);
+		while (1) {
+			char *a;
+			a = va_arg(args, char *);
+			if (a == NULL) break;
+			GString *sss = forward->data;
+	FIXME_xmlfield_set_key_value(xmlfield, osync_strdup(a), osync_strdup(sss->str));
+			if ( forward==reverse) break;
+			forward = forward->next;
+
+		};
+		va_end(args);
+}
+
 static OSyncXMLField *handle_name_attribute(OSyncXMLFormat *xmlformat, VFormatAttribute *attr, OSyncError **error)
 {
 	osync_trace(TRACE_INTERNAL, "Handling name attribute");
@@ -510,11 +539,7 @@
 		osync_trace(TRACE_ERROR, "%s: %s" , __func__, osync_error_print(error));
 		return NULL;
 	}
-	FIXME_xmlfield_set_key_value(xmlfield, "LastName", vformat_attribute_get_nth_value(attr, 0));
-	FIXME_xmlfield_set_key_value(xmlfield, "FirstName", vformat_attribute_get_nth_value(attr, 1));
-	FIXME_xmlfield_set_key_value(xmlfield, "Additional", vformat_attribute_get_nth_value(attr, 2));
-	FIXME_xmlfield_set_key_value(xmlfield, "Prefix", vformat_attribute_get_nth_value(attr, 3));
-	FIXME_xmlfield_set_key_value(xmlfield, "Suffix", vformat_attribute_get_nth_value(attr, 4));
+	handle_property_components(xmlfield, attr, error, "LastName", "FirstName", "Additional", "Prefix", "Suffix", NULL);
 	return xmlfield;
 }
}}

[cstender]

It looks like r5857 changed the behaviour to add also empty fields. IMHO we need some kind of logic in the compare function which ignores additional empty components. If you remove them in vformat you will very likely get a conflict if you compare your vcard (in xmlformat-contact and without those empty fields) with another one which wasn't created by vformat.

[henrik]

I believe there is a very important difference between a missing field and an empty field. I would think that a missing field means that the value is unknown, but an empty field means that the value is "nothing".

So in this example, we do not know or care about the middle name:

  <Name>
    <LastName>Doe</LastName>
    <FirstName>John</FirstName>
  </Name>

Whereas in this example we explicitly say that John has no middle name:

<Name>
 <LastName>Doe</LastName>
 <FirstName>John</FirstName>
 <Additional></Additional> 
</Name>

Or, in vformat, here we do not know or care about the middle name:

N:Doe;John

But here we explicitly say that John has no middle name:

N:Doe;John;;;

With this semantic, there should be no problem in comparing names (same goes for many other attributes) where one peer cares about middle name, but the other does not.

As far as I can see, the problem is still that vformat is inventing empty attributes from missing attributes.

[henrik]

The attached patch makes sure that xmlformat -> vformat conversion does not invent empty property components for N. Can I commit? Should I do the same for other similar cases (address etc)?

[dgollub]

Christopher could you comment the last patch?

[cstender]

Your patch will cause a segfault if you have empty attributes between two other ones.

Example:

BEGIN:VCARD VERSION:3.0 FN:Max Mustermann N:Mustermann;;;Dr. TEL:+49 1234 56788 END:VCARD

Just add the "if(!tmp) /* foo */ tmp=""; from the add_value(..) function. In the second for-loop the name pointer isn't used. Apart from that the patch looks okay.

[cstender]

Again with correct formatting.

BEGIN:VCARD
VERSION:3.0
FN:Max Mustermann
N:Mustermann;;;Dr.
TEL:+49 1234 56788
END:VCARD

[henrik]

Thanks for pointing out the stupid segfault mistake.

[henrik]

(In [5878]) Fixed xmlformat-vcard.c so handle_xml_name_attribute and handle_xml_address_attribute does not invent empty properties. Fixes #1173

[cstender]

Just add the "if(!tmp) /* foo */ tmp=""; from the add_value(..) function.

Your code produced:

BEGIN:VCARD
VERSION:3.0
FN:Max Mustermann
N:Mustermann;Max;Dr.
TEL:+49 1234 56788
END:VCARD

instead of

BEGIN:VCARD
VERSION:3.0
FN:Max Mustermann
N:Mustermann;Max;;Dr.
TEL:+49 1234 56788
END:VCARD

Fixed in r5879.

[henrik]

Thanks for the correction. Guess I must be a bit tired. If only I had listened the first time...