Skip to content

Commit ccb6b18

Browse files
lonvialonvia
authored
Merge pull request #1770 from joto/fix-get-members-in-order
Bugfix: Return relation members in order from pgsql middle
2 parents 401e0b2 + 8dee2da commit ccb6b18

File tree

5 files changed

+102
-44
lines changed

5 files changed

+102
-44
lines changed

‎src/middle-pgsql.cpp‎

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -460,42 +460,39 @@ middle_query_pgsql_t::rel_members_get(osmium::Relation const &rel,
460460
assert(buffer);
461461
assert((types & osmium::osm_entity_bits::relation) == 0);
462462

463-
util::string_id_list_t way_ids;
464-
idlist_t node_ids;
465-
466-
for (auto const &member : rel.members()) {
467-
if (member.type() == osmium::item_type::node &&
468-
(types & osmium::osm_entity_bits::node)) {
469-
node_ids.push_back(member.ref());
470-
} else if (member.type() == osmium::item_type::way &&
471-
(types & osmium::osm_entity_bits::way)) {
472-
way_ids.add(member.ref());
463+
pg_result_t res;
464+
idlist_t wayidspg;
465+
if (types & osmium::osm_entity_bits::way) {
466+
// collect ids from all way members into a list..
467+
util::string_id_list_t way_ids;
468+
for (auto const &member : rel.members()) {
469+
if (member.type() == osmium::item_type::way) {
470+
way_ids.add(member.ref());
471+
}
473472
}
474-
}
475473

476-
std::size_t members_found = node_ids.size();
477-
if (!node_ids.empty()) {
478-
osmium::builder::WayNodeListBuilder builder{*buffer};
479-
for (auto const id : node_ids) {
480-
builder.add_node_ref(id);
474+
// ...and get those ways from database
475+
if (!way_ids.empty()) {
476+
res = m_sql_conn.exec_prepared("get_way_list", way_ids.get());
477+
wayidspg = get_ids_from_result(res);
481478
}
482479
}
483480

484-
if (!way_ids.empty()) {
485-
auto const res =
486-
m_sql_conn.exec_prepared("get_way_list", way_ids.get());
487-
idlist_t const wayidspg = get_ids_from_result(res);
488-
489-
// Match the list of ways coming from postgres in a different order
490-
// back to the list of ways given by the caller
491-
for (auto const &m : rel.members()) {
492-
if (m.type() != osmium::item_type::way) {
493-
continue;
494-
}
481+
std::size_t members_found = 0;
482+
for (auto const &member : rel.members()) {
483+
if (member.type() == osmium::item_type::node &&
484+
(types & osmium::osm_entity_bits::node)) {
485+
osmium::builder::NodeBuilder builder{*buffer};
486+
builder.set_id(member.ref());
487+
++members_found;
488+
} else if (member.type() == osmium::item_type::way &&
489+
(types & osmium::osm_entity_bits::way) && res) {
490+
// Match the list of ways coming from postgres in a different order
491+
// back to the list of ways given by the caller
495492
for (int j = 0; j < res.num_tuples(); ++j) {
496-
if (m.ref() == wayidspg[static_cast<std::size_t>(j)]) {
493+
if (member.ref() == wayidspg[static_cast<std::size_t>(j)]) {
497494
osmium::builder::WayBuilder builder{*buffer};
498-
builder.set_id(m.ref());
495+
builder.set_id(member.ref());
499496

500497
pgsql_parse_nodes(res.get_value(j, 1), buffer, &builder);
501498
pgsql_parse_tags(res.get_value(j, 2), buffer, &builder);

‎src/output-flex.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1298,10 +1298,6 @@ bool output_flex_t::relation_cache_t::add_members(middle_query_t const &middle)
12981298
}
12991299
}
13001300

1301-
for (auto &nodes : m_members_buffer.select<osmium::WayNodeList>()) {
1302-
middle.nodes_get_list(&nodes);
1303-
}
1304-
13051301
for (auto &way : m_members_buffer.select<osmium::Way>()) {
13061302
middle.nodes_get_list(&(way.nodes()));
13071303
}

‎src/pgsql.hpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@
3737
class pg_result_t
3838
{
3939
public:
40+
pg_result_t() {}
41+
4042
explicit pg_result_t(PGresult *result) noexcept : m_result(result) {}
4143

4244
/// Get a pointer to the underlying PGresult object.
@@ -97,6 +99,9 @@ class pg_result_t
9799
return PQfnumber(m_result.get(), ('"' + name + '"').c_str());
98100
}
99101

102+
/// Return true if this holds an actual result.
103+
operator bool() { return m_result.get(); }
104+
100105
private:
101106
struct pg_result_deleter_t
102107
{

‎tests/bdd/flex/geometry-collection.feature‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ Feature: Create geometry collections from relations
2020
end
2121
"""
2222

23-
Scenario: Create geometry collection from different relations
23+
Scenario Outline: Create geometry collection from different relations
2424
Given the 1.0 grid
2525
| 13 | 12 | 17 | | 16 |
2626
| 10 | 11 | | 14 | 15 |
@@ -33,7 +33,9 @@ Feature: Create geometry collections from relations
3333
r32 Tname=mixed Mn17@,w21@
3434
r33 Tname=node Mn17@
3535
"""
36-
When running osm2pgsql flex
36+
When running osm2pgsql flex with parameters
37+
| -c |
38+
| <param> |
3739

3840
Then table osm2pgsql_test_collection contains exactly
3941
| osm_id | name | ST_GeometryType(geom) | ST_NumGeometries(geom) | ST_GeometryType(ST_GeometryN(geom, 1)) |
@@ -49,6 +51,11 @@ Feature: Create geometry collections from relations
4951
| 32 | 17 | 14, 15, 16 |
5052
| 33 | 17 | NULL |
5153

54+
Examples:
55+
| param |
56+
| |
57+
| --slim |
58+
5259
Scenario: NULL entry generated for broken geometries
5360
Given the grid
5461
| 10 |

‎tests/test-middle.cpp‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,14 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default,
213213
// set the relation
214214
auto const &relation =
215215
buffer.add_relation("r123 Mw11@,w10@outer,n1@,w12@inner");
216+
217+
std::vector<std::pair<osmium::item_type, osmid_t>> const expected = {
218+
{osmium::item_type::way, 11},
219+
{osmium::item_type::way, 10},
220+
{osmium::item_type::node, 1},
221+
{osmium::item_type::way, 12},
222+
};
223+
216224
osmium::CRC<osmium::CRC_zlib> orig_crc;
217225
orig_crc.update(relation);
218226

@@ -232,17 +240,62 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default,
232240
crc.update(rel);
233241
CHECK(orig_crc().checksum() == crc().checksum());
234242

235-
// retrieve the supporting ways
236-
REQUIRE(mid_q->rel_members_get(rel, &outbuf,
243+
// retrieve node members only
244+
osmium::memory::Buffer memberbuf{
245+
4096, osmium::memory::Buffer::auto_grow::yes};
246+
REQUIRE(mid_q->rel_members_get(rel, &memberbuf,
247+
osmium::osm_entity_bits::node) == 1);
248+
249+
{
250+
auto const objects = memberbuf.select<osmium::OSMObject>();
251+
auto it = objects.cbegin();
252+
auto const end = objects.cend();
253+
for (auto const &p : expected) {
254+
if (p.first == osmium::item_type::node) {
255+
REQUIRE(it != end);
256+
REQUIRE(it->type() == p.first);
257+
REQUIRE(it->id() == p.second);
258+
++it;
259+
}
260+
}
261+
}
262+
263+
memberbuf.clear();
264+
265+
// retrieve way members only
266+
REQUIRE(mid_q->rel_members_get(rel, &memberbuf,
237267
osmium::osm_entity_bits::way) == 3);
238268

239-
for (auto &w : outbuf.select<osmium::Way>()) {
240-
REQUIRE(w.id() >= 10);
241-
REQUIRE(w.id() <= 12);
242-
auto const &expected = nds[w.id() - 10];
243-
REQUIRE(w.nodes().size() == expected.size());
244-
for (size_t i = 0; i < expected.size(); ++i) {
245-
REQUIRE(w.nodes()[i].ref() == expected[i]);
269+
{
270+
auto const objects = memberbuf.select<osmium::OSMObject>();
271+
auto it = objects.cbegin();
272+
auto const end = objects.cend();
273+
for (auto const &p : expected) {
274+
if (p.first == osmium::item_type::way) {
275+
REQUIRE(it != end);
276+
REQUIRE(it->type() == p.first);
277+
REQUIRE(it->id() == p.second);
278+
++it;
279+
}
280+
}
281+
}
282+
283+
memberbuf.clear();
284+
285+
// retrieve all members
286+
REQUIRE(mid_q->rel_members_get(rel, &memberbuf,
287+
osmium::osm_entity_bits::node |
288+
osmium::osm_entity_bits::way) == 4);
289+
290+
{
291+
auto const objects = memberbuf.select<osmium::OSMObject>();
292+
auto it = objects.cbegin();
293+
auto const end = objects.cend();
294+
for (auto const &p : expected) {
295+
REQUIRE(it != end);
296+
REQUIRE(it->type() == p.first);
297+
REQUIRE(it->id() == p.second);
298+
++it;
246299
}
247300
}
248301

0 commit comments

Comments
 (0)