Changeset 36237 in osm for applications

Timestamp:

2024-04-04T07:22:56+02:00 (9 months ago)

Author:

GerdP

Message:

fix #23584: Reverter plugin crash

• correct fixNodesWithoutCoordinates so that it retrieves the same data as before the use of the multifetch API
• improve handling of rc 404 from multifetch
• move duplicated code into new method readMultiObjectsOrNextOlder() which handles the case that a multifetch did not retrieve all wanted objects

Location:

applications/editors/josm/plugins/reverter/src/reverter

Files:

• ChangesetReverter.java (modified) (9 diffs)
• OsmServerMultiObjectReader.java (modified) (7 diffs)

applications/editors/josm/plugins/reverter/src/reverter/ChangesetReverter.java

@@ -5 +5 @@
 import static org.openstreetmap.josm.tools.I18n.trn;
 
-import java.net.HttpURLConnection;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -47 +46 @@
 import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.io.MultiFetchServerObjectReader;
-import org.openstreetmap.josm.io.OsmApiException;
 import org.openstreetmap.josm.io.OsmServerChangesetReader;
 import org.openstreetmap.josm.io.OsmTransferException;
-import org.openstreetmap.josm.tools.Logging;
 
 /**
@@ -204 +201 @@
     }
 
-    private static void readObjectVersion(OsmServerMultiObjectReader rdr, PrimitiveId id, int version, ProgressMonitor progressMonitor)
-            throws OsmTransferException {
-        boolean readOK = false;
-        while (!readOK && version >= 1) {
-            try {
-                rdr.readObject(id, version, progressMonitor.createSubTaskMonitor(1, true));
-                readOK = true;
-            } catch (OsmApiException e) {
-                if (e.getResponseCode() != HttpURLConnection.HTTP_FORBIDDEN) {
-                    throw e;
-                }
-                String message = "Version " + version + " of " + id + " is unauthorized";
-                Logging.info(version <= 1 ? message : message + ", requesting previous one");
-                version--;
-            }
-        }
-        if (!readOK) {
-            Logging.warn("Cannot retrieve any previous version of "+id);
-        }
-    }
-
     /**
      * fetch objects that were updated or deleted by changeset
@@ -238 +214 @@
                         trn("Downloading history for {0} object", "Downloading history for {0} objects", num, num)), num + 1);
         try {
-            HashMap<Long, Integer> nodeList = new HashMap<>(),
-                    wayList = new HashMap<>(),
-                    relationList = new HashMap<>();
+            HashMap<Long, Integer> nodeList = new HashMap<>();
+            HashMap<Long, Integer> wayList = new HashMap<>();
+            HashMap<Long, Integer> relationList = new HashMap<>();
 
             for (HashSet<HistoryOsmPrimitive> collection : Arrays.asList(updated, deleted)) {
@@ -256 +232 @@
                 }
             }
-            rdr.readMultiObjects(OsmPrimitiveType.NODE, nodeList, progressMonitor);
-            rdr.readMultiObjects(OsmPrimitiveType.WAY, wayList, progressMonitor);
-            rdr.readMultiObjects(OsmPrimitiveType.RELATION, relationList, progressMonitor);
-            if (progressMonitor.isCanceled()) return;
-            // If multi-read failed, retry with regular read
-            for (Map.Entry<Long, Integer> entry : nodeList.entrySet()) {
-                if (progressMonitor.isCanceled()) return;
-                readObjectVersion(rdr, new SimplePrimitiveId(entry.getKey(), OsmPrimitiveType.NODE), entry.getValue(), progressMonitor);
-            }
-            for (Map.Entry<Long, Integer> entry : wayList.entrySet()) {
-                if (progressMonitor.isCanceled()) return;
-                readObjectVersion(rdr, new SimplePrimitiveId(entry.getKey(), OsmPrimitiveType.WAY), entry.getValue(), progressMonitor);
-            }
-            for (Map.Entry<Long, Integer> entry : relationList.entrySet()) {
-                if (progressMonitor.isCanceled()) return;
-                readObjectVersion(rdr, new SimplePrimitiveId(entry.getKey(), OsmPrimitiveType.RELATION), entry.getValue(), progressMonitor);
-            }
+            rdr.readMultiObjectsOrNextOlder(OsmPrimitiveType.NODE, nodeList, progressMonitor);
+            rdr.readMultiObjectsOrNextOlder(OsmPrimitiveType.WAY, wayList, progressMonitor);
+            rdr.readMultiObjectsOrNextOlder(OsmPrimitiveType.RELATION, relationList, progressMonitor);
             if (progressMonitor.isCanceled()) return;
             nds = rdr.parseOsm(progressMonitor.createSubTaskMonitor(1, true));
@@ -513 +475 @@
         // Remove primitives where we already know the LatLon.
         nodes.removeIf(n -> {
+            if (n.isDeleted() || n.isLatLonKnown())
+                return true;
             PrimitiveId id = n.getPrimitiveId();
             OsmPrimitive p = ds.getPrimitiveById(id);
-            return !(p instanceof Node) || ((Node) p).isLatLonKnown();
+            return !(p instanceof Node) || ((Node) p).isLatLonKnown() || p.getVersion() <= 1;
         });
+
         progressMonitor.worked(num - nodes.size());
 
@@ -524 +489 @@
                 .collect(Collectors.toMap(AbstractPrimitive::getUniqueId,
                         id -> Math.max(1, Optional.ofNullable(ds.getPrimitiveById(id)).orElse(id).getVersion() - 1)));
+
         try {
             while (!versionMap.isEmpty()) {
@@ -529 +495 @@
                 final ProgressMonitor subMonitor = progressMonitor.createSubTaskMonitor(num, false);
                 subMonitor.beginTask(tr("Fetching multi-objects"), versionMap.size());
-                rds.readMultiObjects(OsmPrimitiveType.NODE, versionMap, subMonitor);
+                rds.readMultiObjectsOrNextOlder(OsmPrimitiveType.NODE, versionMap, subMonitor);
                 subMonitor.finishTask();
                 final DataSet history = rds.parseOsm(progressMonitor.createSubTaskMonitor(0, false));
+                ds.update(() -> updateNodes(progressMonitor, nodes, versionMap, history));
+                versionMap.values().removeIf(i -> i <= 1);
                 versionMap.replaceAll((key, value) -> value - 1);
-                versionMap.values().removeIf(i -> i <= 0);
-                ds.update(() -> updateNodes(progressMonitor, nodes, versionMap, history));
                 if (progressMonitor.isCanceled()) {
                     break;
@@ -555 +521 @@
             if (!n.isDeleted() && !n.isLatLonKnown()) {
                 final Node historyNode = (Node) history.getPrimitiveById(n);
-                if (historyNode != null && historyNode.isLatLonKnown()
-                        && changeset.getClosedAt().isAfter(historyNode.getInstant())) {
-                    n.load(historyNode.save());
-                    versionMap.remove(n.getUniqueId());
-                    progressMonitor.worked(1);
+                if (historyNode != null) {
+                    if (historyNode.isLatLonKnown()
+                            && changeset.getClosedAt().isAfter(historyNode.getInstant())) {
+                        n.load(historyNode.save());
+                        progressMonitor.worked(1);
+                    } else {
+                        versionMap.put(n.getId(), historyNode.getVersion());
+                    }
                 }
             }

applications/editors/josm/plugins/reverter/src/reverter/OsmServerMultiObjectReader.java

@@ -6 +6 @@
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.HttpURLConnection;
 import java.util.ArrayList;
 import java.util.List;
@@ -16 +17 @@
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
 import org.openstreetmap.josm.io.IllegalDataException;
+import org.openstreetmap.josm.io.OsmApiException;
 import org.openstreetmap.josm.io.OsmServerReader;
 import org.openstreetmap.josm.io.OsmTransferException;
@@ -50 +52 @@
      * @return The queries to make
      */
-    private static List<String> makeQueryStrings(OsmPrimitiveType type, Map<Long,Integer> list) {
+    private static List<String> makeQueryStrings(OsmPrimitiveType type, Map<Long, Integer> list) {
         // This is a "worst-case" calculation. Keep it fast (and err higher rather than lower), not accurate.
         final int expectedSize = (int) (list.entrySet().stream().mapToLong(entry ->
@@ -63 +65 @@
         List<String> result = new ArrayList<>(expectedSize + 1);
         StringBuilder sb = new StringBuilder();
-        int cnt=0;
-        for (Map.Entry<Long,Integer> entry : list.entrySet()) {
+        int cnt = 0;
+        for (Map.Entry<Long, Integer> entry : list.entrySet()) {
             if (cnt == 0) {
                 sb.append(type.getAPIName());
@@ -83 +85 @@
             }
         }
-        if (cnt>0) {
+        if (cnt > 0) {
             result.add(sb.toString());
         }
@@ -109 +111 @@
 
     /**
-     * Parse many objects
+     * Parse many objects.
      * @param type The object type (<i>must</i> be common between all objects)
-     * @param list The map of object id to object version
+     * @param list The map of object id to object version. Successfully retrieved objects are removed.
      * @param progressMonitor The progress monitor to update
      * @throws OsmTransferException If there is an issue getting the data
      */
-    public void readMultiObjects(OsmPrimitiveType type, Map<Long,Integer> list, ProgressMonitor progressMonitor) throws OsmTransferException {
-        for (String query : makeQueryStrings(type,list)) {
+    public void readMultiObjects(OsmPrimitiveType type, Map<Long, Integer> list, ProgressMonitor progressMonitor) throws OsmTransferException {
+        for (String query : makeQueryStrings(type, list)) {
             if (progressMonitor.isCanceled()) {
                 return;
@@ -130 +132 @@
                 Logging.warn(e);
                 throw new OsmTransferException(e);
+            } catch (OsmApiException e) {
+                Logging.warn(e);
+                // allow to continue further bulk requests
+                if (e.getResponseCode() != HttpURLConnection.HTTP_FORBIDDEN
+                        && e.getResponseCode() != HttpURLConnection.HTTP_NOT_FOUND) {
+                    throw e;
+                }
             } finally {
                 rdr.callback = null;
             }
+        }
+    }
+
+    /**
+     * Parse many objects. If redacted elements are requested the method tries to retrieve the next older version.
+     * @param type The object type (<i>must</i> be common between all objects)
+     * @param list The map of object id to object version. Successfully retrieved objects are removed.
+     * @param progressMonitor The progress monitor to update
+     * @throws OsmTransferException If there is an issue getting the data
+     */
+    public void readMultiObjectsOrNextOlder(OsmPrimitiveType type, Map<Long, Integer> list,
+            ProgressMonitor progressMonitor) throws OsmTransferException {
+        readMultiObjects(type, list, progressMonitor);
+        // If multi-read failed, retry with regular read
+        for (Map.Entry<Long, Integer> entry : list.entrySet()) {
+            if (progressMonitor.isCanceled()) return;
+            readObjectVersion(type, entry.getKey(), entry.getValue(), progressMonitor);
+        }
+    }
+
+    private void readObjectVersion(OsmPrimitiveType type, long id, int version, ProgressMonitor progressMonitor)
+            throws OsmTransferException {
+        boolean readOK = false;
+        while (!readOK && version >= 1) {
+            try {
+                readObject(id, version, type, progressMonitor.createSubTaskMonitor(1, true));
+                readOK = true;
+            } catch (OsmApiException e) {
+                if (e.getResponseCode() != HttpURLConnection.HTTP_FORBIDDEN) {
+                    throw e;
+                }
+                String message = "Version " + version + " of " + id + " is unauthorized";
+                Logging.info(version <= 1 ? message : message + ", requesting previous one");
+                version--;
+            }
+        }
+        if (!readOK) {
+            Logging.warn("Cannot retrieve any previous version of {1} {2}", type, id);
         }
     }