-
Notifications
You must be signed in to change notification settings - Fork 916
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
|
@@ -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; | ||
} | ||
|
||
if (o.getEndpointDiscovery() != null && o.getEndpointDiscovery().isRequired()) { | ||
endpointCacheRequired = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deprecate here as well? Can we also remove it for new services? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
|
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" | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.