Context Navigation

Modify ↓

#23049 closed defect (fixed)

Validator has no errors but leaves validator dialog up, should move to upload dialog

Reported by:	watmildon	Owned by:	team
Priority:	normal	Milestone:	23.07
Component:	Core	Version:
Keywords:	template_report	Cc:

Description

What steps will reproduce the problem?

load attached osm file
hit upload

What is the expected result?

validator runs and the upload dialog is shown

What happens instead?

validator runs and has an empty dialog with no issues asking if you want to continue

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

Revision:18772
Build-Date:2023-07-07 10:03:44

Identification: JOSM/1.5 (18772 en) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19045)
Memory Usage: 2688 MB / 4088 MB (1003 MB allocated, but free)
Java version: 17.0.7+7-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1440×2560 (scaling 1.00×1.00) \Display1 3840×2160 (scaling 1.50×1.50)
Maximum Screen Size: 3840×2560
Best cursor sizes: 16×16→48×48, 32×32→48×48
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: en_US
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Djpackage.app-version=1.5.18772, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.base/sun.security.action=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, -Djpackage.app-path=%UserProfile%\AppData\Local\JOSM\JOSM.exe]
Dataset consistency test: No problems found

Plugins:
+ FastDraw (36097)
+ FixAddresses (36062)
+ MapRoulette (21)
+ apache-commons (36034)
+ apache-http (35924)
+ buildings_tools (36097)
+ conflation (0.6.11)
+ continuosDownload (1.3.4)
+ contourmerge (v0.1.9)
+ damn (0.11.3)
+ ejml (35924)
+ geotools (36068)
+ gridify (1606242219)
+ imagery_offset_db (36079)
+ jackson (36034)
+ javafx (36086)
+ jaxb (35952)
+ jna (36005)
+ jts (36004)
+ log4j (36045)
+ mapwithai (794)
+ opendata (36097)
+ reltoolbox (36097)
+ reverter (36066)
+ tageditor (36097)
+ terracer (36079)
+ todo (123)
+ turnrestrictions (36097)
+ utilsplugin2 (36097)
+ wikipedia (605)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/TigerReviewedNo&zip=1
+ %UserProfile%\Address Tags Validator - Highlight unaddressed
- https://josm.openstreetmap.de/josmfile?page=Styles/Modified&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/AddressValidator&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/MapWithAI&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Admin_Boundaries&zip=1
+ C:\OSM\mapwiaipaintstyle

Last errors/warnings:
- 118062.589 E: java.nio.file.AccessDeniedException: <josm.cache>\mirror_https___josm.openstreetmap.de_josmfile_page_Styles_MapWithAI_zip_1.tmp -> <josm.cache>\mirror_https___josm.openstreetmap.de_josmfile_page_Styles_MapWithAI_zip_1
- 118305.831 W: Could not move autosaved file MapWithAI_20230708_195105409.osm to deleted_layers folder
- 118305.832 W: Unable to delete backup file <josm.pref>\autosave\MapWithAI_20230708_195105409.osm
- 122096.002 E: org.openstreetmap.josm.io.OsmApiException: ResponseCode=400, Error Header=<You requested too many nodes (limit is 50000). Either request a smaller area, or use planet.osm>
- 122096.007 E: Bad Request - <html>The OSM server 'api.openstreetmap.org' reported a bad request.<br><br>The area you tried to download is too big or your request was too large.<br>Either request a smaller area or use an export file provided by the OSM community.</html>
- 122096.026 E: org.openstreetmap.josm.io.OsmApiException: ResponseCode=400, Error Header=<You requested too many nodes (limit is 50000). Either request a smaller area, or use planet.osm>
- 122096.034 E: Bad Request - <html>The OSM server 'api.openstreetmap.org' reported a bad request.<br><br>The area you tried to download is too big or your request was too large.<br>Either request a smaller area or use an export file provided by the OSM community.</html>
- 122203.325 E: java.nio.file.AccessDeniedException: <josm.cache>\mirror_https___josm.openstreetmap.de_josmfile_page_Styles_MapWithAI_zip_1.tmp -> <josm.cache>\mirror_https___josm.openstreetmap.de_josmfile_page_Styles_MapWithAI_zip_1
- 122738.286 W: Unable to delete old backup file <josm.pref>\autosave\MapWithAI_20230708_210105476.osm
- 123038.262 W: Unable to delete old backup file <josm.pref>\autosave\MapWithAI_20230708_210605476.osm

Attachments (2)

validatorNoIssuesYesDialog.zip (323.7 KB ) - added by watmildon 17 months ago.: osm file with changes
EmptyDialog.PNG (54.6 KB ) - added by watmildon 17 months ago.

Download all attachments as: .zip

Change History (15)

by watmildon, 17 months ago

Attachment:	validatorNoIssuesYesDialog.zip added

osm file with changes

by watmildon, 17 months ago

Attachment:	EmptyDialog.PNG added

comment:1 by taylor.smock, 17 months ago

It looks like this is probably a bug whereby informational errors are found, but not shown to the user.

comment:2 by taylor.smock, 17 months ago

Resolution:	→ fixed
Status:	new → closed

In 18775/josm:

Fix #23049: Informational errors cause validator dialog to occur on upload, even if they are not enabled

comment:3 by taylor.smock, 17 months ago

Milestone:	→ 23.07

comment:4 by taylor.smock, 17 months ago

Ticket #23043 has been marked as a duplicate of this ticket.

comment:5 by gaben, 17 months ago

#23046 is also duplicate.

comment:6 by taylor.smock, 17 months ago

Ticket #23046 has been marked as a duplicate of this ticket.

comment:7 by gaben, 17 months ago

Is there a reason to put the severity check in the upload hook validator?

As I checked yesterday, the ValidationTask class may be better place for it as the info level warnings always stored whether it's enabled or not. So that way some memory could be saved.

comment:8 by taylor.smock, 17 months ago

We could probably put it in ValidationTask instead; I just wanted to keep the current behavior for now. I haven't had a chance to go through all of the bug reports that were filed over the weekend, so I don't know (yet) if I need to do a hotfix.

A better fix would probably be to use a ternary statement in ValidationTask, such as boolean keepOther = Boolean.TRUE.equals(beforeUpload ? ValidatorPrefHelper.PREF_OTHER_UPLOAD.get() : ValidatorPrefHelper.PREF_OTHER.get());.

comment:9 by taylor.smock, 17 months ago

In 18776/josm:

See #23049, fix an issue where tests might visit the entire dataset during upload

comment:10 by gaben, 17 months ago

I'm thinking something like this:

src/org/openstreetmap/josm/data/validation/ValidationTask.java

+     *
      * @param tests                     the tests to run
      * @param validatedPrimitives       the collection of primitives to validate.
      * @param formerValidatedPrimitives the last collection of primitives being validates. May be null.
+     * @param formerValidatedPrimitives the last collection of primitives being validated. May be null.
      */
     public ValidationTask(Collection<Test> tests,
                           Collection<OsmPrimitive> validatedPrimitives,
 …
      * @param progressMonitor           the progress monitor to update with test progress
      * @param tests                     the tests to run
      * @param validatedPrimitives       the collection of primitives to validate.
      * @param formerValidatedPrimitives the last collection of primitives being validates. May be null.
+     * @param formerValidatedPrimitives the last collection of primitives being validated. May be null.
      * @param beforeUpload              {@code true} if this is being run prior to upload
      * @since 18752
      */
 …
             test.startTest(getProgressMonitor().createSubTaskMonitor(validatedPrimitives.size(), false));
             test.visit(validatedPrimitives);
             test.endTest();
+            errors.addAll(test.getErrors());
+            // Handle info level warnings
+            if (ValidatorPrefHelper.PREF_OTHER.get() || (ValidatorPrefHelper.PREF_OTHER_UPLOAD.get() && this.beforeUpload)) {
+                errors.addAll(test.getErrors());
+            } else {
+                for (TestError e : test.getErrors()) {
+                    if (e.getSeverity() != Severity.OTHER) {
+                        errors.add(e);
+                    }
+                }
+            }
             if (this.testConsumer != null) {
                 this.testConsumer.accept(this, test);
+            }

Haven't checked the tests though.

comment:11 by gaben, 17 months ago

Ooops okay, you were faster :) Let me check what's inside r18776.

comment:12 by taylor.smock, 17 months ago

Your patch will probably work.

One quick word though, if you were looking to decrease the memory/gc requirements, something like

[...]
} else {
    Collection<TestError> tErrors = new ArrayList<>(test.getErrors());
    tErrors.removeIf(error -> error.getSeverity() != Severity.OTHER);
    errors.addAll(tErrors);
}

would be better.

This avoids array allocations in the errors list (it is very inefficient).

Last edited 17 months ago by taylor.smock (previous) (diff)

comment:13 by gaben, 17 months ago

Actually I borrowed the code from a pre r18752 version of JOSM with a minor modification.

Btw where is behind the scene array allocation? Here is the bytecode ("mine" - borrowed):

   L22
    LINENUMBER 143 L22
   FRAME SAME
    ALOAD 3
    INVOKEVIRTUAL org/openstreetmap/josm/data/validation/Test.getErrors ()Ljava/util/List;
    INVOKEINTERFACE java/util/List.iterator ()Ljava/util/Iterator; (itf)
    ASTORE 4
   L24
   FRAME APPEND [java/util/Iterator]
    ALOAD 4
    INVOKEINTERFACE java/util/Iterator.hasNext ()Z (itf)
    IFEQ L23
    ALOAD 4
    INVOKEINTERFACE java/util/Iterator.next ()Ljava/lang/Object; (itf)
    CHECKCAST org/openstreetmap/josm/data/validation/TestError
    ASTORE 5
   L25
    LINENUMBER 144 L25
    ALOAD 5
    INVOKEVIRTUAL org/openstreetmap/josm/data/validation/TestError.getSeverity ()Lorg/openstreetmap/josm/data/validation/Severity;
    GETSTATIC org/openstreetmap/josm/data/validation/Severity.OTHER : Lorg/openstreetmap/josm/data/validation/Severity;
    IF_ACMPEQ L26
   L27
    LINENUMBER 145 L27
    ALOAD 0
    GETFIELD org/openstreetmap/josm/data/validation/ValidationTask.errors : Ljava/util/List;
    ALOAD 5
    INVOKEINTERFACE java/util/List.add (Ljava/lang/Object;)Z (itf)
    POP
   L26
    LINENUMBER 147 L26
   FRAME SAME
    GOTO L24

vs (suggested):

   L22
    LINENUMBER 143 L22
   FRAME SAME
    NEW java/util/ArrayList
    DUP
    ALOAD 3
    INVOKEVIRTUAL org/openstreetmap/josm/data/validation/Test.getErrors ()Ljava/util/List;
    INVOKESPECIAL java/util/ArrayList.<init> (Ljava/util/Collection;)V
    ASTORE 4
   L24
    LINENUMBER 144 L24
    ALOAD 4
    INVOKEDYNAMIC test()Ljava/util/function/Predicate; [
      // handle kind 0x6 : INVOKESTATIC
      java/lang/invoke/LambdaMetafactory.metafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
      // arguments:
      (Ljava/lang/Object;)Z, 
      // handle kind 0x6 : INVOKESTATIC
      org/openstreetmap/josm/data/validation/ValidationTask.lambda$realRun$2(Lorg/openstreetmap/josm/data/validation/TestError;)Z, 
      (Lorg/openstreetmap/josm/data/validation/TestError;)Z
    ]
    INVOKEINTERFACE java/util/Collection.removeIf (Ljava/util/function/Predicate;)Z (itf)
    POP
   L25
    LINENUMBER 145 L25
    ALOAD 0
    GETFIELD org/openstreetmap/josm/data/validation/ValidationTask.errors : Ljava/util/List;
    ALOAD 4
    INVOKEINTERFACE java/util/List.addAll (Ljava/util/Collection;)Z (itf)
    POP

I'm just curious, hoping that I'll learn something along the way.

Last edited 17 months ago by gaben (previous) (diff)

Modify Ticket

Change Properties

Summary:
Type:		Priority:
Milestone:		Component:
Version:		Keywords:
Cc:	Set your email in Preferences

Action

leave as closed The owner will remain team.

change resolution as The resolution will be set.

reopen The resolution will be deleted. Next status will be 'reopened'.

Add Comment

Your email or username:

E-mail address and name can be saved in the Preferences .

You may use WikiFormatting here.

Attachments ↑ Description ↑

Note: See TracTickets for help on using tickets.

Download in other formats:

Context Navigation

#23049 closed defect (fixed)

Validator has no errors but leaves validator dialog up, should move to upload dialog

Description

What steps will reproduce the problem?

What is the expected result?

What happens instead?

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

Attachments (2)

Change History (15)

by watmildon, 17 months ago

by watmildon, 17 months ago

comment:1 by taylor.smock, 17 months ago

comment:2 by taylor.smock, 17 months ago

comment:3 by taylor.smock, 17 months ago

comment:4 by taylor.smock, 17 months ago

comment:5 by gaben, 17 months ago

comment:6 by taylor.smock, 17 months ago

comment:7 by gaben, 17 months ago

comment:8 by taylor.smock, 17 months ago

comment:9 by taylor.smock, 17 months ago

comment:10 by gaben, 17 months ago

src/org/openstreetmap/josm/data/validation/ValidationTask.java

comment:11 by gaben, 17 months ago

comment:12 by taylor.smock, 17 months ago

comment:13 by gaben, 17 months ago

Modify Ticket

Add Comment

by anonymous

Download in other formats: