chore: Add calling AWS SDK from local test model #653
Conversation
| var ddbClient :- expect Dynamodb.DynamoDBClient(); | ||
| var resFailure := client.CallDDBScan(SimpleCallingAWSSDKFromLocalService.Types.CallDDBScanInput(ddbClient := ddbClient, tableArn := TABLE_ARN_FAILURE_CASE)); | ||
|
|
||
| expect resFailure.Failure?; |
There was a problem hiding this comment.
We can also check for Opaque error over here but first we need to check if its DynamoDB or KMS error for Dafny verification to pass and for specifically Opaque error its little fragile in Go codegen. So, it doesn't have it but lmk if you think otherwise.
There was a problem hiding this comment.
Let's add a TODO to verify the error that get's thrown? I guess this is blocked on our codegen error conversion fix?
There was a problem hiding this comment.
I changed my mind and added a check for opaque error.
So, this is how codegen would work in next PR (also in comment)
- 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.
So, there is no KMS opaque error.
There was a problem hiding this comment.
Also, DDB give opaque error when trying to access table not in the account for user with access to limited number of table because they can't say resource not found (because user does not have access to all table) and they give AccessDeniedException which is not DDB exception but an IAM exception.
rishav-karanjit
changed the title
| var ddbClient :- expect Dynamodb.DynamoDBClient(); | ||
| var resFailure := client.CallDDBScan(SimpleCallingAWSSDKFromLocalService.Types.CallDDBScanInput(ddbClient := ddbClient, tableArn := TABLE_ARN_FAILURE_CASE)); | ||
|
|
||
| expect resFailure.Failure?; |
There was a problem hiding this comment.
Let's add a TODO to verify the error that get's thrown? I guess this is blocked on our codegen error conversion fix?
|
|
||
| expect resFailure_NonExistent.Failure?; | ||
| expect resFailure_NonExistent.error.ComAmazonawsKms?; | ||
| expect resFailure_NonExistent.error.ComAmazonawsKms.NotFoundException?; |
There was a problem hiding this comment.
Ohh I see it's here. Why wasn't this possible in DDB case?
There was a problem hiding this comment.
I think my previous comment would address this comment. lmk if it does not.
| const TABLE_ARN_FAILURE_CASE := "arn:aws:dynamodb:us-west-2:370957321024:table/TestTableFailure" | ||
| const NONEXISTENT_KEY_ID := "arn:aws:kms:us-west-2:658956600833:key/b3537ef1-d8dc-4780-9f5a-55776cbb2f7g" | ||
| // The string "asdf" as bytes | ||
| const PLAIN_TEXT: Kms.Types.PlaintextType := [ 97, 115, 100, 102 ] |
There was a problem hiding this comment.
I think we aren't using these, remove them if we are not.
There was a problem hiding this comment.
Yeah. I missed removing this while doing clean. My bad.
|
|
||
| static { | ||
| DISABLED_TESTS.add("AggregateReferences"); | ||
| DISABLED_TESTS.add("CallingAWSSDKFromLocalService"); |
There was a problem hiding this comment.
Why do we have it disabled for Go?
There was a problem hiding this comment.
Just to make CI pass. I will make a PR next to fix codegen next and remove this disabled test
There was a problem hiding this comment.
Also, I was initially think to make it as a single PR but thought that single PR could be too long. So, I divided that PR into 2 parts. This is 1st part and it is already 16 files long.
|
This PR LGTM. Let's enable the tests before merge. |
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.