Skip to content

Commit

Permalink
Revalidate disconnected_way and impossible_oneway errors on change (#…
Browse files Browse the repository at this point in the history 
…8911)
  • Loading branch information
@andrewpmk
andrewpmk authored Feb 15, 2025
1 parent 022fc3c commit a703895
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 23 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
#### :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])
* 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])
#### :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])
Expand All @@ -66,6 +67,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
#### :hammer: Development

[#7381]: https://github.com/openstreetmap/iD/issues/7381
[#8911]: https://github.com/openstreetmap/iD/pull/8911
[#9635]: https://github.com/openstreetmap/iD/pull/9635
[#10003]: https://github.com/openstreetmap/iD/pull/10003
[#10648]: https://github.com/openstreetmap/iD/pull/10648
Expand All @@ -83,6 +85,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
[@Deeptanshu-sankhwar]: https://github.com/Deeptanshu-sankhwar
[@draunger]: https://github.com/draunger
[@burrscurr]: https://github.com/burrscurr
[@andrewpmk]: https://github.com/andrewpmk


# 2.31.1
Expand Down
15 changes: 13 additions & 2 deletions modules/core/validator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,14 +495,25 @@ export function coreValidator(context) {
_headCache.graph = currGraph; // take snapshot
_completeDiff = context.history().difference().complete();
const incrementalDiff = coreDifference(prevGraph, currGraph);
let entityIDs = Object.keys(incrementalDiff.complete());
entityIDs = _headCache.withAllRelatedEntities(entityIDs); // expand set
const diff = Object.keys(incrementalDiff.complete());
const entityIDs = _headCache.withAllRelatedEntities(diff); // expand set

if (!entityIDs.size) {
dispatch.call('validated');
return Promise.resolve();
}

// Re-validate also connected (or previously connected) entities to the current way
// fix #8758
const addConnectedWays = graph => diff
.filter(entityID => graph.hasEntity(entityID))
.map(entityID => graph.entity(entityID))
.flatMap(entity => graph.childNodes(entity))
.flatMap(vertex => graph.parentWays(vertex))
.forEach(way => entityIDs.add(way.id));
addConnectedWays(currGraph);
addConnectedWays(prevGraph);

_headPromise = validateEntitiesAsync(entityIDs, _headCache)
.then(() => updateResolvedIssues(entityIDs))
.then(() => dispatch.call('validated'))
Expand Down
84 changes: 80 additions & 4 deletions test/spec/core/validator.js
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
describe('iD.coreValidator', function () {
describe('iD.coreValidator', function() {
var context;

beforeEach(function() {
context = iD.coreContext().assetPath('../dist/').init();
});

function createInvalidWay() {
var n1 = iD.osmNode({id: 'n-1', loc: [4,4]});
var n2 = iD.osmNode({id: 'n-2', loc: [4,5]});
var w = iD.osmWay({id: 'w-1', nodes: ['n-1', 'n-2']});
var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] });
var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] });
var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'] });

context.perform(
iD.actionAddEntity(n1),
Expand Down Expand Up @@ -47,4 +47,80 @@ describe('iD.coreValidator', function () {
});
});

it('removes validation issue when highway is no longer disconnected', function(done) {
// Add a way which is disconnected from the rest of the map
var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] });
var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] });
var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { 'highway': 'unclassified' } });
context.perform(
iD.actionAddEntity(n1),
iD.actionAddEntity(n2),
iD.actionAddEntity(w)
);
var validator = new iD.coreValidator(context);
validator.init();
validator.validate().then(function() {
// Should produce disconnected way error
let issues = validator.getIssues();
expect(issues).to.have.lengthOf(1);

// Add new node with entrance node to simulate connection with rest of map
var n3 = iD.osmNode({ id: 'n-3', loc: [4, 6], tags: { 'entrance': 'yes' } });
var w2 = iD.osmWay({ id: 'w-2', nodes: ['n-2', 'n-3'], tags: { 'highway': 'unclassified' } });
context.perform(
iD.actionAddEntity(n3),
iD.actionAddEntity(w2)
);
validator.validate().then(function() {
// Should be no errors
issues = validator.getIssues();
expect(issues).to.have.lengthOf(0);
done();
}).catch(function(err) {
done(err);
});
}).catch(function(err) {
done(err);
});
});

it('add validation issue when highway becomes disconnected', function(done) {
// Add a way which is connected to another way with an entrance node to simulate connection with rest of map
var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] });
var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] });
var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { 'highway': 'unclassified' } });
var n3 = iD.osmNode({ id: 'n-3', loc: [4, 6], tags: { 'entrance': 'yes' } });
var w2 = iD.osmWay({ id: 'w-2', nodes: ['n-2', 'n-3'], tags: { 'highway': 'unclassified' } });
context.perform(
iD.actionAddEntity(n1),
iD.actionAddEntity(n2),
iD.actionAddEntity(w),
iD.actionAddEntity(n3),
iD.actionAddEntity(w2)
);
var validator = new iD.coreValidator(context);
validator.init();
validator.validate().then(function() {
// Should be no errors
let issues = validator.getIssues();
expect(issues).to.have.lengthOf(0);

// delete second way -> first way becomes disconnected form the rest of the network
context.perform(
iD.actionDeleteWay(w2.id)
);

validator.validate().then(function() {
// Should produce disconnected way error
issues = validator.getIssues();
expect(issues).to.have.lengthOf(1);
done();
}).catch(function(err) {
done(err);
});
}).catch(function(err) {
done(err);
});
});

});
33 changes: 16 additions & 17 deletions test/spec/validations/disconnected_way.js
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
describe('iD.validations.disconnected_way', function () {
describe('iD.validations.disconnected_way', function() {
var context;

beforeEach(function() {
context = iD.coreContext().assetPath('../dist/').init();
});

function createWay(tags) {
var n1 = iD.osmNode({id: 'n-1', loc: [4,4]});
var n2 = iD.osmNode({id: 'n-2', loc: [4,5]});
var w = iD.osmWay({id: 'w-1', nodes: ['n-1', 'n-2'], tags: tags});
var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4] });
var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] });
var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: tags });

context.perform(
iD.actionAddEntity(n1),
Expand All @@ -18,11 +18,11 @@ describe('iD.validations.disconnected_way', function () {
}

function createConnectingWays(tags1, tags2) {
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: [5,5]});
var w = iD.osmWay({id: 'w-1', nodes: ['n-1', 'n-2'], tags: tags1});
var w2 = iD.osmWay({id: 'w-2', nodes: ['n-1', 'n-3'], tags: tags2});
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: [5, 5] });
var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: tags1 });
var w2 = iD.osmWay({ id: 'w-2', nodes: ['n-1', 'n-3'], tags: tags2 });

context.perform(
iD.actionAddEntity(n1),
Expand Down Expand Up @@ -50,7 +50,7 @@ describe('iD.validations.disconnected_way', function () {
});

it('flags disconnected highway', function() {
createWay({'highway': 'unclassified'});
createWay({ 'highway': 'unclassified' });
var issues = validate();
expect(issues).to.have.lengthOf(1);
var issue = issues[0];
Expand All @@ -62,7 +62,7 @@ describe('iD.validations.disconnected_way', function () {
});

it('flags highway connected only to service area', function() {
createConnectingWays({'highway': 'unclassified'}, {'highway': 'services'});
createConnectingWays({ 'highway': 'unclassified' }, { 'highway': 'services' });
var issues = validate();
expect(issues).to.have.lengthOf(1);
var issue = issues[0];
Expand All @@ -75,11 +75,11 @@ describe('iD.validations.disconnected_way', function () {

it('ignores highway with connected entrance vertex', function() {

var n1 = iD.osmNode({id: 'n-1', loc: [4,4], tags: {'entrance': 'yes'}});
var n2 = iD.osmNode({id: 'n-2', loc: [4,5]});
var n3 = iD.osmNode({id: 'n-3', loc: [5,5]});
var w = iD.osmWay({id: 'w-1', nodes: ['n-1', 'n-2'], tags: {'highway': 'unclassified'}});
var w2 = iD.osmWay({id: 'w-2', nodes: ['n-1', 'n-3']});
var n1 = iD.osmNode({ id: 'n-1', loc: [4, 4], tags: { 'entrance': 'yes' } });
var n2 = iD.osmNode({ id: 'n-2', loc: [4, 5] });
var n3 = iD.osmNode({ id: 'n-3', loc: [5, 5] });
var w = iD.osmWay({ id: 'w-1', nodes: ['n-1', 'n-2'], tags: { 'highway': 'unclassified' } });
var w2 = iD.osmWay({ id: 'w-2', nodes: ['n-1', 'n-3'] });

context.perform(
iD.actionAddEntity(n1),
Expand All @@ -92,5 +92,4 @@ describe('iD.validations.disconnected_way', function () {
var issues = validate();
expect(issues).to.have.lengthOf(0);
});

});

0 comments on commit a703895

Please sign in to comment.