Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#9844 closed defect (fixed)

[PATCH] mysterious validator message - Member for role '<empty>' does not match 'highway=path || highway=footway || highway=track' (1)

Reported by: mkoniecz Owned by: team
Priority: normal Milestone: 14.12
Component: Core validator Version:
Keywords: route relation empty role Cc: Klumbumbus

Description

osmwww:browse/way/163267076

It is unclear which relation is affected, this way belongs to two different ones.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2014-03-18 02:36:30
Last Changed Author: Don-vip
Revision: 6910
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2014-03-18 00:57:31 +0100 (Tue, 18 Mar 2014)
Last Changed Rev: 6910

Identification: JOSM/1.5 (6910 en) Windows 7 32-Bit
Memory Usage: 63 MB / 247 MB (17 MB allocated, but free)
Java version: 1.7.0_51, Oracle Corporation, Java HotSpot(TM) Client VM
Dataset consistency test: No problems found

Plugin: OpeningHoursEditor (30235)
Plugin: RoadSigns (30320)
Plugin: buildings_tools (30277)
Plugin: continuosDownload (28565)
Plugin: lakewalker (30277)
Plugin: notes (v0.9.2)
Plugin: wikipedia (30277)

Attachments (3)

RelationChecker.java.patch (12.2 KB) - added by wiktorn 5 years ago.
Patch for defect
RelationChecker.java-1.1.patch (13.7 KB) - added by wiktorn 5 years ago.
corrected version
RelationChecker.java-1.2.patch (17.8 KB) - added by wiktorn 5 years ago.
patch version 1.2

Download all attachments as: .zip

Change History (30)

comment:1 Changed 5 years ago by Don-vip

Resolution: worksforme
Status: newclosed

the route=hiking can be deduced from message.

comment:2 Changed 5 years ago by mkoniecz

Maybe it would better to not force user to deduce what is happening? Especially in cases with multiple route=hiking at the same way (here it is not so bad).

Maybe at least show relation id.

Last edited 5 years ago by mkoniecz (previous) (diff)

comment:3 Changed 5 years ago by anatoly77g

I am another user affected by this bug/feature.
What can I do to make the warning disappear?

This note is supposed to help me, but I don't know how:
"the route=hiking can be deduced from message"

comment:4 Changed 5 years ago by anonymous

Resolution: worksforme
Status: closedreopened

comment:5 Changed 5 years ago by Don-vip

Please attach a sample data set.

comment:6 Changed 5 years ago by anatoly77g

I can only guess what a "data set" is: I understand it as a bounding box.
Here is a sample bounding box that contains this problem:
min lat 32.7161537
max lat 32.7188798
min lon 35.0836158
max lon 35.0861478
This area contains just two intersecting paths.
Validation reports a bunch of warnings.

Just in case it's needed, here is my Status Report:

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2014-02-27 09:27:48
Last Changed Author: simon04
Revision: 6891
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2014-02-27 08:25:03 +0100 (Thu, 27 Feb 2014)
Last Changed Rev: 6891

Identification: JOSM/1.5 (6891 en) Windows 7 64-Bit
Memory Usage: 123 MB / 494 MB (50 MB allocated, but free)
Java version: 1.7.0_55, Oracle Corporation, Java HotSpot(TM) Client VM
VM arguments: [-Xms16m, -Xmx512m]
Dataset consistency test: No problems found

comment:7 Changed 5 years ago by anatoly77g

I did some research; it seems that the validator warnings are caused by the following lines:

<role key="" text="route segment" requisite="required" type="way" member_expression="highway=path OR highway=footway OR highway=track"/>
<role key="" text="infrastructure" requisite="optional" type="node,closedway" member_expression="tourism OR amenity"/>
<role key="" text="natural" requisite="optional" type="node,closedway" member_expression="natural=peak OR natural=volcano OR mountain_pass=yes OR natural=water OR tourism=viewpoint OR amenity=drinking_water OR natural=spring OR place=locality"/>

They are in the file core/data/defaultpresets.xml; were added in changeset 6810.

If I remove the last two lines (leaving only the first of the 3 lines), the warnings go away.

comment:8 Changed 5 years ago by Don-vip

Milestone: 14.05

comment:9 Changed 5 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 7061/josm:

fix #9844 - incorrect validator warnings if relation preset contain several empty roles + refactoring

comment:10 Changed 5 years ago by aceman

I was also wondering about this message.
Thanks for fixing it.

Changed 5 years ago by wiktorn

Attachment: RelationChecker.java.patch added

Patch for defect

comment:11 Changed 5 years ago by wiktorn

Resolution: fixed
Status: closedreopened

It is still complaining about empty relation type, as for example for:

http://www.openstreetmap.org/relation/3791666

I've attached the fix for this error, which also provides better information about validation errors.

I'm not sure why, it also fixes roles verification warnings with forward/backward role type for biking routes.

comment:12 Changed 5 years ago by wiktorn

Attached patch may fix also: #9992

comment:13 Changed 5 years ago by simon04

Thank you for your submission! This patch fixes the issue #10133.

Some remarks:

  • checkMemberType can be simplified to r.types.contains(member.getDisplayType())
  • For consistency, please use "did not" instead of "didn't"
  • The types in the error "... match accepted list of ..." cannot be translated. To fix this, consider this fragment which transforms a TaggingPresetTypes to a nicer string
                        Utils.join("/", Utils.transform(types, new Utils.Function<TaggingPresetType, Object>() {
                            @Override
                            public Object apply(TaggingPresetType x) {
                                return tr(x.getName());
                            }
                        }))
    
  • Here, one can also use EnumSet<TaggingPresetType> types = EnumSet.noneOf(TaggingPresetType.class);
  • When documenting a method (e.g. checkMemberExpressionAndType), please use the Javadoc style /** Foobar */.
  • The deep nesting of loops/conditionals in checkMemberExpressionAndType is hard to follow …
  • In the same method, do you really want to return true if a single r.memberExpression == null? (for instance)
  • In the same method, the else branch is empty

Changed 5 years ago by wiktorn

corrected version

comment:14 Changed 5 years ago by wiktorn

Thank you for your review. Corrected patch attached.

I've changed order of conditional expressions in checkMemberExpressionAndType, so it should be easier to follow. I don't have yet the idea, how to refactor that part to make it more comprehensive, as I need the possibleError from the innermost condition. I hope that additional comments help understanding how it works.

comment:15 Changed 5 years ago by stoecker

Summary: mysterious validator message - Member for role '<empty>' does not match 'highway=path || highway=fooytway || highway=track' (1)[PATCH] mysterious validator message - Member for role '<empty>' does not match 'highway=path || highway=fooytway || highway=track' (1)

comment:16 Changed 5 years ago by simon04

In 7254/josm:

see #9844 - RelationChecker: add unit tests, fix detected problem concerning role counts

comment:17 Changed 5 years ago by simon04

I added some unit test cases for the current implementation of the relation checker. It seems that your patch breaks the following ones:

  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testOuter2
  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testPowerMemberExpression
  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testRestrictionEmpty
  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testRestrictionViaRelation

Changed 5 years ago by wiktorn

patch version 1.2

comment:18 Changed 5 years ago by wiktorn

Corrected patch attached.

  • testRestrictionViaRelation - I've corrected the presets, so it says, that it is mandatory parameter (as in http://wiki.openstreetmap.org/wiki/Relation:restriction) - TaggingPresetItems.getValidCount() returns 0, if count is not set and requisite="required" is set in preset
  • I've corrected the tests, to they reflect error messages now returned
  • I've corrected bug that relations not present in preset were not detected

comment:19 Changed 5 years ago by wiktorn

And one more note:
checkMemberType can't be simplified to r.types.contains(member.getDisplayType()) - because Role.types uses TaggingPresetType and RelationMember.getDisplayType() returns OsmPrimitiveType.

Maybe TaggingPresetType is surplus and should be refactored to OsmPrimitiveType?

comment:20 in reply to:  17 Changed 5 years ago by wiktorn

Replying to simon04:

I added some unit test cases for the current implementation of the relation checker. It seems that your patch breaks the following ones:

  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testOuter2
  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testPowerMemberExpression
  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testRestrictionEmpty
  • org.openstreetmap.josm.data.validation.tests.RelationCheckerTest#testRestrictionViaRelation

Any chance that you'll have time to look again into revised patch?

comment:21 Changed 5 years ago by skyper

Summary: [PATCH] mysterious validator message - Member for role '<empty>' does not match 'highway=path || highway=fooytway || highway=track' (1)[PATCH] mysterious validator message - Member for role '<empty>' does not match 'highway=path || highway=footway || highway=track' (1)

comment:22 Changed 5 years ago by Klumbumbus

Cc: Klumbumbus added

comment:23 Changed 5 years ago by Don-vip

Keywords: route relation empty role added

comment:24 Changed 5 years ago by mkoniecz

Milestone: 14.0514.12

comment:25 Changed 5 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 7884/josm:

fix #9844, fix #9992, fix #10133 - fix relation checker tests against member expressions (modified patch by wiktorn)

comment:26 Changed 5 years ago by Don-vip

Thanks for the patch! Sorry for taking this long to incorporate it, this is great job! :)

comment:27 Changed 5 years ago by Don-vip

In 7885/josm:

see #9844 - code cleanup

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.