Ticket #9844: RelationChecker.java.patch

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

@@ -5 +5 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
 import java.text.MessageFormat;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedList;
-import java.util.List;
 import java.util.Set;
 
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.DeleteCommand;
-import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.RelationMember;
-import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.Test;
 import org.openstreetmap.josm.data.validation.TestError;
@@ -82 +78 @@
         }
     }
 
+    private static class RolePreset {
+        public RolePreset(LinkedList<Role> roles, String name) {
+            this.roles = roles;
+            this.name = name;
+        }
+        private LinkedList<Role> roles;
+        private String name;
+    }
+
     private static class RoleInfo {
         private int total = 0;
-        private Collection<Node> nodes = new LinkedList<>();
-        private Collection<Way> ways = new LinkedList<>();
-        private Collection<Way> openways = new LinkedList<>();
-        private Collection<Relation> relations = new LinkedList<>();
     }
 
     @Override
     public void visit(Relation n) {
-        LinkedList<Role> allroles = buildAllRoles(n);
+        HashMap<String, RolePreset> allroles = buildAllRoles(n);
         if (allroles.isEmpty() && n.hasTag("type", "route")
                 && n.hasTag("route", "train", "subway", "monorail", "tram", "bus", "trolleybus", "aerialway", "ferry")) {
             errors.add(new TestError(this, Severity.WARNING,
@@ -117 +118 @@
             RoleInfo ri = map.get(role);
             if (ri == null) {
                 ri = new RoleInfo();
+                map.put(role, ri);
             }
             ri.total++;
-            if (m.isRelation()) {
-                ri.relations.add(m.getRelation());
-            } else if(m.isWay()) {
-                ri.ways.add(m.getWay());
-                if (!m.getWay().isClosed()) {
-                    ri.openways.add(m.getWay());
-                }
-            }
-            else if (m.isNode()) {
-                ri.nodes.add(m.getNode());
-            }
-            map.put(role, ri);
         }
         return map;
     }
 
-    private LinkedList<Role> buildAllRoles(Relation n) {
-        LinkedList<Role> allroles = new LinkedList<>();
+    // return Roles grouped by key
+    private HashMap<String, RolePreset> buildAllRoles(Relation n) {
+        HashMap<String, RolePreset> allroles = new HashMap<>();
+
         for (TaggingPreset p : relationpresets) {
             boolean matches = true;
             Roles r = null;
@@ -152 +144 @@
                 }
             }
             if (matches && r != null) {
-                allroles.addAll(r.roles);
+                for(Role role: r.roles) {
+                    String key = role.key;
+                    LinkedList<Role> roleGroup = null;
+                    if (allroles.containsKey(key)) {
+                        roleGroup = allroles.get(key).roles;
+                    } else {
+                        roleGroup = new LinkedList<>();
+                        allroles.put(key, new RolePreset(roleGroup, p.name));
+                    }
+                    roleGroup.add(role);
+                }
             }
         }
         return allroles;
     }
 
-    private void checkRoles(Relation n, LinkedList<Role> allroles, HashMap<String, RoleInfo> map) {
-        List<String> done = new LinkedList<>();
-        // Remove empty roles if several exist (like in route=hiking, see #9844)
-        List<Role> emptyRoles = new LinkedList<>();
-        for (Role r : allroles) {
-            if ("".equals(r.key)) {
-                emptyRoles.add(r);
+    private boolean checkMemberType(Role r, RelationMember member) {
+        if (r.types != null) {
+            for (TaggingPresetType type: r.types) {
+                if (type.equals(TaggingPresetType.CLOSEDWAY) && member.isWay() && member.getWay().isClosed()) {
+                    return true;
+                }
+                if(type.equals(TaggingPresetType.NODE) && member.isNode()) {
+                    return true;
+                }
+                if(type.equals(TaggingPresetType.RELATION) && member.isRelation()) {
+                    return true;
+                }
+                if(type.equals(TaggingPresetType.WAY) && member.isWay()) {
+                    return true;
+                }
             }
+        } else {
+            // if no types specified, then test is passed
+            return true;
         }
-        if (emptyRoles.size() > 1) {
-            allroles.removeAll(emptyRoles);
+        // if no match is found, then test failed
+        return false;
+    }
+
+    // get all role definition for specified key and check, if some definition matches
+    private boolean checkMemberExpressionAndType(RolePreset rolePreset, RelationMember member, Relation n) {
+        TestError possibleMatchError = null;
+        if (rolePreset == null || rolePreset.roles == null) {
+            // no restrictions on role types
+            return true;
         }
-        for (Role r : allroles) {
-            done.add(r.key);
-            String keyname = r.key;
-            if ("".equals(keyname)) {
-                keyname = tr("<empty>");
-            }
-            RoleInfo ri = map.get(r.key);
-            checkRoleCounts(n, r, keyname, ri);
-            if (ri != null) {
-                if (r.types != null) {
-                    checkRoleTypes(n, r, keyname, ri);
-                }
+        for (Role r: rolePreset.roles) {
+            if (checkMemberType(r, member)) {
                 if (r.memberExpression != null) {
-                    checkRoleMemberExpressions(n, r, keyname, ri);
+                    OsmPrimitive primitive = member.getMember();
+                    if(primitive.isUsable()) {
+                        if(r.memberExpression.match(primitive)) {
+                            return true;
+                        } else {
+                            // possible match error
+                            String s = marktr("Role member didn''t match expression {0} in template {1}");
+                            possibleMatchError = new TestError(this, Severity.WARNING, ROLE_VERIF_PROBLEM_MSG,
+                                    tr(s, r.memberExpression, rolePreset.name), s, WRONG_TYPE,
+                                        member.getMember().isUsable() ? member.getMember() : n);
+
+                        }
+                    } else {
+                        // if it is not usable, we can't verify memberExpression
+                        return true;
+                    }
+                } else {
+                    return true;
+                }
+            } else {
+
+            }
+        }
+        if( possibleMatchError != null) {
+            errors.add(possibleMatchError);
+        } else {
+            String s = marktr("Role member type {0} didn''t match accepted list of {1} in template {2}");
+            Set<Enum<TaggingPresetType>> types = new HashSet<>();
+            for (Role r: rolePreset.roles) {
+                types.addAll(r.types);
+            }
+            errors.add(new TestError(this, Severity.WARNING, ROLE_VERIF_PROBLEM_MSG,
+                    tr(s, member.getType(), types, rolePreset.name), s, WRONG_TYPE,
+                        member.getMember().isUsable() ? member.getMember() : n));
+        }
+        return false;
+    }
+
+
+    private void checkRoles(Relation n, HashMap<String, RolePreset> allroles, HashMap<String, RoleInfo> map) {
+        Set<String> done = new HashSet<>();
+
+        // go through all members of relation
+        for (RelationMember member: n.getMembers()) {
+            String role = member.getRole();
+            done.add(role);
+
+            // error reporting done inside
+            checkMemberExpressionAndType(allroles.get(role), member, n);
+        }
+
+        // verify role counts based on whole role sets
+        for(RolePreset rp: allroles.values()) {
+            for (Role r: rp.roles) {
+                String keyname = r.key;
+                if ("".equals(keyname)) {
+                    keyname = tr("<empty>");
                 }
+                checkRoleCounts(n, r, keyname, map.get(r.key));
             }
         }
+        // verify unwanted members
         for (String key : map.keySet()) {
             if (!done.contains(key)) {
+                StringBuilder templates = new StringBuilder();
+                for (RolePreset rp: allroles.values()) {
+                    templates.append(rp.name);
+                }
+
                 if (key.length() > 0) {
-                    String s = marktr("Role {0} unknown");
+                    String s = marktr("Role {0} unknown in templates {1}");
+
                     errors.add(new TestError(this, Severity.WARNING, ROLE_VERIF_PROBLEM_MSG,
-                            tr(s, key), MessageFormat.format(s, key), ROLE_UNKNOWN, n));
+                            tr(s, key, templates.toString()), MessageFormat.format(s, key), ROLE_UNKNOWN, n));
                 } else {
-                    String s = marktr("Empty role found");
+                    String s = marktr("Empty role found in templates {0}");
                     errors.add(new TestError(this, Severity.WARNING, ROLE_VERIF_PROBLEM_MSG,
-                            tr(s), s, ROLE_EMPTY, n));
+                            tr(s, templates), s, ROLE_EMPTY, n));
                 }
             }
         }
     }
 
-    private void checkRoleMemberExpressions(Relation n, Role r, String keyname, RoleInfo ri) {
-        Set<OsmPrimitive> notMatching = new HashSet<>();
-        Collection<OsmPrimitive> allPrimitives = new ArrayList<>();
-        allPrimitives.addAll(ri.nodes);
-        allPrimitives.addAll(ri.ways);
-        allPrimitives.addAll(ri.relations);
-        for (OsmPrimitive p : allPrimitives) {
-            if (p.isUsable() && !r.memberExpression.match(p)) {
-                notMatching.add(p);
-            }
-        }
-        if (!notMatching.isEmpty()) {
-            String s = marktr("Member for role ''{0}'' does not match ''{1}''");
-            LinkedList<OsmPrimitive> highlight = new LinkedList<>(notMatching);
-            highlight.addFirst(n);
-            errors.add(new TestError(this, Severity.WARNING, ROLE_VERIF_PROBLEM_MSG,
-                    tr(s, keyname, r.memberExpression), MessageFormat.format(s, keyname, r.memberExpression), WRONG_TYPE,
-                    highlight, notMatching));
-        }
-    }
-
-    private void checkRoleTypes(Relation n, Role r, String keyname, RoleInfo ri) {
-        Set<OsmPrimitive> wrongTypes = new HashSet<>();
-        if (!r.types.contains(TaggingPresetType.WAY)) {
-            wrongTypes.addAll(r.types.contains(TaggingPresetType.CLOSEDWAY) ? ri.openways : ri.ways);
-        }
-        if (!r.types.contains(TaggingPresetType.NODE)) {
-            wrongTypes.addAll(ri.nodes);
-        }
-        if (!r.types.contains(TaggingPresetType.RELATION)) {
-            wrongTypes.addAll(ri.relations);
-        }
-        if (!wrongTypes.isEmpty()) {
-            String s = marktr("Member for role {0} of wrong type");
-            LinkedList<OsmPrimitive> highlight = new LinkedList<>(wrongTypes);
-            highlight.addFirst(n);
-            errors.add(new TestError(this, Severity.WARNING, ROLE_VERIF_PROBLEM_MSG,
-                    tr(s, keyname), MessageFormat.format(s, keyname), WRONG_TYPE,
-                    highlight, wrongTypes));
-        }
-    }
 
     private void checkRoleCounts(Relation n, Role r, String keyname, RoleInfo ri) {
         long count = (ri == null) ? 0 : ri.total;