From 983f8a29afbd81759a21a48ba28ad302dbed00da Mon Sep 17 00:00:00 2001 From: Easton Pillay Date: Sun, 23 Jan 2022 15:08:12 -0500 Subject: [PATCH 1/3] Added extra check for valid arguments in `upgrade`. --- .../Commands/UpgradeCommand.cpp | 18 ++++++++++++++---- src/AppInstallerCLICore/Resources.h | 1 + .../Shared/Strings/en-us/winget.resw | 3 +++ 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index 5ee1772210..c367c6eef2 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -17,9 +17,9 @@ namespace AppInstaller::CLI { namespace { - bool ShouldListUpgrade(Context& context) + bool ShouldListUpgrade(Execution::Args& execArgs) { - for (Execution::Args::Type type : context.Args.GetTypes()) + for (Execution::Args::Type type : execArgs.GetTypes()) { if (type != Execution::Args::Type::Source && type != Execution::Args::Type::IncludeUnknown) { @@ -121,6 +121,16 @@ namespace AppInstaller::CLI { throw CommandException(Resource::String::BothManifestAndSearchQueryProvided, ""); } + + else if (!ShouldListUpgrade(execArgs) && + (!execArgs.Contains(Execution::Args::Type::Query) && + !execArgs.Contains(Execution::Args::Type::All) && + !execArgs.Contains(Execution::Args::Type::Name) && + !execArgs.Contains(Execution::Args::Type::Id) && + !execArgs.Contains(Execution::Args::Type::Moniker))) + { + throw CommandException(Resource::String::InvalidArgumentWithoutQueryError); + } } void UpgradeCommand::ExecuteInternal(Execution::Context& context) const @@ -129,7 +139,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); } @@ -139,7 +149,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 << diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 5f5af6ce33..9fa6921885 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -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); diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 3b1a323790..cced9e4e1d 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1269,4 +1269,7 @@ Please specify one of them using the `--source` option to proceed. package has a version number that cannot be determined. Use "--include-unknown" to see all results. {Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions. + + The arguments provided can only be used with a query. + \ No newline at end of file From 77df490b8886a127426980ea7b33e285d41c33cf Mon Sep 17 00:00:00 2001 From: Easton Pillay Date: Tue, 25 Jan 2022 08:59:30 -0500 Subject: [PATCH 2/3] Merged updated validation logic from #1795. --- .../Commands/UpgradeCommand.cpp | 90 ++++++++++++++----- 1 file changed, 66 insertions(+), 24 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index c367c6eef2..89fdb36d20 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -17,16 +17,66 @@ namespace AppInstaller::CLI { namespace { + // Determines whether there are any arguments only used in search queries, + // as opposed to listing available upgrades + bool HasSearchQueryArguments(Execution::Args& execArgs) + { + // 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); + } + + // 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) { - for (Execution::Args::Type type : execArgs.GetTypes()) - { - if (type != Execution::Args::Type::Source && type != Execution::Args::Type::IncludeUnknown) - { - return false; - } - } - return true; + // 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); } } @@ -109,25 +159,17 @@ 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, ""); + throw CommandException(Resource::String::BothManifestAndSearchQueryProvided); } - else if (!ShouldListUpgrade(execArgs) && - (!execArgs.Contains(Execution::Args::Type::Query) && - !execArgs.Contains(Execution::Args::Type::All) && - !execArgs.Contains(Execution::Args::Type::Name) && - !execArgs.Contains(Execution::Args::Type::Id) && - !execArgs.Contains(Execution::Args::Type::Moniker))) + + else if (!ShouldListUpgrade(execArgs) + && !HasSearchQueryArguments(execArgs) + && HasArgumentsForInstallOptions(execArgs)) { throw CommandException(Resource::String::InvalidArgumentWithoutQueryError); } From 53b777f81326016266aa3fc327ed6a6525324b82 Mon Sep 17 00:00:00 2001 From: Easton Pillay Date: Mon, 31 Jan 2022 09:11:51 -0500 Subject: [PATCH 3/3] Applied suggestions from @yao-msft's code review. --- src/AppInstallerCLICore/Commands/UpgradeCommand.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index 89fdb36d20..344b33f103 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -43,8 +43,7 @@ namespace AppInstaller::CLI // either for upgrading or for listing available upgrades. bool HasArgumentsForMultiplePackages(Execution::Args& execArgs) { - return execArgs.Contains(Args::Type::All) || - execArgs.Contains(Args::Type::IncludeUnknown); + return execArgs.Contains(Args::Type::All); } // Determines whether there are any arguments only used as options during an upgrade, @@ -169,7 +168,11 @@ namespace AppInstaller::CLI else if (!ShouldListUpgrade(execArgs) && !HasSearchQueryArguments(execArgs) - && HasArgumentsForInstallOptions(execArgs)) + && (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))) { throw CommandException(Resource::String::InvalidArgumentWithoutQueryError); }