Skip to content
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

feat: add codegen wrappers for retries #490

Merged
merged 8 commits into from
Oct 12, 2021
Merged
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
1 change: 1 addition & 0 deletions gradle.properties
Original file line number Diff line number Diff line change
@@ -36,6 +36,7 @@ jacocoVersion=0.8.7
kotlinxBenchmarkVersion=0.3.1

# serialization
kamlVersion=0.36.0
xmlpullVersion=1.1.3.1
xpp3Version=1.1.6

Original file line number Diff line number Diff line change
@@ -4,8 +4,7 @@
*/
package aws.smithy.kotlin.runtime.http

import aws.smithy.kotlin.runtime.http.util.StringValuesMap
import aws.smithy.kotlin.runtime.http.util.StringValuesMapBuilder
import aws.smithy.kotlin.runtime.http.util.*
import aws.smithy.kotlin.runtime.http.util.StringValuesMapImpl

/**
@@ -35,12 +34,13 @@ private object EmptyHeaders : Headers {
/**
* Build an immutable HTTP header map
*/
class HeadersBuilder : StringValuesMapBuilder(true, 8) {
class HeadersBuilder : StringValuesMapBuilder(true, 8), CanDeepCopy<HeadersBuilder> {
override fun toString(): String = "HeadersBuilder ${entries()} "
override fun build(): Headers {
require(!built) { "HeadersBuilder can only build a single Headers instance" }
built = true
return HeadersImpl(values)
override fun build(): Headers = HeadersImpl(values)

override fun deepCopy(): HeadersBuilder {
val originalValues = values.deepCopy()
return HeadersBuilder().apply { values.putAll(originalValues) }
}
}

Original file line number Diff line number Diff line change
@@ -11,7 +11,6 @@ import aws.smithy.kotlin.runtime.io.SdkByteReadChannel
* HTTP payload to be sent to a peer
*/
sealed class HttpBody {

/**
* Specifies the length of this [HttpBody] content
* If null it is assumed to be a streaming source using e.g. `Transfer-Encoding: Chunked`
Original file line number Diff line number Diff line change
@@ -4,9 +4,7 @@
*/
package aws.smithy.kotlin.runtime.http

import aws.smithy.kotlin.runtime.http.util.StringValuesMap
import aws.smithy.kotlin.runtime.http.util.StringValuesMapBuilder
import aws.smithy.kotlin.runtime.http.util.StringValuesMapImpl
import aws.smithy.kotlin.runtime.http.util.*
import aws.smithy.kotlin.runtime.util.text.urlEncodeComponent

/**
@@ -33,12 +31,13 @@ private object EmptyQueryParameters : QueryParameters {
override fun isEmpty(): Boolean = true
}

class QueryParametersBuilder : StringValuesMapBuilder(true, 8) {
class QueryParametersBuilder : StringValuesMapBuilder(true, 8), CanDeepCopy<QueryParametersBuilder> {
override fun toString(): String = "QueryParametersBuilder ${entries()} "
override fun build(): QueryParameters {
require(!built) { "QueryParametersBuilder can only build a single instance" }
built = true
return QueryParametersImpl(values)
override fun build(): QueryParameters = QueryParametersImpl(values)

override fun deepCopy(): QueryParametersBuilder {
val originalValues = values.deepCopy()
return QueryParametersBuilder().apply { values.putAll(originalValues) }
}
}

Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@
*/
package aws.smithy.kotlin.runtime.http

import aws.smithy.kotlin.runtime.http.util.CanDeepCopy
import aws.smithy.kotlin.runtime.util.InternalApi
import aws.smithy.kotlin.runtime.util.text.encodeUrlPath

@@ -105,7 +106,7 @@ data class UserInfo(val username: String, val password: String)
/**
* Construct a URL by it's individual components
*/
class UrlBuilder {
class UrlBuilder : CanDeepCopy<UrlBuilder> {
var scheme = Protocol.HTTPS
var host: String = ""
var port: Int? = null
@@ -130,6 +131,20 @@ class UrlBuilder {
forceQuery
)

override fun deepCopy(): UrlBuilder {
val builder = this
return UrlBuilder().apply {
scheme = builder.scheme
host = builder.host
port = builder.port
path = builder.path
parameters = builder.parameters.deepCopy()
fragment = builder.fragment
userInfo = builder.userInfo?.copy()
forceQuery = builder.forceQuery
}
}

override fun toString(): String =
"UrlBuilder(scheme=$scheme, host='$host', port=$port, path='$path', parameters=$parameters, fragment=$fragment, userInfo=$userInfo, forceQuery=$forceQuery)"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package aws.smithy.kotlin.runtime.http.middleware

import aws.smithy.kotlin.runtime.http.Feature
import aws.smithy.kotlin.runtime.http.FeatureKey
import aws.smithy.kotlin.runtime.http.HttpBody
import aws.smithy.kotlin.runtime.http.HttpClientFeatureFactory
import aws.smithy.kotlin.runtime.http.operation.SdkHttpOperation
import aws.smithy.kotlin.runtime.http.operation.deepCopy
import aws.smithy.kotlin.runtime.http.request.HttpRequestBuilder
import aws.smithy.kotlin.runtime.retries.RetryPolicy
import aws.smithy.kotlin.runtime.retries.RetryStrategy

class RetryFeature(private val strategy: RetryStrategy, private val policy: RetryPolicy<Any?>) : Feature {
class Config {
var strategy: RetryStrategy? = null
var policy: RetryPolicy<Any?>? = null
}

companion object Feature : HttpClientFeatureFactory<Config, RetryFeature> {
override val key: FeatureKey<RetryFeature> = FeatureKey("RetryFeature")

override fun create(block: Config.() -> Unit): RetryFeature {
val config = Config().apply(block)
val strategy = requireNotNull(config.strategy) { "strategy is required" }
val policy = requireNotNull(config.policy) { "policy is required" }
return RetryFeature(strategy, policy)
}
}

override fun <I, O> install(operation: SdkHttpOperation<I, O>) {
operation.execution.finalize.intercept { req, next ->
if (req.subject.isRetryable) {
strategy.retry(policy) {
// Deep copy the request because later middlewares (e.g., signing) mutate it
val reqCopy = req.deepCopy()

when (val body = reqCopy.subject.body) {
// Reset streaming bodies back to beginning
is HttpBody.Streaming -> body.reset()
}

next.call(reqCopy)
}
} else {
next.call(req)
}
}
}
}

/**
* Indicates whether this HTTP request could be retried. Some requests with streaming bodies are unsuitable for
* retries.
*/
val HttpRequestBuilder.isRetryable: Boolean
get() = when (val body = this.body) {
is HttpBody.Empty, is HttpBody.Bytes -> true
is HttpBody.Streaming -> body.isReplayable
}
Original file line number Diff line number Diff line change
@@ -6,6 +6,7 @@
package aws.smithy.kotlin.runtime.http.operation

import aws.smithy.kotlin.runtime.client.ExecutionContext
import aws.smithy.kotlin.runtime.http.util.CanDeepCopy

/**
* Wrapper around a type [subject] with an execution context.
@@ -17,3 +18,11 @@ import aws.smithy.kotlin.runtime.client.ExecutionContext
* @param subject The input type
*/
data class OperationRequest<T>(val context: ExecutionContext, val subject: T)

/**
* Deep copy an [OperationRequest] to a new request. Note that, because [context] is...well, context...it's considered
* transient and is not part of the copy. The subject itself, however, is deeply copied.
* @return A new [OperationRequest] with the same context and a deeply-copied subject.
*/
internal fun <T : CanDeepCopy<T>> OperationRequest<T>.deepCopy(): OperationRequest<T> =
OperationRequest(context, subject.deepCopy())
Original file line number Diff line number Diff line change
@@ -6,35 +6,30 @@ package aws.smithy.kotlin.runtime.http.request

import aws.smithy.kotlin.runtime.http.*
import aws.smithy.kotlin.runtime.http.content.ByteArrayContent
import aws.smithy.kotlin.runtime.http.util.CanDeepCopy
import aws.smithy.kotlin.runtime.io.*
import aws.smithy.kotlin.runtime.util.InternalApi

/**
* Used to construct an HTTP request
* @param method The HTTP method (verb) to use when making the request
* @param url Endpoint to make request to
* @param headers HTTP headers
* @param body Outgoing payload. Initially empty
*/
class HttpRequestBuilder {
/**
* The HTTP method (verb) to use when making the request
*/
var method: HttpMethod = HttpMethod.GET

/**
* Endpoint to make request to
*/
val url: UrlBuilder = UrlBuilder()

/**
* HTTP headers
*/
val headers: HeadersBuilder = HeadersBuilder()

/**
* Outgoing payload. Initially empty
*/
var body: HttpBody = HttpBody.Empty
class HttpRequestBuilder private constructor(
var method: HttpMethod,
val url: UrlBuilder,
val headers: HeadersBuilder,
var body: HttpBody,
) : CanDeepCopy<HttpRequestBuilder> {
constructor() : this(HttpMethod.GET, UrlBuilder(), HeadersBuilder(), HttpBody.Empty)

fun build(): HttpRequest = HttpRequest(method, url.build(), if (headers.isEmpty()) Headers.Empty else headers.build(), body)

override fun deepCopy(): HttpRequestBuilder =
HttpRequestBuilder(method, url.deepCopy(), headers.deepCopy(), body)

override fun toString(): String = buildString {
append("HttpRequestBuilder(method=$method, url=$url, headers=$headers, body=$body)")
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package aws.smithy.kotlin.runtime.http.util

/**
* Indicates that an object supports a [deepCopy] operation which will return a copy that can be safely mutated without
* affecting other instances.
*/
interface CanDeepCopy<out T> {
/**
* Returns a deep copy of this object.
*/
fun deepCopy(): T
}
Original file line number Diff line number Diff line change
@@ -64,8 +64,10 @@ internal open class StringValuesMapImpl(
override val caseInsensitiveName: Boolean = false,
initialValues: Map<String, List<String>> = emptyMap()
) : StringValuesMap {
protected val values: Map<String, List<String>> by lazy {
if (caseInsensitiveName) CaseInsensitiveMap<List<String>>().apply { putAll(initialValues) } else values.toMap()
protected val values: Map<String, List<String>> = run {
// Make a defensive copy so modifications to the initialValues don't mutate our internal copy
val copiedValues = initialValues.deepCopy()
Copy link
Contributor

Choose a reason for hiding this comment

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

question

is this really necessary? The input initialValues is an immutable map with an immutable key and value.

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'm afraid it is necessary. The type of the parameter may be the immutable interface type Map but the actual argument supplied is a mutable implementation. Modifications to that map in the builder will cause already-built instances to mutate unless copied.

Moreover, the map values passed in are mutable lists. They too need to be defensively copied to prevent post hoc mutation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which argument where?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (caseInsensitiveName) CaseInsensitiveMap<List<String>>().apply { putAll(copiedValues) } else copiedValues
}

override fun getAll(name: String): List<String>? = values[name]
@@ -81,13 +83,17 @@ internal open class StringValuesMapImpl(
override fun isEmpty(): Boolean = values.isEmpty()
}

/**
* Perform a deep copy of this map, specifically duplicating the value lists so that they're insulated from changes.
* @return A new map instance with copied value lists.
*/
internal fun Map<String, List<String>>.deepCopy() = mapValues { (_, v) -> v.toMutableList() }

@InternalApi
open class StringValuesMapBuilder(val caseInsensitiveName: Boolean = false, size: Int = 8) {
protected val values: MutableMap<String, MutableList<String>> =
if (caseInsensitiveName) CaseInsensitiveMap() else LinkedHashMap(size)

protected var built = false

fun getAll(name: String): List<String>? = values[name]

operator fun contains(name: String): Boolean = name in values
@@ -154,16 +160,8 @@ open class StringValuesMapBuilder(val caseInsensitiveName: Boolean = false, size

fun clear() = values.clear()

open fun build(): StringValuesMap {
require(!built) { "StringValuesMapBuilder can only build a single instance" }
built = true
return StringValuesMapImpl(caseInsensitiveName, values)
}
open fun build(): StringValuesMap = StringValuesMapImpl(caseInsensitiveName, values)

private fun ensureListForKey(name: String, size: Int): MutableList<String> {
if (built) {
throw IllegalStateException("Cannot modify a builder when final structure has already been built")
}
return values[name] ?: ArrayList<String>(size).also { values[name] = it }
}
private fun ensureListForKey(name: String, size: Int): MutableList<String> =
values[name] ?: ArrayList<String>(size).also { values[name] = it }
}
Original file line number Diff line number Diff line change
@@ -28,4 +28,31 @@ class HeadersTest {
}
assertEquals("Headers [key=[value]]", "$actual2")
}

@Test
fun testSubsequentModificationsDontAffectOriginal() {
val builder = HeadersBuilder()

builder.append("a", "alligator")
builder.append("b", "bunny")
builder.append("c", "chinchilla")
val first = builder.build()
val firstExpected = mapOf(
"a" to listOf("alligator"),
"b" to listOf("bunny"),
"c" to listOf("chinchilla"),
)

builder.append("a", "anteater")
builder.remove("b")
builder["c"] = "crocodile"
val second = builder.build()
val secondExpected = mapOf(
"a" to listOf("alligator", "anteater"),
"c" to listOf("crocodile"),
)

assertEquals(firstExpected.entries, first.entries())
assertEquals(secondExpected.entries, second.entries())
}
}
Loading