fix: codegen changes for calling aws sdk from local service #538

rishav-karanjit · 2024-08-23T16:14:49Z

Issue #, if available:

Description of changes:

Codegen changes for Calling aws sdk from local service
Ignoring smithy-dafny-codegen-cli workflows / gradle-build-smithy-dafny (macos-12) (pull_request) because this will be fixed from main

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This reverts commit 327746c.

…ingAWSSDK

This reverts commit 72b4384.

into rishav-add-CallingAWSSDK

rishav-karanjit · 2024-11-04T20:20:57Z

TestModels/CallingAWSSDKFromLocalService/Makefile


 # Go

 GO_DEPENDENCY_MODULE_NAMES := \
 	--dependency-library-name=com.amazonaws.kms=github.com/smithy-lang/smithy-dafny/kmsv2 \
-	--dependency-library-name=com.amazonaws.dynamodb=github.com/smithy-lang/smithy-dafny/ddbv2
+	--dependency-library-name=com.amazonaws.dynamodb=github.com/smithy-lang/smithy-dafny/ddbv2 \
+	--dependency-library-name=sdk.com.amazonaws.dynamodb=github.com/aws/aws-sdk-go-v2/service/dynamodb \


I need this to import kms or dynamodb and make client in native side

What would happen if the namespace for our model and the sdk collides?

rishav-karanjit · 2024-11-04T20:38:10Z

TestModels/CallingAWSSDKFromLocalService/Model/CallingAWSSDKFromLocalService.smithy

@@ -45,10 +49,47 @@ structure CallDDBScanInput {
 }

 structure CallDDBScanOutput {
-  @required


Removing required because I will have to return a value if its required. Also, having a check expect itemOutput.Value.Some? makes more sense

Why not return 0 since this is an Integer?

If I return 0 and if DDB has 0 item, the success case will have false failure.

rishav-karanjit · 2024-11-04T20:41:41Z

TestModels/CallingAWSSDKFromLocalService/Model/CallingAWSSDKFromLocalService.smithy

+  conditionExpression: com.amazonaws.dynamodb#ConditionExpression
+}
+
+structure CallDDBPutItemOutput {


There is nothing I could return from PutItem call. So, this is empty.

PutItemOutput.Attributes will return empty unless putitem has overridden a item

rishav-karanjit · 2024-11-04T20:43:48Z

TestModels/CallingAWSSDKFromLocalService/src/SimpleCallingAWSSDKFromLocalServiceImpl.dfy

@@ -1,56 +0,0 @@
-// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.


Unlike what git diff shows. This is renaming file. Not removing it.

rishav-karanjit · 2024-11-04T20:44:58Z

TestModels/CallingAWSSDKFromLocalService/test/SimpleCallingAWSSDKFromLocalServiceImplTest.dfy

This file is also a file name renaming change not deleting it.

rishav-karanjit · 2024-11-04T20:51:48Z

.../java/software/amazon/polymorph/smithygo/awssdk/DafnyAwsSdkClientTypeConversionProtocol.java

@@ -806,7 +814,8 @@ private void generateSerializerFunctions(
            );
          var inputType = GoCodegenUtils.getType(
            context.symbolProvider().toSymbol(visitingShape),
-            serviceShape.expectTrait(ServiceTrait.class)
+            serviceShape.expectTrait(ServiceTrait.class),
+            true


GoCodegenUtils.getType has now an update so that we can say to include namespace in the types or not. Previously, this boolean was not there so this is updated.

rishav-karanjit · 2024-11-04T20:53:23Z

.../smithy-dafny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/GoWriter.java

@@ -689,6 +689,8 @@ private class GoSymbolFormatter
    public String apply(Object type, String indent) {
      if (type instanceof Symbol) {
        Symbol resolvedSymbol = (Symbol) type;
+        // get symbol name for literal
+        // Note: This literal could be changed in upcoming codes in this if block


Just some comments because I got confused read this codes.

I don't see any change in the literal though? Aren't we required to change this to use map[string][ddbtypes.AttributeValue] ?

Yes. Its here. https://github.com/smithy-lang/smithy-dafny/pull/538/files#r1828444684

rishav-karanjit · 2024-11-04T21:07:37Z

...hy-dafny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/SymbolVisitor.java

            .build()
        );
      } else {
        builder.putProperty(
          "Referred",
          symbolBuilderFor(
            referredShape,
-            "I".concat(getDefaultShapeName(referredShape))


We have reversed the if else loop. So, this is in diff.

rishav-karanjit · 2024-11-04T21:09:16Z

...ny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/ValidationGenerator.java

        var inputType = GoCodegenUtils.getType(
          symbolProvider.toSymbol(currentShape),
-          currentShape
+          currentShape,
+          isExternalShape


If external shape then include namespace or else don't include

rishav-karanjit · 2024-11-04T21:33:46Z

...in/java/software/amazon/polymorph/smithygo/localservice/nameresolver/SmithyNameResolver.java

@@ -60,9 +62,14 @@ public static String shapeNamespace(final Shape shape) {
  }

  public static String smithyTypesNamespace(final Shape shape) {


The type namespace for service should be "github.com/aws/aws-sdk-go-v2/service/dynamodb/types" (or KMS one) but there can be multiple service in a model and this will cause confusion between kms and ddb. So, I have decided to make it kmstypes or dynamodbtypes.

Yes - we always need to alias it.

ShubhamChaturvedi7

Some suggestions / clarifications below, otherwise LGTM.

ShubhamChaturvedi7 · 2024-11-04T21:53:20Z

TestModels/CallingAWSSDKFromLocalService/Model/CallingAWSSDKFromLocalService.smithy

@@ -45,10 +49,47 @@ structure CallDDBScanInput {
 }

 structure CallDDBScanOutput {
-  @required


Why not return 0 since this is an Integer?

ShubhamChaturvedi7 · 2024-11-04T21:53:25Z

TestModels/CallingAWSSDKFromLocalService/Makefile


 # Go

 GO_DEPENDENCY_MODULE_NAMES := \
 	--dependency-library-name=com.amazonaws.kms=github.com/smithy-lang/smithy-dafny/kmsv2 \
-	--dependency-library-name=com.amazonaws.dynamodb=github.com/smithy-lang/smithy-dafny/ddbv2
+	--dependency-library-name=com.amazonaws.dynamodb=github.com/smithy-lang/smithy-dafny/ddbv2 \
+	--dependency-library-name=sdk.com.amazonaws.dynamodb=github.com/aws/aws-sdk-go-v2/service/dynamodb \


What would happen if the namespace for our model and the sdk collides?

ShubhamChaturvedi7 · 2024-11-04T21:58:25Z

.../smithy-dafny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/GoWriter.java

@@ -689,6 +689,8 @@ private class GoSymbolFormatter
    public String apply(Object type, String indent) {
      if (type instanceof Symbol) {
        Symbol resolvedSymbol = (Symbol) type;
+        // get symbol name for literal
+        // Note: This literal could be changed in upcoming codes in this if block


I don't see any change in the literal though? Aren't we required to change this to use map[string][ddbtypes.AttributeValue] ?

ShubhamChaturvedi7 · 2024-11-04T21:59:31Z

...hy-dafny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/SymbolVisitor.java

+    if (shape.hasTrait(ServiceTrait.class)) {
+      namespace =
+        shape.expectTrait(ServiceTrait.class).getSdkId().toLowerCase();


Ahh so this is where the map case get handled as well.

ShubhamChaturvedi7 · 2024-11-04T22:00:20Z

...ny-codegen/src/main/java/software/amazon/polymorph/smithygo/codegen/ValidationGenerator.java

+        if (isExternalShape) {
+          if (SmithyNameResolver.isShapeFromAWSSDK(currentShape)) {
+            writer.addImportFromModule(
+              SmithyNameResolver.getGoModuleNameForSdkNamespace(
+                currentShape.getId().getNamespace()
+              ),
+              "types",
+              SmithyNameResolver.smithyTypesNamespace(currentShape)
+            );
+          } else {
+            writer.addImportFromModule(
+              SmithyNameResolver.getGoModuleNameForSmithyNamespace(
+                currentShape.getId().getNamespace()
+              ),
+              SmithyNameResolver.smithyTypesNamespace(currentShape)
+            );
+          }
+        }


Suggestion: This could be moved inside the SmithyNameResolver.

ShubhamChaturvedi7 · 2024-11-04T22:01:33Z

...in/java/software/amazon/polymorph/smithygo/localservice/nameresolver/SmithyNameResolver.java

@@ -60,9 +62,14 @@ public static String shapeNamespace(final Shape shape) {
  }

  public static String smithyTypesNamespace(final Shape shape) {


Yes - we always need to alias it.

ShubhamChaturvedi7 · 2024-11-04T22:03:05Z

.../software/amazon/polymorph/smithygo/localservice/shapevisitor/DafnyToSmithyShapeVisitor.java

+    if (SmithyNameResolver.isShapeFromAWSSDK(shape)) {
+      writer.addImportFromModule(
+        SmithyNameResolver.getGoModuleNameForSdkNamespace(
+          shape.getId().getNamespace()
+        ),
+        "types",
+        SmithyNameResolver.smithyTypesNamespace(shape)
+      );
+    }


Same, all this can be moved inside the NameResolver and we can then avoid this repetition.

Let's do it in next PR once positional is merged.

rishav-karanjit changed the title ~~feat (TestModel): Add CallingAWSSDKFromLocal~~ feat(TestModel): Add CallingAWSSDKFromLocal Aug 23, 2024

Add MakeFile

992c669

rishav-karanjit changed the base branch from main-1.x to Golang/dev September 4, 2024 00:30

rishav-karanjit changed the base branch from Golang/dev to main-1.x September 4, 2024 00:31

rishav-karanjit added 5 commits September 3, 2024 17:51

Update Makefile

31ab1d9

Revert "Revert "Change dotnet and test""

8769cee

This reverts commit 327746c.

Add python runtime

046b73c

Update dependencies

38aae8e

Update dependencies

7791bae

robin-aws mentioned this pull request Sep 4, 2024

Add operations on local service references (add to LocalService) #374

Closed

rishav-karanjit changed the base branch from main-1.x to Golang/dev September 4, 2024 16:28

rishav-karanjit added 19 commits September 4, 2024 10:34

Merge remote-tracking branch 'origin/Golang/dev' into rishav-add-Call…

6a4e630

…ingAWSSDK

Revert changes to model.json in KMS

72b4384

Update MakeFile with Go module

4bd8251

Add Go comment

a4431a7

Revert "Revert changes to model.json in KMS"

a2cf766

This reverts commit 72b4384.

Update smithy model for v2 smithy of kms

0e8c581

Update to return keyid:

d37b806

Merge branch 'Golang/dev' of https://github.com/smithy-lang/smithy-dafny

2af2588

into rishav-add-CallingAWSSDK

Merge branch 'Golang/dev' into rishav-add-CallingAWSSDK

a4cb20b

Commit go mod

be7301c

Add everything

db0e27a

Add extern

712cbcf

Push KMS runtime

ec09dc1

Merge from Golang/dev

7783d4e

Update make file

ed8eca6

If dep service has no local service trait then continue

1d832a8

Update make file

b7ace1c

Service does not have a special logic for Referred shape

ba93f86

Update smithyTypesNamespace

51ca930

rishav-karanjit added 27 commits October 29, 2024 11:48

Add imports for aws sdk service types in shape visitor

2c34c17

Update validation Generator for dependent shape

907d090

Add includeNamespace boolean

26afcee

Update validation generator constructor

c26d62f

Fix

e3c9537

Model update

a850adf

fix

8bd1aab

Update test model

32617c8

format dafny

c80b3f4

format java

3036146

maybe verification fix

8c68dab

Update test model

eb2646b

Update aws sdk opaque error

7d194cd

handle new opaque error

27a8eea

Change dafny client to native ones in native go land

aab2c4f

Fix

878687e

Fix imports

88c163e

Create opaque error not panic

eb1534c

Format

830563c

Fix imports

2f7f08b

Formatting

c86e881

Remove required from output

323a09b

Typo fix

d3347e9

Refactor

b4c0d39

Use sdk id name in symbol visitor

c450905

Refactor redundant if else

00a961a

Add import in container shapes

8bb7957

rishav-karanjit commented Nov 4, 2024

View reviewed changes

ShubhamChaturvedi7 approved these changes Nov 4, 2024

View reviewed changes

rishav-karanjit merged commit 02579d9 into rishav-awssdkFromLocal-Testmodel Nov 4, 2024
85 of 86 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: codegen changes for calling aws sdk from local service #538

fix: codegen changes for calling aws sdk from local service #538

rishav-karanjit commented Aug 23, 2024 •

edited

Loading

rishav-karanjit Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

rishav-karanjit Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

rishav-karanjit Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 left a comment

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

ShubhamChaturvedi7 Nov 4, 2024

rishav-karanjit Nov 4, 2024

		@@ -1,56 +0,0 @@
		// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.

		@@ -60,9 +62,14 @@ public static String shapeNamespace(final Shape shape) {
		}

		public static String smithyTypesNamespace(final Shape shape) {

fix: codegen changes for calling aws sdk from local service #538

fix: codegen changes for calling aws sdk from local service #538

Conversation

rishav-karanjit commented Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShubhamChaturvedi7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishav-karanjit commented Aug 23, 2024 •

edited

Loading