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: rpc2: cbor codec #492

Merged
merged 13 commits into from
Feb 20, 2024
Merged

feat: rpc2: cbor codec #492

merged 13 commits into from
Feb 20, 2024

Conversation

lucix-aws
Copy link
Contributor

@lucix-aws lucix-aws commented Jan 10, 2024

CBOR codec implementation.

@lucix-aws lucix-aws requested review from a team as code owners January 10, 2024 15:44
@lucix-aws lucix-aws marked this pull request as draft January 10, 2024 15:44
@lucix-aws lucix-aws marked this pull request as ready for review February 5, 2024 23:19
// Package cbor implements partial encoding/decoding of concise binary object
// representation (CBOR) described in RFC 8949.
//
// This package implements a subset of the specification required to support
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what is meant by specification? does this package implement a subset of the smithy rpcv2-cbor protocol or a subset of CBOR as described in RFC 8949?

Copy link

Choose a reason for hiding this comment

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

Agreed, I think being clear in this preamble documentation on what is/isn't in line with the CBOR spec is important (half-precision, etc.)

// Package cbor implements partial encoding/decoding of concise binary object
// representation (CBOR) described in RFC 8949.
//
// This package implements a subset of the specification required to support
Copy link

Choose a reason for hiding this comment

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

Agreed, I think being clear in this preamble documentation on what is/isn't in line with the CBOR spec is important (half-precision, etc.)

type Uint uint64

// NegInt describes a CBOR negative int (major type 1).
type NegInt uint64
Copy link

Choose a reason for hiding this comment

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

How are customers expected to use this value in a negative context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't ever see it - it's just an intermediate value for encode/decode (like all of these Value implementations are). Behind the scenes we figure out whether this value fits into the type of the modeled field when we deserialize.

NegInt(1),
},
"negint/8/max": {
[]byte{1<<5 | 27, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfe},
Copy link

Choose a reason for hiding this comment

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

What about if this were to end in 0xff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So essentially I chose to build in the -1 bias from type 1 into the codec layer. Since the range of type1 is [-2^64,-1], the wrap to zero that comes from a type1 of 64 1s would basically be that implicit 65-bit left bound.

In practice remember the caller doesn't ever see it, it's something the generated deserializer would reject since -2nd ^64 doesn't fit in long.

We could drop the implicit bias here and instead have the generated deserializers reintroduce it but I think we'd be trading up in terms of code size (it's a special case on NegInt(0) vs a bunch of +1s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe to clarify, this code exists in the in-progress deserializer, this is basically what happens when we're deserializing to something that's modeled as a long.

// AsInt64 coerces a Value to its int64 representation if possible.
func AsInt64(v Value) (int64, error) {
    const max64 = 0x7fffffff_ffffffff

    switch vv := v.(type) {
    case Uint:
        if vv > max64 {
            return 0, fmt.Errorf("cbor uint %d exceeds max int64 value", vv) 
        }   
        return int64(vv), nil 
    case NegInt:
        if vv > max64+1 || vv == 0 { // -1 bias means NegInt(0) is actually -2^64
            return 0, fmt.Errorf("cbor negint %d exceeds min int64 value", vv) 
        }   
        return -int64(vv), nil 
    }   
    return 0, fmt.Errorf("unexpected value type %T", v)
}

return 0, 0, fmt.Errorf("decode argument: %w", err)
}

return NegInt(i + 1), off, nil
Copy link

Choose a reason for hiding this comment

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

Max value of a uint64 would be 2^64 - 1, but this would add that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

@lucix-aws lucix-aws requested a review from kstich February 19, 2024 18:33
@lucix-aws lucix-aws changed the base branch from main to feat-rpc2 February 20, 2024 17:53
@lucix-aws lucix-aws merged commit b461aa4 into feat-rpc2 Feb 20, 2024
11 checks passed
lucix-aws added a commit that referenced this pull request Feb 26, 2024
@lucix-aws lucix-aws deleted the feat-rpc2-cbor branch February 26, 2024 15:41
lucix-aws added a commit that referenced this pull request Mar 1, 2024
lucix-aws added a commit that referenced this pull request Mar 7, 2024
lucix-aws added a commit that referenced this pull request Mar 29, 2024
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.

4 participants