Added extra check for valid arguments in upgrade. (microsoft#1874)

sba923 · Feb 1, 2022 · d386744 · d386744
1 parent 2491b6b
commit d386744
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 21 deletions.
diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp
@@ -17,16 +17,65 @@ 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);  
+        }
+
+        // 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);
         }
     }
 
@@ -109,17 +158,23 @@ 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) 
+                && (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::BothManifestAndSearchQueryProvided, "");
+            throw CommandException(Resource::String::InvalidArgumentWithoutQueryError);
         }
     }
 
@@ -129,7 +184,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 +194,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
@@ -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
@@ -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>