Rest source addition by ashpatil-msft · Pull Request #765 · microsoft/winget-cli

ashpatil-msft · 2021-02-22T05:05:38Z

This PR contains the following changes:

Ability to add a Rest based source by turning on the Rest source experimental feature in settings
Ability to search for a package from rest source. Currently the search implementation doesn't include filters and inclusions and those will be added in subsequent PRs. Searching for a package will fetch all the results in the Rest source currently.

Todos:

Complete implementing other methods that have Todo's in the PR.
Use Search request's filters and inclusions to the Rest API search call.
Add Json parser to support show command and others.
Add Tests

Tests:

Manually tested adding Rest based source and searching for packages returns a set of entries from the Rest source.

Microsoft Reviewers: Open in CodeFlow

github-actions · 2021-02-22T05:06:57Z

Misspellings found, please review:

cpprest
cpprestsdk
IRest
pplx

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('"cpprest cpprestsdk IRest pplx "');
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

jsoref · 2021-02-22T05:16:19Z

+  <package id="cpprestsdk.v142" version="2.10.15" targetFramework="native" />
  <package id="Microsoft.Windows.CppWinRT" version="2.0.200729.8" targetFramework="native" />
  <package id="Microsoft.Windows.ImplementationLibrary" version="1.0.200519.2" targetFramework="native" />
 </packages>


The same thing applies to a number (but not all) of the files here.

Tangentially related, the spelling bot is similarly upset about the patterns file below (I can't comment on that file because it isn't part of the PR), although for some reason (I need to investigate) it can't seem to properly count to the last line of the file... #Resolved

github-actions · 2021-02-22T18:59:46Z

Misspellings found, please review:

cpprest
cpprestsdk
IRest
pplx

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('"cpprest cpprestsdk IRest pplx "');
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

JohnMcPMS · 2021-02-26T00:41:10Z

+            return versionApi;
+        }
+
+        std::string GetStringFromJsonStringValue(const json::value& value)


Return a std::optional<std::string> to preserve ability to model a missing value. #Closed

JohnMcPMS · 2021-02-26T00:42:03Z

+        std::shared_ptr<const RestSource> sharedThis = shared_from_this();
+        for (auto& result : results.Matches)
+        {
+            std::unique_ptr<IPackage> package = std::make_unique<AvailablePackage>(sharedThis, result);


Should std::move the result into the package since it will no longer be needed. #Closed

JohnMcPMS · 2021-02-26T00:46:24Z

+            {
+                std::string version = GetStringFromJsonStringValue(versionItem.at(Version));
+                std::string channel = GetStringFromJsonStringValue(versionItem.at(Channel));
+                versionList.emplace_back(VersionAndChannel(version, channel));


std::move #Closed

JohnMcPMS · 2021-02-26T00:46:34Z

+                versionList.emplace_back(VersionAndChannel(version, channel));
+            }
+
+            PackageInfo packageInfo = PackageInfo(packageId, packageName, publisher);


std::move #Closed

JohnMcPMS · 2021-02-26T00:49:08Z

+
+        if (jsonObject.is_null())
+        {
+            return std::string();


In this case you would want to return {}; to return an empty optional, and probably want to preserve that when you change the function to return std::optional<Manifest>. #Closed

github-actions · 2021-03-03T04:27:47Z

Misspellings found, please review:

cpprest
cpprestsdk
Famil
IRest
pplx

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('"cpprest cpprestsdk Famil IRest pplx "');
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

jsoref · 2021-03-03T05:10:59Z

+    constexpr std::string_view PackageIdentifier = "PackageIdentifier"sv;
+    constexpr std::string_view PackageName = "PackageName"sv;
+    constexpr std::string_view Publisher = "Publisher"sv;
+    constexpr std::string_view PackageFamilName = "PackageFamilyName"sv;


Suggested change

constexpr std::string_view PackageFamilName = "PackageFamilyName"sv;

constexpr std::string_view PackageFamilyName = "PackageFamilyName"sv;

``` #Resolved

jsoref · 2021-03-03T05:11:23Z

+            std::optional<std::string> packageId = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageIdentifier)));
+            std::optional<std::string> packageName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageName)));
+            std::optional<std::string> publisher = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(Publisher)));
+            std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilName)));


Suggested change

std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilName)));

std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilyName)));

``` #Resolved

…ttings

yao-msft · 2021-03-15T21:54:25Z

+    // Interface to this schema version exposed through IRestClient.
+    struct Interface : public IRestClient
+    {
+        Interface(std::string restApi);


std::string [](start = 18, length = 11)

const & #Resolved

yao-msft · 2021-03-15T21:59:58Z

+            return utility::conversions::to_string_t(versionEndpoint);
+        }
+
+        bool IsStringWhitespace(const std::string& value)


bool IsStringWhitespace(const std::string& value) [](start = 8, length = 49)

I think we have this utility in AppInstallerStrings.hpp. If not, it's a good idea to move the utility there #Resolved

My bad, I forgot to delete this. I am already using Utiltity::IsStringEmptyOrWhitespace

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

Ah, I saw you already used Utility::IsEmptyOrWhitespace(result) below. Then we can delete this

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

yao-msft · 2021-03-15T22:10:16Z

+        }
+    }
+
+    Interface::Interface(std::string restApi)


std::string [](start = 25, length = 11)

const & no need for move below

Actually given how GetRestAPIBaseUri is written, taking in a value and moving it to that function was correct. I think it is likely not to matter who makes the copy, so any of the patterns could work in practice. But the higher you can move the forced copy up the stack, the more efficient you can be usually.

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

yao-msft · 2021-03-15T22:10:38Z

+        SearchResult result;
+
+        // TODO: Handle continuation token.
+        HttpClientHelper clientHelper(m_searchEndpoint);


( [](start = 37, length = 1)

{} #Resolved

yao-msft · 2021-03-15T22:13:15Z

+    Interface::Interface(std::string restApi)
+    {
+         m_restApiUri = GetRestAPIBaseUri(std::move(restApi));
+         m_searchEndpoint = GetSearchEndpoint(m_restApiUri);


m_searchEndpoint [](start = 9, length = 16)

Do we want to save a copy for GetManifestByVersionEndpoint too? #WontFix

yao-msft · 2021-03-15T22:14:56Z

+
+    IRestClient::SearchResult Interface::Search(const SearchRequest& request) const
+    {
+        UNREFERENCED_PARAMETER(request);


UNREFERENCED_PARAMETER(request); [](start = 7, length = 33)

not needed, used below #Resolved

yao-msft · 2021-03-15T22:20:37Z

+
+        for (auto& manifestItem : dataArray)
+        {
+            std::optional<std::string> packageId = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageIdentifier)));


packageId [](start = 39, length = 9)

Make sure to revist these fields as some of them are definitely required.
You can add a todo for now since there'll be more error checking, manifest integrity checkto be performed later and they could potentially be in another file. #Resolved

I saw you handled bad cases by skipping them, I think that's probably enough.

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

Yes, I will be adding a json parser that will handle the checks in my next PR.

In reply to: 594726878 [](ancestors = 594726878,594722790)

yao-msft · 2021-03-15T22:27:57Z

+
+        // Parse json and return Manifest
+        auto& manifestObject = jsonObject.at(GetJsonKeyNameString(Data));
+        UNREFERENCED_PARAMETER(manifestObject);


nit: you can just (void)jsonObject.at(GetJsonKeyNameString(Data)); So you don't need the UNREFERENCED_PARAMETER below #Resolved

yao-msft · 2021-03-15T22:41:18Z

+            {
+                if (details.Type.empty())
+                {
+                    // With more than one source implementation, we will probably need to probe first


// With more than one source implementation, we will probably need to probe first [](start = 20, length = 81)

nit: is this comment copy pasted? Does not seem very relevant to what the code is doing. #Resolved

yao-msft · 2021-03-15T22:44:28Z

+            {
+                AICLI_LOG(Repo, Verbose, << "Downloading manifest");
+                Manifest::Manifest manifest = GetReferenceSource()->GetRestClient().GetManifestByVersion(
+                    m_packageInfo.packageIdentifier, m_versionInfo.GetVersion().ToString(), m_versionInfo.GetChannel().ToString()).value();


.value() [](start = 130, length = 8)

std::optional check it's set before calling Value() #Resolved

you are right, I will just receive it as std::optional. I will implementing and using this in my next PR.

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

yao-msft · 2021-03-15T22:49:42Z

+                }
+                else if (versionKey.Version.empty() && versionKey.Channel.empty())
+                {
+                    return GetLatestAvailableVersion();


return GetLatestAvailableVersion(); [](start = 20, length = 35)

nit packageVersion = GetLatestAvailableVersion(); for code consistency #Resolved

yao-msft

JohnMcPMS · 2021-03-15T23:49:19Z

+        SearchResult result;
+
+        // TODO: Handle continuation token.
+        HttpClientHelper clientHelper{ m_searchEndpoint };


HttpClientHelper [](start = 8, length = 16)

Are these really so cheap that we should make one for every search?

JohnMcPMS

ashpatil-msft added 18 commits February 17, 2021 15:24

Adding rest changes

50981dd

Fix spelling errors

89d0d74

Basic search functionality

1c17ecf

Adding cpprest

db1cec4

Adding cpprest nuget package

7e6179d

Adding necessary interfaces

d0edffb

Adding http helper

ba6dedb

adding constants

d248255

changes

f398249

Added experimental feature

9198ff2

minor changes

e3b03d2

Rest changes

e45149c

remove unused methods

712ad8e

Changes 2

edaacf0

Moving rest files to dir

48fc907

Changes

7004070

changes 2

7b49b8d

remove tabs

18ab865

ashpatil-msft requested a review from a team as a code owner February 22, 2021 05:05

jsoref reviewed Feb 22, 2021

View reviewed changes

new line

a83aa71

JohnMcPMS requested changes Feb 26, 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 Feb 26, 2021

CR Comments

98276ab

jsoref reviewed Mar 3, 2021

View reviewed changes

Add experimental rest source flag check while getting sources from se…

9948bc3

…ttings

yao-msft reviewed Mar 15, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/RestSource.cpp Outdated

PR comments

aa73642

yao-msft approved these changes Mar 15, 2021

View reviewed changes

JohnMcPMS reviewed Mar 15, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/Schema/1_0/Interface.cpp Outdated

JohnMcPMS reviewed Mar 15, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/Schema/1_0/Interface.cpp Outdated

JohnMcPMS reviewed Mar 15, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/Schema/1_0/Interface.cpp Outdated

JohnMcPMS reviewed Mar 15, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/HttpClientHelper.cpp Outdated

PR Comments

1efd16e

JohnMcPMS reviewed Mar 16, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/HttpClientHelper.cpp Outdated

JohnMcPMS reviewed Mar 16, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/HttpClientHelper.cpp Outdated

JohnMcPMS reviewed Mar 16, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/Schema/1_0/Interface.cpp Outdated

Comments

9e56c1a

JohnMcPMS approved these changes Mar 16, 2021

View reviewed changes

yao-msft reviewed Mar 16, 2021

View reviewed changes

Comment thread src/AppInstallerRepositoryCore/Rest/HttpClientHelper.cpp Outdated

ashpatil-msft added 2 commits March 15, 2021 20:10

comments

bc1d8c4

todo comment

e01dfb1

AngelaOkpa approved these changes Mar 16, 2021

View reviewed changes

ashpatil-msft merged commit b5a1b64 into microsoft:master Mar 16, 2021

	constexpr std::string_view PackageFamilName = "PackageFamilyName"sv;
	constexpr std::string_view PackageFamilyName = "PackageFamilyName"sv;
	``` #Resolved

	std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilName)));
	std::optional<std::string> packageFamilyName = GetStringFromJsonStringValue(manifestItem.at(GetJsonKeyNameString(PackageFamilyName)));
	``` #Resolved

Uh oh!

Conversation

ashpatil-msft commented Feb 22, 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!

github-actions Bot commented Feb 22, 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!

Uh oh!

jsoref Feb 22, 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!

github-actions Bot commented Feb 22, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JohnMcPMS Feb 26, 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 Feb 26, 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 Feb 26, 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 Feb 26, 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 Feb 26, 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!

github-actions Bot commented Mar 3, 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!

jsoref Mar 3, 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!

jsoref Mar 3, 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!

yao-msft Mar 15, 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!

yao-msft Mar 15, 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 15, 2021

Choose a reason for hiding this comment

Uh oh!

yao-msft Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

yao-msft Mar 15, 2021 • edited by JohnMcPMS 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 15, 2021

Choose a reason for hiding this comment

Uh oh!

yao-msft Mar 15, 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 Feb 22, 2021 •

edited by ghost

Loading

github-actions Bot commented Feb 22, 2021 •

edited by ashpatil-msft

Loading

jsoref Feb 22, 2021 •

edited by ashpatil-msft

Loading

github-actions Bot commented Feb 22, 2021 •

edited by ashpatil-msft

Loading

JohnMcPMS Feb 26, 2021 •

edited

Loading

JohnMcPMS Feb 26, 2021 •

edited

Loading

JohnMcPMS Feb 26, 2021 •

edited

Loading

JohnMcPMS Feb 26, 2021 •

edited

Loading

JohnMcPMS Feb 26, 2021 •

edited

Loading

github-actions Bot commented Mar 3, 2021 •

edited by ashpatil-msft

Loading

jsoref Mar 3, 2021 •

edited by ashpatil-msft

Loading

jsoref Mar 3, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by JohnMcPMS

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading

yao-msft Mar 15, 2021 •

edited by ashpatil-msft

Loading