Skip to content

Commit

Permalink
Continued refactoring of smithy-rules-engine
Browse files Browse the repository at this point in the history 
This commit collects many changes to limit the public interface,
align with Java standards, align with idioms from the rest of the
Smithy codebase, and generally clean up the codebase.

Several packages, classes, and methods have been removed, renamed,
moved, or changed visibility. Direct access to inner mutable types
has been removed. The internal complexity of the CoverageChecker
has been significantly reduced while maintaining used functionality.
  • Loading branch information
@kstich
kstich committed Jun 14, 2023
1 parent f485a6b commit a74c8d8
Show file tree
Hide file tree
Showing 107 changed files with 1,546 additions and 2,000 deletions.
118 changes: 35 additions & 83 deletions ...les-engine/src/main/java/software/amazon/smithy/rulesengine/analysis/CoverageChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,21 @@

package software.amazon.smithy.rulesengine.analysis;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.Set;
import java.util.stream.Stream;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.rulesengine.language.EndpointRuleSet;
import software.amazon.smithy.rulesengine.language.eval.RuleEvaluator;
import software.amazon.smithy.rulesengine.language.eval.value.Value;
import software.amazon.smithy.rulesengine.language.evaluation.RuleEvaluator;
import software.amazon.smithy.rulesengine.language.evaluation.value.Value;
import software.amazon.smithy.rulesengine.language.syntax.Identifier;
import software.amazon.smithy.rulesengine.language.syntax.rule.Condition;
import software.amazon.smithy.rulesengine.language.visit.TraversingVisitor;
import software.amazon.smithy.rulesengine.language.visitors.TraversingVisitor;
import software.amazon.smithy.rulesengine.traits.EndpointTestCase;
import software.amazon.smithy.utils.SetUtils;
import software.amazon.smithy.utils.SmithyUnstableApi;

/**
Expand All @@ -41,7 +42,7 @@ public final class CoverageChecker {

public CoverageChecker(EndpointRuleSet ruleSet) {
this.ruleSet = ruleSet;
this.checkerCore = new CoverageCheckerCore();
checkerCore = new CoverageCheckerCore();
}

/**
Expand All @@ -50,7 +51,7 @@ public CoverageChecker(EndpointRuleSet ruleSet) {
* @param input the map parameters and inputs to test coverage.
*/
public void evaluateInput(Map<Identifier, Value> input) {
this.checkerCore.evaluateRuleSet(ruleSet, input);
checkerCore.evaluateRuleSet(ruleSet, input);
}

/**
Expand All @@ -60,8 +61,10 @@ public void evaluateInput(Map<Identifier, Value> input) {
*/
public void evaluateTestCase(EndpointTestCase testCase) {
Map<Identifier, Value> map = new LinkedHashMap<>();
testCase.getParams().getStringMap().forEach((s, node) -> map.put(Identifier.of(s), Value.fromNode(node)));
this.checkerCore.evaluateRuleSet(ruleSet, map);
for (Map.Entry<String, Node> entry : testCase.getParams().getStringMap().entrySet()) {
map.put(Identifier.of(entry.getKey()), Value.fromNode(entry.getValue()));
}
checkerCore.evaluateRuleSet(ruleSet, map);
}

/**
Expand All @@ -70,78 +73,39 @@ public void evaluateTestCase(EndpointTestCase testCase) {
* @return stream of {@link CoverageResult}.
*/
public Stream<CoverageResult> checkCoverage() {
Stream<Condition> conditions = new CollectConditions().visitRuleset(ruleSet);
return coverageForConditions(conditions);
}

private Stream<CoverageResult> coverageForConditions(Stream<Condition> conditions) {
return conditions.distinct().flatMap(condition -> {
List<BranchResult> conditionResults = checkerCore.conditionResults.getOrDefault(condition,
new ArrayList<>());
List<Boolean> branches = conditionResults.stream()
.map(c -> c.result)
.distinct()
.collect(Collectors.toList());
if (branches.size() == 1) {
return Stream.of(new CoverageResult(condition, !branches.get(0), conditionResults.stream()
.map(c -> c.context.input)
.collect(Collectors.toList())));
} else if (branches.size() == 0) {
return Stream.of(new CoverageResult(condition, false, Collections.emptyList()),
new CoverageResult(condition, true, Collections.emptyList()));
} else {
return Stream.empty();
}
});
return new CollectConditions().visitRuleset(ruleSet).distinct().flatMap(this::getConditionCoverage);
}

private static class BranchResult {
private final boolean result;
private final CoverageCheckerCore.Context context;

BranchResult(boolean result, CoverageCheckerCore.Context context) {
this.result = result;
this.context = context;
private Stream<CoverageResult> getConditionCoverage(Condition condition) {
Set<Boolean> conditionResults = checkerCore.getResult(condition);
if (conditionResults.size() == 1) {
return Stream.of(new CoverageResult(condition, !conditionResults.iterator().next()));
}
if (conditionResults.size() == 0) {
return Stream.of(new CoverageResult(condition, false), new CoverageResult(condition, true));
}
return Stream.empty();
}

private static class CoverageCheckerCore extends RuleEvaluator {
private final Map<Condition, List<BranchResult>> conditionResults = new LinkedHashMap<>();
private Context context = null;
private final Map<Condition, Set<Boolean>> results = new LinkedHashMap<>();

@Override
public Value evaluateRuleSet(EndpointRuleSet ruleset, Map<Identifier, Value> parameterArguments) {
try {
context = new Context(parameterArguments);
return super.evaluateRuleSet(ruleset, parameterArguments);
} finally {
context = null;
}
private Set<Boolean> getResult(Condition condition) {
return results.getOrDefault(condition, SetUtils.of());
}

@Override
public Value evaluateCondition(Condition condition) {
if (context == null) {
throw new RuntimeException("Must call `evaluateRuleSet` before calling `evaluateCondition`");
}

Value result = super.evaluateCondition(condition);
List<BranchResult> list = conditionResults.getOrDefault(condition, new ArrayList<>());
if (result.isEmpty() || result.equals(Value.booleanValue(false))) {
list.add(new BranchResult(false, context));
} else {
list.add(new BranchResult(true, context));
}
conditionResults.put(condition, list);
return result;
}
// evaluateRuleSet needs to be called first. This class is inner private
// and ensures this, so we don't need to worry about that at this point.
Value conditionResult = super.evaluateCondition(condition);
boolean result = !(conditionResult.isEmpty() || conditionResult.equals(Value.booleanValue(false)));

private static class Context {
private final Map<Identifier, Value> input;
Set<Boolean> resultSet = results.getOrDefault(condition, new HashSet<>());
resultSet.add(result);
results.put(condition, resultSet);

Context(Map<Identifier, Value> input) {
this.input = input;
}
return conditionResult;
}
}

Expand All @@ -155,12 +119,10 @@ public Stream<Condition> visitConditions(List<Condition> conditions) {
public static class CoverageResult {
private final Condition condition;
private final boolean result;
private final List<Map<Identifier, Value>> otherUsages;

public CoverageResult(Condition condition, boolean result, List<Map<Identifier, Value>> otherUsages) {
public CoverageResult(Condition condition, boolean result) {
this.condition = condition;
this.result = result;
this.otherUsages = otherUsages;
}

public Condition getCondition() {
Expand All @@ -171,18 +133,8 @@ public boolean getResult() {
return result;
}

public List<Map<Identifier, Value>> getOtherUsages() {
return otherUsages;
}

public String pretty() {
StringBuilder sb = new StringBuilder();
sb.append("leaf: ").append(pretty(condition));
return sb.toString();
}

private String pretty(Condition condition) {
return condition + "(" + condition.getSourceLocation().getFilename() + ":"
return "leaf: " + condition + "(" + condition.getSourceLocation().getFilename() + ":"
+ condition.getSourceLocation().getLine() + ")";
}
}
Expand Down
82 changes: 36 additions & 46 deletions smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/language/Endpoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,14 @@
import software.amazon.smithy.model.node.StringNode;
import software.amazon.smithy.model.node.ToNode;
import software.amazon.smithy.rulesengine.language.error.RuleError;
import software.amazon.smithy.rulesengine.language.eval.Scope;
import software.amazon.smithy.rulesengine.language.eval.TypeCheck;
import software.amazon.smithy.rulesengine.language.eval.type.Type;
import software.amazon.smithy.rulesengine.language.evaluation.Scope;
import software.amazon.smithy.rulesengine.language.evaluation.TypeCheck;
import software.amazon.smithy.rulesengine.language.evaluation.type.Type;
import software.amazon.smithy.rulesengine.language.syntax.Identifier;
import software.amazon.smithy.rulesengine.language.syntax.expressions.Expression;
import software.amazon.smithy.rulesengine.language.syntax.expressions.literal.Literal;
import software.amazon.smithy.utils.BuilderRef;
import software.amazon.smithy.utils.MapUtils;
import software.amazon.smithy.utils.Pair;
import software.amazon.smithy.utils.SmithyBuilder;
import software.amazon.smithy.utils.SmithyUnstableApi;
import software.amazon.smithy.utils.StringUtils;
Expand Down Expand Up @@ -70,10 +69,10 @@ private Endpoint(Builder builder) {
this.url = SmithyBuilder.requiredState("url", builder.url);

List<Literal> authSchemes = new ArrayList<>();
for (Pair<Identifier, Map<Identifier, Literal>> authScheme : builder.authSchemes.get()) {
for (Map.Entry<Identifier, Map<Identifier, Literal>> authScheme : builder.authSchemes.get().entrySet()) {
Map<Identifier, Literal> base = new TreeMap<>(Comparator.comparing(Identifier::toString));
base.put(Identifier.of("name"), Literal.of(authScheme.left.toString()));
base.putAll(authScheme.right);
base.put(Identifier.of("name"), Literal.of(authScheme.getKey().toString()));
base.putAll(authScheme.getValue());
authSchemes.add(Literal.recordLiteral(base));
}
if (!authSchemes.isEmpty()) {
Expand All @@ -91,9 +90,7 @@ private Endpoint(Builder builder) {
*/
public static Endpoint fromNode(Node node) {
ObjectNode objectNode = node.expectObjectNode();

Builder builder = builder()
.sourceLocation(node);
Builder builder = builder().sourceLocation(node);

builder.url(Expression.fromNode(objectNode.expectMember(URL, "URL must be included in endpoint")));
objectNode.expectNoAdditionalProperties(Arrays.asList(PROPERTIES, HEADERS, URL));
Expand All @@ -105,8 +102,8 @@ public static Endpoint fromNode(Node node) {
});

objectNode.getObjectMember(HEADERS, headers -> {
for (Map.Entry<StringNode, Node> header : headers.getMembers().entrySet()) {
builder.putHeader(header.getKey().getValue(),
for (Map.Entry<String, Node> header : headers.getStringMap().entrySet()) {
builder.putHeader(header.getKey(),
header.getValue().expectArrayNode("header values should be an array")
.getElementsAs(Expression::fromNode));
}
Expand Down Expand Up @@ -162,8 +159,26 @@ public Builder toBuilder() {
}

@Override
public int hashCode() {
return Objects.hash(url, properties, headers);
public Node toNode() {
ObjectNode.Builder propertiesBuilder = ObjectNode.builder();
for (Map.Entry<Identifier, Literal> entry : properties.entrySet()) {
propertiesBuilder.withMember(entry.getKey().toString(), entry.getValue().toNode());
}

ObjectNode.Builder headersBuilder = ObjectNode.builder();
for (Map.Entry<String, List<Expression>> entry : headers.entrySet()) {
List<Node> expressionNodes = new ArrayList<>();
for (Expression expression : entry.getValue()) {
expressionNodes.add(expression.toNode());
}
headersBuilder.withMember(entry.getKey(), ArrayNode.fromNodes(expressionNodes));
}

return ObjectNode.builder()
.withMember(URL, url)
.withMember(PROPERTIES, propertiesBuilder.build())
.withMember(HEADERS, headersBuilder.build())
.build();
}

@Override
Expand All @@ -178,6 +193,11 @@ public boolean equals(Object o) {
return url.equals(endpoint.url) && properties.equals(endpoint.properties) && headers.equals(endpoint.headers);
}

@Override
public int hashCode() {
return Objects.hash(url, properties, headers);
}

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
Expand Down Expand Up @@ -225,35 +245,6 @@ public Type typeCheck(Scope<Type> scope) {
return Type.endpointType();
}

@Override
public Node toNode() {
return ObjectNode.builder()
.withMember(URL, url)
.withMember(PROPERTIES, propertiesNode())
.withMember(HEADERS, headersNode())
.build();
}

private Node propertiesNode() {
ObjectNode.Builder builder = ObjectNode.builder();
for (Map.Entry<Identifier, Literal> entry : properties.entrySet()) {
builder.withMember(entry.getKey().toString(), entry.getValue().toNode());
}
return builder.build();
}

private Node headersNode() {
ObjectNode.Builder builder = ObjectNode.builder();
for (Map.Entry<String, List<Expression>> entry : headers.entrySet()) {
List<Node> expressionNodes = new ArrayList<>();
for (Expression expression : entry.getValue()) {
expressionNodes.add(expression.toNode());
}
builder.withMember(entry.getKey(), ArrayNode.fromNodes(expressionNodes));
}
return builder.build();
}

/**
* Builder for {@link Endpoint}.
*/
Expand All @@ -263,8 +254,7 @@ public static class Builder extends RulesComponentBuilder<Builder, Endpoint> {

private final BuilderRef<Map<String, List<Expression>>> headers = BuilderRef.forOrderedMap();
private final BuilderRef<Map<Identifier, Literal>> properties = BuilderRef.forOrderedMap();
// TODO Why a list of pairs and not a map? Are duplicate IDs allowed?
private final BuilderRef<List<Pair<Identifier, Map<Identifier, Literal>>>> authSchemes = BuilderRef.forList();
private final BuilderRef<Map<Identifier, Map<Identifier, Literal>>> authSchemes = BuilderRef.forOrderedMap();
private Expression url;

public Builder(FromSourceLocation sourceLocation) {
Expand Down Expand Up @@ -294,7 +284,7 @@ public Builder authSchemes(List<Identifier> schemes, Map<Identifier, Map<Identif
}

public Builder addAuthScheme(Identifier scheme, Map<Identifier, Literal> parameters) {
this.authSchemes.get().add(Pair.of(scheme, parameters));
this.authSchemes.get().put(scheme, parameters);
return this;
}

Expand Down
Loading

0 comments on commit a74c8d8

Please sign in to comment.