Add support for endpoint trait #795
Conversation
varunnvs92
force-pushed
the
varunkn/SupportEndpointHostTrait
branch
5 times, most recently
from
aa52413 to
bad6d30
Compare
| @@ -104,7 +105,8 @@ protected final SdkClientConfiguration mergeChildDefaults(SdkClientConfiguration | |||
| .option(AwsAdvancedClientOption.ENABLE_DEFAULT_REGION_DETECTION, true) | |||
| .option(AwsClientOption.CREDENTIALS_PROVIDER, DefaultCredentialsProvider.create()) | |||
| .option(SdkClientOption.RETRY_POLICY, AwsRetryPolicy.defaultRetryPolicy()) | |||
| .option(AwsClientOption.SERVICE_SIGNING_NAME, signingName())); | |||
| .option(AwsClientOption.SERVICE_SIGNING_NAME, signingName()) | |||
| .option(SdkAdvancedClientOption.DISABLE_HOST_PREFIX_INJECTION, false)); | |||
There was a problem hiding this comment.
IMO it is an advanced option and not common use case to set it, so I made it an option instead of directing exposing it in ClientOverrideConfiguration. Do you think it makes more sense to expose it as top level member in ClientOverrideConfiguration?
There was a problem hiding this comment.
Can it not be an option at all?
There was a problem hiding this comment.
Talked offline. It can be useful in testing.
| request.setEndpoint(clientConfiguration.option(SdkClientOption.ENDPOINT)); | ||
|
|
||
|
|
||
| request.setEndpoint(resolveEndpoint(clientConfiguration, executionParams.hostPrefixExpression())); |
There was a problem hiding this comment.
This actually changes a bit with the marshaller refactor. I'm curious to get your opinion on this, right now I'm passing the client configured endpoint to the marshaller and setting the endpoint there. I need to do that because SdkHttpFullRequest can't be built without an endpoint. We could a couple of things I think
- Relax constraints in SdkHttpFullRequest to not require endpoint on build so we can open it back up later and add endpoint
- Change marshallers to produce a SdkHttpFullRequest.Builder and fill in the endpoint and build later on in the request lifecycle
- Somehow get the operation level endpoint to the marshaller and set it there
- Continue to just set the client level endpoint knowing that it may be overwritten later
Thoughts?
There was a problem hiding this comment.
I am leaning towards 4.
- Change marshallers to produce a SdkHttpFullRequest.Builder and fill in the endpoint and build later on in the request lifecycle
I think its better to pass built objects to avoid any mutability concerns
- Somehow get the operation level endpoint to the marshaller and set it there
Have to do it for each protocol. Don't like it
- Continue to just set the client level endpoint knowing that it may be overwritten later
I like this as we get marshalled request and we can construct a new request (with new endpoint) if host prefix is set. As we don't reuse the marshalled request at any place other than BaseClientHandler, it seems safe and modification happen at single place.
If we are choosing between 4 and 1, relaxing constraints doesn't give any advantage and 4 seems ideal.
What do you think?
There was a problem hiding this comment.
Discussed offline. Going to keep the logic to update endpoint in BaseClientHandler instead of moving to marshallers as we need to pass more information every time we need to support a feature updating endpoint (like endpoint discovery).
varunnvs92
force-pushed
the
varunkn/SupportEndpointHostTrait
branch
2 times, most recently
from
4886cb0 to
831f9e9
Compare
|
Addressed all the comments |
|
APIs with path params are being resolved incorrectly. Will investigate |
varunnvs92
force-pushed
the
varunkn/SupportEndpointHostTrait
branch
4 times, most recently
from
aa6f549 to
700ed9c
Compare
| @@ -145,6 +146,9 @@ public final String serviceName() { | |||
| @Override | |||
| public CompletableFuture<APostOperationResponse> aPostOperation(APostOperationRequest aPostOperationRequest) { | |||
| try { | |||
| String hostPrefix = "{StringMember}-foo."; | |||
There was a problem hiding this comment.
Is hostPrefix ever used? Also it seems simpler to me to just replace the placeholders rather than change to %s and do a string.format.
Something like
String resolvedHostExpression = "{StringMember}-foo."
.replace("{StringMember}", aPostOperationRequest.stringMember());
varunnvs92
force-pushed
the
varunkn/SupportEndpointHostTrait
branch
from
700ed9c to
d9616a5
Compare
…5492b48f Pull request: release <- staging/dd963b91-03ee-4cec-9700-640a5492b48f
Fixed:
[WIP] APIs with path params are being resolved incorrectly. Will investigateReady to merge