Skip to content

fix(endpoint): fixes to endpoints codegen from user testing #592

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

Merged
merged 3 commits into from
Sep 21, 2022
Merged

fix(endpoint): fixes to endpoints codegen from user testing #592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b6648d
fix(endpoint): fixes to endpoints codegen from user testing
kuhe Sep 21, 2022
c5e16a1
feat(endpoint): check clientContextParam required status from endpoin…
kuhe Sep 21, 2022
07f2084
feat(endpoint): exclude contextParams from operation uri
kuhe Sep 21, 2022
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
6 changes: 4 additions & 2 deletions ...ipt-codegen/src/main/java/software/amazon/smithy/typescript/codegen/CommandGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ServiceShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait;
import software.amazon.smithy.typescript.codegen.endpointsV2.EndpointsParamNameMap;
Expand Down Expand Up @@ -201,13 +202,14 @@ private void generateEndpointParameterInstructionProvider() {
name, EndpointsParamNameMap.getLocalName(name)
);
});
parameterFinder.getStaticContextParamValues(operation).forEach((name, value) -> {
Shape operationInput = model.getShape(operation.getInputShape()).get();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix: the contextParams are traits on input structure members, not the operation itself

parameterFinder.getStaticContextParamValues(operationInput).forEach((name, value) -> {
writer.write(
"$L: { type: \"staticContextParams\", value: $L },",
name, value
);
});
parameterFinder.getContextParams(operation).forEach((name, type) -> {
parameterFinder.getContextParams(operationInput).forEach((name, type) -> {
writer.write(
"$L: { type: \"contextParams\", name: \"$L\" },",
name, EndpointsParamNameMap.getLocalName(name)
Expand Down
32 changes: 20 additions & 12 deletions ...main/java/software/amazon/smithy/typescript/codegen/endpointsV2/EndpointsV2Generator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,9 @@ private void generateEndpointParameters() {
Map<String, String> clientContextParams =
new RuleSetParameterFinder(service).getClientContextParams();

clientContextParams.forEach((k, v) -> {
writer.write("$L: $L,", k, v);
ObjectNode ruleSet = endpointRuleSetTrait.getRuleSet().expectObjectNode();
ruleSet.getObjectMember("parameters").ifPresent(parameters -> {
parameters.accept(new RuleSetParametersVisitor(writer, clientContextParams));
});
}
);
Expand Down Expand Up @@ -115,23 +116,30 @@ private void generateEndpointParameters() {
private void generateEndpointResolver() {
TypeScriptWriter writer = new TypeScriptWriter("");

writer.addImport("EndpointV2", "EndpointV2", "@aws-sdk/types");
writer.addImport("Logger", "Logger", "@aws-sdk/types");
writer.addImport("EndpointV2", null, "@aws-sdk/types");
writer.addImport("Logger", null, "@aws-sdk/types");

writer.addImport("endpointProvider", "endpointProvider", "@aws-sdk/util-endpoints");
writer.addImport("EndpointParameters", "EndpointParameters", "../endpoint/EndpointParameters");
writer.addImport("ruleSet", "ruleSet", "../endpoint/ruleset");
writer.addImport("EndpointParams", null, "@aws-sdk/util-endpoints");
writer.addImport("resolveEndpoint", null, "@aws-sdk/util-endpoints");
writer.addImport("EndpointParameters", null, "../endpoint/EndpointParameters");
writer.addImport("ruleSet", null, "../endpoint/ruleset");

writer.openBlock(
"export const defaultEndpointResolver = ",
"",
() -> {
writer.openBlock(
"(param: EndpointParameters, context: { logger?: Logger } = {}): EndpointV2 => {",
"(endpointParams: EndpointParameters, context: { logger?: Logger } = {}): EndpointV2 => {",
"};",
() -> {
// TODO(endpointsV2) cache
writer.write("return endpointProvider(param, ruleSet, context);");
writer.openBlock(
"return resolveEndpoint(ruleSet, {",
"});",
() -> {
writer.write("endpointParams: endpointParams as EndpointParams,");
writer.write("logger: context.logger,");
}
);
}
);
}
Expand All @@ -149,10 +157,10 @@ private void generateEndpointResolver() {
private void generateEndpointRuleset() {
TypeScriptWriter writer = new TypeScriptWriter("");

writer.addImport("RuleSet", "RuleSet", "@aws-sdk/util-endpoints");
writer.addImport("RuleSetObject", null, "@aws-sdk/util-endpoints");

writer.openBlock(
"export const ruleSet: RuleSet = ",
"export const ruleSet: RuleSetObject = ",
"",
() -> {
new RuleSetSerializer(
Expand Down
95 changes: 57 additions & 38 deletions ...in/java/software/amazon/smithy/typescript/codegen/endpointsV2/RuleSetParameterFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@
import software.amazon.smithy.model.node.NodeVisitor;
import software.amazon.smithy.model.node.ObjectNode;
import software.amazon.smithy.model.node.StringNode;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.ServiceShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.rulesengine.traits.ClientContextParamsTrait;
import software.amazon.smithy.rulesengine.traits.ContextParamTrait;
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait;
Expand Down Expand Up @@ -65,7 +66,7 @@ public Map<String, String> getClientContextParams() {
clientContextParamsTrait.getParameters().forEach((name, definition) -> {
map.put(
name,
definition.getType().toString() // "boolean" and "string" are directly usable in TS.
definition.getType().toString().toLowerCase() // "boolean" and "string" are directly usable in TS.
);
});
}
Expand All @@ -76,46 +77,58 @@ public Map<String, String> getClientContextParams() {
* "The staticContextParams trait defines one or more context parameters that MUST
* be bound to the specified values. This trait MUST target an operation shape."
*/
public Map<String, String> getStaticContextParams(OperationShape operation) {
public Map<String, String> getStaticContextParams(Shape operationInput) {
Map<String, String> map = new HashMap<>();
Optional<StaticContextParamsTrait> trait = operation.getTrait(StaticContextParamsTrait.class);
if (trait.isPresent()) {
StaticContextParamsTrait staticContextParamsTrait = trait.get();
staticContextParamsTrait.getParameters().forEach((name, definition) -> {
map.put(
name,
definition.getValue().getType().toString()
);

if (operationInput.isStructureShape()) {
operationInput.getAllMembers().forEach((String memberName, MemberShape member) -> {
Optional<StaticContextParamsTrait> trait = member.getTrait(StaticContextParamsTrait.class);
if (trait.isPresent()) {
StaticContextParamsTrait staticContextParamsTrait = trait.get();
staticContextParamsTrait.getParameters().forEach((name, definition) -> {
map.put(
name,
definition.getValue().getType().toString()
);
});
}
});
}

return map;
}

/**
* Get map of params to actual values instead of the value type.
*/
public Map<String, String> getStaticContextParamValues(OperationShape operation) {
public Map<String, String> getStaticContextParamValues(Shape operationInput) {
Map<String, String> map = new HashMap<>();
Optional<StaticContextParamsTrait> trait = operation.getTrait(StaticContextParamsTrait.class);
if (trait.isPresent()) {
StaticContextParamsTrait staticContextParamsTrait = trait.get();
staticContextParamsTrait.getParameters().forEach((name, definition) -> {
String value;
if (definition.getValue().isStringNode()) {
value = "`" + definition.getValue().expectStringNode().toString() + "`";
} else if (definition.getValue().isBooleanNode()) {
value = definition.getValue().expectBooleanNode().toString();
} else {
throw new RuntimeException("unexpected type "
+ definition.getValue().getType().toString()
+ " received as staticContextParam.");

if (operationInput.isStructureShape()) {
operationInput.getAllMembers().forEach((String memberName, MemberShape member) -> {
Optional<StaticContextParamsTrait> trait = member.getTrait(StaticContextParamsTrait.class);
if (trait.isPresent()) {
StaticContextParamsTrait staticContextParamsTrait = trait.get();
staticContextParamsTrait.getParameters().forEach((name, definition) -> {
String value;
if (definition.getValue().isStringNode()) {
value = "`" + definition.getValue().expectStringNode().toString() + "`";
} else if (definition.getValue().isBooleanNode()) {
value = definition.getValue().expectBooleanNode().toString();
} else {
throw new RuntimeException("unexpected type "
+ definition.getValue().getType().toString()
+ " received as staticContextParam.");
}
map.put(
name,
value
);
});
}
map.put(
name,
value
);
});
}

return map;
}

Expand All @@ -125,17 +138,23 @@ public Map<String, String> getStaticContextParamValues(OperationShape operation)
* The targeted endpoint parameter MUST be a type that is compatible with member’s
* shape targeted by the trait.
*/
public Map<String, String> getContextParams(OperationShape operation) {
public Map<String, String> getContextParams(Shape operationInput) {
Map<String, String> map = new HashMap<>();
Optional<ContextParamTrait> trait = operation.getTrait(ContextParamTrait.class);
if (trait.isPresent()) {
ContextParamTrait staticContextParamsTrait = trait.get();
String name = staticContextParamsTrait.getName();
map.put(
name,
"unknown"
);

if (operationInput.isStructureShape()) {
operationInput.getAllMembers().forEach((String memberName, MemberShape member) -> {
Optional<ContextParamTrait> trait = member.getTrait(ContextParamTrait.class);
if (trait.isPresent()) {
ContextParamTrait contextParamTrait = trait.get();
String name = contextParamTrait.getName();
map.put(
name,
"unknown"
);
}
});
}

return map;
}

Expand Down
13 changes: 12 additions & 1 deletion .../java/software/amazon/smithy/typescript/codegen/endpointsV2/RuleSetParametersVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package software.amazon.smithy.typescript.codegen.endpointsV2;

import java.util.HashMap;
import java.util.Map;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.node.NodeVisitor;
Expand All @@ -25,9 +26,16 @@

public class RuleSetParametersVisitor extends NodeVisitor.Default<Void> {
private final TypeScriptWriter writer;
private final Map<String, String> clientContextParams;

public RuleSetParametersVisitor(TypeScriptWriter writer) {
this.writer = writer;
this.clientContextParams = new HashMap<>();
}

public RuleSetParametersVisitor(TypeScriptWriter writer, Map<String, String> clientContextParams) {
this.writer = writer;
this.clientContextParams = clientContextParams;
}

@Override
Expand All @@ -38,7 +46,10 @@ public Void objectNode(ObjectNode node) {
Node param = entry.getValue();

ParameterGenerator parameterGenerator = new ParameterGenerator(key, param);
writer.write(parameterGenerator.toCodeString());

if (clientContextParams.isEmpty() || clientContextParams.containsKey(key)) {
writer.write(parameterGenerator.toCodeString());
}
}
return null;
}
Expand Down
32 changes: 29 additions & 3 deletions ...a/software/amazon/smithy/typescript/codegen/integration/HttpBindingProtocolGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,13 @@
import software.amazon.smithy.model.traits.MediaTypeTrait;
import software.amazon.smithy.model.traits.StreamingTrait;
import software.amazon.smithy.model.traits.TimestampFormatTrait.Format;
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait;
import software.amazon.smithy.typescript.codegen.ApplicationProtocol;
import software.amazon.smithy.typescript.codegen.CodegenUtils;
import software.amazon.smithy.typescript.codegen.FrameworkErrorModel;
import software.amazon.smithy.typescript.codegen.TypeScriptDependency;
import software.amazon.smithy.typescript.codegen.TypeScriptWriter;
import software.amazon.smithy.typescript.codegen.endpointsV2.RuleSetParameterFinder;
import software.amazon.smithy.utils.ListUtils;
import software.amazon.smithy.utils.OptionalUtils;
import software.amazon.smithy.utils.SetUtils;
Expand All @@ -93,8 +95,10 @@ public abstract class HttpBindingProtocolGenerator implements ProtocolGenerator
private final Set<StructureShape> deserializingErrorShapes = new TreeSet<>();
private final boolean isErrorCodeInBody;
private final EventStreamGenerator eventStreamGenerator = new EventStreamGenerator();

private final LinkedHashMap<String, String> headerBuffer = new LinkedHashMap<>();
private final Set<String> contextParamDeduplicationControlSet = SetUtils.of(
"Bucket"
);

/**
* Creates a Http binding protocol generator.
Expand Down Expand Up @@ -725,12 +729,34 @@ private void writeResolvedPath(
SymbolProvider symbolProvider = context.getSymbolProvider();
List<HttpBinding> labelBindings = bindingIndex.getRequestBindings(operation, Location.LABEL);

final boolean useEndpointsV2 = context.getService().hasTrait(EndpointRuleSetTrait.class);
final Map<String, String> contextParams = useEndpointsV2
? new RuleSetParameterFinder(context.getService())
.getContextParams(context.getModel().getShape(operation.getInputShape()).get())
: Collections.emptyMap();

// Always write the bound path, but only the actual segments.
writer.write("let resolvedPath = `$L` + $S;",
"${basePath?.endsWith('/') ? basePath.slice(0, -1) : (basePath || '')}",
"/" + trait.getUri().getSegments().stream()
.map(Segment::toString)
.collect(Collectors.joining("/")));
.filter(segment -> {
if (!useEndpointsV2) {
// only applicable in Endpoints 2.0
return true;
}
String content = segment.getContent();
boolean isContextParam = contextParams.containsKey(content);

// If the endpoint also contains the uri segment, e.g. Bucket, we
// do not want to include it in the operation URI to be resolved.
// We use this logic plus a temporary control-list, since it is not yet known
// how many services and param names will have this issue.

return !(isContextParam && contextParamDeduplicationControlSet.contains(content));
})
.map(Segment::toString)
.collect(Collectors.joining("/"))
);

// Handle any label bindings.
if (!labelBindings.isEmpty()) {
Expand Down