Changeset 36233 in osm for applications


Ignore:
Timestamp:
2024-03-28T15:04:04+01:00 (6 months ago)
Author:
taylor.smock
Message:

Fix #23582: Reverter plugin reverts nodes that were not modified by the reverted changeset

Location:
applications/editors/josm/plugins/reverter
Files:
12 added
2 edited

Legend:

Unmodified
Added
Removed

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

    r36230 r36233  
    504504
    505505    public void fixNodesWithoutCoordinates(ProgressMonitor progressMonitor) throws OsmTransferException {
    506         Collection<Node> nodes = nds.getNodes();
     506        Collection<Node> nodes = new ArrayList<>(nds.getNodes());
    507507        int num = nodes.size();
    508508        progressMonitor.beginTask(addChangesetIdPrefix(
    … …  
    510510                // downloads == num ticks, then we get the downloaded data (1 tick), then we process the nodes (num ticks)
    511511                2 * num + 1);
     512
     513        // Remove primitives where we already know the LatLon.
     514        nodes.removeIf(n -> {
     515            PrimitiveId id = n.getPrimitiveId();
     516            OsmPrimitive p = ds.getPrimitiveById(id);
     517            return !(p instanceof Node) || ((Node) p).isLatLonKnown();
     518        });
     519        progressMonitor.worked(num - nodes.size());
    512520
    513521        // Do bulk version fetches first
    … …  
    526534                versionMap.replaceAll((key, value) -> value - 1);
    527535                versionMap.values().removeIf(i -> i <= 0);
    528                 ds.update(() -> {
    529                     for (Node n : nodes) {
    530                         if (!n.isDeleted() && !n.isLatLonKnown()) {
    531                             final Node historyNode = (Node) history.getPrimitiveById(n);
    532                             if (historyNode != null && historyNode.isLatLonKnown()
    533                                     && changeset.getClosedAt().isAfter(historyNode.getInstant())) {
    534                                 n.load(historyNode.save());
    535                                 versionMap.remove(n.getUniqueId());
    536                                 progressMonitor.worked(1);
    537                             }
    538                         }
    539                         if (progressMonitor.isCanceled()) {
    540                             break;
    541                         }
    542                     }
    543                 });
     536                ds.update(() -> updateNodes(progressMonitor, nodes, versionMap, history));
    544537                if (progressMonitor.isCanceled()) {
    545538                    break;
    … …  
    548541        } finally {
    549542            progressMonitor.finishTask();
     543        }
     544    }
     545
     546    /**
     547     * Update nodes lacking a lat/lon
     548     * @param progressMonitor The monitor to update
     549     * @param nodes The nodes to update
     550     * @param versionMap The version map to update for the next round
     551     * @param history The history dataset
     552     */
     553    private void updateNodes(ProgressMonitor progressMonitor, Collection<Node> nodes, Map<Long, Integer> versionMap, DataSet history) {
     554        for (Node n : nodes) {
     555            if (!n.isDeleted() && !n.isLatLonKnown()) {
     556                final Node historyNode = (Node) history.getPrimitiveById(n);
     557                if (historyNode != null && historyNode.isLatLonKnown()
     558                        && changeset.getClosedAt().isAfter(historyNode.getInstant())) {
     559                    n.load(historyNode.save());
     560                    versionMap.remove(n.getUniqueId());
     561                    progressMonitor.worked(1);
     562                }
     563            }
     564            if (progressMonitor.isCanceled()) {
     565                break;
     566            }
    550567        }
    551568    }

  • applications/editors/josm/plugins/reverter/test/unit/reverter/ChangesetReverterTest.java

    r36122 r36233  
    44import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
    55import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
     6import static org.junit.jupiter.api.Assertions.assertEquals;
     7import static org.junit.jupiter.api.Assertions.assertInstanceOf;
    68import static org.junit.jupiter.api.Assertions.assertNotNull;
    79import static org.junit.jupiter.api.Assertions.assertTrue;
    … …  
    2123import java.util.stream.Collectors;
    2224
    23 import jakarta.json.Json;
    24 import jakarta.json.JsonArray;
    25 import jakarta.json.JsonNumber;
    26 import jakarta.json.JsonObject;
    27 import jakarta.json.JsonReader;
    28 import jakarta.json.JsonString;
    29 import jakarta.json.JsonValue;
     25import org.junit.jupiter.api.Test;
     26import org.junit.jupiter.api.extension.RegisterExtension;
     27import org.openstreetmap.josm.TestUtils;
     28import org.openstreetmap.josm.actions.downloadtasks.DownloadOsmTask;
     29import org.openstreetmap.josm.actions.downloadtasks.DownloadParams;
     30import org.openstreetmap.josm.data.UndoRedoHandler;
     31import org.openstreetmap.josm.data.osm.DataSet;
     32import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
     33import org.openstreetmap.josm.data.osm.PrimitiveId;
     34import org.openstreetmap.josm.data.osm.SimplePrimitiveId;
     35import org.openstreetmap.josm.data.osm.Way;
     36import org.openstreetmap.josm.gui.MainApplication;
     37import org.openstreetmap.josm.gui.layer.OsmDataLayer;
     38import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
     39import org.openstreetmap.josm.gui.util.GuiHelper;
     40import org.openstreetmap.josm.spi.preferences.Config;
     41import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
     42import org.openstreetmap.josm.testutils.annotations.HTTP;
     43import org.openstreetmap.josm.testutils.annotations.Main;
     44import org.openstreetmap.josm.testutils.annotations.Projection;
     45import org.openstreetmap.josm.tools.JosmRuntimeException;
     46import org.openstreetmap.josm.tools.Logging;
    3047
    3148import com.github.tomakehurst.wiremock.client.WireMock;
    … …  
    3855import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
    3956import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
    40 import org.junit.jupiter.api.Test;
    41 import org.junit.jupiter.api.extension.RegisterExtension;
    42 import org.openstreetmap.josm.TestUtils;
    43 import org.openstreetmap.josm.actions.downloadtasks.DownloadOsmTask;
    44 import org.openstreetmap.josm.actions.downloadtasks.DownloadParams;
    45 import org.openstreetmap.josm.data.UndoRedoHandler;
    46 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
    47 import org.openstreetmap.josm.data.osm.PrimitiveId;
    48 import org.openstreetmap.josm.data.osm.SimplePrimitiveId;
    49 import org.openstreetmap.josm.gui.MainApplication;
    50 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
    51 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
    52 import org.openstreetmap.josm.gui.util.GuiHelper;
    53 import org.openstreetmap.josm.spi.preferences.Config;
    54 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
    55 import org.openstreetmap.josm.testutils.annotations.HTTP;
    56 import org.openstreetmap.josm.tools.JosmRuntimeException;
    57 import org.openstreetmap.josm.tools.Logging;
     57import jakarta.json.Json;
     58import jakarta.json.JsonArray;
     59import jakarta.json.JsonNumber;
     60import jakarta.json.JsonObject;
     61import jakarta.json.JsonReader;
     62import jakarta.json.JsonString;
     63import jakarta.json.JsonValue;
    5864
    5965/**
    … …  
    6268@BasicPreferences
    6369@HTTP
     70@Main
     71@Projection
    6472class ChangesetReverterTest {
    6573    @RegisterExtension
    … …  
    7482    void testTicket22520(WireMockRuntimeInfo wireMockRuntimeInfo) throws ExecutionException, InterruptedException {
    7583        wireMockRuntimeInfo.getWireMock().loadMappingsFrom(TestUtils.getRegressionDataDir(22520));
    76         wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/nodes")).willReturn(WireMock.aResponse().withTransformers("MultiplePrimitiveTransformer")));
    77         wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/relations")).willReturn(WireMock.aResponse().withTransformers("MultiplePrimitiveTransformer")));
    78         wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/ways")).willReturn(WireMock.aResponse().withTransformers("MultiplePrimitiveTransformer")));
     84        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/nodes")).willReturn(WireMock.aResponse().withTransformer("MultiplePrimitiveTransformer", "ticket", 22520)));
     85        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/relations")).willReturn(WireMock.aResponse().withTransformer("MultiplePrimitiveTransformer", "ticket", 22520)));
     86        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/ways")).willReturn(WireMock.aResponse().withTransformer("MultiplePrimitiveTransformer", "ticket", 22520)));
    7987        Config.getPref().put("osm-server.url", wireMockRuntimeInfo.getHttpBaseUrl());
    8088        PrimitiveId building = new SimplePrimitiveId(233056719, OsmPrimitiveType.WAY);
    … …  
    9098
    9199    /**
     100     * Non-regression test for #23582: Nodes that were not modified by a changeset should not be reverted.
     101     * Note: This might not be the intended behavior, but it was the behavior prior to r36230.
     102     */
     103    @Test
     104    void testTicket23582(WireMockRuntimeInfo wireMockRuntimeInfo) {
     105        wireMockRuntimeInfo.getWireMock().loadMappingsFrom(TestUtils.getRegressionDataDir(23582));
     106        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/nodes")).willReturn(WireMock.aResponse().withTransformer("MultiplePrimitiveTransformer", "ticket", 23582)));
     107        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/relations")).willReturn(WireMock.aResponse().withTransformer("MultiplePrimitiveTransformer", "ticket", 23582)));
     108        wireMockRuntimeInfo.getWireMock().register(WireMock.get(WireMock.urlPathMatching("/0.6/ways")).willReturn(WireMock.aResponse().withTransformer("MultiplePrimitiveTransformer", "ticket", 23582)));
     109        Config.getPref().put("osm-server.url", wireMockRuntimeInfo.getHttpBaseUrl());
     110        final RevertChangesetTask task = new RevertChangesetTask(149181932, ChangesetReverter.RevertType.FULL, true, true);
     111        task.run();
     112        GuiHelper.runInEDTAndWait(() -> { /* Sync UI thread (some actions are taken on this thread which modify primitives) */ });
     113        final DataSet reverted = MainApplication.getLayerManager().getEditDataSet();
     114        assertEquals(1, reverted.allModifiedPrimitives().size());
     115        final Way oldWay = assertInstanceOf(Way.class, reverted.allModifiedPrimitives().iterator().next());
     116        assertEquals(8, oldWay.getNodesCount());
     117    }
     118
     119    /**
    92120     * A transformer for the /nodes?node, /ways?ways, and /relations?relations endpoints. This is needed since we don't always do the requests in the same order.
    93121     */
    … …  
    100128        @Override
    101129        public Response transform(Request request, Response response, FileSource files, Parameters parameters) {
     130            final int ticket = parameters.getInt("ticket");
    102131            final QueryParameter wayParam = request.queryParameter("ways");
    103132            final QueryParameter nodeParam = request.queryParameter("nodes");
    104133            final QueryParameter relParam = request.queryParameter("relations");
    105134            if (wayParam.isPresent()) {
    106                 return Response.Builder.like(response).but().body(getReturnXml(OsmPrimitiveType.WAY, wayParam)).build();
     135                return Response.Builder.like(response).but().body(getReturnXml(ticket, OsmPrimitiveType.WAY, wayParam)).build();
    107136            } else if (nodeParam.isPresent()) {
    108                 return Response.Builder.like(response).but().body(getReturnXml(OsmPrimitiveType.NODE, nodeParam)).build();
     137                return Response.Builder.like(response).but().body(getReturnXml(ticket, OsmPrimitiveType.NODE, nodeParam)).build();
    109138            } else if (relParam.isPresent()) {
    110                 return Response.Builder.like(response).but().body(getReturnXml(OsmPrimitiveType.RELATION, relParam)).build();
     139                return Response.Builder.like(response).but().body(getReturnXml(ticket, OsmPrimitiveType.RELATION, relParam)).build();
    111140            } else {
    112141                IllegalArgumentException e = new IllegalArgumentException("No query parameter present");
    … …  
    121150        }
    122151
    123         private static String getReturnXml(OsmPrimitiveType type, QueryParameter parameter) {
     152        private static String getReturnXml(int ticket, OsmPrimitiveType type, QueryParameter parameter) {
    124153            final String file;
    125154            switch (type) {
    … …  
    140169                    : parameter.values())
    141170                    .stream().map(s -> new OsmParameterInformation(type, s)).collect(Collectors.toSet());
    142             try (InputStream fis = Files.newInputStream(Paths.get(TestUtils.getRegressionDataDir(22520), file));
     171            try (InputStream fis = Files.newInputStream(Paths.get(TestUtils.getRegressionDataDir(ticket), file));
    143172                 JsonReader reader = Json.createReader(fis)) {
    144173                String version = "0.6";
Note: See TracChangeset for help on using the changeset viewer.