Modify

Opened 23 months ago

Closed 23 months ago

Last modified 23 months ago

#22073 closed defect (fixed)

mapcss regexp broken

Reported by: Klumbumbus Owned by: team
Priority: major Milestone: 22.05
Component: Core Version:
Keywords: template_report regression Cc: simon04

Description

What steps will reproduce the problem?

  1. add alt_name=bla to a way
  2. validate

What is the expected result?

warning "alternative name without" name

What happens instead?

no warning

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

This is a regression of r17593 (r17592 works for me, r17610 doesn't)
It seems the $ in [/_name$/] doesn't work.
This rule in https://josm.openstreetmap.de/browser/josm/trunk/resources/data/validator/combinations.mapcss#L541 doesn't work for me:

*[/_name$/][!name][!old_name][!loc_name][!reg_name][!uic_name][!artist_name][!lock_name][!"osak:municipality_name"][!"osak:street_name"][noname!=yes] {
  throwWarning: tr("alternative name without {0}", "{1.key}");
  group: tr("missing tag");
}
Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2022-05-12 16:24:18 +0200 (Thu, 12 May 2022)
Revision:18445
Build-Date:2022-05-13 01:30:54
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18445 de) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19044)
Memory Usage: 254 MB / 4058 MB (125 MB allocated, but free)
Java version: 11.0.15+10-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1680×1050 (scaling 1.00×1.00)
Maximum Screen Size: 1680×1050
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: de_DE
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Dicedtea-web.bin.location=%UserProfile%\AppData\Local\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]
Dataset consistency test: No problems found

Plugins:
+ DirectUpload (35951)
+ HouseNumberTaggingTool (35951)
+ OpeningHoursEditor (35924)
+ RoadSigns (35935)
+ SimplifyArea (35893)
+ apache-commons (35924)
+ buildings_tools (35951)
+ editgpx (35931)
+ imagery-xml-bounds (35893)
+ imagery_offset_db (35893)
+ measurement (35893)
+ photo_geotagging (35933)
+ photoadjust (35893)
+ reltoolbox (35893)
+ reverter (35893)
+ tageditor (35893)
+ tagging-preset-tester (35893)
+ terracer (35893)
+ turnlanes-tagging (v0.0.5)
+ turnrestrictions (35893)
+ undelete (35893)
+ utilsplugin2 (35951)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/COVID-19&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&zip=1
+ %UserProfile%\Documents\OSM\TestNew\newpresets.xml
+ https://josm.openstreetmap.de/josmfile?page=Presets/NewTags&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/Xmas&zip=1
+ %UserProfile%\Documents\OSM\josm\core\resources\data\defaultpresets.xml

Map paint styles:
+ %UserProfile%\Documents\OSM\josm\core\resources\styles\standard\elemstyles.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/HiDPISupport&zip=1
- %UserProfile%\Documents\OSM\josm\core\resources\styles\standard\potlatch2.mapcss
+ %UserProfile%\Documents\OSM\TestNew\newicons.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Modified&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/sac_scale&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ShowID&zip=1
- %UserProfile%\Documents\OSM\eigene styles\PriorityRoad\PriorityRoad_1.0.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/LayerChecker&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ParkingLanes&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Sidewalks&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Cycleways&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/LitObjects&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Lit&style&zip=1
- %UserProfile%\Documents\OSM\eigene styles\SpecificBuildingValues\SpecificBuildingValues.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Incline&zip=1
- %UserProfile%\Documents\OSM\eigene styles\area-symbol.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/hazmat&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Suburb&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Postcode&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/RecyclingMaterials&zip=1
- %UserProfile%\Documents\OSM\eigene styles\maxspeed\maxspeed_2.9_01 basierend auf 2.7_02 Zahlen.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Bench&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/ColourTag&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Admin_Boundaries&zip=1
- %UserProfile%\Documents\OSM\eigene styles\yes-no-unset\YesNoUnset_1.0.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/FixmeAndNote&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Potlatch2&zip=1

Validator rules:
- https://raw.githubusercontent.com/<user.name>n-a-bauer/josm-validators/master/mtb.validator.mapcss
+ %UserProfile%\Documents\OSM\TestNew\new.validator.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Rules/GermanySpecific&zip=1
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\numeric.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\deprecated.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\combinations.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\geometry.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\unnecessary.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\highway.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\religion.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\multiple.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\addresses.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\wikipedia.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\relation.mapcss
+ %UserProfile%\Documents\OSM\josm\core\resources\data\validator\territories.mapcss

Last errors/warnings:
- 00032.995 W: Unable to use English input method
- 00125.459 W: Unable to use English input method
- 00135.970 W: Unable to use English input method
- 00138.007 W: Unable to use English input method
- 00437.829 W: Unable to use English input method
- 00460.415 W: Unable to use English input method
- 00466.760 W: Unable to use English input method
- 00582.598 W: Unable to use English input method
- 03280.300 W: Unable to use English input method

Attachments (0)

Change History (12)

comment:1 by taylor.smock, 23 months ago

We should probably go through and try to add assertMatch/assertNoMatch tests for everything we have. It looks like we are testing those in source:trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java@HEAD:215-228#L215.

Anyway, I wonder which branch is actually taken in r17592. It should be taking the 3rd branch.

comment:2 by Klumbumbus, 23 months ago

The strange thing is, when I add

  assertMatch:   "way alt_name=Foo";
  assertNoMatch: "way alt_name=Foo name=Bar";

I get no warning in the console. But this might be also a shortcoming of the assert tests.

in reply to:  2 comment:3 by taylor.smock, 23 months ago

Replying to Klumbumbus:

I get no warning in the console. But this might be also a shortcoming of the assert tests.

If you are talking about the console when you run JOSM (e.g. java -jar josm.jar), you do need to have the advanced option validator.check_assert_local_rules set to true (see wiki:Help/Validator/MapCSSTagChecker#Syntax -- sixth bullet point).

If you are talking about the unit test, then we have a problem. Which we can probably fix by setting the aforementioned configuration setting in the unit test.

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

comment:4 by Klumbumbus, 23 months ago

In 18450/josm:

see #22073 - Add asserts

comment:5 by Klumbumbus, 23 months ago

I mean the java console when I run JOSM, I have validator.check_assert_local_rules set to true.
Can you reproduce the issue(s) described in this ticket?

in reply to:  5 comment:6 by taylor.smock, 23 months ago

Replying to Klumbumbus:

I mean the java console when I run JOSM, I have validator.check_assert_local_rules set to true.
Can you reproduce the issue(s) described in this ticket?

I'll check. We might not run it on internal validator rules, but that would be surprising behavior (maybe this got optimized away due to having the unit test?). But I was able to reproduce the original problem with a way highway=road alt_name=FooBar.

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

comment:7 by taylor.smock, 23 months ago

It looks like we only check local files, excluding those in jar files with validator.check_assert_local_rules (see source:trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java#L266 and source:trunk/src/org/openstreetmap/josm/tools/Utils.java#L1184 (!url.startsWith("resource://"))).

The test I linked to previously seems to be the only way for us to test JOSM internal mapcss validators. Unless we change the code.

I've just stepped through the assertMatch/assertNoMatch code with the patch you applied (in that unit test), and it is giving us the expected output (i.e., everything works). I've now stepped through the same code with a debugger, with JOSM starting, and the assertions are failing (I overrode the check for non-local files).

Now to debug what changes between unit tests and the application.

comment:8 by taylor.smock, 23 months ago

I've found the root of the problem. We are adding the rule as _name to the index, and then when we are iterating through the rules, we look for alt_name in the index. Solution: Don't add it to the index and instead put it in the remaining set. Also, I think I probably messed something up somewhere in comment:7 (when stepping through the code again, the asserts passed in the unit test).

I think we are probably going to want to have an EnumSet of valid KeyMatchTypes. Probably EQ, TRUE, FALSE.

  • src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRuleIndex.java

    diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRuleIndex.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRuleIndex.java
    index e3e17b21ec..160e24da3b 100644
    a b package org.openstreetmap.josm.gui.mappaint.mapcss;  
    44import java.util.ArrayList;
    55import java.util.BitSet;
    66import java.util.Collections;
     7import java.util.EnumSet;
    78import java.util.HashMap;
    89import java.util.Iterator;
    910import java.util.List;
    public class MapCSSRuleIndex {  
    120121        }
    121122    }
    122123
     124    /** Valid key types for indexing (see {@link ConditionFactory.KeyMatchType}) */
     125    private static final EnumSet<ConditionFactory.KeyMatchType> VALID_INDEX_KEY_TYPES = EnumSet.of(
     126            ConditionFactory.KeyMatchType.EQ, ConditionFactory.KeyMatchType.TRUE, ConditionFactory.KeyMatchType.FALSE);
     127
    123128    /**
    124129     * All rules this index is for. Once this index is built, this list is sorted.
    125130     */
    public class MapCSSRuleIndex {  
    186191    private static String findAnyRequiredKey(List<Condition> conds) {
    187192        String key = null;
    188193        for (Condition c : conds) {
    189             if (c instanceof KeyCondition) {
     194            if (c instanceof KeyCondition && VALID_INDEX_KEY_TYPES.contains(((KeyCondition) c).matchType)) {
    190195                KeyCondition keyCondition = (KeyCondition) c;
    191196                if (!keyCondition.negateResult) {
    192197                    key = keyCondition.label;

Now to writing a unit test for this.

comment:9 by taylor.smock, 23 months ago

Resolution: fixed
Status: newclosed

In 18451/josm:

Fix #22073: Optimized regexes (string starts with, ends with, contains) do not work

This was caused by taking a portion of a regex (for example, _name from
/_name$/), and using that to match against keys. So alt_name would not
match, but _name would. All regex patterns should now be added to the
remaining field in MapCSSRuleIndex, and there now exists a test to ensure
we do not forget to add new non-regex types to the valid key type list.

comment:10 by Klumbumbus, 23 months ago

Milestone: 22.05

Thanks for fixing!

comment:11 by taylor.smock, 23 months ago

I'd say no problem, but it was a PITA to debug.

But thank you for merging various tag patches (I presume this is how you found this bug). I've been meaning to look at those and merge/give feedback, as needed. It doesn't help that I've tried to follow the history of tag changes/additions when a request to make a change is done (most recently #22069) just so I'm not merging something that was quietly changed yesterday.

in reply to:  11 comment:12 by Klumbumbus, 23 months ago

Replying to taylor.smock:

(I presume this is how you found this bug).

Yes, #22033 and first I thought I made some kind of dumb mistake, because the test seemed to be broken for 14 months, but #22033 was created only 4 weeks ago...

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.