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: Support RPCv2 CBOR wire protocol #887

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

feat: Support RPCv2 CBOR wire protocol #887

wants to merge 27 commits into from

Conversation

dayaffe
Copy link
Contributor

@dayaffe dayaffe commented Dec 24, 2024

Issue #

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.

self.children = Self.children(from: cborValue, parent: self)
}

public static func from(data: Data) throws -> Reader {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function calls CBORDecoder which is defined in CRT for actually decoding individual types

return Reader(nodeInfo: "", cborValue: rootValue, parent: nil)
}

private static func children(from cborValue: CBORType?, parent: Reader) -> [Reader] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function helps form a graph for nested and complex types

}
}

public func writeMap<T>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

map and array encoding is particularly complex due to the possibility of nested types. A map can be within an array and vice versa

try valueWritingClosure(val, writer)

// If the writer itself doesn't have a cborValue, build it from its children
if writer.cborValue == nil, !writer.children.isEmpty {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

its possible cborValue wasnt yet assigned, in that case let's move on and parse the children first then come back

import Foundation
@_spi(SmithyReadWrite) import SmithyCBOR

public struct CBORComparator {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is solely used for testing. we need to compare the semantic values rather than the exact binary values. CRT should have sufficiently tested their de-serializer against SEP prescribed tests to ensure that using the Decoder in protocol test comparison is ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the testing in CRT side done already? If not, what's the known timeline?

operationMiddleware.appendMiddleware(operation, AuthSchemeMiddleware(ctx.model, ctx.symbolProvider))
for (integration in ctx.integrations) {
integration.customizeMiddleware(ctx, operation, operationMiddleware)
}
// must be last to support adding business metrics
addUserAgentMiddleware(ctx, operation)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bug fix: addUserAgentMiddleware handles feature id (business metric) tracking. This was supposed to be last in the stack but it never was prior to this change due to protocol-specific middleware

val bodyContent = test.body.get().replace("\\\"", "\\\\\"")
val data: String = if (isCbor) {
// Attempt to decode Base64 data once for CBOR
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CBOR data is binary data but for protocol tests it need to be passed as a base64 encoded string

@dayaffe dayaffe requested a review from jbelkins December 26, 2024 21:13
@dayaffe dayaffe requested a review from sichanyoo December 26, 2024 21:13
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

Mostly Qs

Sources/SmithyCBOR/Reader/Reader.swift Show resolved Hide resolved
Sources/SmithyCBOR/Reader/Reader.swift Outdated Show resolved Hide resolved
Sources/SmithyCBOR/Writer/Writer.swift Show resolved Hide resolved
Sources/SmithyCBOR/Writer/Writer.swift Show resolved Hide resolved
Sources/SmithyCBOR/Writer/Writer.swift Show resolved Hide resolved
Sources/SmithyCBOR/Writer/Writer.swift Show resolved Hide resolved
Sources/SmithyReadWrite/SmithyReader.swift Outdated Show resolved Hide resolved
import Foundation
@_spi(SmithyReadWrite) import SmithyCBOR

public struct CBORComparator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the testing in CRT side done already? If not, what's the known timeline?

@dayaffe dayaffe requested a review from sichanyoo December 31, 2024 18:06
Copy link
Contributor

@sichanyoo sichanyoo left a comment

Choose a reason for hiding this comment

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

LGTM; should be good to merge after Josh's review as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants