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.

Sign up for GitHub

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

Add support for markets #1806

Merged
merged 5 commits into from
Jan 24, 2022
Merged

Add support for markets #1806

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
441ba64
Add manifest filter for markets
Dec 15, 2021
14abc4a
Add tests
Dec 15, 2021
414f582
Update src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
lechacon Jan 21, 2022
4ec3672
Merge branch 'master' into HEAD
Jan 21, 2022
336f65c
PR comments
Jan 21, 2022
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
89 changes: 88 additions & 1 deletion src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ namespace AppInstaller::CLI::Workflow

std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "Installer scope does not matched currently installed scope: ";
std::string result = "Installer scope does not match currently installed scope: ";
result += Manifest::ScopeToString(installer.Scope);
result += " != ";
result += Manifest::ScopeToString(m_requirement);
Expand Down Expand Up @@ -508,12 +508,99 @@ namespace AppInstaller::CLI::Workflow
return result;
}
};

struct MarketFilter : public details::FilterField
{
MarketFilter(Manifest::string_t market) : details::FilterField("Market"), m_market(market)
{
AICLI_LOG(CLI, Verbose, << "Market Comparator created with market: " << m_market);
florelis marked this conversation as resolved.
Show resolved Hide resolved
}

static std::unique_ptr<MarketFilter> Create()
{
return std::make_unique<MarketFilter>(GetCurrentMarket());
}

InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
// If both allowed and excluded lists are provided, we only need to check the allowed markets.
if (!installer.Markets.AllowedMarkets.empty())
{
// Inapplicable if NOT found
if (installer.Markets.AllowedMarkets.end() == std::find(installer.Markets.AllowedMarkets.begin(), installer.Markets.AllowedMarkets.end(), m_market))
Copy link
Member

Choose a reason for hiding this comment

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

Are these market strings normalized to allow for this direct equality comparison? Most likely not, and we need to either normalize them or perform a normalizing comparison. Either way, I would suggest a helper type to make it easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the comparison case insensitive, but after re-reading your comment I'm not sure if that was what you meant. The manifest schema requires the markets to be two uppercase letters, so we shouldn't need any normalization there, although I'm not sure if the region from the OS could not be uppercase.

Copy link
Member

Choose a reason for hiding this comment

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

Unless this is a performance critical path and we find this to be an issue, I would leave it as case insensitive (also unless it turns out that case sensitivity is actual part of the definition of markets). We don't own the validation on both sides, so better to be safe.

{
return InapplicabilityFlags::Market;
}
}
else if (!installer.Markets.ExcludedMarkets.empty())
{
// Inapplicable if found
if (installer.Markets.ExcludedMarkets.end() != std::find(installer.Markets.ExcludedMarkets.begin(), installer.Markets.ExcludedMarkets.end(), m_market))
{
return InapplicabilityFlags::Market;
}
}

return InapplicabilityFlags::None;
}

std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "Current market '" + m_market + "' does not match installer markets." +
" Allowed markets: " + GetMarketsListAsString(installer.Markets.AllowedMarkets) +
" Excluded markets: " + GetMarketsListAsString(installer.Markets.ExcludedMarkets);
return result;
}

private:
Manifest::string_t m_market;

static Manifest::string_t GetCurrentMarket()
florelis marked this conversation as resolved.
Show resolved Hide resolved
{
// TODO: Manifest uses 2-letter codes (ISO 3166?) but GetUserDefaultGeoName can return
// either a 2-letter ISO-3166 code or a numeric UN M.49 code.
auto geoNameSize = GetUserDefaultGeoName(nullptr, 0);
florelis marked this conversation as resolved.
Show resolved Hide resolved
THROW_LAST_ERROR_IF(geoNameSize == 0);

std::vector<wchar_t> geoName(geoNameSize);
geoNameSize = GetUserDefaultGeoName(geoName.data(), geoNameSize);
THROW_LAST_ERROR_IF(geoNameSize == 0);

return Manifest::string_t{ geoName.data() };
}

std::string GetMarketsListAsString(const std::vector<Manifest::string_t>& markets)
{
// TODO: Refactor to merge with GetLocalesListAsString and GetAllowedArchitecturesString
florelis marked this conversation as resolved.
Show resolved Hide resolved
std::string result = "[";

bool first = true;
for (auto const& market : markets)
{
if (first)
{
first = false;
}
else
{
result += ", ";
}

result += market;
}

result += ']';

return result;
}
};
}

ManifestComparator::ManifestComparator(const Execution::Context& context, const Repository::IPackageVersion::Metadata& installationMetadata)
{
AddFilter(std::make_unique<OSVersionFilter>());
AddFilter(InstalledScopeFilter::Create(installationMetadata));
AddFilter(MarketFilter::Create());

// Filter order is not important, but comparison order determines priority.
// TODO: There are improvements to be made here around ordering, especially in the context of implicit vs explicit vs command line preferences.
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace AppInstaller::CLI::Workflow
Locale = 0x10,
Scope = 0x20,
MachineArchitecture = 0x40,
Market = 0x80,
};

DEFINE_ENUM_FLAG_OPERATORS(InapplicabilityFlags);
Expand Down
3 changes: 1 addition & 2 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,6 @@ namespace AppInstaller::CLI::Workflow
std::vector<Utility::Architecture> requiredArchitectures = Settings::User().Get<Settings::Setting::InstallArchitectureRequirement>();
std::vector<Utility::Architecture> optionalArchitectures = Settings::User().Get<Settings::Setting::InstallArchitecturePreference>();


if (!requiredArchitectures.empty())
{
context.Add<Execution::Data::AllowedArchitectures>({ requiredArchitectures.begin(), requiredArchitectures.end() });
Expand All @@ -998,9 +997,9 @@ namespace AppInstaller::CLI::Workflow
{
optionalArchitectures.emplace_back(Utility::Architecture::Unknown);
context.Add<Execution::Data::AllowedArchitectures>({ optionalArchitectures.begin(), optionalArchitectures.end() });

}
}

ManifestComparator manifestComparator(context, installationMetadata);
auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(context.Get<Execution::Data::Manifest>());

Expand Down
81 changes: 74 additions & 7 deletions src/AppInstallerCLITests/ManifestComparator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,39 @@ struct ManifestComparatorTestContext : public NullStream, Context
ManifestComparatorTestContext() : NullStream(), Context(*m_nullOut, *m_nullIn) {}
};

const ManifestInstaller& AddInstaller(Manifest& manifest, Architecture architecture, InstallerTypeEnum installerType, ScopeEnum scope = ScopeEnum::Unknown, std::string minOSVersion = {}, std::string locale = {})
const ManifestInstaller& AddInstaller(
Manifest& manifest,
Architecture architecture,
InstallerTypeEnum installerType,
ScopeEnum scope = ScopeEnum::Unknown,
std::string minOSVersion = {},
std::string locale = {},
MarketsInfo markets = {})
{
ManifestInstaller toAdd;
toAdd.Arch = architecture;
toAdd.InstallerType = installerType;
toAdd.Scope = scope;
toAdd.MinOSVersion = minOSVersion;
toAdd.Locale = locale;
toAdd.Markets = markets;

manifest.Installers.emplace_back(std::move(toAdd));

return manifest.Installers.back();
}

template<typename T>
void RequireVectorsEqual(const std::vector<T>& actual, const std::vector<T>& expected)
{
REQUIRE(actual.size() == expected.size());

for (std::size_t i = 0; i < actual.size(); ++i)
{
REQUIRE(actual[i] == expected[i]);
}
}

void RequireInstaller(const std::optional<ManifestInstaller>& actual, const ManifestInstaller& expected)
{
REQUIRE(actual);
Expand All @@ -46,16 +65,14 @@ void RequireInstaller(const std::optional<ManifestInstaller>& actual, const Mani
REQUIRE(actual->Scope == expected.Scope);
REQUIRE(actual->MinOSVersion == expected.MinOSVersion);
REQUIRE(actual->Locale == expected.Locale);

RequireVectorsEqual(actual->Markets.AllowedMarkets, expected.Markets.AllowedMarkets);
RequireVectorsEqual(actual->Markets.ExcludedMarkets, expected.Markets.ExcludedMarkets);
}

void RequireInapplicabilities(const std::vector<InapplicabilityFlags>& inapplicabilities, const std::vector<InapplicabilityFlags>& expected)
{
REQUIRE(inapplicabilities.size() == expected.size());

for (std::size_t i = 0; i < inapplicabilities.size(); i++)
{
REQUIRE(inapplicabilities[i] == expected[i]);
}
RequireVectorsEqual(inapplicabilities, expected);
}

TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]")
Expand Down Expand Up @@ -532,3 +549,53 @@ TEST_CASE("ManifestComparator_Inapplicabilities", "[manifest_comparator]")
inapplicabilities,
{ InapplicabilityFlags::OSVersion | InapplicabilityFlags::InstalledType | InapplicabilityFlags::Locale | InapplicabilityFlags::Scope | InapplicabilityFlags::MachineArchitecture });
}

TEST_CASE("ManifestComparator_MarketFilter", "[manifest_comparator]")
{
Manifest manifest;

// Get current market.
wchar_t geoName[4]; // All names have two or three chars
REQUIRE(GetUserDefaultGeoName(geoName, ARRAYSIZE(geoName)) != 0);
Manifest::string_t currentMarket{ geoName };

SECTION("Applicable")
{
MarketsInfo markets;
SECTION("Allowed")
{
markets.AllowedMarkets = { currentMarket };
}
SECTION("Not excluded")
{
markets.ExcludedMarkets = { "XX" };
}

ManifestInstaller installer = AddInstaller(manifest, Architecture::X86, InstallerTypeEnum::Exe, {}, {}, {}, markets);
ManifestComparator mc(ManifestComparatorTestContext{}, {});
auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest);

RequireInstaller(result, installer);
REQUIRE(inapplicabilities.empty());
}

SECTION("Inapplicable")
{
MarketsInfo markets;
SECTION("Excluded")
{
markets.ExcludedMarkets = { currentMarket };
}
SECTION("Not allowed")
{
markets.AllowedMarkets = { "XX" };
}

ManifestInstaller installer = AddInstaller(manifest, Architecture::X86, InstallerTypeEnum::Exe, {}, {}, {}, markets);
ManifestComparator mc(ManifestComparatorTestContext{}, {});
auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest);

REQUIRE(!result);
RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::Market});
}
}