Changeset 18658 in josm

Timestamp:

2023-02-09T18:47:09+01:00 (3 years ago)

Author:

taylor.smock

Message:

See #22680: Unexpected exception when downloading GeoJSON

This primarily adds a non-regression test, but also narrows the IOException
to SocketException to better match the exception type and adds a specific
catch for parsing issues.

Location:

trunk

Files:

• src/org/openstreetmap/josm/io/OsmJsonReader.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/io/OsmJsonReaderTest.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/io/OsmJsonReader.java

@@ -4 +4 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
-import java.io.IOException;
 import java.io.InputStream;
+import java.net.SocketException;
 import java.util.Collection;
 import java.util.Map.Entry;
@@ -18 +18 @@
 import javax.json.stream.JsonParser;
 import javax.json.stream.JsonParser.Event;
+import javax.json.stream.JsonParsingException;
 
 import org.openstreetmap.josm.data.osm.DataSet;
@@ -186 +187 @@
                 parse();
             });
+        } catch (JsonParsingException exception) {
+            throw new IllegalDataException(exception);
         } catch (JsonException exception) {
-            if (exception.getCause() instanceof IOException) {
-                IllegalDataException ide = new IllegalDataException(exception.getCause());
-                // PMD 7 should be able to figure out that this is not an issue. See https://github.com/pmd/pmd/issues/2134 .
-                ide.addSuppressed(exception);
-                throw ide; // NOPMD
-            } else {
-                throw exception;
-            }
+            if (exception.getCause() instanceof SocketException) {
+                SocketException soe = (SocketException) exception.getCause();
+                soe.addSuppressed(exception); // Add the caught exception as a suppressed exception
+                throw new IllegalDataException(soe); // NOPMD -- PreserveStackTrace should be fixed with PMD 7
+            }
+            throw exception;
         }
     }

trunk/test/unit/org/openstreetmap/josm/io/OsmJsonReaderTest.java

@@ -2 +2 @@
 package org.openstreetmap.josm.io;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.ByteArrayInputStream;
+import java.io.IOException;
 import java.io.InputStream;
+import java.net.SocketException;
 import java.nio.charset.StandardCharsets;
 import java.time.Instant;
 import java.util.Iterator;
-
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import javax.json.JsonException;
+
+import org.junit.jupiter.api.Test;
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.DataSet;
@@ -20 +28 @@
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
-
-import org.junit.jupiter.api.Test;
+import org.openstreetmap.josm.tools.ExceptionUtil;
 
 /**
@@ -247 +254 @@
         assertEquals("runtime error: Query ran out of memory in \"query\" at line 5.", ds.getRemark());
     }
+
+
+    /**
+     * See #22680: Unexpected exception downloading from Overpass query
+     * The JSON parser throws {@link RuntimeException}s, specifically
+     * <ul>
+     *     <li>{@link javax.json.JsonException}</li>
+     *     <li>{@link javax.json.stream.JsonParsingException}, extends {@link javax.json.JsonException}</li>
+     *     <li>{@link javax.json.stream.JsonGenerationException}, extends {@link javax.json.JsonException}
+     *         (which we don't care about when we are <em>parsing</em> JSON)</li>
+     * </ul>
+     */
+    @SuppressWarnings("resource")
+    @Test
+    void testException() {
+        final ByteArrayInputStream bais =
+                new ByteArrayInputStream("{\"type\", \"node\", \"id\": 1, \"lat\": 1.0, \"lon\": 2.0}".getBytes(StandardCharsets.UTF_8));
+        final AtomicBoolean throwJson = new AtomicBoolean();
+        final InputStream socketExceptionStream = new InputStream() {
+            int read = 0; // Necessary, since otherwise the exception might not be wrapped in a Json exception
+            @Override
+            public int read() throws IOException {
+                try {
+                    if (read > 0 && !throwJson.get()) {
+                        throw new SocketException("Read timed out");
+                    } else if (read > 0 && throwJson.get()) {
+                        throw new JsonException("Some random json exception");
+                    }
+                    return bais.read();
+                } finally {
+                    read++;
+                }
+            }
+        };
+        // Check that a SocketException is properly reported
+        IllegalDataException ide = assertThrows(IllegalDataException.class,
+                () -> OsmJsonReader.parseDataSet(socketExceptionStream, NullProgressMonitor.INSTANCE));
+        assertEquals("java.net.SocketException: Read timed out", ExceptionUtil.explainException(ide));
+        assertDoesNotThrow(socketExceptionStream::close);
+        bais.reset();
+        // Check that a generic exception is properly thrown -- we only want to handle known "good" cases specially
+        throwJson.set(true);
+        assertThrows(JsonException.class, () -> OsmJsonReader.parseDataSet(socketExceptionStream, NullProgressMonitor.INSTANCE));
+        bais.reset();
+        // Check that a generic parsing error is properly reported
+        ide = assertThrows(IllegalDataException.class, () -> OsmJsonReader.parseDataSet(bais, NullProgressMonitor.INSTANCE));
+        assertEquals("javax.json.stream.JsonParsingException: Invalid token=COMMA at (line no=1, column no=8, offset=7). " +
+                "Expected tokens are: [COLON]", ExceptionUtil.explainException(ide));
+        bais.reset();
+        // Check that an unknown exception is thrown properly
+    }
 }