Skip to content

Commit 9240139

Browse files
cmarkcmark
authored
Merge pull request #1322 from b2ihealthcare/issue/SO-6276-lcs-parent-update-cycles
SO-6276: Improve detection of non-connected cycles in SimpleTaxonomyGraph
2 parents 4118d43 + 070ba9a commit 9240139

File tree

3 files changed

+123
-42
lines changed

3 files changed

+123
-42
lines changed

‎core/com.b2international.snowowl.core.tests/src/com/b2international/snowowl/core/taxonomy/SimpleTaxonomyGraphTest.java‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
package com.b2international.snowowl.core.taxonomy;
1717

1818
import static org.assertj.core.api.Assertions.assertThat;
19-
import static org.junit.Assert.*;
19+
import static org.junit.Assert.assertFalse;
20+
import static org.junit.Assert.assertThrows;
21+
import static org.junit.Assert.assertTrue;
2022

2123
import java.util.Set;
2224

@@ -31,17 +33,21 @@ public class SimpleTaxonomyGraphTest {
3133

3234
private SimpleTaxonomyGraph graph = new SimpleTaxonomyGraph(10, 10);
3335

36+
private boolean graphHasErrors() {
37+
return !graph.build().isEmpty();
38+
}
39+
3440
@Test
3541
public void isBuilt() {
3642
assertFalse("Graph should start out in the not-built state.", graph.isBuilt());
3743
}
3844

3945
@Test
4046
public void buildSetsFlag() {
41-
assertFalse("Graph build should report no errors.", graph.build());
47+
assertFalse("Graph build should report no errors.", graphHasErrors());
4248
assertTrue("Graph should be built.", graph.isBuilt());
4349
}
44-
50+
4551
@Test
4652
public void getDescendantIdsWithoutBuild() {
4753
assertThrows(IllegalStateException.class, () -> graph.getDescendantIds("A"));
@@ -69,7 +75,7 @@ public void updateEdge() {
6975
*/
7076
graph.addEdge("A", "B");
7177
graph.addEdge("A", "C");
72-
assertFalse("Graph build should report no errors.", graph.build());
78+
assertFalse("Graph build should report no errors.", graphHasErrors());
7379

7480
assertThat(graph.getParentIds("A")).containsOnly("C");
7581
}
@@ -91,26 +97,26 @@ public void getIndirectAncestorIdsNull() {
9197

9298
@Test
9399
public void getDescendantIdsUnknownId() {
94-
assertFalse("Graph build should report no errors.", graph.build());
100+
assertFalse("Graph build should report no errors.", graphHasErrors());
95101
assertThrows(NotFoundException.class, () -> graph.getDescendantIds("A"));
96102
}
97103

98104
@Test
99105
public void getParentIdsUnknownId() {
100-
assertFalse("Graph build should report no errors.", graph.build());
106+
assertFalse("Graph build should report no errors.", graphHasErrors());
101107
assertThrows(NotFoundException.class, () -> graph.getParentIds("A"));
102108
}
103109

104110
@Test
105111
public void getIndirectAncestorIdsUnknownId() {
106-
assertFalse("Graph build should report no errors.", graph.build());
112+
assertFalse("Graph build should report no errors.", graphHasErrors());
107113
assertThrows(NotFoundException.class, () -> graph.getIndirectAncestorIds("A"));
108114
}
109115

110116
@Test
111117
public void singleNode() {
112118
graph.addNode("A");
113-
assertFalse("Graph build should report no errors.", graph.build());
119+
assertFalse("Graph build should report no errors.", graphHasErrors());
114120

115121
assertThat(graph.getDescendantIds("A")).isEmpty();
116122
assertThat(graph.getParentIds("A")).isEmpty();
@@ -121,7 +127,7 @@ public void singleNode() {
121127
public void twoIsolatedNodes() {
122128
graph.addNode("A");
123129
graph.addNode("B");
124-
assertFalse("Graph build should report no errors.", graph.build());
130+
assertFalse("Graph build should report no errors.", graphHasErrors());
125131

126132
assertThat(graph.getDescendantIds("A")).isEmpty();
127133
assertThat(graph.getParentIds("A")).isEmpty();
@@ -136,7 +142,7 @@ public void parentChild() {
136142
graph.addNode("A");
137143
graph.addNode("B");
138144
graph.addEdge("A", "B");
139-
assertFalse("Graph build should report no errors.", graph.build());
145+
assertFalse("Graph build should report no errors.", graphHasErrors());
140146

141147
assertThat(graph.getDescendantIds("A")).isEmpty();
142148
assertThat(graph.getParentIds("A")).containsOnly("B");
@@ -153,7 +159,7 @@ public void ancestor() {
153159
graph.addNode("C");
154160
graph.addEdge("A", "B");
155161
graph.addEdge("B", "C");
156-
assertFalse("Graph build should report no errors.", graph.build());
162+
assertFalse("Graph build should report no errors.", graphHasErrors());
157163

158164
assertThat(graph.getDescendantIds("A")).isEmpty();
159165
assertThat(graph.getParentIds("A")).containsOnly("B");
@@ -181,7 +187,7 @@ public void multipleAncestors() {
181187
graph.addEdge("A2", "B");
182188
graph.addEdge("A3", "B");
183189
graph.addEdge("B", Set.of("C1", "C2"));
184-
assertFalse("Graph build should report no errors.", graph.build());
190+
assertFalse("Graph build should report no errors.", graphHasErrors());
185191

186192
assertThat(graph.getDescendantIds("A1")).isEmpty();
187193
assertThat(graph.getParentIds("A1")).containsOnly("B");
@@ -223,7 +229,7 @@ public void deepTreeWithShortcut() {
223229
graph.addEdge("A3", "B");
224230
graph.addEdge("B", "C");
225231
graph.addEdge("C", Set.of("D1", "D2"));
226-
assertFalse("Graph build should report no errors.", graph.build());
232+
assertFalse("Graph build should report no errors.", graphHasErrors());
227233

228234
assertThat(graph.getDescendantIds("A1")).isEmpty();
229235
assertThat(graph.getParentIds("A1")).containsOnly("B", "D1");
@@ -268,7 +274,7 @@ public void descendantProcessing() {
268274
graph.addEdge("C1", "D");
269275
graph.addEdge("C2", "D");
270276
graph.addEdge("D", "E");
271-
assertFalse("Graph build should report no errors.", graph.build());
277+
assertFalse("Graph build should report no errors.", graphHasErrors());
272278

273279
assertThat(graph.getDescendantIds("D")).containsOnly("A", "B", "C1", "C2");
274280
assertThat(graph.getDescendantIds("E")).containsOnly("A", "B", "C1", "C2", "D");
@@ -278,15 +284,15 @@ public void descendantProcessing() {
278284
public void buildUnknownSourceId() {
279285
graph.addNode("B");
280286
graph.addEdge("A", "B");
281-
assertTrue("Graph build should report an error.", graph.build());
287+
assertTrue("Graph build should report an error.", graphHasErrors());
282288
}
283289

284290
@Test
285291
public void buildUnknownDestinationId() {
286292
graph.addNode("A");
287293
graph.addNode("B");
288294
graph.addEdge("A", Set.of("B", "C", "D"));
289-
assertTrue("Graph build should report an error.", graph.build());
295+
assertTrue("Graph build should report an error.", graphHasErrors());
290296
}
291297

292298
@Test
@@ -296,7 +302,17 @@ public void buildSelfCycle() {
296302
graph.addNode("C");
297303
graph.addEdge("A", Set.of("A", "B"));
298304
graph.addEdge("B", "C");
299-
assertTrue("Graph build should report an error.", graph.build());
305+
assertTrue("Graph build should report an error.", graphHasErrors());
306+
}
307+
308+
@Test
309+
public void buildShortCycle() {
310+
graph.addNode("A");
311+
graph.addNode("B");
312+
graph.addNode("C");
313+
graph.addEdge("A", "B");
314+
graph.addEdge("B", "A");
315+
assertTrue("Graph build should report an error.", graphHasErrors());
300316
}
301317

302318
@Test
@@ -310,7 +326,7 @@ public void buildLongCycle() {
310326
graph.addEdge("B", "C");
311327
graph.addEdge("C", "D");
312328
graph.addEdge("D", Set.of("E", "B"));
313-
assertTrue("Graph build should report an error.", graph.build());
329+
assertTrue("Graph build should report an error.", graphHasErrors());
314330
}
315331

316332
@Test
@@ -322,6 +338,6 @@ public void removeEdgeDoesNotRemoveInboundEdgesAutomatically() throws Exception
322338
graph.addEdge("B", "C");
323339
graph.removeNode("B");
324340
graph.removeEdge("B");
325-
assertTrue("Graph build should not report an error.", graph.build());
341+
assertTrue("Graph build should not report an error.", graphHasErrors());
326342
}
327343
}

‎core/com.b2international.snowowl.core/src/com/b2international/snowowl/core/taxonomy/SimpleTaxonomyChangeProcessor.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,19 @@
1919
import static com.google.common.collect.Sets.newHashSet;
2020

2121
import java.io.IOException;
22+
import java.util.List;
2223
import java.util.Map;
24+
import java.util.Optional;
2325
import java.util.Set;
2426

2527
import com.b2international.commons.exceptions.BadRequestException;
28+
import com.b2international.commons.exceptions.CycleDetectedException;
2629
import com.b2international.index.revision.RevisionSearcher;
2730
import com.b2international.index.revision.StagingArea;
2831
import com.b2international.snowowl.core.domain.IComponent;
2932
import com.b2international.snowowl.core.repository.ChangeSetProcessorBase;
3033
import com.b2international.snowowl.core.repository.RevisionDocument;
34+
import com.b2international.snowowl.core.taxonomy.SimpleTaxonomyGraph.Issue;
3135
import com.google.common.collect.Sets;
3236

3337
/**
@@ -160,8 +164,19 @@ public void process(final StagingArea staging, final RevisionSearcher searcher)
160164
taxonomy.removeNode(conceptId);
161165
});
162166

163-
if (taxonomy.build()) {
164-
throw new IllegalStateException("Failed to build updated taxonomy.");
167+
final List<SimpleTaxonomyGraph.Issue> issues = taxonomy.build();
168+
if (!issues.isEmpty()) {
169+
final Optional<Issue> cycleIssue = issues.stream()
170+
.filter(SimpleTaxonomyGraph.NodePartOfCycle.class::isInstance)
171+
.findFirst();
172+
173+
if (cycleIssue.isPresent()) {
174+
// Use the message from the first encountered cycle participant
175+
throw new CycleDetectedException(cycleIssue.get().toString());
176+
} else {
177+
// Get the message from the first encountered issue
178+
throw new BadRequestException(issues.iterator().next().toString());
179+
}
165180
}
166181

167182
// Update new concepts, changed concepts (that were not deleted) and their descendants

‎core/com.b2international.snowowl.core/src/com/b2international/snowowl/core/taxonomy/SimpleTaxonomyGraph.java‎

Lines changed: 71 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,11 @@
1616
package com.b2international.snowowl.core.taxonomy;
1717

1818
import static com.google.common.base.Preconditions.checkNotNull;
19+
import static com.google.common.collect.Lists.newArrayList;
1920
import static com.google.common.collect.Maps.newHashMap;
2021
import static com.google.common.collect.Maps.newHashMapWithExpectedSize;
2122

23+
import java.util.List;
2224
import java.util.Map;
2325
import java.util.Set;
2426
import java.util.function.IntConsumer;
@@ -40,6 +42,34 @@
4042
public final class SimpleTaxonomyGraph {
4143

4244
private static final Logger LOGGER = LoggerFactory.getLogger("taxonomy-issues");
45+
46+
private static final int MAX_ISSUES = 100;
47+
48+
public interface Issue {
49+
@Override
50+
public String toString();
51+
}
52+
53+
public record MissingSource(String sourceId) implements Issue {
54+
@Override
55+
public String toString() {
56+
return String.format("Concept '%s' is referenced as a source ID, but it is not registered as a node.", sourceId);
57+
}
58+
}
59+
60+
public record MissingDestination(String destinationId) implements Issue {
61+
@Override
62+
public String toString() {
63+
return String.format("Concept '%s' is referenced as a destination ID, but it is not registered as a node.", destinationId);
64+
}
65+
}
66+
67+
public record NodePartOfCycle(String cycleNodeId) implements Issue {
68+
@Override
69+
public String toString() {
70+
return String.format("Node '%s' is part of a cycle.", cycleNodeId);
71+
}
72+
}
4373

4474
private final OrderedSetImpl<String> nodes;
4575
private final Map<String, SimpleEdge> edges;
@@ -135,8 +165,8 @@ public void clear() {
135165
built = false;
136166
}
137167

138-
public boolean build() {
139-
boolean hasErrors = false;
168+
public List<Issue> build() {
169+
final List<Issue> issues = newArrayList();
140170
final int conceptCount = nodes.size();
141171

142172
parents = new SparseFixedBitSet[conceptCount];
@@ -151,22 +181,49 @@ public boolean build() {
151181
final String sourceId = edges.getSourceId();
152182
final int sourceIdx = nodes.indexOf(sourceId);
153183
if (sourceIdx < 0) {
154-
LOGGER.error("Concept '{}' is referenced as a source ID, but it is not registered as a node.", sourceId);
155-
hasErrors = true;
184+
addIssue(issues, new MissingSource(sourceId));
156185
}
157186

158187
for (final String destinationId : edges.getDestinationIds()) {
159188
final int destinationIdx = nodes.indexOf(destinationId);
160189
if (destinationIdx < 0) {
161-
LOGGER.error("Concept '{}' is referenced as a destination ID, but it is not registered as a node.", destinationId);
162-
hasErrors = true;
190+
issues.add(new MissingDestination(destinationId));
163191
} else if (sourceIdx >= 0) {
164192
getBitSet(parents, sourceIdx).set(destinationIdx);
165193
getBitSet(descendants, destinationIdx).set(sourceIdx);
166194
}
167195
}
168196
}
169197

198+
// Look for cycles based on direct parent information
199+
for (int idx = 0; idx < conceptCount; idx++) {
200+
final SparseFixedBitSet visited = new SparseFixedBitSet(nodes.size());
201+
final IntDeque toProcess = PrimitiveLists.newIntArrayDeque();
202+
visited.set(idx);
203+
toProcess.add(idx);
204+
205+
while (!toProcess.isEmpty()) {
206+
final int currentIdx = toProcess.removeInt(0);
207+
final SparseFixedBitSet parentsOfCurrent = parents[currentIdx];
208+
209+
if (parentsOfCurrent == null || parentsOfCurrent.isEmpty()) {
210+
continue;
211+
}
212+
213+
if (parentsOfCurrent.get(idx)) {
214+
String cycleNodeId = getNodeId(idx);
215+
issues.add(new NodePartOfCycle(cycleNodeId));
216+
}
217+
218+
forEachBit(parentsOfCurrent, parentIdx -> {
219+
if (!visited.get(parentIdx)) {
220+
visited.set(parentIdx);
221+
toProcess.add(parentIdx);
222+
}
223+
});
224+
}
225+
}
226+
170227
for (int idx = 0; idx < conceptCount; idx++) {
171228
final SparseFixedBitSet parentsOfConcept = parents[idx];
172229
if (parentsOfConcept != null && !parentsOfConcept.isEmpty()) {
@@ -285,26 +342,19 @@ public boolean build() {
285342
});
286343
}
287344
}
288-
289-
for (int idx = 0; idx < conceptCount; idx++) {
290-
boolean nodeInCycle = false;
291-
if (!nodeInCycle) { nodeInCycle = checkCycles(parents[idx], idx); }
292-
if (!nodeInCycle) { nodeInCycle = checkCycles(indirectAncestors[idx], idx); }
293-
if (!nodeInCycle) { nodeInCycle = checkCycles(descendants[idx], idx); }
294-
if (nodeInCycle) { hasErrors = true; }
295-
}
296345

297346
built = true;
298-
return hasErrors;
347+
issues.forEach(i -> LOGGER.error(i.toString()));
348+
return issues;
299349
}
300350

301-
private boolean checkCycles(final SparseFixedBitSet bitSet, final int idx) {
302-
if (bitSet != null && bitSet.get(idx)) {
303-
LOGGER.error("Node '{}' is part of a cycle.", getNodeId(idx));
304-
return true;
305-
}
351+
private void addIssue(final List<Issue> list, final Issue issue) {
352+
list.add(issue);
306353

307-
return false;
354+
// Maintain a sliding window of the most recently recorded issues
355+
if (list.size() > MAX_ISSUES) {
356+
list.remove(0);
357+
}
308358
}
309359

310360
private SparseFixedBitSet getBitSet(final SparseFixedBitSet[] bitSets, final int sourceIdx) {

0 commit comments

Comments
 (0)