Add HashingSource and HashingSink #756

lauzadis · 2022-12-02T15:47:07Z

Adds HashingSource and HashingSink, which hash their data as they are being read and written, respectively.

Issue #

Description of changes

This change is required to generalize the CrcSource to support any hashing function. It is planned to be used in the flexible checksums implementation.

Note on digestValue()

Crc32 and Crc32C have an additional function called digestValue() which returns an unsigned int. To keep this function but also provide generic support for any hashing function, the HashingSource and HashingSink provide access to the underlying hash function as a member variable called hash.

With this, the user can cast the hash function to Crc32 and call the digestValue() function.

Example

var hs = HashingSource(Crc32(), ByteArray(1024) { 0xf }.source())  // CRC32 hashing source with 1024 bytes
val sink = SdkBuffer()

hs.read(sink, 1024)

val digestValue = (hs.hash as Crc32).digestValue()

An alternative design would be to create a class like CrcHashingSource which implements HashingSource and also provides digestValue(), but I thought the first option was better.

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

lauzadis · 2022-12-02T17:42:54Z

runtime/hashing/build.gradle.kts

        commonMain {
            dependencies {
                implementation(project(":runtime:utils"))
+                implementation(project(":runtime:io"))


Question: This adds a dependency on I/O, which seems like a potentially unnecessary coupling -- would this HashingSink and HashingSource be better located in the io module?

Yes, I believe they'd be better off in io.

ianbotsf · 2022-12-02T18:07:12Z

runtime/hashing/common/src/aws/smithy/kotlin/runtime/hashing/HashingSink.kt

+ * @param hash The [HashFunction] to hash data with
+ * @param sink the [SdkSink] to hash
+ */
+public class HashingSink(public val hash: HashFunction, sink: SdkSink) : HashFunction by hash, SdkSinkObserver(sink) {


Correctness: Why implement HashFunction? Is there a use case for calling update/reset on a sink? Will we ever pass a HashingSink as an argument to ByteArray.hash?

there's no particular reason, and you're right, there are no strong use cases for directly calling update / reset on this sink, so I'll remove this implementation

ianbotsf · 2022-12-02T18:08:15Z

runtime/hashing/build.gradle.kts

        commonMain {
            dependencies {
                implementation(project(":runtime:utils"))
+                implementation(project(":runtime:io"))


Yes, I believe they'd be better off in io.

ianbotsf · 2022-12-02T18:17:01Z

runtime/hashing/common/src/aws/smithy/kotlin/runtime/hashing/HashingSink.kt

+public class HashingSink(public val hash: HashFunction, sink: SdkSink) : HashFunction by hash, SdkSinkObserver(sink) {
+    override fun observe(data: ByteArray, offset: Int, length: Int) {
+        update(data, offset, length)
+    }
+}


Correctness: This largely duplicates the private class in Canonicalizer.kt. We should remove that and update the surrounding code to use this.

ianbotsf · 2022-12-02T18:19:04Z

runtime/hashing/common/test/aws/smithy/kotlin/runtime/hashing/HashingSinkTest.kt

+    @Test
+    fun testHashingSinkDigest() = run {
+        val byteArray = ByteArray(1024) { 0xf }
+        val buffer = SdkBuffer()
+        buffer.write(byteArray)
+
+        val hashingSink = HashingSink(Crc32c(), SdkSink.blackhole())
+
+        val expectedHash = Crc32c()
+
+        assertEquals(expectedHash.digest().decodeToString(), hashingSink.digest().decodeToString())
+        hashingSink.write(buffer, buffer.size)
+        expectedHash.update(byteArray)
+        assertEquals(expectedHash.digest().decodeToString(), hashingSink.digest().decodeToString())
+    }
+
+    @Test
+    fun testCrcSinkDigest() = run {
+        val byteArray = ByteArray(1024) { 0xf }
+        val buffer = SdkBuffer()
+        buffer.write(byteArray)
+
+        val hashingSink = HashingSink(Crc32(), SdkSink.blackhole())
+
+        val expectedHash = Crc32()
+
+        assertEquals(expectedHash.digest().decodeToString(), hashingSink.digest().decodeToString())
+        hashingSink.write(buffer, buffer.size)
+        expectedHash.update(byteArray)
+        assertEquals(expectedHash.digest().decodeToString(), hashingSink.digest().decodeToString())
+    }


Question: Are these two tests identical?

They weren't initially, but I refactored and they ended up being basically the same, just using a different hash function. I will remove the duplication

Oh my eyes missed the Crc32 vs Crc32c. That's great to test both, maybe just refactor the test itself into a helper method that accepts a hash implementation.

ianbotsf · 2022-12-02T18:20:11Z

runtime/hashing/common/src/aws/smithy/kotlin/runtime/hashing/HashingSink.kt

+ * @param hash The [HashFunction] to hash data with
+ * @param sink the [SdkSink] to hash
+ */
+public class HashingSink(public val hash: HashFunction, sink: SdkSink) : HashFunction by hash, SdkSinkObserver(sink) {


Comment: Making the underlying hash publicly visible is inadvisable since that means its state can be mutated by object holders. We should probably make it private instead.

Or use a HashSupplier instead.

The reason I made this public was for the digestValue() use case for CRC hash functions. I'm not sure how to nicely provide this function if the underlying hash is private, but I understand the problem with mutation by object holders.

From what I see, HashSupplier wouldn't be great here because it returns a new HashFunction each invocation, while we want to use the same HashFunction object to get the hash value of the entire underlying Source/Sink.

For digestValue(), I could do this:

public class HashingSource(private val hash: HashFunction, source: SdkSource) : SdkSourceObserver(source) { override fun observe(data: ByteArray, offset: Int, length: Int) { hash.update(data, offset, length) } public fun digestValue() : UInt? = (hash as Crc32?)?.digestValue() }

But it seems too special-cased to CRC32 to be included in a generic HashingSource

Yes, a single unsigned-int digest value is very specific to CRC32. We shouldn't include it in anything that utilizes generic hash functionality. How will this be used eventually outside of things like tests?

It's being used for event steams today. I'm adding digestValue() only to support event streams and change the usage of the original CrcSource implementation to this new one.

Why does the event streams code need to treat the CRC as a UInt? Could it just as easily use byte arrays?

It's possible. Event streams sends the expected checksum value as UInt, so we need to either

compare using digestValue()'s UInt (how it's done today)

convert the given expected checksum from UInt to ByteArray, and compare using digest() instead

I didn't want to make too many changes in event streams besides replacing the CrcSource/Sink that was used there, which is why I went with option 1.

Discussed offline, we decided to use just digest()/ByteArray everywhere for hash functions including CRC32. We'll remove specialized digestValue() members for the source/sink.

ianbotsf · 2022-12-02T18:25:33Z

runtime/hashing/common/test/aws/smithy/kotlin/runtime/hashing/HashingSourceTest.kt

+    @Test
+    fun testHashingSourceDigest() = run {
+        val byteArray = ByteArray(1024) { 0xf }
+        val source = byteArray.source()
+        val hashingSource = HashingSource(Crc32c(), source)
+
+        val sink = SdkBuffer()
+
+        val expectedHash = Crc32c()
+        assertEquals(expectedHash.digest().decodeToString(), hashingSource.digest().decodeToString())
+
+        hashingSource.read(sink, 1024)
+        expectedHash.update(byteArray)
+
+        assertEquals(expectedHash.digest().decodeToString(), hashingSource.digest().decodeToString())
+    }
+
+    @Test
+    fun testCrcSourceDigest() = run {
+        val byteArray = ByteArray(1024) { 0xf }
+        val source = byteArray.source()
+        val hashingSource = HashingSource(Crc32(), source)
+
+        val sink = SdkBuffer()
+
+        val expectedHash = Crc32()
+        assertEquals(expectedHash.digest().decodeToString(), hashingSource.digest().decodeToString())
+
+        hashingSource.read(sink, 1024)
+        expectedHash.update(byteArray)
+
+        assertEquals(expectedHash.digest().decodeToString(), hashingSource.digest().decodeToString())
+    }


Question: Identical?

… only

aajtodd

Fix and ship

aajtodd · 2022-12-02T20:04:22Z

runtime/hashing/common/src/aws/smithy/kotlin/runtime/hashing/HashingSink.kt

+ * @param hash The [HashFunction] to hash data with
+ * @param sink the [SdkSink] to hash
+ */
+public class HashingSink(public val hash: HashFunction, sink: SdkSink) : HashFunction by hash, SdkSinkObserver(sink) {


I'd probably make these InternalApi

If anything I would create a CrcSource/Sink instead of trying to force them through HashingSource/Sink. It's going to look nearly identical but it won't be awkwardly special casing the generic hashing source/sink

aajtodd · 2022-12-02T20:07:46Z

runtime/hashing/common/src/aws/smithy/kotlin/runtime/hashing/HashingSink.kt

+ * @param hash The [HashFunction] to hash data with
+ * @param sink the [SdkSink] to hash
+ */
+public class HashingSink(private val hash: HashFunction, sink: SdkSink) : SdkSinkObserver(sink) {


@InternalApi

aajtodd · 2022-12-02T20:08:59Z

runtime/hashing/common/src/aws/smithy/kotlin/runtime/hashing/HashingSink.kt

+     * Provides the digest value as an unsigned integer for the CRC32 function family.
+     * @return unsigned integer representing the value of the digest, if the [HashFunction] is [Crc32] or [Crc32c], and null otherwise.
+     */
+    public fun digestValue() : UInt? = (hash as Crc32?)?.digestValue()


I think either special case OR move this to an internal package as an extension function (would require making hash internal rather than private

…erize tests

ianbotsf

Looks great!

aajtodd

Fix and ship

aajtodd · 2022-12-09T15:57:53Z

runtime/io/common/test/aws/smithy/kotlin/runtime/io/HashingSinkTest.kt

+    @ParameterizedTest
+    @ValueSource(strings = ["crc32", "crc32c", "md5", "sha1", "sha256"])
+    fun testHashingSinkDigest(hashFunctionName: String) = run {
+        val byteArray = ByteArray(1024) { 0xf }


fix: let's use something that forces okio to cross segment boundaries, something odd like 19k would be 2 segments + a partial. Same for all source/sink tests

…hing-io

sonarqubecloud · 2022-12-12T23:40:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Add HashingSource and HashingSink

c47bd81

lauzadis mentioned this pull request Dec 2, 2022

Use HashingSource and HashingSink aws/aws-sdk-kotlin#788

Merged

Add changelog

284bada

lauzadis marked this pull request as ready for review December 2, 2022 16:10

lauzadis requested a review from a team as a code owner December 2, 2022 16:10

lauzadis commented Dec 2, 2022

View reviewed changes

remove open from class

ea42490

ianbotsf requested changes Dec 2, 2022

View reviewed changes

make hash private, implement digestValue() for CRC hash functions…

11616e6

… only

aajtodd approved these changes Dec 2, 2022

View reviewed changes

lauzadis added 5 commits December 2, 2022 14:09

ktlint

cb86bf3

relocate HashingSource/Sink to io module, add CrcSource/Sink, paramet…

297e3ae

…erize tests

use new HashingSink

55bb379

default sink to blackhole

5dd1e91

remove Crc32Source/Sink

87096bf

ianbotsf approved these changes Dec 5, 2022

View reviewed changes

aajtodd approved these changes Dec 9, 2022

View reviewed changes

lauzadis added 2 commits December 9, 2022 16:54

increase test data size

a66f3ce

Merge branch 'main' of github.com:awslabs/smithy-kotlin into feat-has…

ada0927

…hing-io

lauzadis merged commit cee4330 into main Dec 12, 2022

aajtodd pushed a commit that referenced this pull request Feb 8, 2023

feat: add tracing framework (#756)

bdd4fa8

aajtodd pushed a commit that referenced this pull request Feb 13, 2023

feat: add tracing framework (#756)

34a649e

lauzadis deleted the feat-hashing-io branch February 22, 2023 22:56

Add HashingSource and HashingSink #756

Add HashingSource and HashingSink #756

Uh oh!

Conversation

lauzadis commented Dec 2, 2022

Issue #

Description of changes

Note on digestValue()

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aajtodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianbotsf left a comment

Choose a reason for hiding this comment

Uh oh!

aajtodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lauzadis Dec 2, 2022 •

edited

Loading

lauzadis Dec 2, 2022 •

edited

Loading

lauzadis Dec 2, 2022 •

edited

Loading