Avoid creating unnecessary intermediate strings

[paulomorgado]

Addresses #3415

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project
• My change requires a change to the documentation
• I have updated the documentation accordingly
• I have read the README document
• I have added tests to cover my changes
• All new and existing tests passed

License

• I confirm that this pull request can be released under the Apache 2 license

/cc @bhoradc

[slang25]

fix typo

[slang25]

This change would be awesome, I spotted this a few months back, basically all JSON APIs are using triple the necessary memory for the request body.

[peterrsongg]

+1 to fixing the typo, otherwise change looks okay. I'm running a version of this branch with the typo fix through our build system, will approve if it passes the dry-run.

[peterrsongg]

The change caused some existing unit tests to fail in our build system. I'll need to take a look at why this is the case

[paulomorgado]

@peterrsongg,

The change caused some existing unit tests to fail in our build system. I'll need to take a look at why this is the case

Is this something I can have a look at?

[peterrsongg]

@peterrsongg,

The change caused some existing unit tests to fail in our build system. I'll need to take a look at why this is the case

Is this something I can have a look at?

If you wanted to take a look that'd be great. I'm caught up in something else right now, but here are the failed tests

====== TestWrapper Run Attempt 3 =====
--
899 | MSTestRunner
900 | Failed Tests :
901 | NotRequiredEnabledNoIdentifiersNoWait
902 |  
903 | RequiredNoIdentifiersFailedDiscovery
904 |  
905 | RequiredWithIdentifiers
906 |  
907 | NotRequiredEnabledHasIdentifiersWait
908 |  
909 | RequiredNoIdentifiersEvictCache
910 |  
911 | CacheEvictionTestUsingEndpointDiscoveryCallStack
912 |  
913 | NotRequiredEnabledHasIdentifiersNoWait
914 |  
915 | NotRequiredNoIdentifiersFailedDiscoveryWait
916 |  
917 | NotRequiredEnabledNoIdentifiersWait
918 |  
919 | NotRequiredNoIdentifiersFailedDiscoveryNoWait
920 |  
921 | RequiredNoIdentifiersCached
922 |  
923 | RequiredWithIdentifiersCached
924 |  
925 | RequiredNoIdentifiers
926 |  
927 | Passed : 0, Failed : 17

All of the failures have the same error message

Error Message:
--
1013 | Test method AWSSDK.UnitTests.EndpointDiscoveryTests.RequiredNoIdentifiers threw exception:
1014 | System.ArgumentNullException: Value cannot be null.
1015 | Parameter name: encoding
1016 | Stack Trace:
1017 | at System.IO.StreamWriter..ctor(Stream stream, Encoding encoding, Int32 bufferSize, Boolean leaveOpen)

at Amazon.DynamoDBv2.Model.Internal.MarshallTransformations.CreateTableRequestMarshaller.Marshall(CreateTableRequest publicRequest) ins rc\aws-sdk-net\sdk\src\Services\DynamoDBv2\Generated\Model\Internal\MarshallTransformations\CreateTableRequestMarshaller.cs:line 68
--
1019 | at AWSSDK.UnitTests.EndpointDiscoveryTests.CreateExecutionContext(EndpointDiscoveryTestClient client, AmazonDynamoDBConfig config, Boolean required, SortedDictionary`2 identifiers) in src\aws-sdk-net\sdk\test\UnitTests\Custom\Runtime\EndpointDiscoveryTests.cs:line 374
1020 | at AWSSDK.UnitTests.EndpointDiscoveryTests.RequiredNoIdentifiers(String endpoint, String expectedEndpoint) in src2154271321\src\aws-sdk-net\sdk\test\UnitTests\Custom\Runtime\EndpointDiscoveryTests.cs:line 64

I haven't had the chance to investigate further yet.

[paulomorgado]

@peterrsongg,

The change caused some existing unit tests to fail in our build system. I'll need to take a look at why this is the case

Is this something I can have a look at?

If you wanted to take a look that'd be great. I'm caught up in something else right now, but here are the failed tests

Are, by any chance, the tests failing only on .NET Framework?

Because MemoryStream allows to invoke ToArray() after being disposed, I've changed to use the StreamWriter(Stream) constructor, that closes the output stream, but uses UTF-8 with no BOM, which is what I intended to guarantee.

[peterrsongg]

@peterrsongg,

The change caused some existing unit tests to fail in our build system. I'll need to take a look at why this is the case

Is this something I can have a look at?

If you wanted to take a look that'd be great. I'm caught up in something else right now, but here are the failed tests

Are, by any chance, the tests failing only on .NET Framework?

Because MemoryStream allows to invoke ToArray() after being disposed, I've changed to use the StreamWriter(Stream) constructor, that closes the output stream, but uses UTF-8 with no BOM, which is what I intended to guarantee.

Yup the tests are only failing in .NET Framework. Looks like your change addresses the issue. I'll run it through our system once more once you feel it is ready.

[paulomorgado]

Yup the tests are only failing in .NET Framework. Looks like your change addresses the issue. I'll run it through our system once more once you feel it is ready.

Check if the new version passes all tests.

I haven't figured out how to run the code generation and tests.

[dscpinheiro]

I'm running your latest commit through our build system. Can you also rebase your branch on top of v4-development? Your other PR (for the user-agent refactoring) was already merged, so it doesn't need to be included here.

---

We don't have a guide for external contributors yet, but this is how I usually make changes to the generator:

• Open the generator\AWSSDKGenerator.sln solution and set ServiceClientGenerator as starting project
• Update any .tt files (since you already modified JsonRPCRequestMarshaller.tt, you may have to right click -> Run Custom Tool in Visual Studio so that the code behind file - JsonRPCRequestMarshaller.cs - gets updated too)
• Run the ServiceClientGenerator project
  • Since you're modifying marshallers, this will update a lot of generated files - you do not need to include them in PRs, our build system takes care of updating them

At this point, you should be able to open the sdk\AWSSDK.CoreAndCustomUnitTests.NetFramework.sln or sdk\AWSSDK.CoreAndCustomUnitTests.NetStandard.sln solutions and run their unit tests (note that the complete set of tests are in the sdk\AWSSDK.NetFramework.sln and sdk\AWSSDK.NetStandard.sln solutions but those include all services and aren't easy to work with locally).

[paulomorgado]

Hi @dscpinheiro,

I've rebased on v4-development.

It would be nice if building would run the templates.

Beware that there are NuGet packages with known vulnerabilities being referenced.

[normj]

With this change we are losing the setting of CultureInfo.InvariantCulture as the IFormatProvider which was previously done with the StringWriter. There isn't a way to set the IFormatProvider directly on the StreamWriter. I'm wondering if we should create a subclass StreamWriter in Amazon.Runtime.Internal.Util so we can set the IFormatProvider and then reference that type instead of StreamWriter in the generated code. Something like this

public class InvariantCultureStreamWriter : StreamWriter
{
    public InvariantCultureStreamWriter (Stream stream)
        : base(stream)
    {
    }

    public override IFormatProvider FormatProvider
    {
        get
        {
            return CultureInfo.InvariantCulture;
        }
    }
}

[paulomorgado]

@normj,

With this change we are losing the setting of CultureInfo.InvariantCulture as the IFormatProvider which was previously done with the StringWriter. There isn't a way to set the IFormatProvider directly on the StreamWriter. I'm wondering if we should create a subclass StreamWriter in Amazon.Runtime.Internal.Util so we can set the IFormatProvider and then reference that type instead of StreamWriter in the generated code. Something like this

public class InvariantCultureStreamWriter : StreamWriter
{
    public InvariantCultureStreamWriter (Stream stream)
        : base(stream)
    {
    }

    public override IFormatProvider FormatProvider
    {
        get
        {
            return CultureInfo.InvariantCulture;
        }
    }
}

Probably the safest option.

Have any of the tests failed? I thought this was used only with JSON and XML writers that already take care of this.

[normj]

Have any of the tests failed? I thought this was used only with JSON and XML writers that already take care of this.

@peterrsongg and @dscpinheiro can speak for the state of the tests. They asked me to take pass because they were concerned about the CultureInfo.InvariantCulture change. It is possible the subclass is overkill but I'm not confident the LitJson JsonWriter handles everything as CultureInfo.InvariantCulture so I would rather play it safe.

[paulomorgado]

Have any of the tests failed? I thought this was used only with JSON and XML writers that already take care of this.

@peterrsongg and @dscpinheiro can speak for the state of the tests. They asked me to take pass because they were concerned about the CultureInfo.InvariantCulture change. It is possible the subclass is overkill but I'm not confident the LitJson JsonWriter handles everything as CultureInfo.InvariantCulture so I would rather play it safe.

It's a safe and small overhead. Where do you want me to put it?

[dscpinheiro]

None of the tests failed, but our integration tests do not validate different cultures at the moment.

You can put the file Norm mentioned in this location: https://github.com/aws/aws-sdk-net/tree/v4-development/sdk/src/Core/Amazon.Runtime/Internal/Util

[paulomorgado]

None of the tests failed, but our integration tests do not validate different cultures at the moment.

You can put the file Norm mentioned in this location: https://github.com/aws/aws-sdk-net/tree/v4-development/sdk/src/Core/Amazon.Runtime/Internal/Util

Done!