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

Fix certain msstore source 404 failures by treating them as empty responses #5179

Merged
merged 4 commits into from
Feb 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ IFACEMETHODIMP
iid
img
inet
innererror
inproc
Insta
installinprogress
Expand Down
24 changes: 23 additions & 1 deletion src/AppInstallerCLITests/RestInterface_1_0.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,17 @@ TEST_CASE("Search_GoodResponse_AllFields", "[RestSource][Interface_1_0]")
REQUIRE(package.Versions.at(0).ProductCodes.at(1) == "pc2");
}

TEST_CASE("Search_GoodResponse_404AsEmpty", "[RestSource][Interface_1_0]")
{
utility::string_t notFoundResponse = _XPLATSTR(
R"delimiter({"code":"DataNotFound","data":[],"details":[],"innererror":{"code":"DataNotFound","data":[],"details":[],"message":"Product is not present","source":"StoreEdgeFD"},"message":"Product is not present","source":"StoreEdgeFD"})delimiter");

HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::NotFound, std::move(notFoundResponse)) };
Interface v1{ TestRestUriString, std::move(helper) };
Schema::IRestClient::SearchResult searchResponse = v1.Search({});
REQUIRE(searchResponse.Matches.size() == 0);
}

TEST_CASE("Search_ContinuationToken", "[RestSource][Interface_1_0]")
{
utility::string_t sample = _XPLATSTR(
Expand Down Expand Up @@ -447,7 +458,7 @@ TEST_CASE("GetManifests_GoodResponse", "[RestSource][Interface_1_0]")
Interface v1{ TestRestUriString, std::move(helper) };
std::vector<Manifest> manifests = v1.GetManifests("Foo.Bar");
REQUIRE(manifests.size() == 1);

// Verify manifest is populated
Manifest manifest = manifests[0];
REQUIRE(manifest.Id == "Foo.Bar");
Expand All @@ -459,6 +470,17 @@ TEST_CASE("GetManifests_GoodResponse", "[RestSource][Interface_1_0]")
sampleManifest.VerifyInstallers_AllFields(manifest);
}

TEST_CASE("GetManifests_GoodResponse_404AsEmpty", "[RestSource][Interface_1_0]")
{
utility::string_t notFoundResponse = _XPLATSTR(
R"delimiter({"code":"DataNotFound","data":[],"details":[],"innererror":{"code":"DataNotFound","data":[],"details":[],"message":"Product is not present","source":"StoreEdgeFD"},"message":"Product is not present","source":"StoreEdgeFD"})delimiter");

HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::NotFound, std::move(notFoundResponse)) };
Interface v1{ TestRestUriString, std::move(helper) };
std::vector<Manifest> manifests = v1.GetManifests("Foo.Bar");
REQUIRE(manifests.size() == 0);
}

TEST_CASE("GetManifests_BadResponse_SuccessCode", "[RestSource][Interface_1_0]")
{
utility::string_t badManifest = _XPLATSTR(
Expand Down
24 changes: 22 additions & 2 deletions src/AppInstallerCommonCore/HttpClientHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,24 @@ namespace AppInstaller::Http
const utility::string_t& uri,
const web::json::value& body,
const HttpClientHelper::HttpRequestHeaders& headers,
const HttpClientHelper::HttpRequestHeaders& authHeaders) const
const HttpClientHelper::HttpRequestHeaders& authHeaders,
const HttpResponseHandler& customHandler) const
{
web::http::http_response httpResponse;
Post(uri, body, headers, authHeaders).then([&httpResponse](const web::http::http_response& response)
{
httpResponse = response;
}).wait();

if (customHandler)
{
auto handlerResult = customHandler(httpResponse);
if (!handlerResult.UseDefaultHandling)
{
return std::move(handlerResult.Result);
}
}

return ValidateAndExtractResponse(httpResponse);
}

Expand Down Expand Up @@ -137,14 +147,24 @@ namespace AppInstaller::Http
std::optional<web::json::value> HttpClientHelper::HandleGet(
const utility::string_t& uri,
const HttpClientHelper::HttpRequestHeaders& headers,
const HttpClientHelper::HttpRequestHeaders& authHeaders) const
const HttpClientHelper::HttpRequestHeaders& authHeaders,
const HttpResponseHandler& customHandler) const
{
web::http::http_response httpResponse;
Get(uri, headers, authHeaders).then([&httpResponse](const web::http::http_response& response)
{
httpResponse = response;
}).wait();

if (customHandler)
{
auto handlerResult = customHandler(httpResponse);
if (!handlerResult.UseDefaultHandling)
{
return std::move(handlerResult.Result);
}
}

return ValidateAndExtractResponse(httpResponse);
}

Expand Down
15 changes: 13 additions & 2 deletions src/AppInstallerCommonCore/Public/winget/HttpClientHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,26 @@ namespace AppInstaller::Http
{
using HttpRequestHeaders = std::unordered_map<utility::string_t, utility::string_t>;

struct HttpResponseHandlerResult
{
// The custom response handler result. Default is empty.
std::optional<web::json::value> Result = std::nullopt;

// Indicates whether to use default handling logic by HttpClientHelper instead (i.e. the custom response handler does not handle the specific response).
bool UseDefaultHandling = false;
};

using HttpResponseHandler = std::function<HttpResponseHandlerResult(const web::http::http_response&)>;

HttpClientHelper(std::shared_ptr<web::http::http_pipeline_stage> = {});

pplx::task<web::http::http_response> Post(const utility::string_t& uri, const web::json::value& body, const HttpRequestHeaders& headers = {}, const HttpRequestHeaders& authHeaders = {}) const;

std::optional<web::json::value> HandlePost(const utility::string_t& uri, const web::json::value& body, const HttpRequestHeaders& headers = {}, const HttpRequestHeaders& authHeaders = {}) const;
std::optional<web::json::value> HandlePost(const utility::string_t& uri, const web::json::value& body, const HttpRequestHeaders& headers = {}, const HttpRequestHeaders& authHeaders = {}, const HttpResponseHandler& customHandler = {}) const;

pplx::task<web::http::http_response> Get(const utility::string_t& uri, const HttpRequestHeaders& headers = {}, const HttpRequestHeaders& authHeaders = {}) const;

std::optional<web::json::value> HandleGet(const utility::string_t& uri, const HttpRequestHeaders& headers = {}, const HttpRequestHeaders& authHeaders = {}) const;
std::optional<web::json::value> HandleGet(const utility::string_t& uri, const HttpRequestHeaders& headers = {}, const HttpRequestHeaders& authHeaders = {}, const HttpResponseHandler& customHandler = {}) const;

void SetPinningConfiguration(const Certificates::PinningConfiguration& configuration);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,26 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0

return {};
}

AppInstaller::Http::HttpClientHelper::HttpResponseHandlerResult CustomRestCallResponseHandler(const web::http::http_response& response)
{
AppInstaller::Http::HttpClientHelper::HttpResponseHandlerResult result;
result.UseDefaultHandling = true;

if (response.status_code() == web::http::status_codes::NotFound &&
response.headers().content_type()._Starts_with(web::http::details::mime_types::application_json))
{
auto responseJson = response.extract_json().get();
if (responseJson.is_object() && responseJson.has_field(L"code") && responseJson.has_field(L"message"))
{
// We'll treat 404 with json response containing code and message fields as empty result.
// Leave the HttpResponseHandlerResult result empty and disable default HttpClientHelper handling.
result.UseDefaultHandling = false;
}
}

return result;
}
}

Interface::Interface(const std::string& restApi, const Http::HttpClientHelper& httpClientHelper) : m_restApiUri(restApi), m_httpClientHelper(httpClientHelper)
Expand Down Expand Up @@ -94,7 +114,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0
searchHeaders.insert_or_assign(AppInstaller::JSON::GetUtilityString(ContinuationToken), continuationToken);
}

std::optional<web::json::value> jsonObject = m_httpClientHelper.HandlePost(m_searchEndpoint, GetValidatedSearchBody(request), searchHeaders, GetAuthHeaders());
std::optional<web::json::value> jsonObject = m_httpClientHelper.HandlePost(m_searchEndpoint, GetValidatedSearchBody(request), searchHeaders, GetAuthHeaders(), CustomRestCallResponseHandler);

utility::string_t ct;
if (jsonObject)
Expand Down Expand Up @@ -221,7 +241,7 @@ namespace AppInstaller::Repository::Rest::Schema::V1_0
std::vector<Manifest::Manifest> results;
utility::string_t continuationToken;
Http::HttpClientHelper::HttpRequestHeaders searchHeaders = m_requiredRestApiHeaders;
std::optional<web::json::value> jsonObject = m_httpClientHelper.HandleGet(GetManifestByVersionEndpoint(m_restApiUri, packageId, validatedParams), searchHeaders, GetAuthHeaders());
std::optional<web::json::value> jsonObject = m_httpClientHelper.HandleGet(GetManifestByVersionEndpoint(m_restApiUri, packageId, validatedParams), searchHeaders, GetAuthHeaders(), CustomRestCallResponseHandler);

if (!jsonObject)
{
Expand Down
Loading