Changeset 18866 in josm

Timestamp:

2023-10-12T17:42:13+02:00 (7 months ago)

Author:

taylor.smock

Message:

Fix #23196: DataIntegrityProblemException: Primitive must be part of the dataset

It turns out that FilterModel doesn't care whether a primitive's referrers are
in the dataset or not.

This additionally adds a non-regression test and modifies WindowMocker to cover
some more headless exceptions. This additionally lets us stop mocking
ExtendedDialog#setupDialog.

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/osm/FilterModel.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/tagging/presets/TaggingPreset.java (modified) (3 diffs)
• test/unit/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditorTest.java (modified) (6 diffs)
• test/unit/org/openstreetmap/josm/testutils/mockers/ExtendedDialogMocker.java (modified) (1 diff)
• test/unit/org/openstreetmap/josm/testutils/mockers/WindowMocker.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/data/osm/FilterModel.java

@@ -440 +440 @@
             }
 
-            for (OsmPrimitive ref: p.getReferrers()) {
+            for (OsmPrimitive ref: p.getReferrers(true)) {
                 stack.push(ref);
             }

trunk/src/org/openstreetmap/josm/gui/tagging/presets/TaggingPreset.java

@@ -85 +85 @@
  * read in all predefined presets, either shipped with JOSM or that are
  * in the config directory.
- *
+ * <p>
  * It is also able to construct dialogs out of preset definitions.
  * @since 294
@@ -405 +405 @@
             /**
              * This hack allows the items to have their own orientation.
-             *
+             * <p>
              * The problem is that
              * {@link org.openstreetmap.josm.gui.ExtendedDialog#showDialog ExtendedDialog} calls
              * {@code applyComponentOrientation} very late in the dialog construction process thus
              * overwriting the orientation the components have chosen for themselves.
-             *
+             * <p>
              * This stops the propagation of {@code applyComponentOrientation}, thus all
              * {@code TaggingPresetItem}s may (and have to) set their own orientation.
@@ -446 +446 @@
 
         if (selected.size() == 1 && Boolean.TRUE.equals(USE_VALIDATOR.get())) {
-            // Fail early -- validateAsync requires the primitive(s) to be part of a dataset. Failing later in validateAsync ''does not'' give us
-            // a usable stack trace. See #21829 for details.
-            selected.forEach(OsmPrimitive::checkDataset);
             itemGuiSupport.addListener((source, key, newValue) ->
                     TaggingPresetValidation.validateAsync(selected.iterator().next(), validationLabel, getChangedTags()));

trunk/test/unit/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditorTest.java

@@ -9 +9 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.fail;
 import static org.openstreetmap.josm.tools.I18n.tr;
 
+import java.awt.Component;
 import java.awt.Container;
+import java.awt.event.MouseEvent;
+import java.awt.event.MouseListener;
 import java.awt.event.WindowEvent;
+import java.util.ArrayDeque;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
 
 import javax.swing.Action;
 import javax.swing.JButton;
+import javax.swing.JLabel;
 import javax.swing.JOptionPane;
 import javax.swing.JPanel;
-
-import mockit.Invocation;
-import mockit.Mock;
-import mockit.MockUp;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -36 +40 @@
 import org.openstreetmap.josm.data.osm.RelationMember;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.gui.ExtendedDialog;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.gui.SideButton;
@@ -45 +50 @@
 import org.openstreetmap.josm.gui.tagging.TagEditorPanel;
 import org.openstreetmap.josm.gui.tagging.ac.AutoCompletingTextField;
+import org.openstreetmap.josm.gui.tagging.presets.TaggingPreset;
 import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetHandler;
+import org.openstreetmap.josm.gui.tagging.presets.TaggingPresetLabel;
+import org.openstreetmap.josm.spi.preferences.Config;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
 import org.openstreetmap.josm.testutils.annotations.Main;
 import org.openstreetmap.josm.testutils.annotations.Projection;
+import org.openstreetmap.josm.testutils.annotations.TaggingPresets;
+import org.openstreetmap.josm.testutils.mockers.ExtendedDialogMocker;
 import org.openstreetmap.josm.testutils.mockers.JOptionPaneSimpleMocker;
 import org.openstreetmap.josm.testutils.mockers.WindowMocker;
+import org.openstreetmap.josm.tools.JosmRuntimeException;
+
+import mockit.Invocation;
+import mockit.Mock;
+import mockit.MockUp;
 
 /**
@@ -105 +120 @@
         new PasteMembersActionMock();
         new WindowMocker();
+    }
+
+    private static AtomicReference<RelationEditor> setupGuiMocks() {
+        AtomicReference<RelationEditor> editorReference = new AtomicReference<>();
+        new MockUp<RelationEditor>() {
+            @Mock public RelationEditor getEditor(Invocation invocation, OsmDataLayer layer, Relation r,
+                                                  Collection<RelationMember> selectedMembers) {
+                editorReference.set(invocation.proceed(layer, r, selectedMembers));
+                return editorReference.get();
+            }
+        };
+        // We want to go through the `setVisible` code, just in case. So we have to mock the window location
+        new MockUp<GenericRelationEditor>() {
+            @Mock public void setVisible(boolean visible) {
+                // Do nothing. Ideally, we would just mock the awt methods called, but that would take a lot of mocking.
+            }
+        };
+        return editorReference;
     }
 
@@ -185 +218 @@
     void testNonRegression23116() {
         // Setup the mocks
-        final AtomicReference<RelationEditor> editorReference = new AtomicReference<>();
-        new MockUp<RelationEditor>() {
-            @Mock public RelationEditor getEditor(Invocation invocation, OsmDataLayer layer, Relation r,
-                    Collection<RelationMember> selectedMembers) {
-                editorReference.set(invocation.proceed(layer, r, selectedMembers));
-                return editorReference.get();
-            }
-        };
-        // We want to go through the `setVisible` code, just in case. So we have to mock the window location
-        new MockUp<GenericRelationEditor>() {
-            @Mock public void setVisible(boolean visible) {
-                // Do nothing. Ideally, we would just mock the awt methods called, but that would take a lot of mocking.
-            }
-        };
+        final AtomicReference<RelationEditor> editorReference = setupGuiMocks();
         // Set up the data
         final DataSet dataSet = new DataSet();
@@ -234 +254 @@
     }
 
+    /**
+     * Ensure that users can create new relations with a preset available and open the preset.
+     * See {@link TaggingPreset#showAndApply} for where a relation may exist without a dataset.
+     */
+    @BasicPreferences
+    @TaggingPresets
+    @Test
+    void testNonRegression23196() {
+        // This happens when the preset validator is enabled (Preferences -> `Tagging Presets` -> `Run data validator on user input`)
+        Config.getPref().putBoolean("taggingpreset.validator", true);
+        // Setup the mocks
+        final AtomicReference<RelationEditor> editorReference = setupGuiMocks();
+        new ExtendedDialogMocker(Collections.singletonMap("Change 1 object", "Apply Preset")) {
+            @Override
+            protected String getString(ExtendedDialog instance) {
+                return instance.getTitle();
+            }
+        };
+        // Set up the data
+        final DataSet dataSet = new DataSet();
+        final OsmDataLayer layer = new OsmDataLayer(dataSet, "GenericRelationEditorTest.testNonRegression23196", null);
+        MainApplication.getLayerManager().addLayer(layer);
+        dataSet.addPrimitive(TestUtils.newNode(""));
+        dataSet.setSelected(dataSet.allPrimitives());
+        try {
+            RelationEditor.getEditor(layer, TestUtils.newRelation("type=multipolygon"),
+                    dataSet.getSelected().stream().map(p -> new RelationMember("", p)).collect(Collectors.toList()));
+            final GenericRelationEditor editor = assertInstanceOf(GenericRelationEditor.class, editorReference.get());
+            TaggingPresetLabel label = getComponentByNameOrText(TaggingPresetLabel.class, editor, "Relations/Multipolygon …");
+            final MouseEvent mouseEvent = new MouseEvent(label, 0, 0, 0, 0, 0, 0, false);
+            for (MouseListener listener : label.getMouseListeners()) {
+                assertDoesNotThrow(() -> listener.mouseClicked(mouseEvent));
+            }
+        } finally {
+            // This avoids an issue with the cleanup code and the mocks for this test
+            if (editorReference.get() != null) {
+                RelationDialogManager.getRelationDialogManager().windowClosed(new WindowEvent(editorReference.get(), 0));
+            }
+        }
+    }
+
+    private static <T extends Component> T getComponentByNameOrText(Class<T> clazz, Container parent, String name) {
+        final ArrayDeque<Component> componentDeque = new ArrayDeque<>(Collections.singletonList(parent));
+        while (!componentDeque.isEmpty()) {
+            final Component current = componentDeque.pop();
+            if (current instanceof Container) {
+                componentDeque.addAll(Arrays.asList(((Container) current).getComponents()));
+            }
+            if (clazz.isInstance(current)) {
+                T component = clazz.cast(current);
+                if (name.equals(component.getName())) {
+                    return component;
+                } else if (component instanceof JLabel && name.equals(((JLabel) component).getText())) {
+                    return component;
+                }
+            }
+        }
+        fail("Component with name " + name + " not found");
+        throw new JosmRuntimeException("This should never happen due to the fail line");
+    }
+
     @SuppressWarnings("unchecked")
     private static <T extends Container> T getComponent(Container parent, int... tree) {

trunk/test/unit/org/openstreetmap/josm/testutils/mockers/ExtendedDialogMocker.java

@@ -171 +171 @@
         }
         return resultField;
-    }
-
-    @Mock
-    private void setupDialog(final Invocation invocation) {
-        if (!GraphicsEnvironment.isHeadless()) {
-            invocation.proceed();
-        }
-        // else do nothing - WindowMocker-ed Windows doesn't work well enough for some of the
-        // component constructions
     }
 

trunk/test/unit/org/openstreetmap/josm/testutils/mockers/WindowMocker.java

@@ -2 +2 @@
 package org.openstreetmap.josm.testutils.mockers;
 
+import java.awt.Component;
 import java.awt.Frame;
 import java.awt.GraphicsConfiguration;
@@ -37 +38 @@
     private void $init(final Invocation invocation, final Window window, final GraphicsConfiguration gc) {
     }
+
+    @Mock
+    public void pack() {
+    }
+
+    @Mock
+    public void setLocationRelativeTo(final Component c) {
+    }
 }