Error in _osync_obj_engine_mapping_find

[henrik]

When slow-syncing two identical members, _osync_obj_engine_mapping_find does not work correctly.

For the first member, osync_obj_engine_map_changes correctly creates new mappings.

However, for the second member, _osync_obj_engine_mapping_find seems to compare changes from second member to changes from second member, where it should compare to changes from first member.

Unfortunately r5869 did not solve the problem.

I believe the attached patch solves the problem.

Any comments? Can I commit the patch?

Index: opensync_obj_engine.c
===================================================================
--- opensync_obj_engine.c	(revision 5870)
+++ opensync_obj_engine.c	(working copy)
@@ -311,14 +311,14 @@
 			osync_trace(TRACE_INTERNAL, "Member1-caps: %p Member2-caps: %p", caps1, caps2);
 
 			if (caps1) {
-				clone_change1 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps1, change2, error);
+				clone_change1 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps1, change1, error);
 				if (!clone_change1)
 					goto error;
 
 			}
 
 			if (caps2) {
-				clone_change2 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps2, change1, error);
+				clone_change2 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps2, change2, error);
 				if (!clone_change2)
 					goto error;

[dgollub]

No actually this "mixup" of change1 with change2 is intended.

The _osync_obj_engine_clone_and_demerge_change() creates a copy/clone of the give change. And then demerges the unsupported fields with the given capabilities. It makes only sense to run the demerge with the capabilities of "other" member. Why?

Why demerge with the other capabilities before compare?

Example: Group with 2 Members

• Member 1 - capabilities: {{{

E-Mail Name Telephone }}}

• Member 2 - capabilities: {{{

Name Telephone }}}

This is now the first sync and both members commit one single entry/change which is "logical" the same contact ... but unfortuantely not 1:1 the same, due to the missing capability "E-Mail".

• Member 1 - (UID:) Change1 {{{

EMAIL:gollub@b1 N:Daniel TEL:555 }}}

• Member 2 - (UID:) Change2 {{{

N:Daniel Tel:555 }}}

Without demerging the unsupported capabilities (e.g. EMAIL) the compare would result in SIMILAR and not SAME. Since we have the capabilities information we can modify the entries to have more "successful" comparison results, which result in SAME. So what we do here is we demerge all fields/capabilities which are not supported by the opponent member:

• Demerge all fields of change1 (from Member1) which aren't part of Member2's capabilities
  • e.g. demerge/remove EMAIL
• Demerge all fields of change2 (from Member2) which aren't part of Member1's capabilities
  • e.g. ... in this example nothing todo no unsupported fields for member1 caps

Note: not the original change get modifiy, a copy/clone.

This modified clone_changes get compared:

clone_change1: {{{ N:Daniel TEL:555 }}}

clone_change2: {{{ N:Daniel TEL:555 }}}

This clone_changes get used for the osync_change_compare call which should result in SAME changes, and a successful mapping, without conflict.

This only works if we really used caps2 for change1 and caps2 for change1 when demerging ..

Your patch would remove this essential detail .. in fact this is a very important detail which is lacking a very detailed description in the source code as comment ...

I hope i can use this ticket as reference and add later some brief description.

Could you provide trace files of the actual error - or a step by step testcase instruction to reproduce the problem?

[henrik]

Thanks for the explanation. But I still think the code is wrong (and my patch wrong as well).

Original code:

clone_change1 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps1, change2, error);

This clones *second* member changes with *first* member caps (fine), but calls it a clone of the *first* member (which it is not). Maybe this is the solution:

clone_change1 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps2, change1, error);

[henrik]

The following patch seems to work:

Index: opensync/engine/opensync_obj_engine.c
===================================================================
--- opensync/engine/opensync_obj_engine.c	(revision 5870)
+++ opensync/engine/opensync_obj_engine.c	(working copy)
@@ -310,15 +310,15 @@
 
 			osync_trace(TRACE_INTERNAL, "Member1-caps: %p Member2-caps: %p", caps1, caps2);
 
-			if (caps1) {
-				clone_change1 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps1, change2, error);
+			if (caps2) {
+				clone_change1 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps2, change1, error);
 				if (!clone_change1)
 					goto error;
 
 			}
 
-			if (caps2) {
-				clone_change2 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps2, change1, error);
+			if (caps1) {
+				clone_change2 = _osync_obj_engine_clone_and_demerge_change(sinkengine->engine, caps1, change2, error);
 				if (!clone_change2)
 					goto error;
 
@@ -333,10 +333,10 @@
 
 			tmp_result = osync_change_compare(change1, change2, error);
 
-			if (caps1)
+			if (caps2)
 				osync_change_unref(clone_change1);
 
-			if (caps2)
+			if (caps1)
 				osync_change_unref(clone_change2);
 
 			if(tmp_result == OSYNC_CONV_DATA_SAME) {

At least it now compares one side to the other!

(I am not able to test in depth, due to bug 1173)

[dgollub]

Ok, looks good. Please commit.

[henrik]

(In [5874]) Correct cloning of mapping change in _osync_obj_engine_mapping_find. Fixes ticket 1170