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

[naoliv]

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

[skyper]

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

[naoliv]

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

[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

[naoliv]

Proof of concept:

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

@@ -77 +77 @@
     public static final int EQUAL_RINGS = 1616;
     /** Multipolygon rings share nodes */
     public static final int RINGS_SHARE_NODES = 1617;
+    /** Multipolygon with a single member */
+    public static final int SINGLE_MEMBER = 1618;
 
     private static final int FOUND_INSIDE = 1;
     private static final int FOUND_OUTSIDE = 2;
@@ -130 +132 @@
     @Override
     public void visit(Relation r) {
         if (r.isMultipolygon() && r.getMembersCount() > 0) {
+            checkSingleMember(r);
             List<TestError> tmpErrors = new ArrayList<>(30);
             boolean hasUnexpectedWayRoles = checkMembersAndRoles(r, tmpErrors);
             boolean hasRepeatedMembers = checkRepeatedWayMembers(r);
@@ -145 +148 @@
         }
     }
 
+    private void checkSingleMember(Relation r) {
+        if (r.getMembersCount() == 1) {
+            errors.add(TestError.builder(this, Severity.OTHER, SINGLE_MEMBER)
+                .message(tr("Multipolygon with a single member"))
+                .primitives(r)
+                .build());
+        }
+    }
+
     /**
      * Various style-related checks:<ul>
      * <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

@@ -55 +55 @@
     public static final int RELATION_UNKNOWN = 1707;
     /** Relation is empty */
     public static final int RELATION_EMPTY   = 1708;
+    /** Relation has only one member */
+    public static final int ONE_MEMBER       = 1709;
     // CHECKSTYLE.ON: SingleSpaceSeparator
 
     /**
@@ -126 +128 @@
             // see #17010: don't report same problem twice
             return;
         }
+        if (n.getMembersCount() == 1) {
+            errors.add(TestError.builder(this, Severity.OTHER, ONE_MEMBER)
+                    .message(tr("Relation has only one member"))
+                    .primitives(n)
+                    .build());
+        }
         Map<Role, String> allroles = buildAllRoles(n);
         if (allroles.isEmpty() && n.hasTag("type", "route")
                 && n.hasTag("route", "train", "subway", "monorail", "tram", "bus", "trolleybus", "aerialway", "ferry")) {

[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.

[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.

[anonymous]

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

[naoliv]

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

[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

@@ -124 +124 @@
         private final Collection<String> keywords = Arrays.asList("id", "version", "type", "user", "role",
                 "changeset", "nodes", "ways", "tags", "areasize", "waylength", "modified", "deleted", "selected",
                 "incomplete", "untagged", "closed", "new", "indownloadedarea",
-                "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset");
+                "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset", "members");
 
         @Override
         public Match get(String keyword, boolean caseSensitive, boolean regexSearch, PushbackTokenizer tokenizer) throws SearchParseError {
@@ -180 +180 @@
                         return new Nth(tokenizer, true);
                     case "hasRole":
                         return new HasRole(tokenizer);
+                    case "members":
+                        return new Members(tokenizer);
                     case "timestamp":
                         // add leading/trailing space in order to get expected split (e.g. "a--" => {"a", ""})
                         String rangeS = ' ' + tokenizer.readTextOrNumber() + ' ';
@@ -1249 +1251 @@
     }
 
     /**
+     * Returns the number of members of a relation
+     */
+    private static class Members extends Match {
+        private final int members;
+
+        Members(PushbackTokenizer tokenizer) throws SearchParseError {
+            this((int) tokenizer.readNumber(tr("Positive integer expected")));
+        }
+
+        private Members(int members) {
+            this.members = members;
+        }
+
+        @Override
+        public boolean match(OsmPrimitive osm) {
+            for (OsmPrimitive p : osm.getReferrers()) {
+                final int num;
+                if (p instanceof Relation) {
+                    Relation r = (Relation) p;
+                    num = r.getMembersCount();
+                } else {
+                    continue;
+                }
+                if (num == members) {
+                    return true;
+                }
+            }
+            return false;
+        }
+    }
+
+    /**
      * Matches objects with the given relation role (i.e. "outer").
      */
     private static class RoleMatch extends Match {

src/org/openstreetmap/josm/gui/dialogs/SearchDialog.java

@@ -347 +347 @@
                 .addKeyword("nodes:<i>20-</i>", "nodes:", tr("ways with at least 20 nodes, or relations containing at least 20 nodes"))
                 .addKeyword("ways:<i>3-</i>", "ways:", tr("nodes with at least 3 referring ways, or relations containing at least 3 ways"))
                 .addKeyword("tags:<i>5-10</i>", "tags:", tr("objects having 5 to 10 tags"))
-                .addKeyword("role:", "role:", tr("objects with given role in a relation"))
+                .addKeyword("members:<i>2</i>", "members:", tr("relations with 2 members"))
+                .addKeyword("role:", "role:", tr("objects with given role in a relation")),
+                GBC.eol());
+            hintPanel.add(new SearchKeywordRow(hcbSearchString)
                 .addKeyword("areasize:<i>-100</i>", "areasize:", tr("closed ways with an area of 100 m\u00b2"))
                 .addKeyword("waylength:<i>200-</i>", "waylength:", tr("ways with a length of 200 m or more")),
-                GBC.eol());
+                GBC.eol().anchor(GBC.CENTER));
             hintPanel.add(new SearchKeywordRow(hcbSearchString)
                 .addTitle(tr("state"))
                 .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?

[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

@@ -124 +124 @@
         private final Collection<String> keywords = Arrays.asList("id", "version", "type", "user", "role",
                 "changeset", "nodes", "ways", "tags", "areasize", "waylength", "modified", "deleted", "selected",
                 "incomplete", "untagged", "closed", "new", "indownloadedarea",
-                "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset");
+                "allindownloadedarea", "timestamp", "nth", "nth%", "hasRole", "preset", "members");
 
         @Override
         public Match get(String keyword, boolean caseSensitive, boolean regexSearch, PushbackTokenizer tokenizer) throws SearchParseError {
@@ -180 +180 @@
                         return new Nth(tokenizer, true);
                     case "hasRole":
                         return new HasRole(tokenizer);
+                    case "members":
+                        return new Members(tokenizer);
                     case "timestamp":
                         // add leading/trailing space in order to get expected split (e.g. "a--" => {"a", ""})
                         String rangeS = ' ' + tokenizer.readTextOrNumber() + ' ';
@@ -1249 +1251 @@
     }
 
     /**
+     * Matches relations with a certain number of members
+     */
+    private static class Members extends RangeMatch {
+        Members(Range range) {
+            super(range);
+        }
+
+        Members(PushbackTokenizer tokenizer) throws SearchParseError {
+            this(tokenizer.readRange(tr("Range of numbers expected")));
+        }
+
+        @Override
+        protected Long getNumber(OsmPrimitive osm) {
+            if(!(osm instanceof Relation))
+                return null;
+            Relation r = (Relation) osm;
+            return (long) r.getMembersCount();
+        }
+
+        @Override
+        protected String getString() {
+            return "members";
+        }
+    }
+
+    /**
      * Matches objects with the given relation role (i.e. "outer").
      */
     private static class RoleMatch extends Match {

src/org/openstreetmap/josm/gui/dialogs/SearchDialog.java

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

[simon04]

In 16581/josm:

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

[simon04]

In 16582/josm:

see #13082 - Extend SearchCompilerTest

[simon04]

Thank you!

[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.");
}

[GerdP]

Yes, see comment:6

[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.");
}

[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.") ?

[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 :)

[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.

[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.

[Klumbumbus]

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

[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.

[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.

[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.

[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.