omniscale / imposm3

Imposm imports OpenStreetMap data into PostGIS
http://imposm.org/docs/imposm3/latest/
Apache License 2.0
723 stars 158 forks source link

Diff is not properly applied when using SingleIdSpace #173

Closed amatissart closed 2 years ago

amatissart commented 6 years ago

Context

This is a bug I encountered when applying updates using a mapping with use_single_id_space: true

Expected Behavior

imposm diff should reinsert all corresponding ways when a node has been moved.

Here is a patch that adds a failing test ```diff diff --git a/test/single_table.osc b/test/single_table.osc index c349766..faf219a 100644 --- a/test/single_table.osc +++ b/test/single_table.osc @@ -23,4 +23,9 @@ + + + + + diff --git a/test/single_table_test.go b/test/single_table_test.go index 19cf852..627b064 100644 --- a/test/single_table_test.go +++ b/test/single_table_test.go @@ -209,6 +209,10 @@ func TestSingleTable(t *testing.T) { } }) + t.Run("NodeHasMoved", func(t *testing.T) { + ts.assertGeomLength(t, checkElem{"osm_all", -20201, "*", nil}, 222639) + }) + t.Run("Cleanup", func(t *testing.T) { ts.dropSchemas() if err := os.RemoveAll(ts.dir); err != nil { ```

Actual Behavior

The test fails, as if the ways that contain the node that have been updated were not in cache. As far as I can understand, this is due to how the writer handle element ids. On the initial import, the way Id is updated before being written to diffcache: https://github.com/omniscale/imposm3/blob/db15f93da1b3bd600a1216a6386ccfb4bd7f02af/writer/ways.go#L91 Whereas the update process expect "normal" positive ids in cache.

Possible Fix

Here is a quick fix for my test case:

diff --git a/writer/ways.go b/writer/ways.go
index 4cc7749..ca16681 100644
--- a/writer/ways.go
+++ b/writer/ways.go
@@ -88,6 +88,7 @@ func (ww *WayWriter) loop() {
                        return true
                }

+               originalId := w.Id
                w.Id = ww.wayId(w.Id)

                var err error
@@ -123,6 +124,8 @@ func (ww *WayWriter) loop() {
                if (inserted || insertedPolygon) && ww.expireor != nil {
                        expire.ExpireProjectedNodes(ww.expireor, w.Nodes, ww.srid, insertedPolygon)
                }
+
+               w.Id = originalId
                if (inserted || insertedPolygon) && ww.diffCache != nil {
                        ww.diffCache.Coords.AddFromWay(w)
                }

But I am not very familiar with the implementation. Maybe this is expected in diffcache, and that's the update process that should be patched ?

Note that a similar issue may exist with relations, since the RelationWriter does the same kind of operations with element ids before writing to diffcache.

amatissart commented 6 years ago

Did anyone get a chance to look into this?
Or would you have some tips to write proper test cases? Using assertGeomLength is a bit cumbersome.