From 9e3df2c4aa342967addf1daf9585fd7f3eea6b8c Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Thu, 26 Aug 2021 16:04:51 -0400 Subject: [PATCH 1/5] Create an issue `key` property that changes when data needs refresh (closes #8655) --- modules/core/validation/models.js | 8 ++++++++ modules/ui/commit_warnings.js | 2 +- modules/ui/sections/entity_issues.js | 2 +- modules/ui/sections/validation_issues.js | 2 +- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/modules/core/validation/models.js b/modules/core/validation/models.js index b9819e060..d6215ba6d 100644 --- a/modules/core/validation/models.js +++ b/modules/core/validation/models.js @@ -14,6 +14,7 @@ export function validationIssue(attrs) { this.hash = attrs.hash; // optional - string to further differentiate the issue this.id = generateID.apply(this); // generated - see below + this.key = generateKey.apply(this); // generated - see below (call after generating this.id) this.autoFix = null; // generated - if autofix exists, will be set below // A unique, deterministic string hash. @@ -39,6 +40,13 @@ export function validationIssue(attrs) { return parts.join(':'); } + // An identifier suitable for use as the second argument to d3.selection#data(). + // (i.e. this should change whenever the data needs to be refreshed) + function generateKey() { + return this.id + ':' + Date.now().toString(); // include time of creation + } + + this.extent = function(resolver) { if (this.loc) { return geoExtent(this.loc); diff --git a/modules/ui/commit_warnings.js b/modules/ui/commit_warnings.js index e02656c2c..b83f528fc 100644 --- a/modules/ui/commit_warnings.js +++ b/modules/ui/commit_warnings.js @@ -43,7 +43,7 @@ export function uiCommitWarnings(context) { var items = container.select('ul').selectAll('li') - .data(issues, function(d) { return d.id; }); + .data(issues, function(d) { return d.key; }); items.exit() .remove(); diff --git a/modules/ui/sections/entity_issues.js b/modules/ui/sections/entity_issues.js index e3e8ebac1..5ca2d1de7 100644 --- a/modules/ui/sections/entity_issues.js +++ b/modules/ui/sections/entity_issues.js @@ -55,7 +55,7 @@ export function uiSectionEntityIssues(context) { _activeIssueID = _issues.length > 0 ? _issues[0].id : null; var containers = selection.selectAll('.issue-container') - .data(_issues, function(d) { return d.id; }); + .data(_issues, function(d) { return d.key; }); // Exit containers.exit() diff --git a/modules/ui/sections/validation_issues.js b/modules/ui/sections/validation_issues.js index b22625136..6b0ceeeac 100644 --- a/modules/ui/sections/validation_issues.js +++ b/modules/ui/sections/validation_issues.js @@ -73,7 +73,7 @@ export function uiSectionValidationIssues(id, severity, context) { var items = list.selectAll('li') - .data(issues, function(d) { return d.id; }); + .data(issues, function(d) { return d.key; }); // Exit items.exit() From 1b5bd4d9c39c061fbc3431cfaff39747c64de28e Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Fri, 27 Aug 2021 17:14:06 -0400 Subject: [PATCH 2/5] Actions performed (e.g. adding midpoint) must trigger validation (re: #8655) --- modules/modes/select.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/modes/select.js b/modules/modes/select.js index 582c956c5..d141ce773 100644 --- a/modules/modes/select.js +++ b/modules/modes/select.js @@ -429,11 +429,14 @@ export function modeSelect(context, selectedIDs) { actionAddMidpoint({ loc: choice.loc, edge: [prev, next] }, osmNode()), t('operations.add.annotation.vertex') ); + context.validator().validate(); } else if (entity.type === 'midpoint') { context.perform( actionAddMidpoint({ loc: entity.loc, edge: entity.edge }, osmNode()), - t('operations.add.annotation.vertex')); + t('operations.add.annotation.vertex') + ); + context.validator().validate(); } } @@ -688,6 +691,7 @@ export function modeSelect(context, selectedIDs) { // the user added this relation but didn't edit it at all, so just delete it var deleteAction = actionDeleteRelation(entity.id, true /* don't delete untagged members */); context.perform(deleteAction, t('operations.delete.annotation.relation')); + context.validator().validate(); } }; From b94151396d43c46bca3b7ce9761a3b2845ab4f1d Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Fri, 27 Aug 2021 17:20:32 -0400 Subject: [PATCH 3/5] Expand set of entities to validate to include _related_ to issues This will catch the situation where an edit or undo affects something related without actually touching the item - for example an undo can cause a connected way to disconnect from the main graph. --- modules/core/validator.js | 83 +++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 26 deletions(-) diff --git a/modules/core/validator.js b/modules/core/validator.js index 0f8a3a1c8..ac04e92a3 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -488,10 +488,10 @@ export function coreValidator(context) { _headCache.graph = currGraph; // take snapshot _completeDiff = context.history().difference().complete(); const incrementalDiff = coreDifference(prevGraph, currGraph); - const entityIDs = Object.keys(incrementalDiff.complete()); + let entityIDs = Object.keys(incrementalDiff.complete()); + entityIDs = _headCache.withAllRelatedEntities(entityIDs); // expand set - // if (!entityIDs.size) { - if (!entityIDs.length) { + if (!entityIDs.size) { dispatch.call('validated'); return Promise.resolve(); } @@ -540,8 +540,8 @@ export function coreValidator(context) { if (!_headCache.graph) _headCache.graph = baseGraph; if (!_baseCache.graph) _baseCache.graph = baseGraph; - const entityIDs = entities.map(entity => entity.id); - // entityIDs = entityIDsToValidate(entityIDs, baseGraph); // expand set + let entityIDs = entities.map(entity => entity.id); + entityIDs = _baseCache.withAllRelatedEntities(entityIDs); // expand set validateEntitiesAsync(entityIDs, _baseCache); }); @@ -629,7 +629,7 @@ export function coreValidator(context) { // - the user did something to one of the entities involved in the issue // // Arguments - // `entityIDs` - Array containing entity IDs. + // `entityIDs` - Array or Set containing entity IDs. // function updateResolvedIssues(entityIDs) { entityIDs.forEach(entityID => { @@ -656,7 +656,7 @@ export function coreValidator(context) { // Schedule validation for many entities. // // Arguments - // `entityIDs` - Array containing entity IDs. + // `entityIDs` - Array or Set containing entityIDs. // `graph` - the graph to validate that contains those entities // `cache` - the cache to store results in (_headCache or _baseCache) // @@ -666,7 +666,7 @@ export function coreValidator(context) { // function validateEntitiesAsync(entityIDs, cache) { // Enqueue the work - const jobs = entityIDs.map(entityID => { + const jobs = Array.from(entityIDs).map(entityID => { if (cache.queuedEntityIDs.has(entityID)) return null; // queued already cache.queuedEntityIDs.add(entityID); @@ -795,24 +795,20 @@ function validationCache(which) { issuesByEntityID: {} // entity.id -> Set(issue.id) }; - cache.cacheIssues = (issues) => { - issues.forEach(issue => { - const entityIDs = issue.entityIds || []; - entityIDs.forEach(entityID => { - if (!cache.issuesByEntityID[entityID]) { - cache.issuesByEntityID[entityID] = new Set(); - } - cache.issuesByEntityID[entityID].add(issue.id); - }); - cache.issuesByIssueID[issue.id] = issue; + + cache.cacheIssue = (issue) => { + (issue.entityIds || []).forEach(entityID => { + if (!cache.issuesByEntityID[entityID]) { + cache.issuesByEntityID[entityID] = new Set(); + } + cache.issuesByEntityID[entityID].add(issue.id); }); + cache.issuesByIssueID[issue.id] = issue; }; + cache.uncacheIssue = (issue) => { - // When multiple entities are involved (e.g. crossing_ways), - // remove this issue from the other entity caches too.. - const entityIDs = issue.entityIds || []; - entityIDs.forEach(entityID => { + (issue.entityIds || []).forEach(entityID => { if (cache.issuesByEntityID[entityID]) { cache.issuesByEntityID[entityID].delete(issue.id); } @@ -820,25 +816,33 @@ function validationCache(which) { delete cache.issuesByIssueID[issue.id]; }; + + cache.cacheIssues = (issues) => { + issues.forEach(cache.cacheIssue); + }; + + cache.uncacheIssues = (issues) => { issues.forEach(cache.uncacheIssue); }; + cache.uncacheIssuesOfType = (type) => { const issuesOfType = Object.values(cache.issuesByIssueID) .filter(issue => issue.type === type); cache.uncacheIssues(issuesOfType); }; + // Remove a single entity and all its related issues from the caches cache.uncacheEntityID = (entityID) => { - const issueIDs = cache.issuesByEntityID[entityID]; - if (issueIDs) { - issueIDs.forEach(issueID => { + const entityIssueIDs = cache.issuesByEntityID[entityID]; + if (entityIssueIDs) { + entityIssueIDs.forEach(issueID => { const issue = cache.issuesByIssueID[issueID]; if (issue) { cache.uncacheIssue(issue); - } else { + } else { // shouldnt happen, clean up delete cache.issuesByIssueID[issueID]; } }); @@ -849,5 +853,32 @@ function validationCache(which) { }; + // Return the expandeded set of entityIDs related to issues for the given entityIDs + // + // Arguments + // `entityIDs` - Array or Set containing entityIDs. + // + cache.withAllRelatedEntities = (entityIDs) => { + let result = new Set(); + (entityIDs || []).forEach(entityID => { + result.add(entityID); // include self + + const entityIssueIDs = cache.issuesByEntityID[entityID]; + if (entityIssueIDs) { + entityIssueIDs.forEach(issueID => { + const issue = cache.issuesByIssueID[issueID]; + if (issue) { + (issue.entityIds || []).forEach(relatedID => result.add(relatedID)) + } else { // shouldnt happen, clean up + delete cache.issuesByIssueID[issueID]; + } + }); + } + }); + + return result; + } + + return cache; } From 113f0794492f8bb88c0b625067c3705863e11def Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Fri, 27 Aug 2021 17:24:45 -0400 Subject: [PATCH 4/5] Uncache the entity before starting the work This fixes a situation where several entities in the queue are involved in a disconnected crossing, and the first one detects the disconnection, but a later one clears out that first detection from the cache. Now we clear caches one time before starting the validation work. --- modules/core/validator.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/core/validator.js b/modules/core/validator.js index ac04e92a3..dd634726b 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -670,9 +670,10 @@ export function coreValidator(context) { if (cache.queuedEntityIDs.has(entityID)) return null; // queued already cache.queuedEntityIDs.add(entityID); + // Clear caches for existing issues related to this entity + cache.uncacheEntityID(entityID); + return () => { - // Clear caches for existing issues related to this entity - cache.uncacheEntityID(entityID); cache.queuedEntityIDs.delete(entityID); const graph = cache.graph; From 78961072a79e66789dadc0486d5979105f7bbe49 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Fri, 27 Aug 2021 17:43:49 -0400 Subject: [PATCH 5/5] We do need to actually validate the head entities, can't skip them re: the fix for #8632 - we can't actually skip validation on these. The better solution is to move the check to getIssues() so the user isn't credited for causing the issues unless it's something they actually touched --- modules/core/validator.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/modules/core/validator.js b/modules/core/validator.js index dd634726b..2e899d9b4 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -189,16 +189,22 @@ export function coreValidator(context) { let seen = new Set(); let results = []; - // collect head issues - caused by user edits + // collect head issues - present in the user edits if (_headCache.graph && _headCache.graph !== _baseCache.graph) { Object.values(_headCache.issuesByIssueID).forEach(issue => { + // In the head cache, only count features that the user is responsible for - #8632 + // For example, a user can undo some work and an issue will still present in the + // head graph, but we don't want to credit the user for causing that issue. + const userModified = (issue.entityIds || []).some(id => _completeDiff.hasOwnProperty(id)); + if (opts.what === 'edited' && !userModified) return; // present in head but user didn't touch it + if (!filter(issue)) return; seen.add(issue.id); results.push(issue); }); } - // collect base issues - not caused by user edits + // collect base issues - present before user edits if (opts.what === 'all') { Object.values(_baseCache.issuesByIssueID).forEach(issue => { if (!filter(issue)) return; @@ -682,11 +688,6 @@ export function coreValidator(context) { const entity = graph.hasEntity(entityID); // Sanity check: don't validate deleted entities if (!entity) return; - // In the head cache, only validate features that the user is responsible for - #8632 - // For example, a user can undo some work and an issue will still present in the - // head graph, but we don't want to credit the user for causing that issue. - if (cache.which === 'head' && !_completeDiff.hasOwnProperty(entityID)) return; - // detect new issues and update caches const result = validateEntity(entity, graph); if (result.provisional) { // provisional result @@ -869,7 +870,7 @@ function validationCache(which) { entityIssueIDs.forEach(issueID => { const issue = cache.issuesByIssueID[issueID]; if (issue) { - (issue.entityIds || []).forEach(relatedID => result.add(relatedID)) + (issue.entityIds || []).forEach(relatedID => result.add(relatedID)); } else { // shouldnt happen, clean up delete cache.issuesByIssueID[issueID]; } @@ -878,7 +879,7 @@ function validationCache(which) { }); return result; - } + }; return cache;