Ticket #13843: patch_13843.diff

src/org/openstreetmap/josm/plugins/opendata/core/io/geographic/ShpReader.java

@@ -115 +115 @@
                 }
             }
 
-            OsmPrimitive primitive = null;
-
-            if (geometry.getValue() instanceof Point) {
-                primitive = createOrGetEmptyNode((Point) geometry.getValue());
-
-            } else if (geometry.getValue() instanceof GeometryCollection) { // Deals with both MultiLineString and MultiPolygon
-                GeometryCollection gc = (GeometryCollection) geometry.getValue();
-                int nGeometries = gc.getNumGeometries();
-                if (nGeometries < 1) {
-                    Logging.error("empty geometry collection found");
-                } else {
-                    Relation r = null;
-                    Way w = null;
-
-                    for (int i = 0; i < nGeometries; i++) {
-                        Geometry g = gc.getGeometryN(i);
-                        if (g instanceof Polygon) {
-                            Polygon p = (Polygon) g;
-                            // Do not create relation if there's only one polygon without interior ring
-                            // except if handler prefers it
-                            if (r == null && (nGeometries > 1 || p.getNumInteriorRing() > 0 ||
-                                    (handler != null && handler.preferMultipolygonToSimpleWay()))) {
-                                r = createMultipolygon();
-                            }
-                            w = createOrGetWay(p.getExteriorRing());
-                            if (r != null) {
-                                addWayToMp(r, "outer", w);
-                                for (int j = 0; j < p.getNumInteriorRing(); j++) {
-                                    addWayToMp(r, "inner", createOrGetWay(p.getInteriorRingN(j)));
-                                }
-                            }
-                        } else if (g instanceof LineString) {
-                            w = createOrGetWay((LineString) g);
-                        } else if (g instanceof Point) {
-                            // Some belgian data sets hold points into collections ?!
-                            readNonGeometricAttributes(feature, createOrGetNode((Point) g));
-                        } else {
-                            Logging.error("unsupported geometry : "+g);
-                        }
-                    }
-                    primitive = r != null ? r : w;
+            Object geomObject = geometry.getValue();
+            if (geomObject instanceof Point) {  // TODO: Support LineString and Polygon.
+                // Sure you could have a Set of 1 object and join these 2 branches of
+                // code, but I feel there would be a performance hit.
+                OsmPrimitive primitive = createOrGetEmptyNode((Point) geomObject);
+                readNonGeometricAttributes(feature, primitive);
+            } else if (geomObject instanceof GeometryCollection) { // Deals with both MultiLineString and MultiPolygon
+                Set<OsmPrimitive> primitives = processGeometryCollection((GeometryCollection) geomObject);
+                for (OsmPrimitive prim : primitives) {
+                    readNonGeometricAttributes(feature, prim);
                 }
             } else {
                 // Debug unknown geometry
@@ -163 +132 @@
                 Logging.debug("\tbounds: "+geometry.getBounds());
                 Logging.debug("\tdescriptor: "+desc);
                 Logging.debug("\tname: "+geometry.getName());
-                Logging.debug("\tvalue: "+geometry.getValue());
+                Logging.debug("\tvalue: "+geomObject);
                 Logging.debug("\tid: "+geometry.getIdentifier());
                 Logging.debug("-------------------------------------------------------------");
             }
+        }
+    }
 
-            if (primitive != null) {
-                // Read primitive non geometric attributes
-                readNonGeometricAttributes(feature, primitive);
+    protected Set<OsmPrimitive> processGeometryCollection(GeometryCollection gc) throws TransformException {
+        // A feture may be a collection.  This set holds the items of the collection.
+        Set<OsmPrimitive> primitives = new HashSet<>();
+        int nGeometries = gc.getNumGeometries();
+        if (nGeometries < 1) {
+            Logging.error("empty geometry collection found");
+        } else {
+            // Create the primitive "op" and add it to the set of primitives.
+            for (int i = 0; i < nGeometries; i++) {
+                OsmPrimitive op = null;
+                Geometry g = gc.getGeometryN(i);
+                if (g instanceof Polygon) {
+                    Relation r = (Relation) op;
+                    Polygon p = (Polygon) g;
+                    // Do not create relation if there's only one polygon without interior ring
+                    // except if handler prefers it
+                    if (r == null && (nGeometries > 1 || p.getNumInteriorRing() > 0 ||
+                            (handler != null && handler.preferMultipolygonToSimpleWay()))) {
+                        r = createMultipolygon();
+                    }
+                    if (r != null) {
+                        addWayToMp(r, "outer", createOrGetWay(p.getExteriorRing()));
+                        for (int j = 0; j < p.getNumInteriorRing(); j++) {
+                            addWayToMp(r, "inner", createOrGetWay(p.getInteriorRingN(j)));
+                        }
+                    }
+                } else if (g instanceof LineString) {
+                    op = createOrGetWay((LineString) g);
+                } else if (g instanceof Point) {
+                    op = createOrGetNode((Point) g);
+                } else {
+                    Logging.error("unsupported geometry : "+g);
+                }
+                primitives.add(op);
             }
         }
+        return primitives;
     }
 
     public DataSet parse(File file, ProgressMonitor instance) throws IOException {

new file test/data/regress/12843/test.prj

@@ +1 @@
+GEOGCS["GCS_WGS_1984",DATUM["D_WGS_1984",SPHEROID["WGS_1984",6378137,298.257223563]],PRIMEM["Greenwich",0],UNIT["Degree",0.017453292519943295]]
+ No newline at end of file

new file test/data/regress/12843/test.qpj

@@ +1 @@
+GEOGCS["WGS 84",DATUM["WGS_1984",SPHEROID["WGS 84",6378137,298.257223563,AUTHORITY["EPSG","7030"]],AUTHORITY["EPSG","6326"]],PRIMEM["Greenwich",0,AUTHORITY["EPSG","8901"]],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"]],AUTHORITY["EPSG","4326"]]

test/unit/org/openstreetmap/josm/plugins/opendata/core/io/geographic/ShpReaderTest.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.opendata.core.io.geographic;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
@@ -8 +9 @@
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.InputStream;
+import java.util.Collection;
 
 import org.junit.Ignore;
 import org.junit.Rule;
@@ -15 +17 @@
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.plugins.opendata.core.io.NonRegFunctionalTests;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
@@ -84 +87 @@
             NonRegFunctionalTests.testGeneric("#8309", ShpReader.parseDataSet(is, file, null, null));
         }
     }
+
+    /**
+     * Non-regression test for ticket <a href="https://josm.openstreetmap.de/ticket/12843">#12843</a>
+     * @throws Exception if an error occurs during reading
+     */
+    @Test
+    public void testTicket12843() throws Exception {
+        File file = new File(TestUtils.getRegressionDataFile(12843, "test.shp"));
+        try (InputStream is = new FileInputStream(file)) {
+            Collection<Way> ways = ShpReader.parseDataSet(is, file, null, null).getWays();
+            assertFalse(ways.isEmpty());
+            for (Way way : ways) {
+                assertEquals("Test", way.get("name"));
+            }
+        }
+    }
 }