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

Added extra check for valid arguments in upgrade. #1874

Merged
merged 3 commits into from
Feb 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
94 changes: 73 additions & 21 deletions src/AppInstallerCLICore/Commands/UpgradeCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,66 @@ namespace AppInstaller::CLI
{
namespace
{
bool ShouldListUpgrade(Context& context)
// Determines whether there are any arguments only used in search queries,
// as opposed to listing available upgrades
bool HasSearchQueryArguments(Execution::Args& execArgs)
{
for (Execution::Args::Type type : context.Args.GetTypes())
{
if (type != Execution::Args::Type::Source && type != Execution::Args::Type::IncludeUnknown)
{
return false;
}
}
return true;
// Note that this does not include Manifest (no search) or source related args (used for listing)
return execArgs.Contains(Args::Type::Query) ||
execArgs.Contains(Args::Type::Id) ||
execArgs.Contains(Args::Type::Name) ||
execArgs.Contains(Args::Type::Moniker) ||
execArgs.Contains(Args::Type::Version) ||
execArgs.Contains(Args::Type::Channel) ||
execArgs.Contains(Args::Type::Exact);
}

// Determines whether there are any arguments only used when upgrading a single package,
// as opposed to upgrading multiple packages or listing all available upgrades
bool HasArgumentsForSinglePackage(Execution::Args& execArgs)
{
return HasSearchQueryArguments(execArgs) ||
execArgs.Contains(Args::Type::Manifest);
}

// Determines whether there are any arguments only used when dealing with multiple packages,
// either for upgrading or for listing available upgrades.
bool HasArgumentsForMultiplePackages(Execution::Args& execArgs)
{
return execArgs.Contains(Args::Type::All) ||
execArgs.Contains(Args::Type::IncludeUnknown);
jedieaston marked this conversation as resolved.
Show resolved Hide resolved
}

// Determines whether there are any arguments only used as options during an upgrade,
// as opposed to listing available upgrades or selecting the packages.
bool HasArgumentsForInstallOptions(Execution::Args& execArgs)
{
return execArgs.Contains(Args::Type::Interactive) ||
execArgs.Contains(Args::Type::Silent) ||
execArgs.Contains(Args::Type::Log) ||
execArgs.Contains(Args::Type::Override) ||
execArgs.Contains(Args::Type::InstallLocation) ||
execArgs.Contains(Args::Type::HashOverride) ||
execArgs.Contains(Args::Type::AcceptPackageAgreements);
}

// Determines whether there are any arguments related to the source.
bool HasArgumentsForSource(Execution::Args& execArgs)
{
return execArgs.Contains(Args::Type::Source) ||
execArgs.Contains(Args::Type::CustomHeader) ||
execArgs.Contains(Args::Type::AcceptSourceAgreements);
}

// Determines whether we should list available upgrades, instead
// of performing an upgrade
bool ShouldListUpgrade(Execution::Args& execArgs)
{
// Valid arguments for list are only those related to the sources and which packages to include.
// Instead of checking for them, we check that there aren't any other arguments present.
return !execArgs.Contains(Args::Type::All) &&
!HasArgumentsForSinglePackage(execArgs) &&
!HasArgumentsForInstallOptions(execArgs);
}
}

Expand Down Expand Up @@ -109,17 +159,19 @@ namespace AppInstaller::CLI
void UpgradeCommand::ValidateArgumentsInternal(Execution::Args& execArgs) const
{
if (execArgs.Contains(Execution::Args::Type::Manifest) &&
(execArgs.Contains(Execution::Args::Type::Query) ||
execArgs.Contains(Execution::Args::Type::Id) ||
execArgs.Contains(Execution::Args::Type::Name) ||
execArgs.Contains(Execution::Args::Type::Moniker) ||
execArgs.Contains(Execution::Args::Type::Version) ||
execArgs.Contains(Execution::Args::Type::Channel) ||
execArgs.Contains(Execution::Args::Type::Source) ||
execArgs.Contains(Execution::Args::Type::Exact) ||
execArgs.Contains(Execution::Args::Type::All)))
(HasSearchQueryArguments(execArgs) ||
HasArgumentsForMultiplePackages(execArgs) ||
HasArgumentsForSource(execArgs)))
{
throw CommandException(Resource::String::BothManifestAndSearchQueryProvided);
}


else if (!ShouldListUpgrade(execArgs)
&& !HasSearchQueryArguments(execArgs)
&& HasArgumentsForInstallOptions(execArgs))
{
throw CommandException(Resource::String::BothManifestAndSearchQueryProvided, "");
throw CommandException(Resource::String::InvalidArgumentWithoutQueryError);
jedieaston marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -129,7 +181,7 @@ namespace AppInstaller::CLI

// Only allow for source failures when doing a list of available upgrades.
// We have to set it now to allow for source open failures to also just warn.
if (ShouldListUpgrade(context))
if (ShouldListUpgrade(context.Args))
{
context.SetFlags(Execution::ContextFlag::TreatSourceFailuresAsWarning);
}
Expand All @@ -139,7 +191,7 @@ namespace AppInstaller::CLI
Workflow::OpenSource() <<
Workflow::OpenCompositeSource(Repository::PredefinedSource::Installed);

if (ShouldListUpgrade(context))
if (ShouldListUpgrade(context.Args))
{
// Upgrade with no args list packages with updates available
context <<
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(InvalidArgumentSpecifierError);
WINGET_DEFINE_RESOURCE_STRINGID(InvalidArgumentValueError);
WINGET_DEFINE_RESOURCE_STRINGID(InvalidArgumentValueErrorWithoutValidValues);
WINGET_DEFINE_RESOURCE_STRINGID(InvalidArgumentWithoutQueryError);
WINGET_DEFINE_RESOURCE_STRINGID(InvalidJsonFile);
WINGET_DEFINE_RESOURCE_STRINGID(InvalidNameError);
WINGET_DEFINE_RESOURCE_STRINGID(LicenseAgreement);
Expand Down
3 changes: 3 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
Original file line number Diff line number Diff line change
Expand Up @@ -1269,4 +1269,7 @@ Please specify one of them using the `--source` option to proceed.</value>
<value>package has a version number that cannot be determined. Use "--include-unknown" to see all results.</value>
<comment>{Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions.</comment>
</data>
<data name="InvalidArgumentWithoutQueryError" xml:space="preserve">
<value>The arguments provided can only be used with a query.</value>
</data>
</root>