Modify

Opened 4 years ago

Closed 5 weeks ago

Last modified 4 weeks ago

#13082 closed enhancement (fixed)

[RFC][PATCH] Warning for relations with only one member

Reported by: naoliv Owned by: team
Priority: normal Milestone: 20.06
Component: Core validator Version:
Keywords: Cc:

Description

Could we have a message (at informational level) for relation with exactly 1 member?

Such test would help to find some really unnecessary (or even wrong) relations with only one member.

JOSM:

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2016-06-28 00:39:58 +0200 (Tue, 28 Jun 2016)
Build-Date:2016-06-28 01:32:14
Revision:10501
Relative:URL: ^/trunk

Identification: JOSM/1.5 (10501 pt_BR) Linux Debian GNU/Linux testing (stretch)
Memory Usage: 2159 MB / 10206 MB (1491 MB allocated, but free)
Java version: 1.8.0_91-8u91-b14-2-b14, Oracle Corporation, OpenJDK 64-Bit Server VM
VM arguments: [-Dawt.useSystemAAFontSettings=on]
Dataset consistency test: No problems found

Attachments (0)

Change History (26)

comment:1 Changed 4 years ago by skyper

Please, only inform about type=node if the member has no tags except source*=* fixme*=* note*=* and description*=*. Thanks

comment:2 Changed 4 years ago by naoliv

What exactly is a type=node relation?
There are only 36 relations like this http://taginfo.openstreetmap.org/tags/type=node

comment:3 Changed 4 years ago by skyper

You need it for nodes above of each other like transponders on the same pole but different height. I can only tag one on the node and need a relation for the other.

See OSMwiki page

comment:4 Changed 20 months ago by naoliv

Proof of concept:

  • src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java

    diff --git a/src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java b/src/org/openstreetmap/josm/data/validation/tests/MultipolygonTest.java
    index ba7191f54..d6c6dbfb6 100644
    a b public class MultipolygonTest extends Test { 
    7777    public static final int EQUAL_RINGS = 1616;
    7878    /** Multipolygon rings share nodes */
    7979    public static final int RINGS_SHARE_NODES = 1617;
     80    /** Multipolygon with a single member */
     81    public static final int SINGLE_MEMBER = 1618;
    8082
    8183    private static final int FOUND_INSIDE = 1;
    8284    private static final int FOUND_OUTSIDE = 2;
    public class MultipolygonTest extends Test { 
    130132    @Override
    131133    public void visit(Relation r) {
    132134        if (r.isMultipolygon() && r.getMembersCount() > 0) {
     135            checkSingleMember(r);
    133136            List<TestError> tmpErrors = new ArrayList<>(30);
    134137            boolean hasUnexpectedWayRoles = checkMembersAndRoles(r, tmpErrors);
    135138            boolean hasRepeatedMembers = checkRepeatedWayMembers(r);
    public class MultipolygonTest extends Test { 
    145148        }
    146149    }
    147150
     151    private void checkSingleMember(Relation r) {
     152        if (r.getMembersCount() == 1) {
     153            errors.add(TestError.builder(this, Severity.OTHER, SINGLE_MEMBER)
     154                .message(tr("Multipolygon with a single member"))
     155                .primitives(r)
     156                .build());
     157        }
     158    }
     159
    148160    /**
    149161     * Various style-related checks:<ul>
    150162     * <li>{@link #NO_STYLE_POLYGON}: Multipolygon relation should be tagged with area tags and not the outer way</li>
  • src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java

    diff --git a/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java b/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java
    index 00ec95584..d6b3b7207 100644
    a b public class RelationChecker extends Test { 
    5555    public static final int RELATION_UNKNOWN = 1707;
    5656    /** Relation is empty */
    5757    public static final int RELATION_EMPTY   = 1708;
     58    /** Relation has only one member */
     59    public static final int ONE_MEMBER       = 1709;
    5860    // CHECKSTYLE.ON: SingleSpaceSeparator
    5961
    6062    /**
    public class RelationChecker extends Test { 
    126128            // see #17010: don't report same problem twice
    127129            return;
    128130        }
     131        if (n.getMembersCount() == 1) {
     132            errors.add(TestError.builder(this, Severity.OTHER, ONE_MEMBER)
     133                    .message(tr("Relation has only one member"))
     134                    .primitives(n)
     135                    .build());
     136        }
    129137        Map<Role, String> allroles = buildAllRoles(n);
    130138        if (allroles.isEmpty() && n.hasTag("type", "route")
    131139                && n.hasTag("route", "train", "subway", "monorail", "tram", "bus", "trolleybus", "aerialway", "ferry")) {
Last edited 20 months ago by naoliv (previous) (diff)

comment:5 Changed 20 months ago by naoliv

Summary: Warning for relations with only one member[RFC][PATCH] Warning for relations with only one member

comment:6 Changed 20 months ago by GerdP

I think Multipolygons are the only case where a single member is OK, I would not even want to see an information for them. On the other hand, other relations with a single member are probably of Severity.Warning.

comment:7 Changed 12 months ago by naoliv

Instead implementing a validation rule (to annoy everybody with it :-)), is it easy to have a new search expression for this? (members, for example)

This would allow us to search for relations with a specific number of member and also use it in custom MapCSS validation rules.

comment:8 Changed 12 months ago by anonymous

I think the osmose external rule has this validation. It can be downloaded from JOSM settings.

comment:9 Changed 12 months ago by naoliv

The osmose test isn't working as expected right now #17964

comment:10 Changed 12 months ago by naoliv

With this I am able to properly search for relations with any number of members inside JOSM:

  • src/org/openstreetmap/josm/data/osm/search/SearchCompiler.java

     
    124124        private final Collection<String> keywords = Arrays.asList("id", "version", "type", "user", "role",
    125125                "changeset", "nodes", "ways", "tags", "areasize", "waylength", "modified", "deleted", "selected",
    126126                "incomplete", "untagged", "closed", "new", "indownloadedarea",
    127                 "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset");
     127                "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset", "members");
    128128
    129129        @Override
    130130        public Match get(String keyword, boolean caseSensitive, boolean regexSearch, PushbackTokenizer tokenizer) throws SearchParseError {
     
    180180                        return new Nth(tokenizer, true);
    181181                    case "hasRole":
    182182                        return new HasRole(tokenizer);
     183                    case "members":
     184                        return new Members(tokenizer);
    183185                    case "timestamp":
    184186                        // add leading/trailing space in order to get expected split (e.g. "a--" => {"a", ""})
    185187                        String rangeS = ' ' + tokenizer.readTextOrNumber() + ' ';
     
    12491251    }
    12501252
    12511253    /**
     1254     * Returns the number of members of a relation
     1255     */
     1256    private static class Members extends Match {
     1257        private final int members;
     1258
     1259        Members(PushbackTokenizer tokenizer) throws SearchParseError {
     1260            this((int) tokenizer.readNumber(tr("Positive integer expected")));
     1261        }
     1262
     1263        private Members(int members) {
     1264            this.members = members;
     1265        }
     1266
     1267        @Override
     1268        public boolean match(OsmPrimitive osm) {
     1269            for (OsmPrimitive p : osm.getReferrers()) {
     1270                final int num;
     1271                if (p instanceof Relation) {
     1272                    Relation r = (Relation) p;
     1273                    num = r.getMembersCount();
     1274                } else {
     1275                    continue;
     1276                }
     1277                if (num == members) {
     1278                    return true;
     1279                }
     1280            }
     1281            return false;
     1282        }
     1283    }
     1284
     1285    /**
    12521286     * Matches objects with the given relation role (i.e. "outer").
    12531287     */
    12541288    private static class RoleMatch extends Match {
  • src/org/openstreetmap/josm/gui/dialogs/SearchDialog.java

     
    347347                .addKeyword("nodes:<i>20-</i>", "nodes:", tr("ways with at least 20 nodes, or relations containing at least 20 nodes"))
    348348                .addKeyword("ways:<i>3-</i>", "ways:", tr("nodes with at least 3 referring ways, or relations containing at least 3 ways"))
    349349                .addKeyword("tags:<i>5-10</i>", "tags:", tr("objects having 5 to 10 tags"))
    350                 .addKeyword("role:", "role:", tr("objects with given role in a relation"))
     350                .addKeyword("members:<i>2</i>", "members:", tr("relations with 2 members"))
     351                .addKeyword("role:", "role:", tr("objects with given role in a relation")),
     352                GBC.eol());
     353            hintPanel.add(new SearchKeywordRow(hcbSearchString)
    351354                .addKeyword("areasize:<i>-100</i>", "areasize:", tr("closed ways with an area of 100 m\u00b2"))
    352355                .addKeyword("waylength:<i>200-</i>", "waylength:", tr("ways with a length of 200 m or more")),
    353                 GBC.eol());
     356                GBC.eol().anchor(GBC.CENTER));
    354357            hintPanel.add(new SearchKeywordRow(hcbSearchString)
    355358                .addTitle(tr("state"))
    356359                .addKeyword("modified", "modified ", tr("all modified objects"))

But this doesn't work:

relation[eval(JOSM_search("members:1"))] {
        throwWarning: tr("ha");
}

Using this it works, however:

relation[eval(JOSM_search("tags:1"))] {
        throwWarning: tr("ha");
}

JOSM_search is working as expected, but somehow I am missing to include this members somewhere else. Any ideas and feedback about this, please?

Last edited 12 months ago by naoliv (previous) (diff)

comment:11 Changed 12 months ago by naoliv

This one works as expected (search in JOSM, search with MapCSS)
My only doubt is about the code quality :-)

  • src/org/openstreetmap/josm/data/osm/search/SearchCompiler.java

     
    124124        private final Collection<String> keywords = Arrays.asList("id", "version", "type", "user", "role",
    125125                "changeset", "nodes", "ways", "tags", "areasize", "waylength", "modified", "deleted", "selected",
    126126                "incomplete", "untagged", "closed", "new", "indownloadedarea",
    127                 "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset");
     127                "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset", "members");
    128128
    129129        @Override
    130130        public Match get(String keyword, boolean caseSensitive, boolean regexSearch, PushbackTokenizer tokenizer) throws SearchParseError {
     
    180180                        return new Nth(tokenizer, true);
    181181                    case "hasRole":
    182182                        return new HasRole(tokenizer);
     183                    case "members":
     184                        return new Members(tokenizer);
    183185                    case "timestamp":
    184186                        // add leading/trailing space in order to get expected split (e.g. "a--" => {"a", ""})
    185187                        String rangeS = ' ' + tokenizer.readTextOrNumber() + ' ';
     
    12491251    }
    12501252
    12511253    /**
     1254     * Matches relations with a certain number of members
     1255     */
     1256    private static class Members extends RangeMatch {
     1257        Members(Range range) {
     1258            super(range);
     1259        }
     1260
     1261        Members(PushbackTokenizer tokenizer) throws SearchParseError {
     1262            this(tokenizer.readRange(tr("Range of numbers expected")));
     1263        }
     1264
     1265        @Override
     1266        protected Long getNumber(OsmPrimitive osm) {
     1267            if(!(osm instanceof Relation))
     1268                return null;
     1269            Relation r = (Relation) osm;
     1270            return (long) r.getMembersCount();
     1271        }
     1272
     1273        @Override
     1274        protected String getString() {
     1275            return "members";
     1276        }
     1277    }
     1278
     1279    /**
    12521280     * Matches objects with the given relation role (i.e. "outer").
    12531281     */
    12541282    private static class RoleMatch extends Match {
  • src/org/openstreetmap/josm/gui/dialogs/SearchDialog.java

     
    347347                .addKeyword("nodes:<i>20-</i>", "nodes:", tr("ways with at least 20 nodes, or relations containing at least 20 nodes"))
    348348                .addKeyword("ways:<i>3-</i>", "ways:", tr("nodes with at least 3 referring ways, or relations containing at least 3 ways"))
    349349                .addKeyword("tags:<i>5-10</i>", "tags:", tr("objects having 5 to 10 tags"))
    350                 .addKeyword("role:", "role:", tr("objects with given role in a relation"))
     350                .addKeyword("members:<i>2</i>", "members:", tr("relations with 2 members"))
     351                .addKeyword("role:", "role:", tr("objects with given role in a relation")),
     352                GBC.eol());
     353            hintPanel.add(new SearchKeywordRow(hcbSearchString)
    351354                .addKeyword("areasize:<i>-100</i>", "areasize:", tr("closed ways with an area of 100 m\u00b2"))
    352355                .addKeyword("waylength:<i>200-</i>", "waylength:", tr("ways with a length of 200 m or more")),
    353                 GBC.eol());
     356                GBC.eol().anchor(GBC.CENTER));
    354357            hintPanel.add(new SearchKeywordRow(hcbSearchString)
    355358                .addTitle(tr("state"))
    356359                .addKeyword("modified", "modified ", tr("all modified objects"))

comment:12 Changed 5 weeks ago by simon04

Resolution: fixed
Status: newclosed

In 16581/josm:

fix #13082 - SearchCompiler: add members: keyword to search for relations with the specified number of members (patch by naoliv, modified)

comment:13 Changed 5 weeks ago by simon04

In 16582/josm:

see #13082 - Extend SearchCompilerTest

comment:14 Changed 5 weeks ago by simon04

Milestone: 20.06

Thank you!

comment:15 Changed 4 weeks ago by Klumbumbus

Any concers (e.g. regarding false positives or performance) to add this to our internal relation.mapcss validator file?

relation[type!=node][eval(JOSM_search("members:1"))] {
        throwWarning: tr("Relation with only one member.");
}

comment:16 Changed 4 weeks ago by GerdP

Yes, see comment:6

comment:17 Changed 4 weeks ago by Klumbumbus

I think 1 member MPs are needed only in rare cases. I would like to see a notification about 1 member MPs to get rid of the not useful ones. I think severity other is OK. What about:

relation[type!=node][type!=multipolygon][eval(JOSM_search("members:1"))] {
        throwWarning: tr("Relation with only one member.");
}

relation[type=multipolygon][eval(JOSM_search("members:1"))] {
        throwOther: tr("Relation with only one member.");
}

comment:18 Changed 4 weeks ago by GerdP

Maybe add the type to the 2nd message text. Else "Ignore error" will probably ignore both.
Or just tr("Multipolygon with only one member.") ?

comment:19 in reply to:  18 ; Changed 4 weeks ago by Klumbumbus

Replying to GerdP:

Maybe add the type to the 2nd message text.

I wanted to save one i18n string.

Else "Ignore error" will probably ignore both.

Indeed. So we have this problem already with other validator tests. The ignore list would need to include the severity. I'm not sure if you want to fiddle around with the ignore list handling again :)

comment:20 in reply to:  17 ; Changed 4 weeks ago by stoecker

Replying to Klumbumbus:

I think 1 member MPs are needed only in rare cases.

What are rare cases? The shouldn't be a single case where this should be necessary.

BTW: I also find the single object relation of type=node a strange construct. As said in the comment section creating a second node is much easier.

comment:21 in reply to:  20 Changed 4 weeks ago by Klumbumbus

Replying to stoecker:

What are rare cases? The shouldn't be a single case where this should be necessary.

Like mapping several shops as area in one building where each takes a whole storey.
You could either draw several ways on top of each other or use several MPs.

comment:22 Changed 4 weeks ago by Klumbumbus

(Partly related tickets: #14624, #12385, #15360)

comment:23 Changed 4 weeks ago by GerdP

Cases where a one way multipolygon is used quite often:
1) a closed way with natural=coastline describes the outline of an island which is covered with wood. Same with other natural values. The MP is used because natural=wood cannot be added to the way. Other mappers duplicate the way.
2) a closed way with barrier=fence encloses a landuse=*. Some mappers don't like to combine both tags because one thing is a line object and the other an area.

comment:24 in reply to:  19 ; Changed 4 weeks ago by GerdP

Replying to Klumbumbus:

Else "Ignore error" will probably ignore both.

Indeed. So we have this problem already with other validator tests. The ignore list would need to include the severity. I'm not sure if you want to fiddle around with the ignore list handling again :)

I did not find other strings in mpacss rules with this problem so far.

comment:25 in reply to:  20 Changed 4 weeks ago by skyper

Somehow related #19148, where one member MPs are a solution to get validator calm.

Replying to stoecker:

Replying to Klumbumbus:

I think 1 member MPs are needed only in rare cases.

What are rare cases? The shouldn't be a single case where this should be necessary.

They are not that rare. Two amenity, shop, leisure or a mix out of the three on top of each other exists in reality and will get more common.

BTW: I also find the single object relation of type=node a strange construct. As said in the comment section creating a second node is much easier.

I get a warning Nodes at same position, if I do not use a node relation.

Replying to GerdP:

Cases where a one way multipolygon is used quite often:
1) a closed way with natural=coastline describes the outline of an island which is covered with wood. Same with other natural values. The MP is used because natural=wood cannot be added to the way. Other mappers duplicate the way.
2) a closed way with barrier=fence encloses a landuse=*. Some mappers don't like to combine both tags because one thing is a line object and the other an area.

The problem is almost all the time either tags which conflict or the "one feature one object" rule. Which by the way is the reason in the first case, too.

comment:26 in reply to:  24 Changed 4 weeks ago by Klumbumbus

Replying to GerdP:

I did not find other strings in mpacss rules with this problem so far.

You're right, the ones I was thinking of use placeholders so they are different.

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.