From bd71ccba17ecb95df8ba4564cce15273769307b4 Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 27 May 2025 14:43:37 +0200 Subject: [PATCH] fix false positive "unreachable oneway" validation cased by oneway=-1 fixes #11068 --- CHANGELOG.md | 2 + modules/validations/impossible_oneway.js | 57 +++---- test/spec/validations/impossible_oneway.js | 187 +++++++++++++++++++++ 3 files changed, 216 insertions(+), 30 deletions(-) create mode 100644 test/spec/validations/impossible_oneway.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee90bffd..c76f8b2b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ _Breaking developer changes, which may affect downstream projects or sites that #### :bug: Bugfixes * Refresh numeric input fields after leaving focus with the value that is stored in the tag ([#11027]) * Fix oneway field falsely showing "Assumed to be Yes" if cycled through all options back to the default state +* Fix false positives in "unreachable oneway" validation when `oneway=-1` tag is present ([#11068]) #### :earth_asia: Localization #### :hourglass: Performance #### :mortar_board: Walkthrough / Help @@ -60,6 +61,7 @@ _Breaking developer changes, which may affect downstream projects or sites that [#11014]: https://github.com/openstreetmap/iD/pull/11014 [#11027]: https://github.com/openstreetmap/iD/pull/11027 [#11054]: https://github.com/openstreetmap/iD/issues/1104 +[#11068]: https://github.com/openstreetmap/iD/issues/11068 [@keiffer213]: https://github.com/keiffer213 [@haipq07]: https://github.com/haipq07 diff --git a/modules/validations/impossible_oneway.js b/modules/validations/impossible_oneway.js index 65022572b..fe2ad6106 100644 --- a/modules/validations/impossible_oneway.js +++ b/modules/validations/impossible_oneway.js @@ -7,22 +7,18 @@ import { validationIssue, validationIssueFix } from '../core/validation'; import { services } from '../services'; export function validationImpossibleOneway() { - var type = 'impossible_oneway'; - - var validation = function checkImpossibleOneway(entity, graph) { + const type = 'impossible_oneway'; + const validation = function checkImpossibleOneway(entity, graph) { if (entity.type !== 'way' || entity.geometry(graph) !== 'line') return []; - if (entity.isClosed()) return []; - if (!typeForWay(entity)) return []; - if (!entity.isOneWay()) return []; - var firstIssues = issuesForNode(entity, entity.first()); - var lastIssues = issuesForNode(entity, entity.last()); - - return firstIssues.concat(lastIssues); + return [ + ...issuesForNode(entity, entity.first()), + ...issuesForNode(entity, entity.last()) + ]; function typeForWay(way) { if (way.geometry(graph) !== 'line') return null; @@ -33,10 +29,10 @@ export function validationImpossibleOneway() { } function nodeOccursMoreThanOnce(way, nodeID) { - var occurrences = 0; - for (var index in way.nodes) { + let occurrences = 0; + for (const index in way.nodes) { if (way.nodes[index] === nodeID) { - occurrences += 1; + occurrences++; if (occurrences > 1) return true; } } @@ -90,25 +86,21 @@ export function validationImpossibleOneway() { } function issuesForNode(way, nodeID) { - - var isFirst = nodeID === way.first(); - - var wayType = typeForWay(way); + const isFirst = (nodeID === way.first()) ^ way.isOneWayBackwards(); + const wayType = typeForWay(way); // ignore if this way is self-connected at this node if (nodeOccursMoreThanOnce(way, nodeID)) return []; - var osm = services.osm; + const osm = services.osm; if (!osm) return []; - - var node = graph.hasEntity(nodeID); - + const node = graph.hasEntity(nodeID); // ignore if this node or its tile are unloaded if (!node || !osm.isDataLoaded(node.loc)) return []; if (isConnectedViaOtherTypes(way, node)) return []; - var attachedWaysOfSameType = graph.parentWays(node).filter(function(parentWay) { + const attachedWaysOfSameType = graph.parentWays(node).filter(parentWay => { if (parentWay.id === way.id) return false; return typeForWay(parentWay) === wayType; }); @@ -116,25 +108,30 @@ export function validationImpossibleOneway() { // assume it's okay for waterways to start or end disconnected for now if (wayType === 'waterway' && attachedWaysOfSameType.length === 0) return []; - var attachedOneways = attachedWaysOfSameType.filter(function(attachedWay) { - return attachedWay.isOneWay(); - }); + const attachedOneways = attachedWaysOfSameType + .filter(attachedWay => attachedWay.isOneWay()); // ignore if the way is connected to some non-oneway features if (attachedOneways.length < attachedWaysOfSameType.length) return []; if (attachedOneways.length) { - var connectedEndpointsOkay = attachedOneways.some(function(attachedOneway) { - if ((isFirst ? attachedOneway.first() : attachedOneway.last()) !== nodeID) return true; + const connectedEndpointsOkay = attachedOneways.some(attachedOneway => { + const isAttachedBackwards = attachedOneway.isOneWayBackwards(); + if ((isFirst ^ isAttachedBackwards + ? attachedOneway.first() + : attachedOneway.last() + ) !== nodeID) { + return true; + } if (nodeOccursMoreThanOnce(attachedOneway, nodeID)) return true; return false; }); if (connectedEndpointsOkay) return []; } - var placement = isFirst ? 'start' : 'end', - messageID = wayType + '.', - referenceID = wayType + '.'; + const placement = isFirst ? 'start' : 'end'; + let messageID = wayType + '.'; + let referenceID = wayType + '.'; if (wayType === 'waterway') { messageID += 'connected.' + placement; diff --git a/test/spec/validations/impossible_oneway.js b/test/spec/validations/impossible_oneway.js new file mode 100644 index 000000000..b60abf062 --- /dev/null +++ b/test/spec/validations/impossible_oneway.js @@ -0,0 +1,187 @@ +describe('iD.validations.impossible_oneway', function() { + let context; + + beforeEach(function() { + context = iD.coreContext().assetPath('../dist/').init(); + iD.services.osm = { isDataLoaded: () => true }; + }); + + function validate() { + const validator = iD.validationImpossibleOneway(context); + const changes = context.history().changes(); + const entities = changes.modified.concat(changes.created); + const issues = entities.flatMap(entity => + validator(entity, context.graph())); + return issues; + } + + it('has no errors on init', function() { + var issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + describe('highways', function() { + it('does not flag properly connecting oneway roads', function() { + context.perform(...[ + iD.osmNode({ id: 'n-0', loc: [2, 1] }), + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmNode({ id: 'n-3', loc: [3, 0] }), + iD.osmWay({ id: 'w-0', nodes: ['n-1', 'n-0', 'n-3'], tags: { + 'highway': 'unclassified' + }}), + iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}), + iD.osmWay({ id: 'w-2', nodes: ['n-2', 'n-3'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('flags dangling oneway end', function() { + context.perform(...[ + iD.osmNode({ id: 'n-0', loc: [0, 0] }), + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmWay({ id: 'w-0', nodes: ['n-0', 'n-1'], tags: { + 'highway': 'unclassified' + }}), + iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(1); + const issue1 = issues[0]; + expect(issue1.type).to.eql('impossible_oneway'); + expect(issue1.subtype).to.eql('highway'); + expect(issue1.severity).to.eql('warning'); + expect(issue1.entityIds).to.eql(['w-1', 'n-2']); + }); + + it('flags unconnected oneway start', function() { + context.perform(...[ + iD.osmNode({ id: 'n-0', loc: [0, 0] }), + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmWay({ id: 'w-0', nodes: ['n-0', 'n-1'], tags: { + 'highway': 'unclassified' + }}), + iD.osmWay({ id: 'w-1', nodes: ['n-2', 'n-1'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(1); + const issue1 = issues[0]; + expect(issue1.type).to.eql('impossible_oneway'); + expect(issue1.subtype).to.eql('highway'); + expect(issue1.severity).to.eql('warning'); + expect(issue1.entityIds).to.eql(['w-1', 'n-2']); + }); + + it('flags oneway pointing to each other', function() { + context.perform(...[ + iD.osmNode({ id: 'n-0', loc: [2, 1] }), + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmNode({ id: 'n-3', loc: [3, 0] }), + iD.osmWay({ id: 'w-0', nodes: ['n-1', 'n-0', 'n-3'], tags: { + 'highway': 'unclassified' + }}), + iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}), + iD.osmWay({ id: 'w-2', nodes: ['n-3', 'n-2'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(2); + const issue1 = issues[0]; + expect(issue1.type).to.eql('impossible_oneway'); + expect(issue1.subtype).to.eql('highway'); + expect(issue1.severity).to.eql('warning'); + expect(issue1.entityIds).to.eql(['w-1', 'n-2']); + const issue2 = issues[1]; + expect(issue2.type).to.eql('impossible_oneway'); + expect(issue2.entityIds).to.eql(['w-2', 'n-2']); + }); + + it('does not flags oneway with reverse "-1" oneway direction', function() { + context.perform(...[ + iD.osmNode({ id: 'n-0', loc: [2, 1] }), + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmNode({ id: 'n-3', loc: [3, 0] }), + iD.osmWay({ id: 'w-0', nodes: ['n-1', 'n-0', 'n-3'], tags: { + 'highway': 'unclassified' + }}), + iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { + 'highway': 'unclassified', + 'oneway' : 'yes' + }}), + iD.osmWay({ id: 'w-2', nodes: ['n-3', 'n-2'], tags: { + 'highway': 'unclassified', + 'oneway' : '-1' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + }); + + describe('waterways', function() { + it('does not flag unconnected start or end points', function() { + context.perform(...[ + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { + 'waterway': 'stream' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(0); + }); + + it('flags waterways pointing to each other', function() { + context.perform(...[ + iD.osmNode({ id: 'n-1', loc: [1, 0] }), + iD.osmNode({ id: 'n-2', loc: [2, 0] }), + iD.osmNode({ id: 'n-3', loc: [3, 0] }), + iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { + 'waterway': 'stream' + }}), + iD.osmWay({ id: 'w-2', nodes: ['n-3', 'n-2'], tags: { + 'waterway': 'stream' + }}) + ].map(iD.actionAddEntity)); + + const issues = validate(); + expect(issues).to.have.lengthOf(2); + const issue1 = issues[0]; + expect(issue1.type).to.eql('impossible_oneway'); + expect(issue1.subtype).to.eql('waterway'); + expect(issue1.severity).to.eql('warning'); + expect(issue1.entityIds).to.eql(['w-1', 'n-2']); + const issue2 = issues[1]; + expect(issue2.type).to.eql('impossible_oneway'); + expect(issue2.entityIds).to.eql(['w-2', 'n-2']); + }); + }); +});