Fixed a bug in DynamicRelation.members() that broke BareAtlas.relatio…

…nsLowerOrderFirst() (#216) * Added unit test to expose the bug * Fixed integration test to expose the bug * Implemented a fix in DynamicRelation.members * Switched if/else instanceof to switch on entity type
osmlab · Sep 13, 2018 · 05d4bfa · 05d4bfa
1 parent 97bd3f4
commit 05d4bfa
Show file tree

Hide file tree

Showing 4 changed files with 184 additions and 2 deletions.
diff --git a/...grationTest/java/org/openstreetmap/atlas/geography/atlas/raw/RawAtlasIntegrationTest.java b/...grationTest/java/org/openstreetmap/atlas/geography/atlas/raw/RawAtlasIntegrationTest.java
@@ -287,7 +287,15 @@ public void testSectioningFromShard()
         Assert.assertEquals(5126, finalAtlas.numberOfAreas());
         Assert.assertEquals(184, finalAtlas.numberOfPoints());
         Assert.assertEquals(271, finalAtlas.numberOfLines());
-        Assert.assertEquals(14, finalAtlas.numberOfRelations());
+
+        /*
+         * This has been updated to 16 instead of 14. This is because two of the relations in
+         * 8-122-122-trimmed.osm.pbf have subrelations, and so the WaySectionProcessor was simply
+         * dropping them when using the old BareAtlas.relationsLowerOrderFirstCode (We have now
+         * fixed BareAtlas.relationsLowerOrderFirst to not drop relations when the BareAtlas is a
+         * DynamicAtlas). In reality, we should process them and include then in the final atlas.
+         */
+        Assert.assertEquals(16, finalAtlas.numberOfRelations());
     }
 
     @Test

diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicRelation.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicRelation.java
@@ -1,12 +1,15 @@
 package org.openstreetmap.atlas.geography.atlas.dynamic;
 
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
 import org.openstreetmap.atlas.exception.CoreException;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasEntity;
 import org.openstreetmap.atlas.geography.atlas.items.Relation;
+import org.openstreetmap.atlas.geography.atlas.items.RelationMember;
 import org.openstreetmap.atlas.geography.atlas.items.RelationMemberList;
 
 /**
@@ -28,6 +31,10 @@ protected DynamicRelation(final DynamicAtlas atlas, final long identifier)
     @Override
     public RelationMemberList allKnownOsmMembers()
     {
+        /*
+         * TODO this will return AtlasEntities which are not of type DynamicX. Ideally, we should be
+         * recreating returned entities as DynamicX instead of the underlying PackedX or MultiX.
+         */
         return subRelation().allKnownOsmMembers();
     }
 
@@ -54,7 +61,41 @@ public Map<String, String> getTags()
     @Override
     public RelationMemberList members()
     {
-        return subRelation().members();
+        final RelationMemberList subRelationMemberList = subRelation().members();
+        final List<RelationMember> newMemberList = new ArrayList<>();
+
+        for (final RelationMember member : subRelationMemberList)
+        {
+            final AtlasEntity entity = member.getEntity();
+            AtlasEntity dynamicEntity = null;
+            switch (entity.getType())
+            {
+                case NODE:
+                    dynamicEntity = new DynamicNode(dynamicAtlas(), entity.getIdentifier());
+                    break;
+                case EDGE:
+                    dynamicEntity = new DynamicEdge(dynamicAtlas(), entity.getIdentifier());
+                    break;
+                case POINT:
+                    dynamicEntity = new DynamicPoint(dynamicAtlas(), entity.getIdentifier());
+                    break;
+                case LINE:
+                    dynamicEntity = new DynamicLine(dynamicAtlas(), entity.getIdentifier());
+                    break;
+                case AREA:
+                    dynamicEntity = new DynamicArea(dynamicAtlas(), entity.getIdentifier());
+                    break;
+                case RELATION:
+                    dynamicEntity = new DynamicRelation(dynamicAtlas(), entity.getIdentifier());
+                    break;
+                default:
+                    throw new CoreException("Invalid entity type {}", entity.getType());
+            }
+            newMemberList.add(new RelationMember(member.getRole(), dynamicEntity,
+                    member.getRelationIdentifier()));
+        }
+
+        return new RelationMemberList(newMemberList);
     }
 
     @Override

diff --git a/src/test/java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicAtlasTest.java b/src/test/java/org/openstreetmap/atlas/geography/atlas/dynamic/DynamicAtlasTest.java
@@ -1,8 +1,10 @@
 package org.openstreetmap.atlas.geography.atlas.dynamic;
 
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.function.Supplier;
 
 import org.junit.Assert;
@@ -11,21 +13,29 @@
 import org.junit.Test;
 import org.openstreetmap.atlas.geography.Rectangle;
 import org.openstreetmap.atlas.geography.atlas.Atlas;
+import org.openstreetmap.atlas.geography.atlas.BareAtlas;
 import org.openstreetmap.atlas.geography.atlas.delta.AtlasDelta;
 import org.openstreetmap.atlas.geography.atlas.dynamic.policy.DynamicAtlasPolicy;
 import org.openstreetmap.atlas.geography.atlas.dynamic.rules.DynamicAtlasTestRule;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasEntity;
 import org.openstreetmap.atlas.geography.atlas.items.Relation;
 import org.openstreetmap.atlas.geography.atlas.multi.MultiAtlas;
+import org.openstreetmap.atlas.geography.atlas.multi.MultiRelation;
+import org.openstreetmap.atlas.geography.atlas.packed.PackedRelation;
 import org.openstreetmap.atlas.geography.sharding.Shard;
 import org.openstreetmap.atlas.geography.sharding.SlippyTile;
 import org.openstreetmap.atlas.geography.sharding.SlippyTileSharding;
 import org.openstreetmap.atlas.utilities.collections.Iterables;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * @author matthieun
  */
 public class DynamicAtlasTest
 {
+    private static final Logger logger = LoggerFactory.getLogger(DynamicAtlasTest.class);
+
     @Rule
     public DynamicAtlasTestRule rule = new DynamicAtlasTestRule();
 
@@ -243,6 +253,58 @@ public void testLoadRelationWithOverlappingMembersByIdentifier()
 
     }
 
+    /**
+     * Check to make sure that {@link Atlas#relationsLowerOrderFirst()} works when the {@link Atlas}
+     * is a {@link DynamicAtlas}. In older versions of the code, any relations that had members
+     * which were also relations would be dropped from the set returned by
+     * {@link BareAtlas#relationsLowerOrderFirst()}. This was due to a flaw in the membership
+     * assumptions made by {@link BareAtlas#relationsLowerOrderFirst()}, which assumed that
+     * relations in the main {@link DynamicAtlas} and their equivalent representation as a member
+     * {@link AtlasEntity} of another relation in the same {@link DynamicAtlas} were of consistent
+     * types. However, this was not the case. (The former representation would be of type
+     * {@link DynamicRelation} and the latter would be of type {@link MultiRelation} or
+     * {@link PackedRelation}). This has now been fixed, so this test should always pass.
+     */
+    @Test
+    public void testRelationsLowerOrderFirstConsistency()
+    {
+        final DynamicAtlas localDynamicAtlas;
+        final Map<Shard, Atlas> localStore = new HashMap<>();
+        localStore.put(new SlippyTile(0, 0, 0), this.rule.getAtlasForRelationsTest());
+        final Supplier<DynamicAtlasPolicy> localPolicySupplier = () -> new DynamicAtlasPolicy(
+                shard ->
+                {
+                    if (localStore.containsKey(shard))
+                    {
+                        return Optional.of(localStore.get(shard));
+                    }
+                    else
+                    {
+                        return Optional.empty();
+                    }
+                }, new SlippyTileSharding(0), new SlippyTile(0, 0, 0), Rectangle.MAXIMUM);
+        localDynamicAtlas = new DynamicAtlas(localPolicySupplier.get());
+
+        final Set<Relation> returnedByRelations = new HashSet<>();
+        final Set<Relation> returnedByRelationsLowerOrderFirst = new HashSet<>();
+
+        for (final Relation relation : localDynamicAtlas.relations())
+        {
+            returnedByRelations.add(relation);
+        }
+
+        for (final Relation relation : localDynamicAtlas.relationsLowerOrderFirst())
+        {
+            returnedByRelationsLowerOrderFirst.add(relation);
+        }
+
+        // Assert that their sizes equal, if not then we can fail fast
+        Assert.assertEquals(returnedByRelations.size(), returnedByRelationsLowerOrderFirst.size());
+
+        // Now that we know they have equal sizes, check member equality
+        Assert.assertEquals(returnedByRelations, returnedByRelationsLowerOrderFirst);
+    }
+
     @Test
     public void testRuleIntegrity()
     {

diff --git a/...test/java/org/openstreetmap/atlas/geography/atlas/dynamic/rules/DynamicAtlasTestRule.java b/...test/java/org/openstreetmap/atlas/geography/atlas/dynamic/rules/DynamicAtlasTestRule.java
@@ -349,11 +349,82 @@ public class DynamicAtlasTestRule extends CoreTestRule
     )
     private Atlas atlasz12x1349y1870;
 
+    @TestAtlas(
+
+            nodes = {
+
+                    @Node(id = "10", coordinates = @Loc(value = ONE)),
+                    @Node(id = "11", coordinates = @Loc(value = SIX)),
+                    @Node(id = "12", coordinates = @Loc(value = SEVEN)),
+                    @Node(id = "13", coordinates = @Loc(value = EIGHT))
+
+            },
+
+            relations = {
+
+                    @Relation(id = "31", tags = { "type=relation" }, members = {
+
+                            @Member(id = "11", role = "a", type = "node")
+
+                    }),
+
+                    @Relation(id = "32", tags = { "type=relation" }, members = {
+
+                            @Member(id = "12", role = "a", type = "node"),
+
+                            @Member(id = "13", role = "a", type = "node")
+
+                    }),
+
+                    @Relation(id = "33", tags = { "type=relation" }, members = {
+
+                            @Member(id = "10", role = "a", type = "node"),
+
+                            @Member(id = "11", role = "a", type = "node"),
+
+                            @Member(id = "31", role = "b", type = "relation")
+
+                    }),
+
+                    @Relation(id = "34", tags = { "type=relation" }, members = {
+
+                            @Member(id = "10", role = "a", type = "node"),
+
+                            @Member(id = "13", role = "a", type = "node")
+
+                    }),
+
+                    @Relation(id = "35", tags = { "type=relation" }, members = {
+
+                            @Member(id = "11", role = "a", type = "node"),
+
+                            @Member(id = "12", role = "a", type = "node")
+
+                    }),
+
+                    @Relation(id = "36", tags = { "type=relation" }, members = {
+
+                            @Member(id = "12", role = "a", type = "node"),
+
+                            @Member(id = "34", role = "a", type = "relation")
+
+                    }),
+
+            }
+
+    )
+    private Atlas atlasForRelationsTest;
+
     public Atlas getAtlas()
     {
         return this.atlas;
     }
 
+    public Atlas getAtlasForRelationsTest()
+    {
+        return this.atlasForRelationsTest;
+    }
+
     public Atlas getAtlasz12x1349y1869()
     {
         return this.atlasz12x1349y1869;