From 918126e5f894d9b00dd65cd03709bbc6f7e82f7a Mon Sep 17 00:00:00 2001 From: Martin Raifer Date: Tue, 18 Feb 2025 13:18:09 +0100 Subject: [PATCH] revalidate entities with changed relation memberships --- CHANGELOG.md | 1 + modules/core/validator.js | 22 +++++++++++-- test/spec/core/validator.js | 63 +++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed3308514..bdff6cc2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ _Breaking developer changes, which may affect downstream projects or sites that * 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]) * The Suspicious Names validator warning now compares the Name field to the preset’s name in the user’s language, not just the raw tag value (typically in British English). ([#9522], thanks [@k-yle]) * Revalidate ways that are connected to the currently edited way to also properly update/catch _disconnected way_s and _impossible oneway_ errors ([#8911], thanks [@andrewpmk]) +* Revalidate ways that are added to or removed from relations ([#10786]) * Preserve `crossing:markings` tag when fixing missing connection of crossing path and road ([#9586], thanks [@jtracey]) * Add a dedicated description to fix waterway-road intersections by adding a _culvert_ ([#10778], thanks [@matkoniecz]) #### :bug: Bugfixes diff --git a/modules/core/validator.js b/modules/core/validator.js index ce5cf4b22..0ad198ddc 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -4,7 +4,7 @@ import { prefs } from './preferences'; import { coreDifference } from './difference'; import { geoExtent } from '../geo/extent'; import { modeSelect } from '../modes/select'; -import { utilArrayChunk, utilArrayGroupBy, utilEntityAndDeepMemberIDs, utilRebind } from '../util'; +import { utilArrayChunk, utilArrayDifference, utilArrayGroupBy, utilArrayIntersection, utilArrayUnion, utilEntityAndDeepMemberIDs, utilRebind } from '../util'; import * as Validations from '../validations/index'; @@ -503,8 +503,8 @@ export function coreValidator(context) { return Promise.resolve(); } - // Re-validate also connected (or previously connected) entities to the current way - // fix #8758 + // revalidate also connected (or previously connected) entities to the current way + // https://github.com/openstreetmap/iD/issues/8758 const addConnectedWays = graph => diff .filter(entityID => graph.hasEntity(entityID)) .map(entityID => graph.entity(entityID)) @@ -514,6 +514,22 @@ export function coreValidator(context) { addConnectedWays(currGraph); addConnectedWays(prevGraph); + // revalidate entities with changed relation memberships + // https://github.com/openstreetmap/iD/issues/10786 + Object.values({...incrementalDiff.created(), ...incrementalDiff.deleted()}) + .filter(e => e.type === 'relation') + .flatMap(r => r.members) + .forEach(m => entityIDs.add(m.id)); + Object.values(incrementalDiff.modified()) + .filter(e => e.type === 'relation') + .map(r => ({ baseEntity: prevGraph.entity(r.id), headEntity: r })) + .forEach(({ baseEntity, headEntity }) => { + const bm = baseEntity.members.map(m => m.id); + const hm = headEntity.members.map(m => m.id); + const symDiff = utilArrayDifference(utilArrayUnion(bm, hm), utilArrayIntersection(bm, hm)); + symDiff.forEach(id => entityIDs.add(id)); + }); + _headPromise = validateEntitiesAsync(entityIDs, _headCache) .then(() => updateResolvedIssues(entityIDs)) .then(() => dispatch.call('validated')) diff --git a/test/spec/core/validator.js b/test/spec/core/validator.js index b86261b09..1edae4b08 100644 --- a/test/spec/core/validator.js +++ b/test/spec/core/validator.js @@ -102,4 +102,67 @@ describe('iD.coreValidator', function() { issues = validator.getIssues(); expect(issues).to.have.lengthOf(1); }); + + it('removes validation issue when untagged way is becomes part of a boundary relation', async () => { + var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] }); + var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] }); + var n3 = iD.osmNode({ id: 'n-3', loc: [4, 6] }); + var w1 = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { 'note': 'foo' } }); + var w2 = iD.osmWay({ id: 'w-2', nodes: ['n-2', 'n-3'], tags: {} }); + var r = iD.osmRelation({ id: 'r-1', members: [{ id: 'w-2' }], tags: { 'type': 'boundary' } }); + context.perform( + iD.actionAddEntity(n1), + iD.actionAddEntity(n2), + iD.actionAddEntity(n3), + iD.actionAddEntity(w1), + iD.actionAddEntity(w2), + iD.actionAddEntity(r) + ); + var validator = new iD.coreValidator(context); + validator.init(); + await validator.validate(); + // There should be a validation error about the untagged way + let issues = validator.getIssues(); + expect(issues).to.have.lengthOf(1); + + // add way to relation + context.perform( + iD.actionAddMember(r.id, { id: w1.id }) + ); + + await validator.validate(); + // Validation error should be fixed + issues = validator.getIssues(); + expect(issues).to.have.lengthOf(0); + }); + + it('add validation issue when untagged way is removed from boundary relation', async () => { + // A way is "untagged", but part of a larger (boundary) relation + var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] }); + var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] }); + var w1 = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { 'note': 'foo' } }); + var r = iD.osmRelation({ id: 'r-1', members: [{ id: 'w-1' }], tags: { 'type': 'boundary' } }); + context.perform( + iD.actionAddEntity(n1), + iD.actionAddEntity(n2), + iD.actionAddEntity(w1), + iD.actionAddEntity(r) + ); + var validator = new iD.coreValidator(context); + validator.init(); + await validator.validate(); + // Should be no errors + let issues = validator.getIssues(); + expect(issues).to.have.lengthOf(0); + + // delete relation + context.perform( + iD.actionDeleteRelation(r.id) + ); + + await validator.validate(); + // Should produce untagged feature error + issues = validator.getIssues(); + expect(issues).to.have.lengthOf(1); + }); });