docs: add design for operation interceptors

[aajtodd]

Issue #

#122

Description of changes

Adds design doc for interceptors

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lauzadis]

nit: retired (👴) -> retried

[lauzadis]

what is the use case for reading RequestInterceptorContext.request after it has been serialized into protocolRequest?

[lauzadis]

nit: unnecessary extra return

[lucix-aws]

Minor: alternatively

typealias HttpInterceptor<I, O> = Interceptor<I, O, HttpRequest, HttpResponse>

typealias GenericHttpInterceptor = Interceptor<Any, Any, HttpRequest, HttpResponse>

(i.e. give them a typealias to use for operation-scoped interceptors AND the generic one).

[ianbotsf]

Style: This description sounds imprecise to me. Perhaps something like "an end-to-end invocation of an operation provided by a service client"?

[ianbotsf]

Nit: Doesn't use * bullets and thus renders as a single paragraph:

Each interceptor hook can: • Read the messages generated so far during in the execution • Save information to a context object that is available across all hooks • (Write Hooks Only) Modify the input message, transport request message, transport response message or output

[ianbotsf]

Nit: > { indentation should be 0 not 4

[aajtodd]

not according to ktlint :)

[ianbotsf]

Comment: I don't find the "ALWAYS" part of these descriptions very useful, particularly when they're immediately negated by a subsequent "except" clause.

[ianbotsf]

Question: What is the value of describing the expected interval between hooks? How would users act on that information and what would be the consequences of these expectations being inaccurate?

[aajtodd]

These descriptions largely follow ref arch. I see some minor value as it helps infer how the operation is executed but I don't feel strongly about whether we include this verbiage or not.

[ianbotsf]

I think describing how the operation is executed is very important but probably as a cohesive, higher-level diagram/description and not as implications from the descriptions of intervals.

[ianbotsf]

Nit: "implementators" → "implementors" or "implementations"

[ianbotsf]

Comment: In principle, what do we think about providing types for hooks which only execute on a single operation (and ignore other operations)? The benefit would be allowing users to easily add strongly-typed interceptors without casting or checking themselves:

val client = S3Client {
  interceptors += ListObjectsMaxKeysInterceptor(10)
}

class ListObjectsMaxKeysInterceptor(defaultMaxKeys: Int): ListObjectsInterceptor() {
    override fun modifyBeforeSerialization(context: RequestInterceptorContext<ListObjectsRequest>) = when {
        context.request.maxKeys == null -> context.request.copy { maxKeys = defaultMaxKeys }
        else -> context.request
    }
}

To achieve this, I think we'd need the following:

Remove input/output types on Interceptor

interface Interceptor<ProtocolRequest, ProtocolResponse> {
    fun modifyBeforeSerialization(context: RequestInterceptorContext<Any>): Any = context.request
    fun modifyBeforeCompletion(context: ResponseInterceptorContext<Any, Any, ProtocolRequest?, ProtocolResponse?>): Result<Any> = context.response
}

typealias HttpInterceptor = Interceptor<HttpRequest, HttpResponse>

(only including a few hooks for demonstration purposes)

Add a new TypedInterceptor with input/output types (effectively the old Interceptor)

interface TypedInterceptor<Input, Output, ProtocolRequest, ProtocolResponse> {
    fun modifyBeforeSerialization(context: RequestInterceptorContext<Input>): Input = context.request
    fun modifyBeforeCompletion(context: ResponseInterceptorContext<Input, Output, ProtocolRequest?, ProtocolResponse?>): Result<Output> = context.response
}

typealias TypedHttpInterceptor<Input, Output> = TypedInterceptor<Input, Output, HttpRequest, HttpResponse>

Codegen per-op interceptor classes

Example for ListObjects:

open class ListObjectsInterceptor : HttpInterceptor, TypedHttpInterceptor<ListObjectsRequest, ListObjectsResponse> {
    final override fun modifyBeforeSerialization(context: RequestInterceptorContext<Any>): Any = when {
        context.opName == "ListObjects" -> {
            val typedContext = object : RequestInterceptorContext<ListObjectsRequest> {
                override val opName = "ListObjects"
                override val request = context.request as ListObjectsRequest
            }
            modifyBeforeSerialization(typedContext)
        }
        else -> context.request
    }
    
    final override fun modifyBeforeCompletion(context: ResponseInterceptorContext<Any, Any, HttpRequest?, HttpResponse?>): Result<Any> = when {
        context.opName == "ListObjects" -> {
            val typedContext = object : ResponseInterceptorContext<ListObjectsRequest, ListObjectsResponse, HttpRequest?, HttpResponse?> {
                override val opName = "ListObjects"
                override val request = context.request as ListObjectsRequest
                override val protocolRequest = context.protocolRequest
                override val protocolResponse = context.protocolResponse
                override val response = context.response as Result<ListObjectsResponse>
            }
            modifyBeforeCompletion(typedContext)
        }
        else -> context.response
    }
}

Ignore the inline anonymous contexts, I'm assuming we'll have some default implementation class to use at runtime. Also, the operation name checking and casting may be reusable enough to encapsulate in a base class to shrink the per-op interceptor classes down to just the types and operation name...not sure yet.

[aajtodd]

Ya something like this may work and is in back of my mind. I actually think we may not even need new types depending on if we thread Any as a bound on some of the generics.

Let's get interceptors wired up and before we land it come back to revisit typed interceptors (it will give us more room to play with these ideas once we have a working framework).

[ianbotsf]

Nit: "it's" → "its"

[ianbotsf]

Comment: May want to include a note indicating that these config-specified interceptors are strictly additive to any pre-existing, non-configurable interceptors.

[ianbotsf]

Style: Repeating "Interceptors registered via" on every item is unnecessary. Also, this may be clearer as a numbered (rather than bulleted) list.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information