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

fix(dotnet): broken error name deserialization #3855

Merged
merged 5 commits into from
Nov 24, 2022

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Nov 24, 2022

Fixes a bug where the name field of an ErrorResponse would not deserialize into our enum of known names correctly. Initially it was thought that if the name field was non-null, it would only be one of our known values, but the kernel is in fact not setting this field explicitly in all cases of "user land" errors. This results in the name field being "Error"in the default case and would cause a JSON deserialization error.

Implements a custom deserializer for the ErrorName to explicitly return null if the string is not defined or unknown. Additionally makes the name field nullable.

fix: aws/aws-cdk#22369


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

Fixes a bug where the `name` field of an `ErrorResponse` would not
deserialize into our enum of known names correctly. Initially it was
thought that if the `name` field was non-null, it would only be one of
our known values, but the kernel is in fact not setting this field
explicitly in all cases of "user land" errors. This results in the
`name` field being `"Error"`in the default case and would cause a JSON
deserialization error.

Implements a custom deserializer for the `ErrorResponseName` to
explicitly return null if the string is not defined or unknown.
Additionally makes the `name` field nullable.

fix: aws/aws-cdk#22369
@MrArnoldPalmer MrArnoldPalmer requested a review from a team November 24, 2022 00:55
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 24, 2022
@MrArnoldPalmer
Copy link
Contributor Author

This took me a min to repro as it only occurs in create calls so just throwing from a method worked as expected.

@mergify
Copy link
Contributor

mergify bot commented Nov 24, 2022

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Nov 24, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 24, 2022

Merging (with squash)...

@RomainMuller RomainMuller merged commit be008d4 into main Nov 24, 2022
@RomainMuller RomainMuller deleted the fix/dotnet-error-serialization branch November 24, 2022 15:41
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Amazon.CDK.Lib): .NET- JsonSerializationException - Amazon.JSII.JsonModel.Api.Response.ErrorResponseName
2 participants