GNOME Bugzilla – Bug 740937
Wrong information for non-node objects
Last modified: 2014-12-03 18:39:21 UTC
When searching for an object that is not a single node in OSM (i.e. a "way", such as a building), the query passed on to overpass retrieves the wrong information. The query is always using "node(osm_id)" (where osm_id is the ID in OSM). But the OSM IDs are only unique per object type (node, way, relation). For example, searching for "Uppsala slott" will generate the following overpass query: http://overpass-api.de/api/interpreter?data=[timeout:180][out:json][maxsize:536870912];node(24433739);out%20body%20qt%201000000; Adjusting this manually (and pasting into a browser location field): http://overpass-api.de/api/interpreter?data=[timeout:180][out:json][maxsize:536870912];way(24433739);out%20body%20qt%201000000; will generate the correct query response (with f.ex. the wikipedia reference). I suspect this would also affect the place store (as far as I can see, it uses the same OSM ID to store json objects, so there is the risk of getting duplicates).
Dang! This is indeed an issue :( We have assumed that osm_id are unique across all objects. So we indeed use them to index in place store. This is a big issue since I don't think we have a clear way to know if a place we get from geocodeglib is a way, node or relation. We would need to patch geocodeglib to expose the osm_type tag. Thanks for exposing our incompetence Marcus! So we can fix this mess up.
Created attachment 291873 [details] [review] User osm-type+osm-id as index for places. The osm id is only unique within a type of place such as node, way or relation. We need the osm-type to make the index unique.
Created attachment 291874 [details] [review] overpass: Use the osm-type to look up data
So could you try with these two patches applied? And also the one against geocode-glib in the "depends on" field? You might have to clear the placestore. (~/.local/share/maps-places.json is the location of mine).
Created attachment 291875 [details] [review] User osm-type+osm-id as index for places. The osm id is only unique within a type of place such as node, way or relation. We need the osm-type to make the index unique.
Created attachment 291876 [details] [review] overpass: Use the osm-type to look up data
Created attachment 291888 [details] [review] User osm-type+osm-id as index for places. The osm id is only unique within a type of place such as node, way or relation. We need the osm-type to make the index unique.
Created attachment 291889 [details] [review] overpass: Use the osm-type to look up data
Tested the patch (including the one against geocode-glib) and it works great!
Review of attachment 291888 [details] [review]: ::: src/placeStore.js @@ +94,3 @@ + + return type + '-' + place.osm_id; + }, If possible I'd like this to be a method or property on Place instead. @@ +109,3 @@ this._removeIf((function(model, iter) { let p = model.get_value(iter, Columns.PLACE); + return this._getKey(p) === this._getKey(place); Then this would be p.getKey() === place.getKey() or p.key === place.key (depending on whether we make this a property or method).
Review of attachment 291889 [details] [review]: ACK
Review of attachment 291888 [details] [review]: Thanks for review! ::: src/placeStore.js @@ +94,3 @@ + + return type + '-' + place.osm_id; + }, Are you sure? The key is pretty specific to the placeStore, it does not feel like a property of the place. Maybe if we call it place.uniqueID?
Created attachment 291959 [details] [review] mapBubble: Always convert to GeocodePlace to Place
Created attachment 291960 [details] [review] Use osm-type+osm-id as index for places. The osm id is only unique within a type of place such as node, way or relation. We need the osm-type to make the index unique.
Created attachment 291961 [details] [review] Use osm-type+osm-id as index for places. The osm id is only unique within a type of place such as node, way or relation. We need the osm-type to make the index unique.
Review of attachment 291888 [details] [review]: ::: src/placeStore.js @@ +94,3 @@ + + return type + '-' + place.osm_id; + }, Not entirely sure actually (after having thought about it), but leaning towards yes. place.uniqueId is probably better in that case as you say.
Review of attachment 291961 [details] [review]: ACK
Review of attachment 291959 [details] [review]: Nit: Always convert GeocodePlace to Place (ACK after that)
Created attachment 292047 [details] [review] Use osm-type+osm-id as index for places. The osm id is only unique within a type of place such as node, way or relation. We need the osm-type to make the index unique.
Created attachment 292048 [details] [review] overpass: Use the osm-type to look up data
Review of attachment 292047 [details] [review]: ACK
Review of attachment 292048 [details] [review]: ACK
Attachment 291959 [details] pushed as ad416a2 - mapBubble: Always convert to GeocodePlace to Place Attachment 292047 [details] pushed as fed9e06 - Use osm-type+osm-id as index for places. Attachment 292048 [details] pushed as d86b27c - overpass: Use the osm-type to look up data