Modify

Opened 14 months ago

Last modified 9 months ago

#19956 reopened defect

[RFC] [Patch] Double check if error still exists before executing autofix

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone:
Component: Core validator Version:
Keywords: template_report Cc:

Description

What steps will reproduce the problem?

  1. load attached file with duplicate relations
  2. run validator, should produce error "Duplicated relations (1)"
  3. double click on the error entry so that both relations are selected
  4. select one of the two relations
  5. change name from "same" to "other"
  6. click on Fix button without running validator again

What is the expected result?

No further change, error was fixed manually

What happens instead?

One relation is deleted

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

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-10-03 13:42:38 +0200 (Sat, 03 Oct 2020)
Build-Date:2020-10-04 01:30:47
Revision:17084
Relative:URL: ^/trunk

Identification: JOSM/1.5 (17084 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 2004 (19041)
Memory Usage: 1144 MB / 3641 MB (661 MB allocated, but free)
Java version: 1.8.0_221-b11, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920x1080 (scaling 1.0x1.0)
Maximum Screen Size: 1920x1080
Best cursor sizes: 16x16 -> 32x32, 32x32 -> 32x32
VM arguments: [-XX:StartFlightRecording=name=MyRecording2,settings=d:\dbg\gerd.jfc, -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true,dumponexitpath=e:\ld\perf_20201017_154712.jfr]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35579)
+ PolygonCutOut (v0.7)
+ apache-commons (35524)
+ buildings_tools (35579)
+ continuosDownload (91)
+ ejml (35313)
+ geotools (35169)
+ jaxb (35092)
+ jts (35122)
+ merge-overlap (35583)
+ o5m (35248)
+ opendata (35513)
+ pbf (35446)
+ poly (35248)
+ reverter (35579)
+ undelete (35521)
+ utilsplugin2 (35580)

Attachments (3)

dup-rels.osm (551 bytes) - added by GerdP 14 months ago.
19956.patch (7.3 KB) - added by GerdP 14 months ago.
19956.2.patch (3.7 KB) - added by GerdP 14 months ago.
implement double check in class TestError, should work for all java tests, but cannot double check fixable mapcss errors

Download all attachments as: .zip

Change History (18)

Changed 14 months ago by GerdP

Attachment: dup-rels.osm added

comment:1 Changed 14 months ago by GerdP

Milestone: 20.10
Owner: changed from team to GerdP
Status: newassigned
Summary: Duplicate Relation fix removes already fixed relatiosDuplicate Relation fix removes already fixed relations

comment:2 Changed 14 months ago by GerdP

Similar problem with other tests like DuplicateWay. Both don't re-evaluate the problem with the primitives in their current state.

Changed 14 months ago by GerdP

Attachment: 19956.patch added

comment:3 Changed 14 months ago by GerdP

Patch contains a few minor changes, this is the important part:

        // see #19956 check again
        Test test2 = new DuplicateRelation();
        test2.startTest(NullProgressMonitor.INSTANCE);
        toFix.forEach(test2::visit);
        test2.endTest();
        boolean errorStillExists = false;
        for (TestError e : test2.getErrors()) {
            if (e.getCode() == testError.getCode() && e.getPrimitives().size() >= 2) {
                // refresh primitives, might be fewer than before
                toFix = e.primitives(Relation.class).collect(Collectors.toSet());
                errorStillExists = true;
                break;
            }
        }
        if (!errorStillExists)
            return null;

I don't like the idea to implement such a double check in each test that offers an autofix (11 java tests in core) and I think the problem also exists in the mapcss tests.

I'll try again to code this in double check in ValidatorDialog.

Changed 14 months ago by GerdP

Attachment: 19956.2.patch added

implement double check in class TestError, should work for all java tests, but cannot double check fixable mapcss errors

comment:4 Changed 14 months ago by GerdP

Summary: Duplicate Relation fix removes already fixed relations[RFC] [Patch] Double check if error still exists before executing autofix

comment:5 Changed 14 months ago by GerdP

Resolution: fixed
Status: assignedclosed

In 17252/josm:

fix #19956 Double check if error still exists before executing autofix

  • let ValidatorDialog do this so that different manual changes or autofixes don't disturb each other

comment:6 Changed 13 months ago by simon04

Resolution: fixed
Status: closedreopened

This has caused a few compilation/error-prone warnings:

    [javac] Compiling 7 source files to /home/simon/src/josm.svngit/build
    [javac] warning: [options] bootstrap class path not set in conjunction with -source 8
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/data/validation/TestError.java:562: warning: [deprecation] newInstance() in Class has been deprecated
    [javac]             tester2 = getTester().getClass().newInstance();
    [javac]                                             ^
    [javac]   where T is a type-variable:
    [javac]     T extends Object declared in class Class
    [javac] /home/simon/src/josm.svngit/src/org/openstreetmap/josm/data/validation/TestError.java:562: warning: [ClassNewInstance] Class.newInstance() bypasses exception checking; prefer getDeclaredConstructor().newInstance()
    [javac]             tester2 = getTester().getClass().newInstance();
    [javac]                                                         ^
    [javac]     (see https://errorprone.info/bugpattern/ClassNewInstance)
    [javac]   Did you mean 'tester2 = getTester().getClass().getDeclaredConstructor().newInstance();'?
    [javac] 3 warnings

Have you noticed any performance implications when fixing a large number of primitives (say, 100 opening_hours warnings)?

comment:7 Changed 13 months ago by GerdP

This has caused a few compilation/error-prone warnings:

Yes, found that yesterday but I found no better solution with Java 8. The suggested change doesn't work with JDK 8 (1.8.0_201)

Have you noticed any performance implications when fixing a large number of primitives (say, 100 opening_hours warnings)?

I didn't try but I would not be surprised. Do you have test data for me? If opening_hours test does the double check on its own - or doesn't have to - we could introudce a flag in class Test. Or I might limit the double check to a fixed list of tests.

comment:8 in reply to:  7 Changed 13 months ago by GerdP

Replying to GerdP:

This has caused a few compilation/error-prone warnings:

Yes, found that yesterday but I found no better solution with Java 8. The suggested change doesn't work with JDK 8 (1.8.0_201)

Forget that. Seems I was mislead by Eclipse. I just have to add more code to the catch blocks.

comment:9 Changed 13 months ago by GerdP

Seems I have to exclude OpeningHoursTest, it produces EDT violations with this code.

comment:10 Changed 13 months ago by GerdP

In 17261/josm:

see #19956 Double check if error still exists before executing autofix

  • revert r17252. There are more tests that don't work with this. Have to double check my patch first ;)

comment:11 Changed 13 months ago by GerdP

Milestone: 20.1020.11

comment:12 Changed 13 months ago by Don-vip

Milestone: 20.1120.12

Milestone renamed

comment:13 Changed 11 months ago by GerdP

Milestone: 20.1221.01

comment:14 Changed 10 months ago by stoecker

Milestone: 21.0121.02

Milestone renamed

comment:15 Changed 9 months ago by Don-vip

Milestone: 21.02

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain GerdP.
as The resolution will be set.
to The owner will be changed from GerdP to the specified user.
The owner will change to GerdP
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


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

 
Note: See TracTickets for help on using tickets.