Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: properly prevent unreachable error codegen in endpoint rules #446

Merged
merged 1 commit into from
Aug 16, 2023
Merged

fix: properly prevent unreachable error codegen in endpoint rules #446

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions .../src/main/java/software/amazon/smithy/go/codegen/endpoints/EndpointResolverGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,25 +224,36 @@ private GoWriter.Writable generateRulesList(List<Rule> rules, Scope scope) {
w.write("$W", generateRule(rule, rule.getConditions(), scope));
});

if (!rules.isEmpty() && !(rules.get(rules.size() - 1).getConditions().isEmpty())) {
// FIXME: there's currently a bug in traversal that sometimes causes subsequent returns to be generated
// which fails vet. Patch over it via debounce for now
ensureFinalTreeRuleError(w);
if (!rules.isEmpty()) {
Rule lastRule = rules.get(rules.size() - 1);
// Trees are terminal, so we must ensure there's a final fallback condition at the end of each one.
// Generally we know we need to insert one when the final rule in a tree is not "static" i.e. it has
// conditions that might mean it is not selected. Since it may not be chosen (its set of conditions may
// evaluate to false) we MUST put a fallback error return after which we know will get executed.
//
// However, assignment statements are conflated with conditions in the rules language, and while certain
// assignments DO have a condition associated with them (basically, checking that the result of the
// assignment is not nil), some do not. Therefore, remove "static" condition/assignments from
// consideration.
boolean needsFallback = !lastRule.getConditions().stream().filter(
condition -> {
// You can't assert into a FunctionDefinition from an Expression - we have to inspect the fn
// member of the node directly.
String fn = condition.toNode().expectObjectNode().expectStringMember("fn").getValue();
// the only static assignment condition, as of this writing...
return !fn.equals("uriEncode");
}
).toList().isEmpty();
if (needsFallback) {
w.writeGoTemplate(
"return endpoint, $fmtErrorf:T(\"" + ERROR_MESSAGE_ENDOFTREE + "\")",
commonCodegenArgs
);
}
}
};
}

private void ensureFinalTreeRuleError(GoWriter w) {
final String[] lines = w.toString().split("\n");
final String lastLine = lines[lines.length - 1];
if (!lastLine.trim().startsWith("return")) {
w.writeGoTemplate(
"return endpoint, $fmtErrorf:T(\"" + ERROR_MESSAGE_ENDOFTREE + "\")",
commonCodegenArgs
);
}
}

private GoWriter.Writable generateRule(Rule rule, List<Condition> conditions, Scope scope) {
if (conditions.isEmpty()) {
return rule.accept(new RuleVisitor(scope, this.fnProvider));
Expand Down
Loading