Modify

Opened 3 months ago

Closed 3 months ago

#17358 closed defect (fixed)

mapcss regular expression matches for key

Reported by: maxerickson@… Owned by: team
Priority: normal Milestone: 19.02
Component: Core Version:
Keywords: regression Cc: michael2402

Description (last modified by Klumbumbus)

Between 14460 and 14469, regular expressions for keys in the validator stopped working. Given simple data like (the only part used for the example tests is "name=Test St"):

<?xml version='1.0' encoding='UTF-8'?>
<osm version='0.6' generator='JOSM'>
  <node id='-160658' action='modify' visible='true' lat='25.49069474072' lon='-80.4196570082' />
  <node id='-160660' action='modify' visible='true' lat='25.4906898985' lon='-80.41959934071' />
  <way id='-160661' action='modify' visible='true'>
    <nd ref='-160658' />
    <nd ref='-160660' />
    <tag k='highway' v='residential' />
    <tag k='name' v='Test St' />
  </way>
</osm>

A selector like [/^name/] has stopped working:

*[name=~/Test/]{
  throwWarning: "Value match";
}

*[/name/=~/Test/]{
  throwWarning: "Simple key value match.";
}

*[/^name/=~/Test/]{
  throwWarning: "Key value match";
}

In 14460, the "Key value match" test warns, in 14469 it does not. The other 2 work in both.

Probably related to the work on #17021.

Attachments (1)

17358.patch (874 bytes) - added by GerdP 3 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 months ago by GerdP

Thanks for the detailed report.
Yes, it seems that the routines which create the index don't "understand" the meaning of the last rule. If I got that right the problem is the same when such a selector is used for rendering.
I don't yet know where this has to be fixed and I am currently too sick to dig deeper into this :(

Last edited 3 months ago by Don-vip (previous) (diff)

comment:2 Changed 3 months ago by Klumbumbus

Description: modified (diff)

comment:3 Changed 3 months ago by Don-vip

Keywords: regression added
Milestone: 19.02

comment:4 in reply to:  1 Changed 3 months ago by Don-vip

Replying to GerdP:

I am currently too sick to dig deeper into this :(

Take some well deserved rest :) I just got out of my own winter sickness, I'm looking into it.

comment:5 Changed 3 months ago by Don-vip

In 14796/josm:

see #17358 - add non-regression test (disabled for now)

comment:6 Changed 3 months ago by Don-vip

Cc: michael2402 added

I have added this non-regression test:

    @Test
    @Ignore("not fixed yet")
    public void testTicket17358() throws ParseException {
        final Collection<TestError> errors = buildTagChecker(
                "*[/^name/=~/Test/]{" +
                "  throwWarning: \"Key value match\";" +
                "}").getErrorsForPrimitive(OsmUtils.createPrimitive("way name=Test St"), false);
        assertEquals(1, errors.size());
    }

But I don't know how to fix it yet. Michael, it seems MapCSSRuleIndex does not support regular expressions?

Changed 3 months ago by GerdP

Attachment: 17358.patch added

comment:7 Changed 3 months ago by michael2402

The index can only optimize static keys and static key/value pairs. Other rules should be put to the remaining set.

Dirty fix: add a !(c instanceof RegexpKeyValueRegexpCondition) test in MapCSSStyleSource.java Line 331

Nicer fix: Add a requiresExactKeyMatch() method to KeyValueCondition:

// KeyValueCondition:
public boolean requiresExactKeyMatch() { return !Op.NEGATED_OPS.contains(op); }
// RegexpKeyValueRegexpCondition
public boolean requiresExactKeyMatch() { return false; }
// MapCSSStyleSource.java Line 331
if (keyValueCondition.requiresExactKeyMatch()) {

(Sorry, I'm on my laptop and don't have eclipse/ant/svn installed, otherwise I would have fixed it myself.)

Last edited 3 months ago by Don-vip (previous) (diff)

comment:8 Changed 3 months ago by GerdP

The attached patch should work, but Question is what effects this has. Maybe existing correct rules never matched because of this bug, and when they now do we might see strange effects.

comment:9 Changed 3 months ago by michael2402

@GerdP: Your patch would fix the issue, but also exclude the regexp value-only matches. This will reduce performance for many styles.

I would not worry about breaking styles by this "fix". So far, no styles seem to use regexp key matches, because they seem to never have worked with the style index.

comment:10 Changed 3 months ago by GerdP

OK, and yes, your solutions are both better than mine.

comment:11 Changed 3 months ago by GerdP

Hm, we have quite a lot of validator rules which use regex for keys, so I think we should see changes. Remember that we did not use the index before r14460.

comment:12 Changed 3 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 14801/josm:

fix #17358 - mapcss regular expression matches for key (patch by michael2402, regression from #17021)

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.

Add Comment


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

 
Note: See TracTickets for help on using tickets.