From e404a6559dd54f3d617019e3dcdd409d0ab0c938 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 27 Jun 2023 10:32:18 +0200 Subject: [PATCH] Call middle::after_nodes/ways/relations() in tests in right order The middle should always be called with these functions in the right order if the middle data is changed in any way, because there might be postprocessing done or buffers flushed etc. This adds some code that checks in the middle that it is in the correct state. This code is only compiled in in debug mode. --- src/middle-pgsql.cpp | 28 ++++++++++++++++++++++++++ src/middle-ram.cpp | 5 +++++ src/middle-ram.hpp | 9 ++++++++- src/middle.hpp | 39 ++++++++++++++++++++++++++++++++++--- tests/test-middle.cpp | 33 +++++++++++++++++++++++++++++++ tests/test-parse-osmium.cpp | 26 +++++++++++++++++++++++-- 6 files changed, 134 insertions(+), 6 deletions(-) diff --git a/src/middle-pgsql.cpp b/src/middle-pgsql.cpp index 290639c90..09f297297 100644 --- a/src/middle-pgsql.cpp +++ b/src/middle-pgsql.cpp @@ -347,6 +347,8 @@ std::size_t middle_query_pgsql_t::get_way_node_locations_db( void middle_pgsql_t::node(osmium::Node const &node) { + assert(m_middle_state == middle_state::node); + if (node.deleted()) { node_delete(node.id()); } else { @@ -358,6 +360,8 @@ void middle_pgsql_t::node(osmium::Node const &node) } void middle_pgsql_t::way(osmium::Way const &way) { + assert(m_middle_state == middle_state::way); + if (way.deleted()) { way_delete(way.id()); } else { @@ -369,6 +373,8 @@ void middle_pgsql_t::way(osmium::Way const &way) { } void middle_pgsql_t::relation(osmium::Relation const &relation) { + assert(m_middle_state == middle_state::relation); + if (relation.deleted()) { relation_delete(relation.id()); } else { @@ -664,6 +670,11 @@ void middle_pgsql_t::relation_delete(osmid_t osm_id) void middle_pgsql_t::after_nodes() { + assert(m_middle_state == middle_state::node); +#ifndef NDEBUG + m_middle_state = middle_state::way; +#endif + m_db_copy.sync(); if (!m_options->append && m_options->flat_node_file.empty()) { auto const &table = m_tables.nodes(); @@ -673,6 +684,11 @@ void middle_pgsql_t::after_nodes() void middle_pgsql_t::after_ways() { + assert(m_middle_state == middle_state::way); +#ifndef NDEBUG + m_middle_state = middle_state::relation; +#endif + m_db_copy.sync(); if (!m_options->append) { auto const &table = m_tables.ways(); @@ -682,6 +698,11 @@ void middle_pgsql_t::after_ways() void middle_pgsql_t::after_relations() { + assert(m_middle_state == middle_state::relation); +#ifndef NDEBUG + m_middle_state = middle_state::done; +#endif + m_db_copy.sync(); if (!m_options->append) { auto const &table = m_tables.relations(); @@ -706,6 +727,11 @@ middle_query_pgsql_t::middle_query_pgsql_t( void middle_pgsql_t::start() { + assert(m_middle_state == middle_state::constructed); +#ifndef NDEBUG + m_middle_state = middle_state::node; +#endif + if (m_options->append) { // Disable JIT and parallel workers as they are known to cause // problems when accessing the intarrays. @@ -732,6 +758,8 @@ void middle_pgsql_t::start() void middle_pgsql_t::stop() { + assert(m_middle_state == middle_state::done); + m_cache.reset(); if (!m_options->flat_node_file.empty()) { m_persistent_cache.reset(); diff --git a/src/middle-ram.cpp b/src/middle-ram.cpp index 3027a62ec..f34161101 100644 --- a/src/middle-ram.cpp +++ b/src/middle-ram.cpp @@ -61,6 +61,8 @@ void middle_ram_t::set_requirements(output_requirements const &requirements) void middle_ram_t::stop() { + assert(m_middle_state == middle_state::done); + auto const mbyte = 1024 * 1024; log_debug("Middle 'ram': Node locations: size={} bytes={}M", @@ -148,6 +150,7 @@ static void add_delta_encoded_way_node_list(std::string *data, void middle_ram_t::node(osmium::Node const &node) { + assert(m_middle_state == middle_state::node); assert(node.visible()); if (m_store_options.locations) { @@ -162,6 +165,7 @@ void middle_ram_t::node(osmium::Node const &node) void middle_ram_t::way(osmium::Way const &way) { + assert(m_middle_state == middle_state::way); assert(way.visible()); if (m_store_options.way_nodes) { @@ -177,6 +181,7 @@ void middle_ram_t::way(osmium::Way const &way) void middle_ram_t::relation(osmium::Relation const &relation) { + assert(m_middle_state == middle_state::relation); assert(relation.visible()); if (m_store_options.relations) { diff --git a/src/middle-ram.hpp b/src/middle-ram.hpp index e48a15b6c..e466d05c3 100644 --- a/src/middle-ram.hpp +++ b/src/middle-ram.hpp @@ -47,7 +47,14 @@ class middle_ram_t : public middle_t, public middle_query_t ~middle_ram_t() noexcept override = default; - void start() override {} + void start() override + { + assert(m_middle_state == middle_state::constructed); +#ifndef NDEBUG + m_middle_state = middle_state::node; +#endif + } + void stop() override; void node(osmium::Node const &node) override; diff --git a/src/middle.hpp b/src/middle.hpp index 85659b964..97ea5ea2e 100644 --- a/src/middle.hpp +++ b/src/middle.hpp @@ -122,13 +122,31 @@ class middle_t virtual void relation(osmium::Relation const &relation) = 0; /// Called after all nodes from the input file(s) have been processed. - virtual void after_nodes() {} + virtual void after_nodes() + { + assert(m_middle_state == middle_state::node); +#ifndef NDEBUG + m_middle_state = middle_state::way; +#endif + } /// Called after all ways from the input file(s) have been processed. - virtual void after_ways() {} + virtual void after_ways() + { + assert(m_middle_state == middle_state::way); +#ifndef NDEBUG + m_middle_state = middle_state::relation; +#endif + } /// Called after all relations from the input file(s) have been processed. - virtual void after_relations() {} + virtual void after_relations() + { + assert(m_middle_state == middle_state::relation); +#ifndef NDEBUG + m_middle_state = middle_state::done; +#endif + } virtual idlist_t get_ways_by_node(osmid_t) { return {}; } virtual idlist_t get_rels_by_node(osmid_t) { return {}; } @@ -145,6 +163,21 @@ class middle_t return *m_thread_pool; } +#ifndef NDEBUG + enum class middle_state + { + constructed, + started, + node, + way, + relation, + done + }; + + // NOLINTNEXTLINE(cppcoreguidelines-non-private-member-variables-in-classes, misc-non-private-member-variables-in-classes) + middle_state m_middle_state = middle_state::constructed; +#endif + private: std::shared_ptr m_thread_pool; }; // class middle_t diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index c8904195b..9bcda1cbf 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -130,6 +130,8 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, // set the node mid->node(node); mid->after_nodes(); + mid->after_ways(); + mid->after_relations(); // getting it back works only via a waylist auto &nodes = buffer.add_way("w3 Nn1234").nodes(); @@ -170,6 +172,7 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, // set the way mid->way(buffer.add_way(way_id, nds)); mid->after_ways(); + mid->after_relations(); // get it back osmium::memory::Buffer outbuf{4096, @@ -372,6 +375,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update node", "", mid->node(node10); mid->node(node11); mid->after_nodes(); + mid->after_ways(); mid->after_relations(); check_node(mid, node10); @@ -402,6 +406,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update node", "", mid->node(node10d); mid->node(node42d); mid->after_nodes(); + mid->after_ways(); mid->after_relations(); REQUIRE(no_node(mid, 5)); @@ -432,6 +437,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update node", "", mid->node(node12d); mid->node(node12); mid->after_nodes(); + mid->after_ways(); mid->after_relations(); check_node(mid, node10a); @@ -457,6 +463,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update node", "", mid->node(node12); mid->after_nodes(); + mid->after_ways(); mid->after_relations(); REQUIRE(no_node(mid, 5)); @@ -585,6 +592,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update way", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way20); mid->way(way21); mid->after_ways(); @@ -614,6 +622,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update way", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way5d); mid->way(way20d); mid->way(way42d); @@ -643,6 +652,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update way", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way20d); mid->way(way20a); mid->way(way22d); @@ -675,6 +685,7 @@ TEMPLATE_TEST_CASE("middle: add, delete and update way", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way22); mid->after_ways(); mid->after_relations(); @@ -728,6 +739,7 @@ TEMPLATE_TEST_CASE("middle: add way with attributes", "", options_slim_default, auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way20); mid->after_ways(); mid->after_relations(); @@ -831,6 +843,8 @@ TEMPLATE_TEST_CASE("middle: add, delete and update relation", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); + mid->after_ways(); mid->relation(relation30); mid->relation(relation31); mid->after_relations(); @@ -859,6 +873,8 @@ TEMPLATE_TEST_CASE("middle: add, delete and update relation", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); + mid->after_ways(); mid->relation(relation5d); mid->relation(relation30d); mid->relation(relation42d); @@ -887,6 +903,8 @@ TEMPLATE_TEST_CASE("middle: add, delete and update relation", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); + mid->after_ways(); mid->relation(relation30d); mid->relation(relation30a); mid->relation(relation32d); @@ -918,6 +936,8 @@ TEMPLATE_TEST_CASE("middle: add, delete and update relation", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); + mid->after_ways(); mid->relation(relation32); mid->after_relations(); @@ -967,6 +987,8 @@ TEMPLATE_TEST_CASE("middle: add relation with attributes", "", auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); + mid->after_ways(); mid->relation(relation30); mid->after_relations(); @@ -1052,6 +1074,8 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, mid->node(node10a); dependency_manager.node_changed(10); mid->after_nodes(); + mid->after_ways(); + mid->after_relations(); REQUIRE(dependency_manager.has_pending()); idlist_t const way_ids = dependency_manager.get_pending_way_ids(); @@ -1067,9 +1091,11 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way22); mid->after_ways(); mid->after_relations(); + check_way(mid, way22); } { @@ -1081,6 +1107,8 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, mid->node(node10a); dependency_manager.node_changed(10); mid->after_nodes(); + mid->after_ways(); + mid->after_relations(); REQUIRE(dependency_manager.has_pending()); idlist_t const way_ids = dependency_manager.get_pending_way_ids(); @@ -1099,6 +1127,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, auto mid = std::make_shared(thread_pool, &options); mid->start(); + mid->after_nodes(); mid->way(way20d); mid->way(way20a); mid->after_ways(); @@ -1117,6 +1146,8 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, mid->node(node10a); dependency_manager.node_changed(10); mid->after_nodes(); + mid->after_ways(); + mid->after_relations(); REQUIRE_FALSE(dependency_manager.has_pending()); } @@ -1179,6 +1210,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in relation", "", options_slim_default, mid->node(node10a); dependency_manager.node_changed(10); mid->after_nodes(); + mid->after_ways(); mid->after_relations(); REQUIRE(dependency_manager.has_pending()); @@ -1198,6 +1230,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in relation", "", options_slim_default, mid->node(node11a); dependency_manager.node_changed(11); mid->after_nodes(); + mid->after_ways(); mid->after_relations(); REQUIRE(dependency_manager.has_pending()); diff --git a/tests/test-parse-osmium.cpp b/tests/test-parse-osmium.cpp index 38309e825..0c77c4ee2 100644 --- a/tests/test-parse-osmium.cpp +++ b/tests/test-parse-osmium.cpp @@ -28,11 +28,21 @@ struct counting_middle_t : public middle_t : middle_t(nullptr), m_append(append) {} - void start() override {} - void stop() override {} + void start() override + { + assert(m_middle_state == middle_state::constructed); +#ifndef NDEBUG + m_middle_state = middle_state::node; +#endif + } + + void stop() override { assert(m_middle_state == middle_state::done); } + void cleanup() {} void node(osmium::Node const &node) override { + assert(m_middle_state == middle_state::node); + if (m_append) { ++node_count.deleted; if (!node.deleted()) { @@ -44,6 +54,8 @@ struct counting_middle_t : public middle_t } void way(osmium::Way const &way) override { + assert(m_middle_state == middle_state::way); + if (m_append) { ++way_count.deleted; if (!way.deleted()) { @@ -55,6 +67,8 @@ struct counting_middle_t : public middle_t } void relation(osmium::Relation const &relation) override { + assert(m_middle_state == middle_state::relation); + if (m_append) { ++relation_count.deleted; if (!relation.deleted()) { @@ -156,6 +170,8 @@ TEST_CASE("parse xml file") options_t const options = testing::opt_t().slim(); auto const middle = std::make_shared(false); + middle->start(); + auto const output = std::make_shared(options); auto counts = std::make_shared(); @@ -195,6 +211,8 @@ TEST_CASE("parse diff file") options_t const options = testing::opt_t().slim().append(); auto const middle = std::make_shared(true); + middle->start(); + auto const output = std::make_shared(options); auto counts = std::make_shared(); @@ -232,6 +250,8 @@ TEST_CASE("parse xml file with extra args") options.extra_attributes = true; auto const middle = std::make_shared(false); + middle->start(); + auto const output = std::make_shared(options); auto counts = std::make_shared(); @@ -271,6 +291,8 @@ TEST_CASE("invalid location") options_t const options = testing::opt_t(); auto const middle = std::make_shared(false); + middle->start(); + auto const output = std::make_shared(options); auto counts = std::make_shared();