[S3CrtClient]: please declare client methods 'virtual', to support testability

[grrtrr]

Problem description

Most of the S3CrtClient methods have been declared as virtual, which supports a well-established pattern of creating a mock class based on the parent class.

The only exceptions are the following methods, which are thus very difficult to test:

• Aws::String GeneratePresignedUrl (x 2)
• Aws::String GeneratePresignedUrlWithSSES3 (x 2)
• Aws::String GeneratePresignedUrlWithSSEKMS (x 2)
• Aws::String GeneratePresignedUrlWithSSEC (x 2)

(It is a bit unfortunate that these return Aws::String instead of Aws::Outcome<Aws::String>, since the only way to detect an error is to detect if the returned result is empty; but this would mean a breaking change).

It would also be nice to have the following public methods as virtual:

• void OverrideEndpoint()
• Aws::Client::XmlOutcome GenerateXmlOutcome
• Aws::Client::StreamOutcome GenerateStreamOutcome

Proposal

Please declare the above listed methods as virtual.

Other Information

This is present on today's master (1.11.580 and above).

Acknowledgements

• I may be able to implement this feature request.
• This feature does not incur a breaking change.

[sbaluja]

Thanks for creating this ticket. What's the idea behind wanting to override these functions? Both the GenerateXmlOutcome and GenerateStreamOutcome are used in callbacks (ex. GetObjectRequestShutdownCallback, PutObjectRequestShutdownCallback) for some operations, so I'm trying to understand your use case.

[grrtrr]

Thanks for creating this ticket. What's the idea behind wanting to override these functions? Both the GenerateXmlOutcome and GenerateStreamOutcome are used in callbacks (ex. GetObjectRequestShutdownCallback, PutObjectRequestShutdownCallback) for some operations, so I'm trying to understand your use case.

Please see this document for an introduction. Mocking is used to avoid making network calls, and uses gmock to create mock objects. Doing so requires the parent class to declare the to-be-mocked methods as virtual. Otherwise, testing is very difficult.

Ok, callbacks may not need to be virtual.

[sbiscigl]

So a little bit of background on this so you understand where we're coming from, but yes will add virtual to the methods in question.

A little bit of history though is that the SDK code generator has a argument called enable-virtual-operations, which is described by us

Code generator raw argument to be passed through to mark operation functions in service client as virtual functions. Always on by default

This argument exists so that customers using our client can re-generate the clients without virtual functions so that customers can have more aggressive link time optimizations and not consume the functions they are not using when statically linking.

That being said ALL functions in the class should follow this pattern, i.e. virtual by default, non-virtual by opt-in. so the pre-signer function alongside override endpoint you mention just dont follow this pattern and thats just a bug fixing it. afterwards this argument will honor the virtualness of that codegeneration option.

as for the other functions mentioned

Aws::Client::XmlOutcome GenerateXmlOutcome
Aws::Client::StreamOutcome GenerateStreamOutcome

I'm sort of against making those virtual, as i would hope all mocking could be done at the sdk request request api layer but if there is a use case willing to hear it out.

will make a change that does two thigs:

• makes it easier to invoke the enable virtual client functions from the python script used for running code gen
• fixes code gen to correctly mark those as virutal/non-virtual when that argument is provided

[grrtrr]

Thank you for getting back and for the background, this all makes sense.

The Generate{Xml,Stream}Outcome likely need not be virtual, I just noted them in passing as I was going through the header file line-by line.
Thinking about it, they are called by the other functions, so likely not needed when creating mocks.

[sbiscigl]

should be merged on main now, tagged this afternoon. lemme know if you have any questions otherwise should be ready to use!

[grrtrr]

Awesome, thank you for the quick response.