Reverter plugin crash

[zstadler]

What steps will reproduce the problem?

1. Data -> Revert changeset -> Changeset id: 97259223 -> Revert changeset fully

What is the expected result?

The changeset revert is ready for editing

What happens instead?

The plugin crashed

Please provide any additional information below. Attach a screenshot if possible.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2024-03-12 17:31:19 +0100 (Tue, 12 Mar 2024)
Revision:19017
Build-Date:2024-03-13 02:30:59
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (19017 en) Windows 11 64-Bit
OS Build number: Windows 10 Pro 2009 (22631)
Memory Usage: 324 MB / 4064 MB (142 MB allocated, but free)
Java version: 21.0.1+12-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.25×1.25)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: UTF-8
System property sun.jnu.encoding: Cp1252
Locale info: en_US
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Dicedtea-web.bin.location=%UserProfile%\AppData\Local\Programs\OpenWebStart\javaws, -Djava.util.Arrays.useLegacyMergeSort=true, --add-exports=jdk.deploy/com.sun.deploy.config=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-reads=java.naming=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.action=ALL-UNNAMED,java.desktop, --add-reads=java.base=ALL-UNNAMED,java.desktop, --add-exports=java.naming/com.sun.jndi.toolkit.url=ALL-UNNAMED,java.desktop, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-exports=java.desktop/com.apple.eawt=ALL-UNNAMED, --add-exports=java.desktop/sun.awt=ALL-UNNAMED,java.desktop, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-exports=java.base/sun.security.validator=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.base/sun.net.www.protocol.jar=ALL-UNNAMED,java.desktop, --add-exports=java.base/jdk.internal.util.jar=ALL-UNNAMED,java.desktop, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, --add-exports=java.base/com.sun.net.ssl.internal.ssl=ALL-UNNAMED,java.desktop, --add-exports=javafx.graphics/com.sun.javafx.application=ALL-UNNAMED, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.desktop/sun.awt.X11=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/sun.applet=ALL-UNNAMED,java.desktop,jdk.jsobject, --add-exports=java.base/sun.net.www.protocol.http=ALL-UNNAMED,java.desktop, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-exports=java.base/sun.security.util=ALL-UNNAMED,java.desktop, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-reads=java.desktop=ALL-UNNAMED,java.naming, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-exports=java.base/sun.security.x509=ALL-UNNAMED,java.desktop, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-exports=java.desktop/javax.jnlp=ALL-UNNAMED,java.desktop, --add-exports=java.base/sun.security.provider=ALL-UNNAMED,java.desktop, -Djava.security.manager=allow]
Dataset consistency test: No problems found

Plugins:
+ DirectDownload (36178)
+ continuosDownload (103)
+ reverter (36230)

Map paint styles:
+ https://josm.openstreetmap.de/josmfile?page=Styles/Potlatch2&zip=1

Last errors/warnings:
- 00000.340 W: extended font config - overriding 'filename.Malgun_Gothic=malgun.ttf' with 'MALGUN.TTF'
- 00000.340 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.341 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00002.460 W: Unable to request certificate of https://roottest-g3.pkioverheid.nl
- 00002.702 W: Unable to request certificate of https://roottest-g3.pkioverheid.nl
- 00019.619 E: Handled by bug report queue: java.lang.IllegalArgumentException: Version > 0 expected. Got 0.



=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: AWT-EventQueue-1 (48) of JOSM
java.lang.IllegalArgumentException: Version > 0 expected. Got 0.
	at org.openstreetmap.josm.data.osm.OsmPrimitive.setOsmId(OsmPrimitive.java:245)
	at reverter.DataSetCommandMerger.mergePrimitive(DataSetCommandMerger.java:86)
	at reverter.DataSetCommandMerger.mergeNode(DataSetCommandMerger.java:102)
	at reverter.DataSetCommandMerger.merge(DataSetCommandMerger.java:195)
	at reverter.DataSetCommandMerger.<init>(DataSetCommandMerger.java:53)
	at reverter.ChangesetReverter.getCommands(ChangesetReverter.java:397)
	at reverter.RevertChangesetTask.revertChangeset(RevertChangesetTask.java:188)
	at reverter.RevertChangesetTask.realRun(RevertChangesetTask.java:105)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.doRealRun(PleaseWaitRunnable.java:94)
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.run(PleaseWaitRunnable.java:142)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)

[GerdP]

I can reproduce this also with the latest version of the plugin. I do not yet see how to fix this without reverting the change in the method fixNodesWithoutCoordinates().
I think with those changes the method will only fetch one version of a missing node, but multiple versions may be needed.
On the other hand it seems to fetch many versions which are not needed.

[GerdP]

I think this patch makes the new, possibly faster code work as before.

[GerdP]

@Taylor: You probably know a revert which has lots of missing nodes so that the use of the multifetch really improves something?

[GerdP]

Thinking again about it I am not sure how this will work when one (or more) of the missing nodes where changed in redacted CS.
IIRC the multifetch returns no data when any of the required object versions was redacted.
The current code doesn't handle that in readMultiObjects(), instead it is done afterwards. See ChangesetReverter

            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);
            }

No idea why this postprocessing is repeated like that instead of doing it in readMultiObjects()

[taylor.smock]

I'll need to add changeset 97259223 to the regression suite.

IIRC the multifetch returns no data when any of the required object versions was redacted.

TIL. I'll see what the actual api responses were.

@Taylor: You probably know a revert which has lots of missing nodes so that the use of the multifetch really improves something?

Yes. Changeset 145544675 was really bad -- someone complained about it on #josm IRC.

And by "really bad", I mean > 1 hour bad (I killed it after an hour). With the changes I made in r36230/osm, it went down to <5 minutes.

Message from ​IRC for posterity:

sherbetsosm: @Taylor Smock: can you help me with a JOSM issue?
sherbetsosm: I'm trying to use the reverter plugin to revert a changeset, but apparently when it does the 'check coordinates of x nodes' command, the only problem is that the command apparently only checks 1 node every ~0.5 seconds. this is a massive deal when > I'm trying to check >70,000 nodes. Is there a way to make this faster? I've been staring at my computer for at least 20 mins waiting for it to finish.

vorpalblade77: I can take a look. What change set are you trying to revert?

sherbetsosm: ​https://www.openstreetmap.org/changeset/145544675
sherbetsosm: the changeset happens to delete thousands of ways/relations that reference nodes that were deleted in other changesets. I'd imagine that's why it is trying to check so many nodes

[GerdP]

OK, I see. See also #16508 and #17291.

[GerdP]

Improved patch with better handling of rc 404 (Not found) and reduced code duplication

[GerdP]

I still don't know how to test the reaction on a redacted object as I don't know how to find an id (or version) that was redacted.
I also cannot help with the unit test for cs 97259223, I think I don't have the tools on my Windows machine.
I tried the reaction on a non-existing node in the multi fetch with code like this after the creation of the collection versionMap:

    versionMap.put(12344L, 1);

This node existed before the licence change but it doesn't produce a return code 403 (HTTP_FORBIDDEN)

[taylor.smock]

In 36235/osm:

See #23584: Add non-regression test for changeset 97259223

Note: The test currently fails since the problem hasn't been fixed yet.

[taylor.smock]

I still don't know how to test the reaction on a redacted object as I don't know how to find an id (or version) that was redacted.

I don't know either. From what I understand, the redacted object no longer exists in the DB, period. I could be wrong about that though.
Anyway, I've added a test for this ticket (which currently fails, since you haven't applied your patch yet).

[GerdP]

The unit test fails with my patch. Seems a file is missing?

[taylor.smock]

Nope. It is just a wiremock thing. I think I just have to remove "persistent": true from the committed mapping files.

[GerdP]

Can you explain what the test is actually checking? It seems to mock the server responses? How will it work when we change the logic in the code to e.g. retrieve more or other data?

[taylor.smock]

In 36236/osm:

See #23584: Add non-regression test for changeset 97259223

This drops "persistent": true in the mappings files -- otherwise wiremock will
try to do stuff in src/test/resources.

[taylor.smock]

Replying to GerdP:

Can you explain what the test is actually checking?

The tests are effectively integration tests; I'm testing everything from fetching to what happens after the fetch.

It seems to mock the server responses?

Yep. I don't want to make the tests dependent upon server state.

How will it work when we change the logic in the code to e.g. retrieve more or other data?

That is where the (nodes|ways|relations).json files come in; they make the tests more robust against changing the order of fetches via the ChangesetReverterTest.(|Multiple)PrimitiveTransformer classes. Those two classes look for api requests (examples: /:type/:id/:version and /:types?:types=:idversion) and return the appropriate responses read from the json files.

Otherwise, if we completely change the API calls, I'll have to redo the transformers and json. But I probably won't have to.

[GerdP]

OK, I think I understand.
So, it will require changes when we change the code to request other (esp. more) objects?
Does it also compare the final result, means wether all the objects in the dataset are as expected?

[taylor.smock]

The primary thing I'm checking in the tests right now is that the modified object counts are as expected.

The test for #23580 does check that the reverted way has the expected number of nodes.

[GerdP]

OK, thanks.

[GerdP]

In 36237/osm:

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

[GerdP]

In 36238/osm:

fix #23584: Reverter plugin crash

• dist