Added/deleted "uninteresting" tags are "reverted" by downloading area again (twice)

[mdk]

What steps will reproduce the problem?

1. open attached note.osm
2. select node with note-tag and delete this tag
3. download an area which includes the above node
4. download a second area which also includes the above node (can be the same as in 3)

What is the expected result?

The node stays unchanged (note tag stays deleted).

What happens instead?

After the second download, the node get the deleted note tag back!

Please provide any additional information below. Attach a screenshot if possible.

Here are some hopefully helpful observations:

• If you move the node before downloading, the tag is not "restored".
• If you delete tags like natural=tree or aminity=toilett from a single node, the tag is not "restored".
• If you delete a highway=turning_cycle tag from (the end of) a way, the tag is not "restored".

So far I observe this only on note=* tags in the middle of waterway=stream ways. But I'm not shure if there are other scenarios where this happens.

It's independebt of the size of the downloaded areas used for step 3 and 4. Only the node must be inside the bounding box in both downloads.

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2016-03-04 21:52:59 +0100 (Fri, 04 Mar 2016)
Build-Date:2016-03-04 22:51:10
Revision:9924
Relative:URL: ^/trunk

Identification: JOSM/1.5 (9924 en) Linux Ubuntu 15.10
Memory Usage: 599 MB / 876 MB (189 MB allocated, but free)
Java version: 1.8.0_66-internal-b17, Oracle Corporation, OpenJDK Server VM
VM arguments: [-Djosm.restart=true, -Djosm.home=/home/michael/.josm-latest, -Djava.net.useSystemProxies=true]

Plugins:
- ColumbusCSV (31772)
- FastDraw (31895)
- HouseNumberTaggingTool (31772)
- Mapillary (32040)
- OpeningHoursEditor (31772)
- RoadSigns (31895)
- SimplifyArea (31895)
- apache-commons (31895)
- apache-http (31895)
- buildings_tools (31895)
- contourmerge (1014)
- imagery-xml-bounds (31772)
- imagery_offset_db (32046)
- pbf (31772)
- poly (31772)
- public_transport (31895)
- reltoolbox (31895)
- reverter (32005)
- terracer (31895)
- turnrestrictions (31895)
- utilsplugin2 (32097)

Last errors/warnings:
- W: java.net.SocketTimeoutException: connect timed out
- E: java.net.SocketTimeoutException: connect timed out
- W: java.net.SocketTimeoutException: connect timed out
- E: java.net.SocketTimeoutException: connect timed out
- E: org.openstreetmap.josm.io.OsmTransferException: Could not connect to the OSM server. Please check your internet connection.. Cause: java.net.SocketTimeoutException: connect timed out

[Don-vip]

I can reproduce with description. Looks like a problem concerning all "uninteresting" keys (see OsmPrimitive.getUninterestingKeys):
"source", "source_ref", "source:", "comment", "converted_by", "watch", "watch:", "description", "attribution", "note", "fixme", "FIXME"

[Don-vip]

Looks like DataSetMerger fails to merge properly primitives having only uninteresting tags. Probably a bug in mergeById or mergePrimitive function (the two methods calling hasEqualSemanticAttributes method which itself calls hasSameInterestingTags)

[Don-vip]

This test reproduces the bug:

    /**
     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/12599">Bug #12599</a>.
     */
    @Test
    public void testTicket12599() {
        // Server node: no modifications
        Node n1 = new Node(1, 1);
        n1.setCoor(LatLon.ZERO);
        assertFalse(n1.isModified());
        their.addPrimitive(n1);

        // Local node: one modification: addition of an uninteresting tag
        Node n1b = new Node(n1);
        n1b.setModified(true);
        n1b.put("note", "something");
        assertTrue(n1b.isModified());
        assertEquals(0, n1b.getInterestingTags().size());
        my.addPrimitive(n1b);

        // Merge
        DataSetMerger visitor = new DataSetMerger(my, their);
        visitor.merge();

        // Check that modification is still here
        Node n = (Node) my.getPrimitiveById(1, OsmPrimitiveType.NODE);
        assertNotNull(n);
        assertEquals("something", n.get("note"));
        assertTrue(n.isModified());

        // Merge again
        visitor = new DataSetMerger(my, their);
        visitor.merge();

        // Check that modification is still here
        n = (Node) my.getPrimitiveById(1, OsmPrimitiveType.NODE);
        assertNotNull(n);
        assertEquals("something", n.get("note"));
        assertTrue(n.isModified());
    }

[Don-vip]

Problem comes from:

        } else if (target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) {
            // target is same as source but target is modified
            // => keep target and reset modified flag if target and source are semantically equal
            if (target.hasEqualSemanticAttributes(source)) {
                target.setModified(false);
            }
        }

This comes from r2634 in order to fix #4088. The fix was correct because at that time, hasEqualSemanticAttributes called hasSameTags. Now it calls hasSameInterestingTags since r6629, I think the bug comes from here.

[simon04]

What about…?

src/org/openstreetmap/josm/data/osm/DataSetMerger.java

@@ -94 +94 @@
                 if (!target.isNew() || target.isDeleted()) {
                     continue;
                 }
-                if (target.hasEqualSemanticAttributes(source)) {
+                if (target.hasEqualSemanticAttributes(source) && target.getKeys().equals(source.getKeys())) {
                     mergedMap.put(source.getPrimitiveId(), target.getPrimitiveId());
                     // copy the technical attributes from other version
                     target.setVisible(source.isVisible());
@@ -356 +356 @@
         } else if (target.isModified() && !source.isModified() && target.getVersion() == source.getVersion()) {
             // target is same as source but target is modified
             // => keep target and reset modified flag if target and source are semantically equal
-            if (target.hasEqualSemanticAttributes(source)) {
+            if (target.hasEqualSemanticAttributes(source) && target.getKeys().equals(source.getKeys())) {
                 target.setModified(false);
             }
         } else if (source.isDeleted() != target.isDeleted()) {

[simon04]

In 9961/josm:

fix #12599 - Added/deleted "uninteresting" tags are "reverted" by downloading area again (unit test by Don-vip)

[Don-vip]

Thanks! :)

[Don-vip]

In 9979/josm:

see #12599, fix #12616 - Random repositioning of nodes (incomplete fix from r9961)

[simon04]

In the last minute I moved from the suggested change from comment:8 for a version w/ better performance w/o the Map creation hasSameInterestingTags, but overlooked that subclasses override the method. Sorry.

[Don-vip]

Milestone renamed

[GerdP]

The changes in r6629 caused a lot more similar problems. See #20313. Also the reverter plugin expects that method hasEqualSemanticAttributes() compares all tags (and the javadoc says it does)
I don't understand why it was needed to change this logic in hasEqualSemanticAttributes() for the implementation of the :sametags. To me this looks like an unintended change.