feat: add support for `smithy.protocols#rpcv2Cbor` protocol #1103

lauzadis · 2024-06-19T04:26:37Z

This PR adds support for a new protocol, smithy.protocols#rpcv2Cbor.
This is a subset of the Concise Binary Object Representation (CBOR) RFC.
Check out the Smithy docs to see where this protocol differs from the RFC.

(potentially) Controversial changes:

Added deserializeByteArray() and deserializeInstant() to PrimitiveDeserializer interface
- I believe these should have been there from the beginning -- these are primitives (aka simple types) in Smithy (blob and timestamp)
Added serializeByteArray() to PrimitiveSerializer interface
- note: serializeInstant() already exists

Bugs fixed:

Fixed codegen of equals() functions for structs with:
- Lists of blobs. Currently this will do a naive comparison (==) which is prone to failure because ByteArrays need to be compared with contentEquals.
- Floats / doubles will now be compared with .equals(). This is because Float.NaN == Float.NaN always returns false, but if two structures have the exact same members and a NaN floating point, I believe they should be treated as equal? (at least according to Smithy protocol tests, which were failing without this change)
When normalizing input/output shapes, if we come across a shape that targets the smithy.api#Unit shape, we would set the archetype to the operation shape. I updated this to set the unit shape as the archetype so we don't lose that information.

Issue #

Closes #843 and part of awslabs/aws-sdk-kotlin#1302

Description of changes

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

Added new deserializeBlob and deserializeTimestamp functions to deserializer interface.

…-rpcv2-cbor

lauzadis · 2024-07-09T17:53:57Z

Note: acknowledge-api-break was added because of a bug in Kotlin around our use of typealias in BigDecimalJVM / BigIntegerJVM. This was going to break during our upgrade to K2, but I had to move it forward into this PR to add new functions like operator plus, operator minus, toByteArray

runtime/serde/serde-cbor/common/src/aws/smithy/kotlin/runtime/serde/cbor/encoding/Simple.kt

ianbotsf

Approved pending positive outcomes of remaining nits/questions.

...hy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/RuntimeTypes.kt

...hy/kotlin/runtime/awsprotocol/rpcv2/cbor/RpcV2CborSmithyProtocolResponseHeaderInterceptor.kt

ianbotsf · 2024-07-10T16:58:40Z

runtime/serde/serde-cbor/common/src/aws/smithy/kotlin/runtime/serde/cbor/CborSerializer.kt

+    public fun toHttpBody(): HttpBody = buffer.readByteArray().toHttpBody()
+


Question: I'm still concerned about reading this all into memory at once although I note that RPC v2 does not (yet) support blob streams, merely event streams. What's the behavior for event streams wrt CborSerializer.toHttpBody()? Is that invoked for the initial request and separately for each subsequent event (as opposed to once for the entire stream)?

Yes that's correct, each message will be encoded separately in the CborSerializer instead of all-at-once. I wasn't able to functionally test this because there's no services that have an input event stream and support CBOR at the moment. I was able to test output event streams using CloudWatch Logs and they work as expected.

runtime/serde/serde-cbor/common/src/aws/smithy/kotlin/runtime/serde/cbor/encoding/Number.kt

runtime/serde/serde-cbor/common/src/aws/smithy/kotlin/runtime/serde/cbor/encoding/Simple.kt

ianbotsf · 2024-07-10T18:17:33Z

runtime/serde/serde-cbor/common/src/aws/smithy/kotlin/runtime/serde/cbor/encoding/Tag.kt

+            val sb = StringBuilder()
+
+            // Append mantissa
+            sb.append(
+                when (mantissa) {
+                    is UInt -> mantissa.value.toString()
+                    is NegInt -> "-${mantissa.value}"
+                    is Tag -> when (mantissa.value) {
+                        is NegBigNum -> mantissa.value.value.toString()
+                        is BigNum -> mantissa.value.value.toString()
+                        else -> throw DeserializationException("Expected BigNum or NegBigNum for CBOR tagged decimal fraction mantissa, got ${mantissa.id}")
+                    }
+                    else -> throw DeserializationException("Expected UInt, NegInt, or Tag for CBOR decimal fraction mantissa, got $mantissa")
+                },
+            )
+
+            when (exponent) {
+                is UInt -> { // Positive exponent, suffix with zeroes
+                    sb.append("0".repeat(exponent.value.toInt()))
+                    sb.append(".")
+                }
+                is NegInt -> { // Negative exponent, prefix with zeroes if necessary
+                    val exponentValue = exponent.value.toInt().absoluteValue
+                    val insertIndex = if (sb[0] == '-') 1 else 0
+                    if (exponentValue > sb.length - insertIndex) {
+                        sb.insert(insertIndex, "0".repeat(exponentValue - sb.length + insertIndex))
+                        sb.insert(insertIndex, '.')
+                    } else {
+                        sb.insert(sb.length - exponentValue, '.')
+                    }
+                }
+                else -> throw DeserializationException("Expected integer for CBOR decimal fraction exponent value, got $exponent.")
+            }
+
+            return DecimalFraction(BigDecimal(sb.toString()))


Suggestion: Apologies but I missed this the first time around when commenting on string manipulation for BigInteger but same suggestion applies: I suggest we delegate these down to JVM's BigDecimal, which has a constructor for accepting a BigInteger mantissa (which JVM calls the unscaledVal) and an Int exponent (which JVM calls the scale).

Actually I think you did mention doing this for BigDecimal and I missed it, either way, I think it's the right change so I'll do it

ianbotsf · 2024-07-10T18:28:39Z

...e/serde-cbor/common/test/aws/smithy/kotlin/runtime/serde/cbor/CborDeserializerSuccessTest.kt

            val list = mutableListOf<UByte>()
            while (hasNextElement()) {
-                list.add(deserializeByte().toUByte())
+                list.add(deserializeLong().toUByte())
            }
            return@deserializeList list


Question: Why did so many tests from deserializing narrow types (e.g., Byte) to deserializing Long?

Tests of this form started failing after adding the out-of-bounds check for number deserialization because the numeric value is 255 which doesn't fit in a Byte. Trying to call deserializeByte here would result in the out-of-bounds check throwing an exception, so it's updated instead to deserializeLong

...me/serde/serde-cbor/common/test/aws/smithy/kotlin/runtime/serde/cbor/CborDeserializerTest.kt

…Collections.kt and rename it to TextString

ianbotsf

Latest changes look great to me!

github-actions · 2024-07-11T18:50:10Z

Affected Artifacts

Significantly increased in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
smithy-rpcv2-protocols-jvm.jar	6,110	(does not exist)	6,110	NaN%
serde-cbor-jvm.jar	104,350	(does not exist)	104,350	NaN%

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
smithy-test-jvm.jar	66,665	63,752	2,913	4.57%
serde-json-jvm.jar	85,727	83,960	1,767	2.10%
serde-form-url-jvm.jar	83,091	81,768	1,323	1.62%
serde-xml-jvm.jar	223,945	220,837	3,108	1.41%
aws-protocol-core-jvm.jar	24,245	24,042	203	0.84%
http-test-jvm.jar	66,311	65,788	523	0.79%
runtime-core-jvm.jar	886,604	883,520	3,084	0.35%
serde-jvm.jar	50,921	50,880	41	0.08%
aws-json-protocols-jvm.jar	6,906	7,176	-270	-3.76%

lauzadis added 30 commits May 9, 2024 15:19

commit all

6eb23d0

Add dependency on smithy-protocol-traits

46473e0

latest commit

f210817

commit unstaged files

714b04b

tests

aa57a2d

decode tests are passing

d212ac0

Upgrade to Smithy 1.49.0

0d8daae

Add Rpcv2Cbor as a protocol generator

c3f6f31

Latest commit

add554f

Latest commit

61cc03a

Fix encoding/decoding of negative integers

0e92a58

Fix error deserializer

6c8b1fa

Small fixes

2d998f3

ktlintFormat

b3f6d0c

remove unused variable

0f2a9ef

apiDump

ac31d50

Generate tests from cbor-decode-error-tests.json

1184b7b

Cleanup

6718936

ktlintFormat

859111a

test list and map serde

1f777c6

ktlintFormat

1763955

Replace CborSerialName

20468df

Replace CborSerialName

b0b1b87

fix some codegen failures

5046ddc

revert temporary BigIntegerTest changes

4ee5366

use peekMinorByte

a7a62e4

Making requests to CBOR services.

b895e4f

Added new deserializeBlob and deserializeTimestamp functions to deserializer interface.

ktlintFormat

045fb5b

remove temporary variable

4057079

correct serializer name

e7fdc80

lauzadis added 4 commits July 9, 2024 11:19

Throw exception when deserialized number does not fit in desired type

5cad413

Add TODOs, FIXMEs, and safely handle potential overflow

cb83f4d

ktlint

b6e36a5

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

2c41bc3

…-rpcv2-cbor

This comment has been minimized.

Sign in to view

Fix warning

6791e40

This comment has been minimized.

Sign in to view

Fix "Stream must be replayable to calculate a body hash" error

764fea6

lauzadis added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Jul 9, 2024

This comment has been minimized.

Sign in to view

lauzadis commented Jul 9, 2024

View reviewed changes

runtime/serde/serde-cbor/common/src/aws/smithy/kotlin/runtime/serde/cbor/encoding/Simple.kt Outdated Show resolved Hide resolved

ianbotsf approved these changes Jul 10, 2024

View reviewed changes

lauzadis added 7 commits July 10, 2024 15:24

Remove duplicate deserializeByte

5d397c1

Number.kt -> Numbers.kt

186735c

Capitalize V in SmithyRpcv2Protocols

6c80127

Simple.kt -> SimpleTypes.kt, make Null an object, relocate String to …

bcca021

…Collections.kt and rename it to TextString

throw ServiceException

88a42bd

Delegate to java.math.BigDecimal for mantissa/exponent operations

061ee7d

ktlintFormat

febe4e7

This comment has been minimized.

Sign in to view

unmark CBOR as an RPC-bound protocol

db158d2

This comment has been minimized.

Sign in to view

ianbotsf approved these changes Jul 11, 2024

View reviewed changes

lauzadis added 2 commits July 11, 2024 14:46

Add Native stubs for new features

840ab18

ktlintFormat

49f07fa

lauzadis merged commit 14cf870 into main Jul 12, 2024
15 checks passed

lauzadis deleted the feat-rpcv2-cbor branch July 12, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for `smithy.protocols#rpcv2Cbor` protocol #1103

feat: add support for `smithy.protocols#rpcv2Cbor` protocol #1103

lauzadis commented Jun 19, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lauzadis commented Jul 9, 2024

This comment has been minimized.

ianbotsf left a comment

ianbotsf Jul 10, 2024

lauzadis Jul 11, 2024

ianbotsf Jul 10, 2024

lauzadis Jul 10, 2024

ianbotsf Jul 10, 2024

lauzadis Jul 10, 2024

This comment has been minimized.

This comment has been minimized.

ianbotsf left a comment

github-actions bot commented Jul 11, 2024

		public fun toHttpBody(): HttpBody = buffer.readByteArray().toHttpBody()

feat: add support for smithy.protocols#rpcv2Cbor protocol #1103

feat: add support for smithy.protocols#rpcv2Cbor protocol #1103

Conversation

lauzadis commented Jun 19, 2024 • edited Loading

Issue #

Description of changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lauzadis commented Jul 9, 2024

This comment has been minimized.

ianbotsf left a comment

Choose a reason for hiding this comment

ianbotsf Jul 10, 2024

Choose a reason for hiding this comment

lauzadis Jul 11, 2024

Choose a reason for hiding this comment

ianbotsf Jul 10, 2024

Choose a reason for hiding this comment

lauzadis Jul 10, 2024

Choose a reason for hiding this comment

ianbotsf Jul 10, 2024

Choose a reason for hiding this comment

lauzadis Jul 10, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

ianbotsf left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 11, 2024

Affected Artifacts

feat: add support for `smithy.protocols#rpcv2Cbor` protocol #1103

feat: add support for `smithy.protocols#rpcv2Cbor` protocol #1103

lauzadis commented Jun 19, 2024 •

edited

Loading