Refactored the test for invalid locations and added a test

Author: joto

src/osmdata.cpp

@@ -36,28 +36,26 @@ osmdata_t::osmdata_t(std::unique_ptr<dependency_manager_t> dependency_manager,
 
 void osmdata_t::node(osmium::Node const &node)
 {
-    if (!m_bbox.valid() || node.deleted() || m_bbox.contains(node.location())) {
-        m_mid->node(node);
-    }
-
-    if (node.deleted()) {
-        node_delete(node.id());
-    } else {
-        // if the node is not valid, then node.location.lat/lon() can throw.
-        // we probably ought to treat invalid locations as if they were
-        // deleted and ignore them.
+    if (node.visible()) {
         if (!node.location().valid()) {
-            log_warn("Ignored invalid location on node {} (version {})",
+            log_warn("Ignored node {} (version {}) with invalid location.",
                      node.id(), node.version());
             return;
         }
+        if (m_bbox.valid() && !m_bbox.contains(node.location())) {
+            return;
+        }
+    }
 
-        if (!m_bbox.valid() || m_bbox.contains(node.location())) {
-            if (m_append) {
-                node_modify(node);
-            } else {
-                node_add(node);
-            }
+    m_mid->node(node);
+
+    if (node.deleted()) {
+        node_delete(node.id());
+    } else {
+        if (m_append) {
+            node_modify(node);
+        } else {
+            node_add(node);
         }
     }
 }

tests/data/test_invalid_location.osm

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<osm version="0.6">
+ <node id="1" visible="true" version="1" changeset="1" timestamp="2018-10-31T10:20:19Z" user="a" uid="1" lon="3.141" lat="200.0">
+   <tag k="amenity" v="restaurant" />
+ </node>
+</osm>

tests/test-parse-osmium.cpp

@@ -257,3 +257,24 @@ TEST_CASE("parse xml file with extra args")
     REQUIRE(counts->nodes_changed == 0);
     REQUIRE(counts->ways_changed == 0);
 }
+
+TEST_CASE("invalid location")
+{
+    options_t options = testing::opt_t();
+
+    auto const middle = std::make_shared<counting_middle_t>(false);
+    std::shared_ptr<output_t> output{new counting_output_t{options}};
+
+    auto counts = std::make_shared<counts_t>();
+    auto dependency_manager = std::unique_ptr<dependency_manager_t>(
+        new counting_dependency_manager_t{counts});
+
+    testing::parse_file(options, std::move(dependency_manager), middle,
+                        {output}, "test_invalid_location.osm", false);
+
+    auto const *out_test = static_cast<counting_output_t *>(output.get());
+    REQUIRE(out_test->node.added == 0);
+    REQUIRE(out_test->way.added == 0);
+    REQUIRE(out_test->relation.added == 0);
+}
+