Optimized search and json deserialization by ashpatil-msft · Pull Request #800 · microsoft/winget-cli

ashpatil-msft · 2021-03-17T07:45:00Z

This PR contains the following updates:

Optimized search method that will get triggered when user wants to install a package with specific params.
Deserializers for Rest API responses for manifest, search response and information APIs.
Basic call to /information endpoint to get supported version.

Todo:

Will be adding unit tests and E2E tests in the upcoming PRs for the above changes.
Implementing Search Request usage in rest source search method and version decision logic.

Microsoft Reviewers: Open in CodeFlow

…ySearch

JohnMcPMS · 2021-03-17T20:39:18Z

+        if (!info.has_value() || info.value().ServerSupportedVersions.size() == 0)
+        {
+            AICLI_LOG(Repo, Verbose, << "Incomplete information returned by rest source");
+            THROW_HR_MSG(E_UNEXPECTED, "Missing supported versions from rest source information");


Given that this is coming from an external party, we should have a real error code. Also, it makes the log line above unnecessary. #Resolved

JohnMcPMS · 2021-03-17T20:47:38Z


+        static utility::string_t GetInformationEndpoint(const std::string& restApiUri);
+
+        static std::optional<std::string> GetSupportedRestClientVersion(const std::string& restApi);


I'm not a fan of repeating the type name in the methods, can these three drop RestClient from the name? #Resolved

JohnMcPMS · 2021-03-17T20:50:04Z

-                AICLI_LOG(Repo, Verbose, << "Downloading manifest");
+                AICLI_LOG(Repo, Verbose, << "Getting manifest");
+
+                if (!m_versionInfo.Manifest.Id.empty())


Make Manifest a std::optional instead of guessing at it's validity based on a member. #Resolved

JohnMcPMS · 2021-03-17T20:56:55Z

    }

-    Interface::Interface(const std::string& restApi)
+    Interface::Interface(const std::string& restApi, const std::string& restApiVersion)


Shouldn't this version value be implicit in the version of the interface? Seems strange to define it as 1.0 in the filesystem but pass in the value that it uses. #Resolved

This was supposed to be the rest API Version, but now that we have removed it from our schema, I will remove this.

In reply to: 596376550 [](ancestors = 596376550)

JohnMcPMS · 2021-03-17T21:17:48Z

+        if (!data.has_value())
+        {
+            AICLI_LOG(Repo, Verbose, << "Missing data");
+            return {};


General error handling strategy for these deserialization methods:

Log as Error

Returning an empty optional is fine as long as the caller treats that as an error and throws an appropriate error understandable to our stack.

Wrap in a function level try/catch that also returns an empty optional.

I would suggest combining 2 and 3 by making the public method non-optional and throw and a private one that does the work. #Resolved

Basically, don't ignore error conditions by just skipping them and also don't propagate the REST specific exception types out of the source.

If a value is truly optional, then you wouldn't return early. #Resolved

Have added the errors and made public methods throw when it gets an optional value.

In reply to: 596389291 [](ancestors = 596389291)

JohnMcPMS · 2021-03-17T21:19:37Z

+        constexpr std::string_view SourceIdentifier = "SourceIdentifier"sv;
+        constexpr std::string_view ServerSupportedVersions = "ServerSupportedVersions"sv;
+        constexpr std::string_view APIVersion = "APIVersion"sv;
+        constexpr std::string_view ObjectVersions = "ObjectVersions"sv;


I don't understand what all of these versions are for. #Resolved

Updated based on offline conversation.

In reply to: 596390423 [](ancestors = 596390423)

JohnMcPMS · 2021-03-17T21:27:47Z

+
+                if (!JsonHelper::IsValidNonEmptyStringValue(packageId) || !JsonHelper::IsValidNonEmptyStringValue(packageName) || !JsonHelper::IsValidNonEmptyStringValue(packageName))
+                {
+                    AICLI_LOG(Repo, Verbose, << "Skipping manifest item because of missing required package fields.");


We should fail if required values are missing. #Resolved

JohnMcPMS · 2021-03-17T21:28:48Z

+        }
+        catch (...)
+        {
+            AICLI_LOG(Repo, Verbose, << "Error encountered while deserializing search result...");


TODO: Catch known types and log error information from them #Resolved

JohnMcPMS · 2021-03-17T21:30:33Z

+    namespace
+    {
+        // Manifest response constants specific to this deserializer
+        constexpr std::string_view PackageIdentifier = "PackageIdentifier"sv;


Proof that having the tree walking interface and single definition of the manifest parser would be much less code. This would literally not need to know anything about the manifest structure (assuming that we actually defined the same tree for the REST response... please tell me we did that).

Not heavily reviewed because I think we should do that work sooner rather than later.

github-actions · 2021-03-18T20:22:04Z

Misspellings found, please review:

params
RESTSOURCE
retured

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"validator valijson valueiterator "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"params RESTSOURCE retured "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

#Resolved

ashpatil-msft · 2021-03-18T20:23:47Z

+            case APPINSTALLER_CLI_ERROR_UNSUPPORTED_RESTSOURCE:
+                return "The configured rest source is not supported";
+            case APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA:
+                return "Invalid data returned by rest source";


Let me know if you want to change the wording/name of these errors #Resolved

JohnMcPMS

A few minor things.

JohnMcPMS · 2021-03-19T19:35:46Z


-    pplx::task<web::http::http_response> HttpClientHelper::Post(const web::json::value& body)
+    pplx::task<web::http::http_response> HttpClientHelper::Post(
+        const web::json::value& body, std::optional<std::vector<std::pair<utility::string_t, utility::string_t>>> headers)


While an empty optional might be more efficient to construct than an empty vector, {} works equally well for both. I don't think a small runtime efficiency is worth the programmer overhead of this being an optional. #Resolved

JohnMcPMS · 2021-03-19T19:41:33Z

        // Call information endpoint
        HttpClientHelper httpClientHelper{ GetInformationEndpoint(restApi) };
-        web::json::value response = httpClientHelper.HandleGet();
+        web::json::value response = httpClientHelper.HandleGet({});


Default to {} in the header for these methods so that the caller need to explicitly specify it. #Resolved

JohnMcPMS · 2021-03-19T19:45:08Z

                }
-
+
+                m_versionInfo.Manifest = manifest.value();


I don't know the rules around copy elision well enough to know if this will only produce one copy. But you could be sure of that by moving the value in the the version info and returning a copy of that. #Resolved

JohnMcPMS · 2021-03-19T19:47:53Z

        private:
            IRestClient::PackageInfo m_packageInfo;
-            IRestClient::VersionInfo m_versionInfo;
+            mutable IRestClient::VersionInfo m_versionInfo;


I think a specific const cast would be better here, or simply removal of the const from GetManifest as I was wrong to have it. #Resolved

JohnMcPMS · 2021-03-19T19:51:25Z

-            {
-                std::vector<AppInstaller::Manifest::ValidationError> validationErrors =
-                    AppInstaller::Manifest::ValidateManifest(manifest.value());
+        THROW_HR_IF(APPINSTALLER_CLI_ERROR_MULTIPLE_APPLICATIONS_FOUND, manifests.size() > 1);


That is not the appropriate error to return in this case, nor do I think this case is an error. I'm fine with a TODO for now though. #Resolved

ashpatil-msft · 2021-03-19T23:51:37Z

Addressed the comments..

In reply to: 616735978 [](ancestors = 616735978)

ashpatil-msft added 3 commits March 16, 2021 23:43

Added deserializers for Rest api responses & implemented GetManifestB…

267c67a

…ySearch

Adding comments and other small changes.

1108178

spell check

eb90c8f

ashpatil-msft requested a review from a team as a code owner March 17, 2021 07:45

JohnMcPMS requested changes Mar 17, 2021

View reviewed changes

ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Mar 17, 2021

ashpatil-msft added 3 commits March 18, 2021 12:43

PR Comments

2df7e6f

Merge branch 'master' into user/ashpatil/RestSourcePart2

6461c37

Caching manifest and using installer modes enum

280436a

ashpatil-msft commented Mar 18, 2021

View reviewed changes

Spell check

f6be407

ashpatil-msft requested a review from JohnMcPMS March 19, 2021 17:26

JohnMcPMS approved these changes Mar 19, 2021

View reviewed changes

PR comments

c9c0cbc

ashpatil-msft merged commit bafc20f into microsoft:master Mar 20, 2021


		static utility::string_t GetInformationEndpoint(const std::string& restApiUri);

		static std::optional<std::string> GetSupportedRestClientVersion(const std::string& restApi);

Uh oh!

Conversation

ashpatil-msft commented Mar 17, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashpatil-msft Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashpatil-msft Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashpatil-msft Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 18, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Misspellings found, please review:

Uh oh!

ashpatil-msft Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS left a comment

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 19, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 19, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnMcPMS Mar 19, 2021 • edited by ashpatil-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ashpatil-msft commented Mar 17, 2021 •

edited by ghost

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 17, 2021 •

edited by ashpatil-msft

Loading

github-actions Bot commented Mar 18, 2021 •

edited by ashpatil-msft

Loading

ashpatil-msft Mar 18, 2021 •

edited

Loading

JohnMcPMS Mar 19, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 19, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 19, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 19, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Mar 19, 2021 •

edited by ashpatil-msft

Loading