Use existing type HttpRequest rather than specific PresignedRequest to represent presigned requests #292
Conversation
There was a problem hiding this comment.
Overall looks good, one minor fix requested.
As I was reviewing it occurred to me is there any additional data that might need added over time to a presigned request such that using HttpRequest directly would become a bad idea? Should we keep PresignedRequest but have it only have a single member for now such that we leave room for additions?
e.g.
data class PresignedRequest(val request: HttpRequest)| "${endpoint.protocol}://${endpoint.hostname}${signedRequest.encodedPath}", | ||
| signedRequest.headers.toSdkHeaders() | ||
| // The signer returns the path as an encoded string, we need to partially decode this to load into [HttpRequest]. | ||
| val path = signedRequest.encodedPath.substringBefore('?') |
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice! They're also much better functions than what I slapped together. I wish there was a better way of type/function discovery in the IDE.
What additional kinds of data can you think of that might be part of an |
I don't know that is why I brought it up as a possible extensibility concern. If we can't think of anything and no other teams can provide any reason I'm not overly concerned. |
3 participants
Issue #
fixes #267
Description of changes
HttpRequestrather than aPresignedRequest. This allows for easy use with theHttpClientEngineimplementations in the SDK.Testing Done
The following clients were run against the generated SDKs for STS, S3, and Polly to verify functionality.
presigner-tests.zip
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.