Skip to content

Implement CborErrorResponseUnmarshaller with proper error type mapping #3947

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

Conversation

muhammad-othman
Copy link
Member

Description

  • Implemented CborErrorResponseUnmarshaller.
  • Implemented CborResponseUnmarshaller template.
  • Generate CBOR exceptions protocol tests.

Motivation and Context

DOTNET-8163

Testing

  • Added smithy-rpc-v2-cbor to CloudWatch and SecretsManager models protocols, generated the marshallers/unmarshallers, and executed some requests that caused exceptions using CBOR protocol.
  • CBOR exceptions protocol tests.
  • DRY_RUN-189b330f-eb5f-4133-ac78-d7348196911e.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements CBOR error response unmarshalling functionality to properly handle exception mapping in CBOR protocol responses. The changes enable AWS SDK to correctly parse and handle error responses when using the CBOR protocol, particularly addressing the difference between shape names and error codes in exception handling.

  • Implements CborErrorResponseUnmarshaller with proper error parsing logic
  • Updates exception matching in CborResponseUnmarshaller template to use shape original names
  • Enables CBOR protocol exception tests by removing skip conditions

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
CborErrorResponseUnmarshaller.cs Implements complete error response parsing logic for CBOR protocol
CborResponseUnmarshaller.tt Updates exception matching to use ShapeOriginalName and handle code mapping
CborResponseUnmarshaller.cs Generated code changes from template updates
ExceptionShape.cs Adds ShapeOriginalName property to support CBOR exception identification
HttpProtocolTestGenerator.java Enables CBOR exception protocol tests by removing skip condition

Comment on lines +97 to +103
{
int start = type.IndexOf('#');
start = start == -1 ? 0 : start + 1;

int end = type.IndexOf(':', start);
end = end == -1 ? type.Length : end;

Copy link
Preview

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

Potential ArgumentOutOfRangeException if 'type' parameter is null or if the calculated substring indices are invalid. The method should validate the input parameter and handle edge cases where start >= end.

Suggested change
{
int start = type.IndexOf('#');
start = start == -1 ? 0 : start + 1;
int end = type.IndexOf(':', start);
end = end == -1 ? type.Length : end;
{
if (string.IsNullOrEmpty(type))
{
return string.Empty;
}
int start = type.IndexOf('#');
start = start == -1 ? 0 : start + 1;
int end = type.IndexOf(':', start);
end = end == -1 ? type.Length : end;
// Validate indices
if (start < 0 || end > type.Length || start >= end)
{
return type.Trim();
}

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to include these checks since this means we received a malformed response from the service.

reader.SkipValue();
break;
}
}
Copy link
Preview

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

The CBOR map reading logic doesn't call reader.ReadEndMap() after the while loop, which could leave the reader in an inconsistent state. Consider adding reader.ReadEndMap() after the while loop.

Suggested change
}
}
reader.ReadEndMap();

Copilot uses AI. Check for mistakes.

Copy link
Member Author

@muhammad-othman muhammad-othman Aug 6, 2025

Choose a reason for hiding this comment

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

I skipped that ReadEndMap() since in the next step we re-read the response anyway to generate the typed exception and reset the reader anyway and didn’t want to add an unnecessary step.

…rm/CborErrorResponseUnmarshaller.cs

Co-authored-by: Copilot <[email protected]>
@muhammad-othman muhammad-othman merged commit 74051ae into cbor-protocol Aug 11, 2025
1 check passed
@muhammad-othman muhammad-othman deleted the muhamoth/DOTNET-8163-cbor-exceptions-unmarshaller branch August 11, 2025 17:04
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.

3 participants