Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support query compatible trait only for json protocol #3204

Merged
merged 16 commits into from
Nov 26, 2024
3 changes: 3 additions & 0 deletions src/aws-cpp-sdk-core/include/aws/core/client/AWSClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,8 @@ namespace Aws
std::shared_ptr<Aws::Http::HttpResponse> MakeHttpRequest(std::shared_ptr<Aws::Http::HttpRequest>& request) const;
Aws::String m_region;

void SetFeatureHeaders(Aws::Http::HeaderValueCollection&& headers);

/**
* Adds "X-Amzn-Trace-Id" header with the value of _X_AMZN_TRACE_ID if both
* environment variables AWS_LAMBDA_FUNCTION_NAME and _X_AMZN_TRACE_ID are set.
Expand Down Expand Up @@ -356,6 +358,7 @@ namespace Aws
Aws::String m_serviceName = "AWSBaseClient";
Aws::Client::RequestCompressionConfig m_requestCompressionConfig;
Aws::Vector<std::shared_ptr<smithy::interceptor::Interceptor>> m_interceptors;
Aws::Http::HeaderValueCollection m_featureHeaders;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess smithy client needs to be updated as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will extend this to smithy client

};

AWS_CORE_API Aws::String GetAuthorizationHeader(const Aws::Http::HttpRequest& httpRequest);
Expand Down
22 changes: 15 additions & 7 deletions src/aws-cpp-sdk-core/include/aws/core/client/AWSErrorMarshaller.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,24 @@ namespace Aws
{
using AWSErrorMarshaller::Marshall;
public:
/**
* Converts an exceptionName and message into an Error object, if it can be parsed. Otherwise, it returns
* and AWSError with CoreErrors::UNKNOWN as the error type.
*/
AWSError<CoreErrors> Marshall(const Aws::Http::HttpResponse& response) const override;

AWSError<CoreErrors> BuildAWSError(const std::shared_ptr<Http::HttpResponse>& httpResponse) const override;
JsonErrorMarshaller(bool queryCompatibilityMode);
JsonErrorMarshaller() = default;
/**
* Converts an exceptionName and message into an Error object, if it
* can be parsed. Otherwise, it returns and AWSError with
* CoreErrors::UNKNOWN as the error type.
*/
AWSError<CoreErrors> Marshall(
const Aws::Http::HttpResponse& response) const override;

AWSError<CoreErrors> BuildAWSError(
const std::shared_ptr<Http::HttpResponse>& httpResponse)
const override;

protected:
const Aws::Utils::Json::JsonValue& GetJsonPayloadFromError(const AWSError<CoreErrors>&) const;
bool isQueryCompatibleMode() const;
const bool m_queryCompatibilityMode{false};
};

class AWS_CORE_API XmlErrorMarshaller : public AWSErrorMarshaller
Expand Down
5 changes: 5 additions & 0 deletions src/aws-cpp-sdk-core/source/client/AWSClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,7 @@ void AWSClient::BuildHttpRequest(const Aws::AmazonWebServiceRequest& request, co
//do headers first since the request likely will set content-length as its own header.
AddHeadersToRequest(httpRequest, request.GetHeaders());
AddHeadersToRequest(httpRequest, request.GetAdditionalCustomHeaders());
AddHeadersToRequest(httpRequest, m_featureHeaders);

if (request.IsEventStreamRequest())
{
Expand Down Expand Up @@ -1049,3 +1050,7 @@ void AWSClient::AppendRecursionDetectionHeader(std::shared_ptr<Aws::Http::HttpRe

ioRequest->SetHeaderValue(Aws::Http::X_AMZN_TRACE_ID_HEADER, xAmznTraceIdVal);
}

void AWSClient::SetFeatureHeaders(Aws::Http::HeaderValueCollection&& headers) {
m_featureHeaders = std::move(headers);
}
52 changes: 43 additions & 9 deletions src/aws-cpp-sdk-core/source/client/AWSErrorMarshaller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ static CoreErrors GuessBodylessErrorType(const Aws::Http::HttpResponseCode respo
}
}

JsonErrorMarshaller::JsonErrorMarshaller(bool queryCompatibilityMode)
: AWSErrorMarshaller(), m_queryCompatibilityMode{queryCompatibilityMode} {}

bool JsonErrorMarshaller::isQueryCompatibleMode() const {
return m_queryCompatibilityMode;
}

AWSError<CoreErrors> JsonErrorMarshaller::Marshall(const Aws::Http::HttpResponse& httpResponse) const
{
Aws::StringStream memoryStream;
Expand Down Expand Up @@ -70,23 +77,50 @@ AWSError<CoreErrors> JsonErrorMarshaller::Marshall(const Aws::Http::HttpResponse
error.SetMessage(message);
}

if (httpResponse.HasHeader(QUERY_ERROR_HEADER))
{
if (isQueryCompatibleMode() && !error.GetExceptionName().empty()) {
/*
AWS Query-Compatible mode: This is a special setting that allows
certain AWS services to communicate using a specific "query"
format, which can send customized error codes. Users are divided
into different groups based on how they communicate with the
service: Group #1: Users using the AWS Query format, receiving
custom error codes. Group #2: Users using the regular AWS JSON
format without the trait, receiving standard error codes. Group #3:
Users using the AWS JSON format with the trait, receiving custom
error codes.

The header "x-amzn-query-error" shouldn't be present if it's not
awsQueryCompatible, so added checks for it.
*/

if (httpResponse.HasHeader(QUERY_ERROR_HEADER)) {
auto errorCodeString = httpResponse.GetHeader(QUERY_ERROR_HEADER);
auto locationOfSemicolon = errorCodeString.find_first_of(';');
Aws::String errorCode;

if (locationOfSemicolon != Aws::String::npos)
{
errorCode = errorCodeString.substr(0, locationOfSemicolon);
}
else
{
errorCode = errorCodeString;
if (locationOfSemicolon != Aws::String::npos) {
errorCode = errorCodeString.substr(0, locationOfSemicolon);
} else {
errorCode = errorCodeString;
}

error.SetExceptionName(errorCode);

//@todo: add support for Type from
// x-amzn-query-error:AWS.SimpleQueueService.NonExistentQueue;Sender
// , type is sender
}
// check for exception name from payload field 'type'
else if (payloadView.ValueExists(TYPE)) {
// handle missing header and parse code from message
const auto& typeStr = payloadView.GetString(TYPE);
auto locationOfPound = typeStr.find_first_of('#');
if (locationOfPound != Aws::String::npos) {
error.SetExceptionName(typeStr.substr(locationOfPound + 1));
}
}
}

}
else
{
Expand Down
12 changes: 8 additions & 4 deletions src/aws-cpp-sdk-core/source/internal/AWSHttpResourceClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -602,12 +602,16 @@ namespace Aws
SSOCredentialsClient::SSOCredentialsClient(const Aws::Client::ClientConfiguration& clientConfiguration, Aws::Http::Scheme scheme, const Aws::String& region)
: AWSHttpResourceClient(clientConfiguration, SSO_RESOURCE_CLIENT_LOG_TAG)
{
SetErrorMarshaller(Aws::MakeUnique<Aws::Client::JsonErrorMarshaller>(SSO_RESOURCE_CLIENT_LOG_TAG));
(Aws::MakeUnique<Aws::Client::JsonErrorMarshaller>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???
why this file is even changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure. I will revert this

SSO_RESOURCE_CLIENT_LOG_TAG));

m_endpoint = buildEndpoint(scheme, region, "portal.sso.", "federation/credentials");
sbera87 marked this conversation as resolved.
Show resolved Hide resolved
m_oidcEndpoint = buildEndpoint(scheme, region, "oidc.", "token");
m_endpoint = buildEndpoint(scheme, region, "portal.sso.",
"federation/credentials");
m_oidcEndpoint = buildEndpoint(scheme, region, "oidc.", "token");

AWS_LOGSTREAM_INFO(SSO_RESOURCE_CLIENT_LOG_TAG, "Creating SSO ResourceClient with endpoint: " << m_endpoint);
AWS_LOGSTREAM_INFO(
SSO_RESOURCE_CLIENT_LOG_TAG,
"Creating SSO ResourceClient with endpoint: " << m_endpoint);
}

Aws::String SSOCredentialsClient::buildEndpoint(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,10 @@ TEST_F(AWSErrorMarshallerTest, TestErrorsWithPrefixParse)
ASSERT_EQ(requestId, error.GetRequestId());
ASSERT_FALSE(error.ShouldRetry());

error = awsErrorMarshaller.Marshall(*BuildHttpResponse(exceptionPrefix + "AccessDeniedException", message, requestId, LowerCaseMessage, "AwsQueryErrorCode"));
JsonErrorMarshaller awsErrorMarshaller2(true);
error = awsErrorMarshaller2.Marshall(
*BuildHttpResponse(exceptionPrefix + "AccessDeniedException", message,
requestId, LowerCaseMessage, "AwsQueryErrorCode"));
ASSERT_EQ(CoreErrors::ACCESS_DENIED, error.GetErrorType());
ASSERT_EQ("AwsQueryErrorCode", error.GetExceptionName());
ASSERT_EQ(message, error.GetMessage());
Expand Down Expand Up @@ -741,7 +744,10 @@ TEST_F(AWSErrorMarshallerTest, TestErrorsWithoutPrefixParse)
ASSERT_EQ(requestId, error.GetRequestId());
ASSERT_FALSE(error.ShouldRetry());

error = awsErrorMarshaller.Marshall(*BuildHttpResponse(exceptionPrefix + "AccessDeniedException", message, requestId, LowerCaseMessage, "AwsQueryErrorCode"));
JsonErrorMarshaller awsErrorMarshaller2(true);
error = awsErrorMarshaller2.Marshall(
*BuildHttpResponse(exceptionPrefix + "AccessDeniedException", message,
requestId, LowerCaseMessage, "AwsQueryErrorCode"));
ASSERT_EQ(CoreErrors::ACCESS_DENIED, error.GetErrorType());
ASSERT_EQ("AwsQueryErrorCode", error.GetExceptionName());
ASSERT_EQ(message, error.GetMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,14 @@ public ServiceModel convert() {
serviceModel.getMetadata().setHasEndpointDiscoveryTrait(hasEndpointDiscoveryTrait && !endpointOperationName.isEmpty());
serviceModel.getMetadata().setRequireEndpointDiscovery(requireEndpointDiscovery);
serviceModel.getMetadata().setEndpointOperationName(endpointOperationName);
serviceModel.getMetadata().setAwsQueryCompatible(c2jServiceModel.getMetadata().getAwsQueryCompatible() != null);

// add protocol check. only for json, query protocols
if (serviceModel.getMetadata().getProtocol().equals("json")) {
serviceModel.getMetadata().setAwsQueryCompatible(
c2jServiceModel.getMetadata().getAwsQueryCompatible() != null);
} else {
serviceModel.getMetadata().setAwsQueryCompatible(false);
}

if (c2jServiceModel.getEndpointRules() != null) {
ObjectMapper objectMapper = new ObjectMapper();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,11 @@ ${className}& ${className}::operator=(${className} &&rhs) noexcept {
#end
BASECLASS(clientConfiguration,
Aws::MakeShared<${signerToMake}>(ALLOCATION_TAG, bearerTokenProvider),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
${ctorMemberInit}#if( $foreach.hasNext ),

Expand Down Expand Up @@ -205,7 +209,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
${signerWSpace} ${signerCtorArg}#if( $foreach.hasNext ),
#else#end
#end),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
${ctorMemberInit}#if( $foreach.hasNext ),
#else#end
Expand Down Expand Up @@ -244,7 +252,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
${signerWSpace} ${signerCtorArg}#if( $foreach.hasNext ),
#else#end
#end),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
${ctorMemberInit}#if( $foreach.hasNext ),
#else#end
Expand Down Expand Up @@ -283,7 +295,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
${signerWSpace} ${signerCtorArg}#if( $foreach.hasNext ),
#else#end
#end),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
${ctorMemberInit}#if( $foreach.hasNext ),
#else#end
Expand All @@ -309,7 +325,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
#end
BASECLASS(clientConfiguration,
signerProvider,
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
${virtualAddressingInit}
{
init(m_clientConfiguration);
Expand Down Expand Up @@ -400,7 +420,11 @@ void ${className}::init(const ${clientConfigurationCls}& config)
m_enableHostPrefixInjection = config.enableHostPrefixInjection;
#end##if($metadata.hasEndpointTrait)
#end##-#if($serviceModel.endpointRules)
#if ($metadata.awsQueryCompatible)
SetFeatureHeaders({{"x-amzn-query-mode","True"}});
#end
}

#if(!$serviceModel.endpointRules && $metadata.hasEndpointDiscoveryTrait)

void ${className}::Load${metadata.classNamePrefix}SpecificConfig(const Aws::Client::ClientConfiguration& clientConfiguration)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@
#end
BASECLASS(clientConfiguration,
Aws::MakeShared<${signerToMake}>(ALLOCATION_TAG, bearerTokenProvider),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
m_clientConfiguration(clientConfiguration${signPayloadsClientConfigParam}${virtualAddressingInit}${USEast1RegionalEndpointInitString})#if($ctorMemberInitList.isEmpty())
#else,#end
#foreach($ctorMemberInit in $ctorMemberInitList)
Expand Down Expand Up @@ -122,7 +126,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
${signerWSpace} ${signerCtorArg}#if( $foreach.hasNext ),
#else#end
#end),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
m_clientConfiguration(clientConfiguration${signPayloadsClientConfigParam}${virtualAddressingInit}${USEast1RegionalEndpointInitString})#if($ctorMemberInitList.isEmpty())#else,
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
Expand Down Expand Up @@ -160,7 +168,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
${signerWSpace} ${signerCtorArg}#if( $foreach.hasNext ),
#else#end
#end),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
m_clientConfiguration(clientConfiguration${signPayloadsClientConfigParam}${virtualAddressingInit}${USEast1RegionalEndpointInitString})#if($ctorMemberInitList.isEmpty())#else,
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
Expand Down Expand Up @@ -198,7 +210,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
${signerWSpace} ${signerCtorArg}#if( $foreach.hasNext ),
#else#end
#end),
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
m_clientConfiguration(clientConfiguration${signPayloadsClientConfigParam}${virtualAddressingInit}${USEast1RegionalEndpointInitString})#if($ctorMemberInitList.isEmpty())#else,
#end
#foreach($ctorMemberInit in $ctorMemberInitList)
Expand Down Expand Up @@ -226,7 +242,11 @@ ${clsWSpace} ${clsWSpace} ${ctorArgument}#if( $foreach.hasNext ),#else) :#end
#end
BASECLASS(clientConfiguration,
signerProvider,
#if ($metadata.awsQueryCompatible)
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG, true)),
#else
Aws::MakeShared<${metadata.classNamePrefix}ErrorMarshaller>(ALLOCATION_TAG)),
#end
m_clientConfiguration(clientConfiguration${signPayloadsClientConfigParam}${virtualAddressingInit}${USEast1RegionalEndpointInitString})
{
init(m_clientConfiguration);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ namespace ${serviceNamespace}
class ${CppViewHelper.computeExportValue($metadata.classNamePrefix)} $className : public Aws::Client::JsonErrorMarshaller
{
public:

#if ($metadata.awsQueryCompatible)
explicit $className(bool queryCompatibilityMode) : Aws::Client::JsonErrorMarshaller(queryCompatibilityMode){}
Copy link
Contributor

@sbiscigl sbiscigl Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we can make this code a little bit more maintainable using a decorator instead of a boolean. This also forces the query compatible decision to compile time and not runtime. i.e. we could do

class JsonErrorMarshallerQueryCompatible: public JsonErrorMarshaller {
  public:
    AWSError<CoreErrors> Marshall(const Aws::Http::HttpResponse& response) const override {
       //do special query compatible stuff
    }

AWSError<CoreErrors> BuildAWSError(const std::shared_ptr<Http::HttpResponse>& httpResponse) const override {
       //do special query compatible stuff
    }
};

then at code generation time we could do

  explicit $className : public Aws::Client::JsonErrorMarshallerQueryCompatible

and avoid passing around true and falses

#end
Aws::Client::AWSError<Aws::Client::CoreErrors> FindErrorByName(const char* exceptionName) const override;
};

Expand Down
Loading