Add support for string array type in endpoint parameter #3085

sbera87 · 2024-08-22T17:17:30Z

Issue #, if available:

Description of changes:

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

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

SergeyRyabinin · 2024-08-26T07:47:34Z

tests/aws-cpp-sdk-core-tests/endpoint/endpointParamTest.cpp

@@ -0,0 +1,93 @@
+


I guess you created a separate package for this test suite and forgot to delete this file.
Please remove this file if so.

SergeyRyabinin · 2024-08-26T07:49:32Z

...ava/com/amazonaws/util/awsclientgenerator/generators/cpp/s3/S3RestXmlCppClientGenerator.java

@@ -225,21 +225,21 @@ public SdkFileEntry[] generateSourceFiles(ServiceModel serviceModel) throws Exce
                });

        serviceModel.getOperations().values().stream()
-                .filter(operationEntry -> operationEntry.getName().equals("WriteGetObjectResponse"))
+                .filter(operationEntry -> operationEntry != null && operationEntry.getName() != null && "WriteGetObjectResponse".equals(operationEntry.getName() ))


I'm not sure why this piece of legacy code needs to be updated for the given feature.
Please provide some context.

Initially I used S3 client to create the blob and found some null checks missing, so added it . But this is not necessary as with null operation name, we expect to fail

SergeyRyabinin · 2024-08-26T07:52:22Z

...onaws/util/awsclientgenerator/domainmodels/endpoints/EndpointParameterValueDeserializer.java

@@ -9,28 +10,55 @@

 import java.lang.reflect.Type;
 import java.util.Map;
+import java.util.Vector;


I think most of the SDK code gen uses java's List and ArrayList, I don't think "synchronization" feature of java's Vector is needed here.
I let Sam comment and provide guidance here.

ack. you are correct

sbera87 · 2024-09-01T01:29:24Z

tests/aws-cpp-sdk-dynamodb-unit-tests/endpointParamTest.cpp

+'q','u','i','r','e','d','"',':','t','r','u','e',',','"','d','e','f','a','u','l','t','"',':','[','"',
+'d','e','f','a','u','l','t','V','a','l','u','e','1','"',',','"','d','e','f','a','u','l','t','V','a',
+'l','u','e','2','"',']',',','"','d','o','c','u','m','e','n','t','a','t','i','o','n','"',':','"','d',
+'o','c','s','"','}','}',',','"','r','u','l','e','s','"',':','[','{','"','d','o','c','u','m','e','n',


blob generated from codegen on rules json with test staticontext params

sbera87 · 2024-09-01T20:55:42Z

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

+    }
+
+    private static final class CppEndpointsJmesPathVisitor implements
+    software.amazon.smithy.jmespath.ExpressionVisitor<Pair<String, Shape>> 


For example, the path expression
"keys(map)"
Where json:
"MapOperationRequest": {
"type": "structure",
"members": {
"map":{"shape":"Map"}
}
},
"Map":{
"type":"map",
"key":{"shape":"String"},
"value":{"shape":"String"}
},
will cause the code to generate

MapOperationRequest::EndpointParameters MapOperationRequest::GetEndpointContextParams() const { EndpointParameters parameters; //operation context params go here parameters.emplace_back(Aws::String{"stringArrayParam"}, this->GetOperationContextParams(), Aws::Endpoint::EndpointParameter::ParameterOrigin::OPERATION_CONTEXT); return parameters; } //Accessor for dynamic context endpoint params Aws::Vector<Aws::String> MapOperationRequest::GetOperationContextParams() const{ Aws::Vector<Aws::String> result; auto& mapElem = (*this).GetMap(); for (auto& keysElem : mapElem) { result.emplace_back(keysElem.first); } return result; }```

sbera87 · 2024-09-01T21:00:01Z

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

+    {
+        final OperationContextCppCodeGenerator context;
+        final Shape input;
+


Another example for autogenerated code for jmespath expression "nested.listOfObjects[*].key"
where json is:
"ListOfObjectsOperationRequest": {
"type": "structure",
"members": {
"nested":{"shape":"Nested"}
}
},
"Nested": {
"type": "structure",
"members": {
"listOfObjects":{"shape":"ListOfObjects"}
}
},
"ListOfObjects": {
"type": "list",
"member":{"shape":"ObjectMember"}
},
"ObjectMember": {
"type": "structure",
"members": {
"key":{"shape":"String"}
}
},

Code:

ListOfObjectsOperationRequest::EndpointParameters ListOfObjectsOperationRequest::GetEndpointContextParams() const { EndpointParameters parameters; //operation context params go here parameters.emplace_back(Aws::String{"stringArrayParam"}, this->GetOperationContextParams(), Aws::Endpoint::EndpointParameter::ParameterOrigin::OPERATION_CONTEXT); return parameters; } //Accessor for dynamic context endpoint params Aws::Vector<Aws::String> ListOfObjectsOperationRequest::GetOperationContextParams() const{ Aws::Vector<Aws::String> result; auto& nestedElem = (*this).GetNested().GetListOfObjects(); for (auto& listOfObjectsElem : nestedElem) { auto& keyElem = listOfObjectsElem.GetKey(); result.emplace_back(keyElem); } return result; }

sbiscigl

Please remove commented code, unused code/changes, and white space changes

src/aws-cpp-sdk-core/include/aws/core/endpoint/EndpointParameter.h

src/aws-cpp-sdk-core/source/endpoint/DefaultEndpointProvider.cpp

sbiscigl · 2024-09-03T17:26:23Z

tests/aws-cpp-sdk-dynamodb-unit-tests/endpointParamTest.cpp

+
+    auto res = endpointProvider_sp->ResolveEndpoint(parameters);
+    EXPECT_TRUE(res.IsSuccess());
+    //std::cout<<"url="<<res.GetResult().GetURL()<<std::endl;


remove commented out std::out

tests/aws-cpp-sdk-dynamodb-unit-tests/endpointParamTest.cpp

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

tools/code-generation/generator/pom.xml

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

...n/java/com/amazonaws/util/awsclientgenerator/transform/OperationContextCppCodeGenerator.java

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

sbiscigl · 2024-09-04T15:07:15Z

Please remove commented code, unused code/changes, and white space changes

please look through the PR and remove white space changes, commented, code, and comments in files that have no code changes

tests/aws-cpp-sdk-dynamodb-unit-tests/endpointParamTest.cpp

sbiscigl · 2024-09-05T01:45:00Z

...c/main/java/com/amazonaws/util/awsclientgenerator/domainmodels/codegeneration/Operation.java

@@ -11,6 +11,7 @@
 import java.util.List;
 import java.util.Map;

+


white space

sbiscigl · 2024-09-05T01:45:54Z

...ava/com/amazonaws/util/awsclientgenerator/domainmodels/endpoints/EndpointParameterValue.java

    }
    ParameterType type;
    Boolean boolValue;
    Integer intValue;
    String strValue;
+    ArrayList<String> strArrayValue;


never use ArrayList, use List or Collection, the actual underlying impl should not be a member variable type

got it, will change

...onaws/util/awsclientgenerator/domainmodels/endpoints/EndpointParameterValueDeserializer.java

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

sbiscigl · 2024-09-05T13:59:33Z

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

        operation.setStaticContextParams(c2jOperation.getStaticContextParams());
-
+       


white space and comment without changed code

sbiscigl · 2024-09-05T14:19:00Z

...ava/com/amazonaws/util/awsclientgenerator/domainmodels/endpoints/EndpointParameterValue.java

@@ -22,6 +24,23 @@ public String getValue() throws Exception {
        } else if (ParameterType.STRING == this.type) {
            String strValueEscaped = (strValue.contains("\"") || strValue.contains("\\")) ? "R\"(" + strValue + ")\"" : "\"" + strValue + "\"";
            return strValueEscaped;
+        } else if (ParameterType.STRING_ARRAY == this.type){
+            StringBuilder sb = new StringBuilder();


using a string builder and a for loop is a little old fashioned we should be using a streaming and joining that to a formatted string i.e.

public static final String AWS_VECTOR_FORMAT = "Aws::Vector<Aws::String>{%s}"; if (...) { ... } else if (ParameterType.STRING_ARRAY == this.type){ final String values = strArrayValue.stream() .map(elem -> (elem.contains("\"") || elem.contains("\\")) ? "R\"(" + elem + ")\"" : "\"" + elem + "\"") .collect(Collectors.joining(",")); return String.format(AWS_VECTOR_FORMAT, values); } ...

Got it. Will change

...ava/com/amazonaws/util/awsclientgenerator/transform/C2jModelToGeneratorModelTransformer.java

sbiscigl · 2024-09-05T15:15:34Z

...c/main/java/com/amazonaws/util/awsclientgenerator/transform/CppEndpointsJmesPathVisitor.java

+    }
+    @Override
+    public Pair<String, Shape> visitProjection(ProjectionExpression expression) {
+        Pair<String, Shape> left = expression.getLeft().accept(this);


lets modernize this and use streams

return Optional.of(expression.getLeft().accept(this)) .filter(shape -> shape.right.isList()) .map(shape -> { String varName = shape.left + "Elem"; context.rangeBasedForLoop(varName); context.openVariableScope(varName); Pair<String, Shape> right = expression.getRight().accept( new CppEndpointsJmesPathVisitor(context, shape.right.getListMember().getShape())); context.closeVariableScope(); return Pair.of(shape.left,right.right); }) .orElseThrow(() -> new SmithyBuildException("Unsupported JMESPath expression"));

sbiscigl · 2024-09-05T20:21:45Z

tests/aws-cpp-sdk-core-tests/endpoint/endpointParamTest.cpp

+
+}
+
+class  DynamoDBEndpointProviderTest : public Aws::Endpoint::DefaultEndpointProvider<>


nit: rename it without dynamo

SergeyRyabinin · 2024-09-04T17:13:57Z

src/aws-cpp-sdk-core/include/aws/core/endpoint/BuiltInParameters.h

@@ -33,7 +33,7 @@ namespace Aws
            void SetParameter(EndpointParameter param);
            void SetStringParameter(Aws::String name, Aws::String value);
            void SetBooleanParameter(Aws::String name, bool value);
-
+            void SetStringArrayParameter(Aws::String name, const Aws::Vector<Aws::String>&& value);


what is the point of const r-value reference?
I think "pass-by-value and move" or "pass-by-const-rer and copy; pass-by-rval and move" are the most common ways, while the first is the shortest one (in terms of number of methods written)

sorry missed this comment, this is a typo, i will make a pr to fix this

Not a huge deal, can be dealt with some other next pr.

SergeyRyabinin · 2024-09-04T17:15:29Z

src/aws-cpp-sdk-core/include/aws/core/endpoint/ClientContextParameters.h

@@ -28,6 +28,7 @@ namespace Aws
            void SetParameter(EndpointParameter param);
            void SetStringParameter(Aws::String name, Aws::String value);
            void SetBooleanParameter(Aws::String name, bool value);
+            void SetStringArrayParameter(Aws::String name, const Aws::Vector<Aws::String>& value);


nit: inconsistent style with the previous diff.

SergeyRyabinin · 2024-09-04T17:21:13Z

src/aws-cpp-sdk-core/source/endpoint/DefaultEndpointProvider.cpp

+                AWS_LOGSTREAM_TRACE(DEFAULT_ENDPOINT_PROVIDER_TAG, 
+                "Endpoint str array eval parameter: " << 
+                    parameter.GetName() << " = " << 
+                    []() ->  Aws::OStringStream {


I think I miss something here, but how does this lambda work without capturing parameter?

sbera87 requested review from sbiscigl and SergeyRyabinin August 22, 2024 17:17

sbera87 force-pushed the array branch from 867446f to 51fd58c Compare August 22, 2024 17:47

SergeyRyabinin reviewed Aug 26, 2024

View reviewed changes

sbera87 added 18 commits September 1, 2024 01:27

add support for string array

fa449f9

crt interface hook

c124cb3

fixes

bdb20ba

support add stringarray in endpointprovider autogen

8feb4fd

fix compiler error

a4c3b18

add fixes + test for string array endpoint param

3077f2d

add tests to cmake

6c7a633

test tweak

58828f1

test tweak

020b604

add autogen for specific jmespath field, object projections

da30382

codegen updates

5467ff1

add support for keys() smithy expression

bb88926

tweaks to support jmespath expressions

ba94a2b

integrate with existing endpoint resolution

9358db3

remove stray whitespace

5974245

name changes for convention

a9e0975

replace vector in java, fix method name case

4aa793f

header method name case fix

11b273f

sbera87 force-pushed the array branch from 62a37c7 to 11b273f Compare September 1, 2024 01:28

sbera87 requested a review from SergeyRyabinin September 1, 2024 01:28

sbera87 commented Sep 1, 2024

View reviewed changes

sbera87 added 2 commits September 1, 2024 01:32

remove unused packages

c4354c4

fix method case

e878beb

sbera87 commented Sep 1, 2024

View reviewed changes

sbera87 changed the title ~~add support for string array type in endpoint parameter [Work in Progress]~~ Add support for string array type in endpoint parameter Sep 1, 2024

sbiscigl reviewed Sep 3, 2024

View reviewed changes

address suggestions

1678c10

sbera87 requested a review from sbiscigl September 4, 2024 13:51

fix cmake

7370528

sbera87 added 8 commits September 4, 2024 17:06

fix whitespace + namespace

d2d2418

fix ws

100284b

tweaks

da6957f

whitespace fixes

4c13d5c

fix namespace

dc4f376

remove globals

2a0ea1d

remove unused vars

67dfd07

moved test to core

8e4bb85

sbiscigl reviewed Sep 5, 2024

View reviewed changes

sbera87 added 5 commits September 5, 2024 19:04

changes with stream placeholder

4911831

fixes

cca3d26

remove redundant deps

2e55271

add streams

5e2bef8

cleanup stray code

f4b39fa

sbera87 requested a review from sbiscigl September 5, 2024 20:17

sbiscigl approved these changes Sep 5, 2024

View reviewed changes

rename test class

68e8952

SergeyRyabinin reviewed Sep 5, 2024

View reviewed changes

sbera87 merged commit d759f4a into main Sep 6, 2024
4 checks passed

sbera87 deleted the array branch September 6, 2024 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for string array type in endpoint parameter #3085

Add support for string array type in endpoint parameter #3085

sbera87 commented Aug 22, 2024 •

edited

Loading

SergeyRyabinin Aug 26, 2024

sbera87 Aug 27, 2024

SergeyRyabinin Aug 26, 2024

sbera87 Aug 27, 2024

SergeyRyabinin Aug 26, 2024

sbera87 Aug 27, 2024

sbera87 Sep 1, 2024

sbera87 Sep 1, 2024 •

edited

Loading

sbera87 Sep 1, 2024

sbiscigl left a comment

sbiscigl Sep 3, 2024

sbera87 Sep 3, 2024

sbiscigl commented Sep 4, 2024

sbiscigl Sep 5, 2024

sbera87 Sep 5, 2024

sbiscigl Sep 5, 2024

sbera87 Sep 5, 2024

sbiscigl Sep 5, 2024

sbiscigl Sep 5, 2024

sbera87 Sep 5, 2024

sbiscigl Sep 5, 2024

sbiscigl Sep 5, 2024

sbera87 Sep 5, 2024

SergeyRyabinin Sep 4, 2024

sbera87 Sep 6, 2024

SergeyRyabinin Sep 6, 2024

SergeyRyabinin Sep 4, 2024

SergeyRyabinin Sep 4, 2024

		@@ -11,6 +11,7 @@
		import java.util.List;
		import java.util.Map;

		operation.setStaticContextParams(c2jOperation.getStaticContextParams());


		}

		class DynamoDBEndpointProviderTest : public Aws::Endpoint::DefaultEndpointProvider<>

Add support for string array type in endpoint parameter #3085

Add support for string array type in endpoint parameter #3085

Conversation

sbera87 commented Aug 22, 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

sbera87 Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbiscigl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbiscigl commented Sep 4, 2024

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

sbera87 commented Aug 22, 2024 •

edited

Loading

sbera87 Sep 1, 2024 •

edited

Loading