ways with more than 2000 nodes: prevent lockup and provide validator-fix (#8808)

* add validation & fix for way vertices limit imposed by OSM API (try to choose splitting locations at existing intersection vertices if possible)
* fix splitting of closed ways at two or more nodes:
  this bug resulted sometimes in one extra split point in the result, or one of the split  vertices to be left unsplit

ARCHITECTURE.md

@@ -483,6 +483,10 @@ A feature does not have enough tags to define what it is.
 * `relation_type`: the OSM entity type is `relation` but there is no `type` tag
 * `highway_classification`: the OSM entity type is `way` and the feature is tagged as `highway=road`
 
+##### `osm_api_limits`
+
+A feature does not conform to the limits and rules imposed by the OSM API, such as a way with too many nodes for example.
+
 ##### `outdated_tags`
 
 A feature has nonstandard tags.

CHANGELOG.md

@@ -40,9 +40,11 @@ _Breaking developer changes, which may affect downstream projects or sites that
 #### :sparkles: Usability & Accessibility
 * Also show search result for coordinates in `lon/lat` order in search results ([#10720], thanks [@Deeptanshu-sankhwar])
 #### :scissors: Operations
+* Fix splitting of closed ways (or areas) when two or more split-points are selected
 #### :camera: Street-Level
 #### :white_check_mark: Validation
 * Add warning if aeroways cross each other, buildings or highways ([#9315], thanks [@k-yle])
+* Warn when a way with more than the maximum allowed number of nodes is to be uploaded and provide a way to fix it ([#7381])
 #### :bug: Bugfixes
 * Prevent degenerate ways caused by deleting a corner of a triangle ([#10003], thanks [@k-yle])
 * Fix briefly disappearing data layer during background layer tile layer switching transition ([#10748])
@@ -55,6 +57,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 #### :rocket: Presets
 #### :hammer: Development
 
+[#7381]: https://github.com/openstreetmap/iD/issues/7381
 [#10003]: https://github.com/openstreetmap/iD/pull/10003
 [#10720]: https://github.com/openstreetmap/iD/issues/10720
 [#10747]: https://github.com/openstreetmap/iD/issues/10747

data/core.yaml

@@ -1954,6 +1954,12 @@ en:
     vertex_as_point:
       message: '{feature} should be attached to a line or area based on its tags'
       reference: "Some features shouldn't be standalone points."
+    osm_api_limits:
+      title: OSM Data Limitations
+      tip: Limits and rules imposed by the OSM API on uploaded features
+      max_way_nodes:
+        message: Way has too many vertices
+        reference: "The maximum number of vertices of a way is {maxWayNodes}. Split the feature into smaller parts or otherwise reduce the number of nodes of the affected way."
     fix:
       add_a_bridge:
         title: Add a bridge
@@ -2032,6 +2038,8 @@ en:
         title: Set as inner
       set_as_outer:
         title: Set as outer
+      split_way:
+        title: Split the way into smaller pieces.
       square_feature:
         title: Square this feature
       tag_as_disconnected:

modules/actions/split.js

@@ -95,7 +95,7 @@ export function actionSplit(nodeIds, newWayIds) {
         return totalLength;
     }
 
-    function split(graph, nodeId, wayA, newWayId) {
+    function split(graph, nodeId, wayA, newWayId, otherNodeIds) {
         var wayB = osmWay({ id: newWayId, tags: wayA.tags });   // `wayB` is the NEW way
         var nodesA;
         var nodesB;
@@ -104,7 +104,7 @@ export function actionSplit(nodeIds, newWayIds) {
         if (wayA.isClosed()) {
             var nodes = wayA.nodes.slice(0, -1);
             var idxA = nodes.indexOf(nodeId);
-            var idxB = splitArea(nodes, idxA, graph);
+            var idxB = otherNodeIds.length > 0 ? nodes.indexOf(otherNodeIds[0]) : splitArea(nodes, idxA, graph);
 
             if (idxB < idxA) {
                 nodesA = nodes.slice(idxA).concat(nodes.slice(0, idxB + 1));
@@ -398,7 +398,7 @@ export function actionSplit(nodeIds, newWayIds) {
             var nodeId = nodeIds[i];
             var candidates = action.waysForNode(nodeId, graph);
             for (var j = 0; j < candidates.length; j++) {
-                graph = split(graph, nodeId, candidates[j], newWayIds && newWayIds[newWayIndex]);
+                graph = split(graph, nodeId, candidates[j], newWayIds && newWayIds[newWayIndex], nodeIds.slice(j + 1));
                 newWayIndex += 1;
             }
         }

modules/validations/index.js

@@ -11,6 +11,7 @@ export { validationMismatchedGeometry } from './mismatched_geometry';
 export { validationMissingRole } from './missing_role';
 export { validationMissingTag } from './missing_tag';
 export { validationMutuallyExclusiveTags } from './mutually_exclusive_tags';
+export { validationOsmApiLimits } from './osm_api_limits';
 export { validationOutdatedTags } from './outdated_tags';
 export { validationPrivateData } from './private_data';
 export { validationSuspiciousName } from './suspicious_name';

modules/validations/osm_api_limits.js

@@ -0,0 +1,98 @@
+import { t } from '../core/localizer';
+import { validationIssue, validationIssueFix } from '../core/validation';
+import { operationSplit } from '../operations/split';
+
+export function validationOsmApiLimits(context) {
+    const type = 'osm_api_limits';
+
+    const validation = function checkOsmApiLimits(entity) {
+        const issues = [];
+        const osm = context.connection();
+        if (!osm) return issues; // cannot check if there is no connection to the osm api, e.g. during unit tests
+        const maxWayNodes = osm.maxWayNodes();
+
+        if (entity.type === 'way') {
+            if (entity.nodes.length > maxWayNodes) {
+                issues.push(new validationIssue({
+                    type: type,
+                    subtype: 'exceededMaxWayNodes',
+                    severity: 'error',
+                    message: function() {
+                        return t.html('issues.osm_api_limits.max_way_nodes.message');
+                    },
+                    reference: function(selection) {
+                        selection.selectAll('.issue-reference')
+                            .data([0])
+                            .enter()
+                            .append('div')
+                            .attr('class', 'issue-reference')
+                            .html(t.html('issues.osm_api_limits.max_way_nodes.reference', { maxWayNodes }));
+                    },
+                    entityIds: [entity.id],
+                    dynamicFixes: splitWayIntoSmallChunks
+                }));
+            }
+        }
+
+        return issues;
+    };
+
+    function splitWayIntoSmallChunks() {
+        const fix = new validationIssueFix({
+            icon: 'iD-operation-split',
+            title: t.html('issues.fix.split_way.title'),
+            entityIds: this.entityIds,
+            onClick: function(context) {
+                const maxWayNodes = context.connection().maxWayNodes();
+                const g = context.graph();
+
+                const entityId = this.entityIds[0];
+                const entity = context.graph().entities[entityId];
+                const numberOfParts = Math.ceil(entity.nodes.length / maxWayNodes);
+                let splitVertices;
+
+                if (numberOfParts === 2) {
+                    // simple case: try to split at the an intersection vertex
+                    const splitIntersections = entity.nodes
+                        .map(nid => g.entity(nid))
+                        .filter(n => g.parentWays(n).length > 1)
+                        .map(n => n.id)
+                        .filter(nid => {
+                            const splitIndex = entity.nodes.indexOf(nid);
+                            return splitIndex < maxWayNodes &&
+                                entity.nodes.length - splitIndex < maxWayNodes;
+                        });
+                    if (splitIntersections.length > 0) {
+                        splitVertices = [
+                            splitIntersections[Math.floor(splitIntersections.length / 2)]
+                        ];
+                    }
+                }
+
+                if (splitVertices === undefined) {
+                    // general case: either more than one split is needed or no possible
+                    // intersection split point was found -> just split at regular intervals
+                    splitVertices = [...Array(numberOfParts - 1)].map((_, i) =>
+                        entity.nodes[Math.floor(entity.nodes.length * (i + 1) / numberOfParts)]);
+                }
+
+                if (entity.isClosed()) {
+                    // add extra split for closed ways at start of way
+                    splitVertices.push(entity.nodes[0]);
+                }
+
+                const operation = operationSplit(context, splitVertices.concat(entityId));
+                if (!operation.disabled()) {
+                    operation();
+                }
+            }
+        });
+
+        return [fix];
+    }
+
+
+    validation.type = type;
+
+    return validation;
+}

test/spec/actions/split.js

@@ -511,6 +511,26 @@ describe('iD.actionSplit', function () {
             expect(g4.entity('-').nodes).to.eql(['b', 'c', 'd']);
             expect(g4.entity('=').nodes).to.eql(['d', 'a', 'b']);
         });
+
+        it('splits a closed way at the given points', function () {
+            //
+            // Situation:
+            //    a ---- b
+            //    |      |
+            //    d ---- c
+            //
+            var graph = iD.coreGraph([
+                iD.osmNode({ id: 'a', loc: [0, 1] }),
+                iD.osmNode({ id: 'b', loc: [1, 1] }),
+                iD.osmNode({ id: 'c', loc: [1, 0] }),
+                iD.osmNode({ id: 'd', loc: [0, 0] }),
+                iD.osmWay({ id: '-', nodes: ['a', 'b', 'c', 'd', 'a']})
+            ]);
+
+            var g1 = iD.actionSplit(['a', 'b'], ['='])(graph);
+            expect(g1.entity('-').nodes).to.eql(['b', 'c', 'd', 'a']);
+            expect(g1.entity('=').nodes).to.eql(['a', 'b']);
+        });
     });
 
 

test/spec/validations/osm_api_limits.js

@@ -0,0 +1,96 @@
+describe('iD.validations.osm_api_limits', function () {
+    var context;
+
+    beforeEach(function() {
+        iD.services.osm = { maxWayNodes: function() { return 10; } };
+        context = iD.coreContext().assetPath('../dist/').init();
+        context.surface = () => d3.select('#nop'); // mock with NOP
+    });
+
+    function createWay(numNodes) {
+        var nodes = [];
+        for (var i = 0; i < numNodes; i++) {
+            nodes.push(iD.osmNode({ id: 'n-' + i, loc: [i, i]}));
+        }
+        var w = iD.osmWay({id: 'w-1', tags: {},
+            nodes: nodes.map(function(n) { return n.id; }) });
+
+        context.perform.apply(null, nodes
+            .map(function(n) { return iD.actionAddEntity(n); })
+            .concat(iD.actionAddEntity(w))
+        );
+    }
+
+    function validate() {
+        var validator = iD.validationOsmApiLimits(context);
+        var changes = context.history().changes();
+        var entities = changes.modified.concat(changes.created);
+        var issues = [];
+        entities.forEach(function(entity) {
+            issues = issues.concat(validator(entity, context.graph()));
+        });
+        return issues;
+    }
+
+    it('has no errors on init', function() {
+        var issues = validate();
+        expect(issues).to.have.lengthOf(0);
+    });
+
+    it('flags way with more than the maximum number of allowed nodes', function() {
+        createWay(12);
+        var issues = validate();
+        expect(issues).to.have.lengthOf(1);
+        var issue = issues[0];
+        expect(issue.type).to.eql('osm_api_limits');
+        expect(issue.subtype).to.eql('exceededMaxWayNodes');
+        expect(issue.severity).to.eql('error');
+        expect(issue.entityIds).to.have.lengthOf(1);
+        expect(issue.entityIds[0]).to.eql('w-1');
+
+        var fixes = issue.fixes(context);
+        expect(fixes).to.have.lengthOf(1);
+        fixes[0].onClick(context);
+        issues = validate();
+        expect(issues).to.have.lengthOf(0);
+    });
+
+    it('can fix an extreme case', function() {
+        createWay(33);
+        var issues = validate();
+        expect(issues).to.have.lengthOf(1);
+        var issue = issues[0];
+
+        var fixes = issue.fixes(context);
+        expect(fixes).to.have.lengthOf(1);
+        fixes[0].onClick(context);
+        issues = validate();
+        expect(issues).to.have.lengthOf(0);
+    });
+
+    it('fix a simple case at an intersection vertex', function() {
+        createWay(12);
+
+        var n2 = iD.osmNode({id: 'n-0', loc: [0,0]});
+        var n1 = iD.osmNode({id: 'n-8', loc: [8,8]});
+        var w = iD.osmWay({id: 'w-2', nodes: ['n-0', 'n-8'], tags: {}});
+
+        context.perform(
+            iD.actionAddEntity(n1),
+            iD.actionAddEntity(n2),
+            iD.actionAddEntity(w)
+        );
+
+        var issues = validate();
+        expect(issues).to.have.lengthOf(1);
+        var issue = issues[0];
+
+        var fixes = issue.fixes(context);
+        expect(fixes).to.have.lengthOf(1);
+        fixes[0].onClick(context);
+        issues = validate();
+        expect(issues).to.have.lengthOf(0);
+
+        context.graph().entity('w-1').nodes.length === 8;
+    });
+});