Skip to content

Commit

Permalink
Make cycle detection dependent on success of other validators
Browse files Browse the repository at this point in the history
  • Loading branch information
@fbiville
fbiville committed Mar 1, 2024
1 parent a4858d3 commit 1fe6653
Show file tree
Hide file tree
Showing 8 changed files with 200 additions and 16 deletions.
48 changes: 44 additions & 4 deletions src/main/java/org/neo4j/importer/v1/ImportSpecificationDeserializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,17 @@
import com.networknt.schema.SpecVersion.VersionFlag;
import java.io.IOException;
import java.io.Reader;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.TreeSet;
import java.util.stream.Collectors;
import org.neo4j.importer.v1.actions.Action;
import org.neo4j.importer.v1.graph.CycleDetector;
import org.neo4j.importer.v1.graph.Pair;
import org.neo4j.importer.v1.validation.InvalidSpecificationException;
import org.neo4j.importer.v1.validation.SpecificationException;
import org.neo4j.importer.v1.validation.SpecificationValidationResult;
Expand Down Expand Up @@ -125,25 +131,59 @@ private static void runExtraValidations(ImportSpecification spec) throws Specifi
validators.forEach(validator -> validator.visitAction(index, actions.get(index)));
}

Set<Class<? extends SpecificationValidator>> failedValidations = new HashSet<>(validators.size());
var builder = SpecificationValidationResult.builder();
validators.forEach(validator -> validator.accept(builder));
validators.forEach(validator -> {
for (Class<? extends SpecificationValidator> dependent : validator.requires()) {
if (failedValidations.contains(dependent)) {
return;
}
}
if (validator.report(builder)) {
failedValidations.add(validator.getClass());
}
});
SpecificationValidationResult result = builder.build();
if (!result.passes()) {
throw new InvalidSpecificationException(result);
}
}

private static List<SpecificationValidator> loadValidators() {
List<SpecificationValidator> result = new ArrayList<>();
private static Set<SpecificationValidator> loadValidators() {
Set<SpecificationValidator> result = new TreeSet<>();
ServiceLoader.load(SpecificationValidator.class).forEach(result::add);
checkValidatorCycles(result);
return result;
}

private static void checkValidatorCycles(Set<SpecificationValidator> validators) {
var validatorDependencyGraph = new HashMap<Class<?>, Class<?>>(validators.size());
validators.forEach((validator) -> {
for (Class<? extends SpecificationValidator> dependency : validator.requires()) {
validatorDependencyGraph.put(validator.getClass(), dependency);
}
});
var cycles = CycleDetector.run(validatorDependencyGraph);
if (!cycles.isEmpty()) {
throw new IllegalStateException(String.format(
"%d validator dependency cycle(s) detected:%n%s",
cycles.size(), validatorCycleDescription(cycles)));
}
}

private static JsonNode parse(Reader spec) throws SpecificationException {
try {
return MAPPER.readTree(spec);
} catch (IOException e) {
throw new UnparseableSpecificationException(e);
}
}

private static String validatorCycleDescription(List<List<Pair<Class<?>, Class<?>>>> cycles) {
return cycles.stream()
.map(cycle -> cycle.stream()
.map(pair -> String.format("%s depends on %s", pair.getFirst(), pair.getSecond()))
.collect(Collectors.joining(", ")))
.collect(Collectors.joining("\n\t- ", "\t-", ""));
}
}
27 changes: 24 additions & 3 deletions src/main/java/org/neo4j/importer/v1/validation/SpecificationValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@

import java.io.Reader;
import java.util.Map;
import java.util.function.Consumer;
import java.util.Set;
import org.neo4j.importer.v1.actions.Action;
import org.neo4j.importer.v1.sources.Source;
import org.neo4j.importer.v1.targets.CustomQueryTarget;
import org.neo4j.importer.v1.targets.NodeTarget;
import org.neo4j.importer.v1.targets.RelationshipTarget;
import org.neo4j.importer.v1.validation.SpecificationValidationResult.Builder;

/**
* This is the SPI for custom validators.
Expand All @@ -37,13 +38,33 @@
* 4. visitRelationshipTarget (as many times as there are relationship targets)
* 5. visitCustomQueryTarget (as many times as there are custom query targets)
* 6. visitAction (as many times as there are actions)
* Then {@link SpecificationValidator#accept} is called with a {@link SpecificationValidationResult.Builder}, where
* Then {@link SpecificationValidator#report(Builder)} is called with a {@link SpecificationValidationResult.Builder}, where
* errors are reported via {@link SpecificationValidationResult.Builder#addError(String, String, String)} and warnings
* via {@link SpecificationValidationResult.Builder#addWarning(String, String, String)}.
* Implementations are not expected to be thread-safe.
* Modifying the provided arguments via any of the visitXxx or accept calls is considered undefined behavior.
*/
public interface SpecificationValidator extends Consumer<SpecificationValidationResult.Builder> {
public interface SpecificationValidator extends Comparable<SpecificationValidator> {

/**
* Reports validation errors and warnings via {@link SpecificationValidationResult.Builder}
* @return true if at least 1 error was reported, false otherwise
*/
boolean report(SpecificationValidationResult.Builder builder);

default int compareTo(SpecificationValidator other) {
if (this.requires().contains(other.getClass())) {
return 1;
}
if (other.requires().contains(this.getClass())) {
return -1;
}
return this.getClass().getName().compareTo(other.getClass().getName());
}

default Set<Class<? extends SpecificationValidator>> requires() {
return Set.of();
}

default void visitConfiguration(Map<String, Object> configuration) {}

Expand Down
6 changes: 5 additions & 1 deletion src/main/java/org/neo4j/importer/v1/validation/plugin/NoDanglingDependsOnValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import org.neo4j.importer.v1.actions.Action;
import org.neo4j.importer.v1.targets.CustomQueryTarget;
import org.neo4j.importer.v1.targets.NodeTarget;
Expand Down Expand Up @@ -61,10 +62,12 @@ public void visitAction(int index, Action action) {
}

@Override
public void accept(Builder builder) {
public boolean report(Builder builder) {
AtomicBoolean result = new AtomicBoolean(false);
pathToDependsOn.entrySet().stream()
.filter(entry -> !names.contains(entry.getValue()))
.forEachOrdered(entry -> {
result.set(true);
String path = entry.getKey();
String invalidDependsOn = entry.getValue();
builder.addError(
Expand All @@ -73,6 +76,7 @@ public void accept(Builder builder) {
String.format(
"%s depends on a non-existing action or target \"%s\".", path, invalidDependsOn));
});
return result.get();
}

private void track(Target target, String path) {
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/neo4j/importer/v1/validation/plugin/NoDanglingNodeReferenceValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,13 @@ public void visitRelationshipTarget(int index, RelationshipTarget target) {
}

@Override
public void accept(Builder builder) {
public boolean report(Builder builder) {
invalidPathToNodeReferences.forEach((path, invalidNodeReference) -> {
builder.addError(
path,
ERROR_CODE,
String.format("%s refers to a non-existing node target \"%s\".", path, invalidNodeReference));
});
return !invalidPathToNodeReferences.isEmpty();
}
}
3 changes: 2 additions & 1 deletion src/main/java/org/neo4j/importer/v1/validation/plugin/NoDanglingSourceValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void visitCustomQueryTarget(int index, CustomQueryTarget target) {
}

@Override
public void accept(Builder builder) {
public boolean report(Builder builder) {
pathToSourceName.forEach((path, source) -> {
builder.addError(
path,
Expand All @@ -71,6 +71,7 @@ public void accept(Builder builder) {
"%s refers to the non-existing source \"%s\". Possible names are: \"%s\"",
path, source, String.join("\", \"", sourceNames)));
});
return !pathToSourceName.isEmpty();
}

private void checkSource(Target target, Supplier<String> path) {
Expand Down
13 changes: 10 additions & 3 deletions src/main/java/org/neo4j/importer/v1/validation/plugin/NoDependencyCycleValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Collectors;
import org.neo4j.importer.v1.actions.Action;
import org.neo4j.importer.v1.graph.CycleDetector;
Expand All @@ -42,6 +43,11 @@ public NoDependencyCycleValidator() {
namedPaths = new HashMap<>();
}

@Override
public Set<Class<? extends SpecificationValidator>> requires() {
return Set.of(NoDuplicatedNameValidator.class, NoDanglingDependsOnValidator.class);
}

@Override
public void visitNodeTarget(int index, NodeTarget target) {
trackDependency(target, String.format("$.targets.nodes[%d]", index));
Expand All @@ -63,9 +69,9 @@ public void visitAction(int index, Action action) {
}

@Override
public void accept(Builder builder) {
Map<Element, Element> graph = dependencyGraph();
CycleDetector.run(graph).forEach((cycle) -> {
public boolean report(Builder builder) {
var cycles = CycleDetector.run(dependencyGraph());
cycles.forEach((cycle) -> {
String cycleDescription = cycle.stream()
.map(pair -> {
Element dependent = pair.getFirst();
Expand All @@ -81,6 +87,7 @@ public void accept(Builder builder) {
ERROR_CODE,
String.format("A dependency cycle has been detected:%n %s", cycleDescription));
});
return !cycles.isEmpty();
}

private void trackDependency(Target target, String path) {
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/org/neo4j/importer/v1/validation/plugin/NoDuplicatedNameValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import org.neo4j.importer.v1.actions.Action;
import org.neo4j.importer.v1.sources.Source;
import org.neo4j.importer.v1.targets.CustomQueryTarget;
Expand Down Expand Up @@ -62,8 +63,8 @@ public void visitAction(int index, Action action) {
}

@Override
public void accept(Builder builder) {
nameCounter.reportErrorsIfAny(builder);
public boolean report(Builder builder) {
return nameCounter.reportErrorsIfAny(builder);
}
}

Expand All @@ -75,17 +76,20 @@ public void track(String name, String path) {
pathsUsingName.computeIfAbsent(name, (ignored) -> new ArrayList<>()).add(String.format("%s.name", path));
}

public void reportErrorsIfAny(Builder builder) {
public boolean reportErrorsIfAny(Builder builder) {
AtomicBoolean result = new AtomicBoolean(false);
pathsUsingName.entrySet().stream()
.filter(entry -> entry.getValue().size() > 1)
.forEach(entry -> {
List<String> paths = entry.getValue();
result.set(true);
builder.addError(
paths.get(0),
"DUPN",
String.format(
"Name \"%s\" is duplicated across the following paths: %s",
entry.getKey(), String.join(", ", paths)));
});
return result.get();
}
}
106 changes: 106 additions & 0 deletions src/test/java/org/neo4j/importer/v1/ImportSpecificationDeserializerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1120,4 +1120,110 @@ void fails_if_longer_dependency_cycle_is_detected() {
"\"an-action\" ($.actions[0]) depends on \"a-query-target\" ($.targets.queries[0])",
"\"a-query-target\" ($.targets.queries[0]) depends on \"a-relationship-target\" ($.targets.relationships[0])");
}

@Test
void does_not_report_cycles_if_names_are_duplicated() {
assertThatThrownBy(() -> deserialize(new StringReader(
"""
{
"sources": [{
"type": "bigquery",
"name": "a-source",
"query": "SELECT id, name FROM my.table"
}],
"targets": {
"relationships": [{
"name": "a-target",
"depends_on": "an-action",
"source": "a-source",
"type": "TYPE",
"start_node": {
"label": "Label1",
"key_properties": [
{"source_field": "field_1", "target_property": "property1"}
]
},
"end_node": {
"label": "Label2",
"key_properties": [
{"source_field": "field_2", "target_property": "property2"}
]
}
}],
"queries": [{
"name": "a-target",
"source": "a-source",
"depends_on": "an-action",
"query": "UNWIND $rows AS row CREATE (n:ANode) SET n = row"
}]
},
"actions": [{
"name": "an-action",
"depends_on": "a-target",
"type": "http",
"method": "get",
"url": "https://example.com"
}]
}
"""
.stripIndent())))
.isInstanceOf(InvalidSpecificationException.class)
.hasMessageContainingAll(
"1 error(s)",
"0 warning(s)",
"Name \"a-target\" is duplicated across the following paths: $.targets.relationships[0].name, $.targets.queries[0].name");
}

@Test
void does_not_report_cycles_if_depends_on_are_dangling() {
assertThatThrownBy(() -> deserialize(new StringReader(
"""
{
"sources": [{
"type": "bigquery",
"name": "a-source",
"query": "SELECT id, name FROM my.table"
}],
"targets": {
"relationships": [{
"name": "a-target",
"depends_on": "an-action",
"source": "a-source",
"type": "TYPE",
"start_node": {
"label": "Label1",
"key_properties": [
{"source_field": "field_1", "target_property": "property1"}
]
},
"end_node": {
"label": "Label2",
"key_properties": [
{"source_field": "field_2", "target_property": "property2"}
]
}
}],
"queries": [{
"name": "a-query-target",
"source": "a-source",
"depends_on": "invalid-depends-on",
"query": "UNWIND $rows AS row CREATE (n:ANode) SET n = row"
}]
},
"actions": [{
"name": "an-action",
"depends_on": "a-target",
"type": "http",
"method": "get",
"url": "https://example.com"
}]
}
"""
.stripIndent())))
.isInstanceOf(InvalidSpecificationException.class)
.hasMessageContainingAll(
"1 error(s)",
"0 warning(s)",
"$.targets.queries[0] depends on a non-existing action or target \"invalid-depends-on\"");
}
}

0 comments on commit 1fe6653

Please sign in to comment.