-
Notifications
You must be signed in to change notification settings - Fork 8
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: codegen changes for calling aws sdk from local service #538
base: rishav-awssdkFromLocal-Testmodel
Are you sure you want to change the base?
fix: codegen changes for calling aws sdk from local service #538
Conversation
// Note: As of this writing, Go delegates the error first to KMS. | ||
// If KMS is opaque, it is going to delegate to DDB. And return a DDB error (opaque or non opaque) | ||
// If KMS is not opaque, it returns ComAmazonawsKms error. | ||
expect resFailure.error.ComAmazonawsDynamodb?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was this was opaque and this could be created by KMS or DDB in this model as the smithy model has both KMS and DDB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is the AcessDeniedException
?
EDIT: Maybe in a follow up PR we assert that we do end up with non-opaque exceptions?
SmithyNameResolver.shapeNamespace(depShape) | ||
); | ||
} | ||
handleDepErrorSerializer(context, w, dependencies); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method was getting long. So, broke down into a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comments and edits.
@@ -0,0 +1,44 @@ | |||
// Code generated by smithy-go-codegen DO NOT EDIT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this checked-in? It's a new model so we should be able to name this properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can name this package for shim in lowercase but I thought it won't align with other test model (For example CodegenPatches
so I checked in Shim.
// Note: As of this writing, Go delegates the error first to KMS. | ||
// If KMS is opaque, it is going to delegate to DDB. And return a DDB error (opaque or non opaque) | ||
// If KMS is not opaque, it returns ComAmazonawsKms error. | ||
expect resFailure.error.ComAmazonawsDynamodb?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is the AcessDeniedException
?
EDIT: Maybe in a follow up PR we assert that we do end up with non-opaque exceptions?
if (sdkDepShape != null) { | ||
sdkErrHandler.append( | ||
""" | ||
return %s.Create_%s_(%s) | ||
""".formatted( | ||
DafnyNameResolver.getDafnyErrorCompanion(serviceShape), | ||
DafnyNameResolver.dafnyNamespace(sdkDepShape), | ||
sdkDepShape | ||
.expectTrait(ServiceTrait.class) | ||
.getSdkId() | ||
.concat("Error") | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarification: Didn't we already handle this at line 1028?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for opaque. Overall, the sdk dependency error will be like
case smithy.APIError:
DynamoDBError := comamazonawsdynamodbsmithygenerated.Error_ToDafny(err)
if !DynamoDBError.Is_Opaque() {
return SimpleCallingawssdkfromlocalserviceTypes.Companion_Error_.Create_ComAmazonawsDynamodb_(DynamoDBError)
}
KMSError := comamazonawskmssmithygenerated.Error_ToDafny(err)
if !KMSError.Is_Opaque() {
return SimpleCallingawssdkfromlocalserviceTypes.Companion_Error_.Create_ComAmazonawsKms_(KMSError)
}
return SimpleCallingawssdkfromlocalserviceTypes.Companion_Error_.Create_ComAmazonawsKms_(KMSError)
@@ -158,6 +158,10 @@ public static String getDafnyClient(final String sdkId) { | |||
return sdkId.concat(DOT).concat(sdkId).concat("Client"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: Does not of these work and we have to create a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work when we need client name with namespace. But in my case I just need client name without namespace
…lang/smithy-dafny into rishav-add-CallingAWSSDK
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.