feat: add support for flexible checksums on Normal payloads

[dayaffe]

Issue #

#1287

Description of changes

Scope

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

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

[jbelkins]

Not sure, should this be logged as an error?

[dayaffe]

Yeah let's log this as an error

[jbelkins]

Was this addressed? Still looks like logger.info() to me

[dayaffe]

Oops I think I might've reverted this change back accidentally, will change this

[dayaffe]

@jbelkins Thinking about it some more, I believe this should be a logger.info as it is not an unexpected case. The FlexibleChecksumsRequestMiddleware should be added to any method that has the httpChecksum trait with a requestAlgorithmMember enum. But the user does not have to use checksums, it is an opt-in feature.

[jbelkins]

A lot of questions and a few little changes, also hit the Swiftlint flags

[sichanyoo]

Just some questions and suggestions!

[sichanyoo]

Wouldn't the response already be received & deserialized by the time we reach this point of this middleware, since we're placing the middleware after the DeserializeMiddleware?

[dayaffe]

I could be wrong and I thought the same thing, but I think with middlewares the Input/Output types have to conform to the related Step regardless of position. So for DeserializeStep that means Input has to be of type SdkHttpRequest forcing me to call try await next.handle() to get the response

smithy-swift/Sources/ClientRuntime/Middleware/Steps/DeserializeStep.swift

Line 13 in a89f313

public typealias DeserializeStep<OperationStackOutput> = MiddlewareStep<HttpContext,

[sichanyoo]

Note: In-progress offline discussion on whether calling next.handle here and one more time later in the same middleware means executing request twice & if not, how calling next.handle here conveniently gives a copy of the response both times

[dayaffe]

After doing some research, and perhaps someone else can confirm, I do not believe calling next.handle() issues the request multiple times.

Evidence for this:

• The deserializeMiddleware is actually being added to the stack before subsequent middlewares such as FlexibleChecksumsResponseMiddleware. The line let response = try await next.handle(...) is a recursive call saying "come back to me with the response we get after the entire middleware stack is done". The next call in the stack that happens is going to FlexibleChecksumsResponseMiddleware where we again say let response = try await next.handle(...) and ask that the result of calling subsequent middlewares is given back to me here. Then the response should be issued and goes back to FlexibleChecksumsResponseMiddleware. And once we're done there then we go back to DeserializeMiddleware where eventually we return either a throw try await httpResponseErrorClosure(copiedResponse.httpResponse) or return copiedResponse. While this is not the last middleware added to the stack, it ends up being the last middleware since it is the first in the stack to ask for the response back and does not end up returning a try await next.handle(...) at the end.
• Inspecting the response in both places this is called I can see that the memory address of the stream in httpResponse is same
• Looking at the logs we would expect to see a completion handler execute more than once with the bytes streamed down before it if the same call to pull down a file was executed more than once, but this is not the case

My Conclusion: It is ok from a networking perspective to have response held in more than one middleware as long as it is after the DeserializeMiddleware in the stack. However, we are holding this thing in memory so it should be a stream with a known content length (not event streams).

[sichanyoo]

Lgtm, just need swiftlint warnings taken care of.

[jbelkins]

See my comments plus fix the lint issues.

[jbelkins]

How does this behave differently if on a big- or little-endian system? From what I can tell, reversed() does the same on either.

[jbelkins]

I think you want to use UInt32.littleEndian or UInt32.bigEndian to get the byte order you want, no matter the platform.

[dayaffe]

Ah great point, Ill change this

[jbelkins]

From what I gather, Apple platforms (Apple Silicon & x86_64) are little-endian. But when we start talking about proprietary ARM on Linux, it could be either... IDK

[dayaffe]

forcing one or the other using littleEndian vs bigEndian regardless seems to change the base64encoded output causing the request to fail against S3's servers. A solution I thought of was to reverse only if the system is littleEndian:

// Reverse bytes if on a little-endian architecture
if self == self.littleEndian {
    data = Data(data.reversed())
}

[jbelkins]

Calling .littleEndian should have no effect on the value on a little-endian system. Vice-versa for calling .bigEndian on a big-endian system.

Assuming you want the bytes in big-endian order:

let value: UInt32 = 123456  // or whatever value
let bigEndianValue = value.bigEndian
let data = Data(bytes: &bigEndianValue, count: MemoryLayout<UInt32>.size) 

data should be your integer's 4 bytes in big-endian order.

[jbelkins]

Was this addressed? Still looks like logger.info() to me