Skip to content

Default endpointDiscovery to true for services that require it #1845

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.

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 1 commit into from
May 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changes/next-release/feature-AWSSDKforJavav2-534a8f5.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"type": "feature",
"category": "AWS SDK for Java v2",
"description": "Endpoint discovery is now enabled by default for future services that will require it. A new method 'endpointDiscoveryEnabled' has been added to client builders that support endpoint discovery allowing a true or false value to be set. 'enableEndpointDiscovery' has been deprecated on the client builders as it is now superseded by 'endpointDiscoveryEnabled'."
}
Original file line number Diff line number Diff line change
Expand Up @@ -106,13 +106,25 @@ public IntermediateModel build() {
Map<String, AuthorizerModel> authorizers =
new HashMap<>(new AddCustomAuthorizers(this.service, getNamingStrategy()).constructAuthorizers());

// Iterate through every operation and build an 'endpointOperation' if at least one operation that supports
// endpoint discovery is found. If -any operations that require- endpoint discovery are found, then the flag
// 'endpointCacheRequired' will be set on the 'endpointOperation'. This 'endpointOperation' summary is then
// passed directly into the constructor of the intermediate model and is referred to by the codegen.
OperationModel endpointOperation = null;
boolean endpointCacheRequired = false;

for (OperationModel o : operations.values()) {
if (o.isEndpointOperation()) {
endpointOperation = o;
break;
}
Comment on lines 117 to 119
Copy link
Contributor

Choose a reason for hiding this comment

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

We just use the last endpoint operation? Is that the right behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll admit this is not how I would have written it if I had done it myself, but it is actually a copy/paste from the v1 SDK. The logic is correct because it only needs to know there is at least one endpoint operation (and the last one suffices for that purpose), and the flag as to whether an endpoint is required is cumulative (once it gets set it doesn't get unset) so ends up effectively meaning 'at least one endpoint is required'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a couple of tests to assert this logic was working correctly as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify it a bit and make it a boolean or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing it will require more refactoring than the change is worth in my opinion. At least it's in sync with v1 in this state. I'll add comments to explain what it's doing.


if (o.getEndpointDiscovery() != null && o.getEndpointDiscovery().isRequired()) {
endpointCacheRequired = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

So the "new" behavior is: if any operation requires endpoint discovery, enable it by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. I also stated that in the PR description.

}
}

if (endpointOperation != null) {
endpointOperation.setEndpointCacheRequired(endpointCacheRequired);
}

for (IntermediateModelShapeProcessor processor : shapeProcessors) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@ public class CustomizationConfig {
*/
private UtilitiesMethod utilitiesMethod;

/**
* Force generation of deprecated client builder method 'enableEndpointDiscovery'. Only services that already had
* this method when it was deprecated require this flag to be set.
*/
private boolean enableEndpointDiscoveryMethodRequired = false;

private CustomizationConfig() {
}

Expand Down Expand Up @@ -406,4 +412,12 @@ public UtilitiesMethod getUtilitiesMethod() {
public void setUtilitiesMethod(UtilitiesMethod utilitiesMethod) {
this.utilitiesMethod = utilitiesMethod;
}

public boolean isEnableEndpointDiscoveryMethodRequired() {
return enableEndpointDiscoveryMethodRequired;
}

public void setEnableEndpointDiscoveryMethodRequired(boolean enableEndpointDiscoveryMethodRequired) {
this.enableEndpointDiscoveryMethodRequired = enableEndpointDiscoveryMethodRequired;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ public class OperationModel extends DocumentationModel {

private boolean endpointOperation;

private boolean endpointCacheRequired;

private EndpointDiscovery endpointDiscovery;

@JsonIgnore
Expand Down Expand Up @@ -207,6 +209,14 @@ public void setEndpointOperation(boolean endpointOperation) {
this.endpointOperation = endpointOperation;
}

public boolean isEndpointCacheRequired() {
return endpointCacheRequired;
}

public void setEndpointCacheRequired(boolean endpointCacheRequired) {
this.endpointCacheRequired = endpointCacheRequired;
}

public boolean isPaginated() {
return isPaginated;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,32 @@ public TypeSpec poetSpec() {
.addJavadoc("Internal implementation of {@link $T}.", builderInterfaceName);

if (model.getEndpointOperation().isPresent()) {
builder.addMethod(enableEndpointDiscovery());
builder.addMethod(endpointDiscoveryEnabled());

if (model.getCustomizationConfig().isEnableEndpointDiscoveryMethodRequired()) {
builder.addMethod(enableEndpointDiscovery());
}
}

return builder.addMethod(buildClientMethod()).build();
}

private MethodSpec endpointDiscoveryEnabled() {
return MethodSpec.methodBuilder("endpointDiscoveryEnabled")
.addAnnotation(Override.class)
.addModifiers(Modifier.PUBLIC)
.returns(builderClassName)
.addParameter(boolean.class, "endpointDiscoveryEnabled")
.addStatement("this.endpointDiscoveryEnabled = endpointDiscoveryEnabled")
.addStatement("return this")
.build();
}

private MethodSpec enableEndpointDiscovery() {
return MethodSpec.methodBuilder("enableEndpointDiscovery")
Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecate here as well? Can we also remove it for new services?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will deprecate here as well. Do you have a strategy for removing it for new services? How do we identify 'new services'? One strategy is just to say 'service != DDB' which seems yuck.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add a customization: "enableEndpointDiscoveryMethodRequired" and just enable it for DynamoDB.

.addAnnotation(Override.class)
.addAnnotation(Deprecated.class)
.addJavadoc("@deprecated Use {@link #endpointDiscoveryEnabled($T)} instead.", boolean.class)
.addModifiers(Modifier.PUBLIC)
.returns(builderClassName)
.addStatement("endpointDiscoveryEnabled = true")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import software.amazon.awssdk.awscore.client.builder.AwsDefaultClientBuilder;
import software.amazon.awssdk.codegen.internal.Utils;
import software.amazon.awssdk.codegen.model.intermediate.IntermediateModel;
import software.amazon.awssdk.codegen.model.intermediate.OperationModel;
import software.amazon.awssdk.codegen.model.service.AuthType;
import software.amazon.awssdk.codegen.poet.ClassSpec;
import software.amazon.awssdk.codegen.poet.PoetUtils;
Expand Down Expand Up @@ -78,10 +79,12 @@ public TypeSpec poetSpec() {
ClassName.get(basePackage, model.getMetadata().getSyncBuilder()),
ClassName.get(basePackage, model.getMetadata().getAsyncBuilder()));

// Only services that require endpoint discovery for at least one of their operations get a default value of
// 'true'
if (model.getEndpointOperation().isPresent()) {
builder.addField(FieldSpec.builder(boolean.class, "endpointDiscoveryEnabled")
.addModifiers(PROTECTED)
.initializer("false")
.initializer(resolveDefaultEndpointDiscovery() ? "true" : "false")
.build());
}

Expand All @@ -102,6 +105,12 @@ public TypeSpec poetSpec() {
return builder.build();
}

private boolean resolveDefaultEndpointDiscovery() {
return model.getEndpointOperation()
.map(OperationModel::isEndpointCacheRequired)
.orElse(false);
}

private MethodSpec signingNameMethod() {
return MethodSpec.methodBuilder("signingName")
.addAnnotation(Override.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ public TypeSpec poetSpec() {
.addJavadoc(getJavadoc());

if (model.getEndpointOperation().isPresent()) {
builder.addMethod(enableEndpointDiscovery());
if (model.getCustomizationConfig().isEnableEndpointDiscoveryMethodRequired()) {
builder.addMethod(enableEndpointDiscovery());
}

builder.addMethod(endpointDiscovery());
}

if (model.getCustomizationConfig().getServiceSpecificClientConfigClass() != null) {
Expand All @@ -72,6 +76,16 @@ private MethodSpec enableEndpointDiscovery() {
return MethodSpec.methodBuilder("enableEndpointDiscovery")
.addModifiers(Modifier.PUBLIC, Modifier.ABSTRACT)
.returns(TypeVariableName.get("B"))
.addAnnotation(Deprecated.class)
.addJavadoc("@deprecated Use {@link #endpointDiscoveryEnabled($T)} instead.", boolean.class)
.build();
}

private MethodSpec endpointDiscovery() {
return MethodSpec.methodBuilder("endpointDiscoveryEnabled")
.addModifiers(Modifier.PUBLIC, Modifier.ABSTRACT)
.returns(TypeVariableName.get("B"))
.addParameter(boolean.class, "endpointDiscovery")
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,32 @@ public TypeSpec poetSpec() {
.addJavadoc("Internal implementation of {@link $T}.", builderInterfaceName);

if (model.getEndpointOperation().isPresent()) {
builder.addMethod(enableEndpointDiscovery());
builder.addMethod(endpointDiscoveryEnabled());

if (model.getCustomizationConfig().isEnableEndpointDiscoveryMethodRequired()) {
builder.addMethod(enableEndpointDiscovery());
}
}

return builder.addMethod(buildClientMethod()).build();
}

private MethodSpec endpointDiscoveryEnabled() {
return MethodSpec.methodBuilder("endpointDiscoveryEnabled")
.addAnnotation(Override.class)
.addModifiers(Modifier.PUBLIC)
.returns(builderClassName)
.addParameter(boolean.class, "endpointDiscoveryEnabled")
.addStatement("this.endpointDiscoveryEnabled = endpointDiscoveryEnabled")
.addStatement("return this")
.build();
}

private MethodSpec enableEndpointDiscovery() {
return MethodSpec.methodBuilder("enableEndpointDiscovery")
.addAnnotation(Override.class)
.addAnnotation(Deprecated.class)
.addJavadoc("@deprecated Use {@link #endpointDiscoveryEnabled($T)} instead.", boolean.class)
.addModifiers(Modifier.PUBLIC)
.returns(builderClassName)
.addStatement("endpointDiscoveryEnabled = true")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import java.io.File;
import org.junit.Test;
Expand Down Expand Up @@ -59,4 +61,32 @@ public void sharedOutputShapesLinkCorrectlyToOperationOutputs() {
assertEquals("SecurePingResponse", testModel.getOperation("SecurePing").getOutputShape().getShapeName());
}

@Test
public void defaultEndpointDiscovery_true() {
final File modelFile = new File(IntermediateModelBuilderTest.class
.getResource("poet/client/c2j/endpointdiscovery/service-2.json").getFile());
IntermediateModel testModel = new IntermediateModelBuilder(
C2jModels.builder()
.serviceModel(ModelLoaderUtils.loadModel(ServiceModel.class, modelFile))
.customizationConfig(CustomizationConfig.create())
.build())
.build();

assertTrue(testModel.getEndpointOperation().get().isEndpointCacheRequired());
}

@Test
public void defaultEndpointDiscovery_false() {
final File modelFile = new File(IntermediateModelBuilderTest.class
.getResource("poet/client/c2j/endpointdiscoveryoptional/service-2.json").getFile());
IntermediateModel testModel = new IntermediateModelBuilder(
C2jModels.builder()
.serviceModel(ModelLoaderUtils.loadModel(ServiceModel.class, modelFile))
.customizationConfig(CustomizationConfig.create())
.build())
.build();

assertFalse(testModel.getEndpointOperation().get().isEndpointCacheRequired());
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
{
"version":"2.0",
"metadata":{
"apiVersion":"2018-08-31",
"endpointPrefix":"awsendpointdiscoverytestservice",
"jsonVersion":"1.1",
"protocol":"json",
"serviceAbbreviation":"AwsEndpointDiscoveryTest",
"serviceFullName":"AwsEndpointDiscoveryTest",
"serviceId":"AwsEndpointDiscoveryTest",
"signatureVersion":"v4",
"signingName":"awsendpointdiscoverytestservice",
"targetPrefix":"AwsEndpointDiscoveryTestService"
},
"operations":{
"DescribeEndpoints":{
"name":"DescribeEndpoints",
"http":{
"method":"POST",
"requestUri":"/"
},
"input":{"shape":"DescribeEndpointsRequest"},
"output":{"shape":"DescribeEndpointsResponse"},
"endpointoperation":true
},
"TestDiscoveryOptional":{
"name":"TestDiscoveryOptional",
"http":{
"method":"POST",
"requestUri":"/"
},
"input":{"shape":"TestDiscoveryOptionalRequest"},
"output":{"shape":"TestDiscoveryOptionalResponse"},
"endpointdiscovery":{
}
}
},
"shapes": {
"Boolean": {
"type": "boolean"
},
"DescribeEndpointsRequest": {
"type": "structure",
"members": {
"Operation": {
"shape": "String"
},
"Identifiers": {
"shape": "Identifiers"
}
}
},
"DescribeEndpointsResponse": {
"type": "structure",
"required": [
"Endpoints"
],
"members": {
"Endpoints": {
"shape": "Endpoints"
}
}
},
"Endpoint": {
"type": "structure",
"required": [
"Address",
"CachePeriodInMinutes"
],
"members": {
"Address": {
"shape": "String"
},
"CachePeriodInMinutes": {
"shape": "Long"
}
}
},
"Endpoints": {
"type": "list",
"member": {
"shape": "Endpoint"
}
},
"Identifiers": {
"type": "map",
"key": {
"shape": "String"
},
"value": {
"shape": "String"
}
},
"Long": {
"type": "long"
},
"String": {
"type": "string"
},
"TestDiscoveryIdentifiersRequiredRequest": {
"type": "structure",
"required": [
"Sdk"
],
"members": {
"Sdk": {
"shape": "String",
"endpointdiscoveryid": true
}
}
},
"TestDiscoveryIdentifiersRequiredResponse": {
"type": "structure",
"members": {
"DiscoveredEndpoint": {
"shape": "Boolean"
}
}
},
"TestDiscoveryOptionalRequest": {
"type": "structure",
"members": {
}
},
"TestDiscoveryOptionalResponse": {
"type": "structure",
"members": {
"DiscoveredEndpoint": {
"shape": "Boolean"
}
}
}
}
}
Loading