[skip-changelog] Add query parameters to urls generated by lib commands (#2034)

MatteoPologruto · web-flow · commit c666277ea54f · 2023-01-17T16:06:20.000+01:00
* Add query parameters to urls generated by lib commands

* Add test to check query parameters

* Fix comments on functions to make them clearer
diff --git a/arduino/cores/packagemanager/download.go b/arduino/cores/packagemanager/download.go
@@ -127,7 +127,7 @@ func (pme *Explorer) DownloadToolRelease(tool *cores.ToolRelease, config *downlo
 			Message: tr("Error downloading tool %s", tool),
 			Cause:   errors.New(tr("no versions available for the current OS"))}
 	}
-	return resource.Download(pme.DownloadDir, config, tool.String(), progressCB)
+	return resource.Download(pme.DownloadDir, config, tool.String(), progressCB, "")
 }
 
 // DownloadPlatformRelease downloads a PlatformRelease. If the platform is already downloaded a
@@ -136,5 +136,5 @@ func (pme *Explorer) DownloadPlatformRelease(platform *cores.PlatformRelease, co
 	if platform.Resource == nil {
 		return &arduino.PlatformNotFoundError{Platform: platform.String()}
 	}
-	return platform.Resource.Download(pme.DownloadDir, config, platform.String(), progressCB)
+	return platform.Resource.Download(pme.DownloadDir, config, platform.String(), progressCB, "")
 }
diff --git a/arduino/cores/packagemanager/profiles.go b/arduino/cores/packagemanager/profiles.go
@@ -176,7 +176,7 @@ func (pmb *Builder) installMissingProfileTool(toolRelease *cores.ToolRelease, de
 		return &arduino.InvalidVersionError{Cause: fmt.Errorf(tr("version %s not available for this operating system", toolRelease))}
 	}
 	taskCB(&rpc.TaskProgress{Name: tr("Downloading tool %s", toolRelease)})
-	if err := toolResource.Download(pmb.DownloadDir, nil, toolRelease.String(), downloadCB); err != nil {
+	if err := toolResource.Download(pmb.DownloadDir, nil, toolRelease.String(), downloadCB, ""); err != nil {
 		taskCB(&rpc.TaskProgress{Name: tr("Error downloading tool %s", toolRelease)})
 		return &arduino.FailedInstallError{Message: tr("Error installing tool %s", toolRelease), Cause: err}
 	}
diff --git a/arduino/httpclient/httpclient.go b/arduino/httpclient/httpclient.go
@@ -25,14 +25,20 @@ import (
 	"github.com/arduino/arduino-cli/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/arduino/go-paths-helper"
+	"github.com/sirupsen/logrus"
 	"go.bug.st/downloader/v2"
 )
 
 var tr = i18n.Tr
 
 // DownloadFile downloads a file from a URL into the specified path. An optional config and options may be passed (or nil to use the defaults).
 // A DownloadProgressCB callback function must be passed to monitor download progress.
-func DownloadFile(path *paths.Path, URL string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) (returnedError error) {
+// If a not empty queryParameter is passed, it is appended to the URL for analysis purposes.
+func DownloadFile(path *paths.Path, URL string, queryParameter string, label string, downloadCB rpc.DownloadProgressCB, config *downloader.Config, options ...downloader.DownloadOptions) (returnedError error) {
+	if queryParameter != "" {
+		URL = URL + "?query=" + queryParameter
+	}
+	logrus.WithField("url", URL).Info("Starting download")
 	downloadCB.Start(URL, label)
 	defer func() {
 		if returnedError == nil {
diff --git a/arduino/resources/download.go b/arduino/resources/download.go
@@ -27,7 +27,8 @@ import (
 
 // Download performs a download loop using the provided downloader.Config.
 // Messages are passed back to the DownloadProgressCB using label as text for the File field.
-func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.Config, label string, downloadCB rpc.DownloadProgressCB) error {
+// queryParameter is passed for analysis purposes.
+func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.Config, label string, downloadCB rpc.DownloadProgressCB, queryParameter string) error {
 	path, err := r.ArchivePath(downloadDir)
 	if err != nil {
 		return fmt.Errorf(tr("getting archive path: %s"), err)
@@ -51,5 +52,5 @@ func (r *DownloadResource) Download(downloadDir *paths.Path, config *downloader.
 	} else {
 		return fmt.Errorf(tr("getting archive file info: %s"), err)
 	}
-	return httpclient.DownloadFile(path, r.URL, label, downloadCB, config)
+	return httpclient.DownloadFile(path, r.URL, queryParameter, label, downloadCB, config)
 }
diff --git a/arduino/resources/helpers_test.go b/arduino/resources/helpers_test.go
@@ -56,7 +56,7 @@ func TestDownloadApplyUserAgentHeaderUsingConfig(t *testing.T) {
 
 	httpClient := httpclient.NewWithConfig(&httpclient.Config{UserAgent: goldUserAgentValue})
 
-	err = r.Download(tmp, &downloader.Config{HttpClient: *httpClient}, "", func(progress *rpc.DownloadProgress) {})
+	err = r.Download(tmp, &downloader.Config{HttpClient: *httpClient}, "", func(progress *rpc.DownloadProgress) {}, "")
 	require.NoError(t, err)
 
 	// leverage the download helper to download the echo for the request made by the downloader itself
diff --git a/arduino/resources/index.go b/arduino/resources/index.go
@@ -55,7 +55,7 @@ func (res *IndexResource) Download(destDir *paths.Path, downloadCB rpc.DownloadP
 	// Download index file
 	indexFileName := path.Base(res.URL.Path) // == package_index.json[.gz]
 	tmpIndexPath := tmp.Join(indexFileName)
-	if err := httpclient.DownloadFile(tmpIndexPath, res.URL.String(), tr("Downloading index: %s", indexFileName), downloadCB, nil, downloader.NoResume); err != nil {
+	if err := httpclient.DownloadFile(tmpIndexPath, res.URL.String(), "", tr("Downloading index: %s", indexFileName), downloadCB, nil, downloader.NoResume); err != nil {
 		return &arduino.FailedDownloadError{Message: tr("Error downloading index '%s'", res.URL), Cause: err}
 	}
 
@@ -112,7 +112,7 @@ func (res *IndexResource) Download(destDir *paths.Path, downloadCB rpc.DownloadP
 		// Download signature
 		signaturePath = destDir.Join(signatureFileName)
 		tmpSignaturePath = tmp.Join(signatureFileName)
-		if err := httpclient.DownloadFile(tmpSignaturePath, res.SignatureURL.String(), tr("Downloading index signature: %s", signatureFileName), downloadCB, nil, downloader.NoResume); err != nil {
+		if err := httpclient.DownloadFile(tmpSignaturePath, res.SignatureURL.String(), "", tr("Downloading index signature: %s", signatureFileName), downloadCB, nil, downloader.NoResume); err != nil {
 			return &arduino.FailedDownloadError{Message: tr("Error downloading index signature '%s'", res.SignatureURL), Cause: err}
 		}
 
diff --git a/arduino/resources/resources_test.go b/arduino/resources/resources_test.go
@@ -48,7 +48,7 @@ func TestDownloadAndChecksums(t *testing.T) {
 	require.NoError(t, err)
 
 	downloadAndTestChecksum := func() {
-		err := r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {})
+		err := r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {}, "")
 		require.NoError(t, err)
 
 		data, err := testFile.ReadFile()
@@ -62,7 +62,7 @@ func TestDownloadAndChecksums(t *testing.T) {
 	downloadAndTestChecksum()
 
 	// Download with cached file
-	err = r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {})
+	err = r.Download(tmp, &downloader.Config{}, "", func(*rpc.DownloadProgress) {}, "")
 	require.NoError(t, err)
 
 	// Download if cached file has data in excess (redownload)
diff --git a/commands/daemon/daemon.go b/commands/daemon/daemon.go
@@ -343,6 +343,7 @@ func (s *ArduinoCoreServerImpl) LibraryDownload(req *rpc.LibraryDownloadRequest,
 	resp, err := lib.LibraryDownload(
 		stream.Context(), req,
 		func(p *rpc.DownloadProgress) { stream.Send(&rpc.LibraryDownloadResponse{Progress: p}) },
+		"",
 	)
 	if err != nil {
 		return convertErrorToRPCStatus(err)
@@ -356,6 +357,7 @@ func (s *ArduinoCoreServerImpl) LibraryInstall(req *rpc.LibraryInstallRequest, s
 		stream.Context(), req,
 		func(p *rpc.DownloadProgress) { stream.Send(&rpc.LibraryInstallResponse{Progress: p}) },
 		func(p *rpc.TaskProgress) { stream.Send(&rpc.LibraryInstallResponse{TaskProgress: p}) },
+		"",
 	)
 	if err != nil {
 		return convertErrorToRPCStatus(err)
diff --git a/commands/instances.go b/commands/instances.go
@@ -415,7 +415,7 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 					responseError(err.ToRPCStatus())
 					continue
 				}
-				if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback); err != nil {
+				if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback, ""); err != nil {
 					taskCallback(&rpc.TaskProgress{Name: tr("Error downloading library %s", libraryRef)})
 					e := &arduino.FailedLibraryInstallError{Cause: err}
 					responseError(e.ToRPCStatus())
diff --git a/commands/lib/download.go b/commands/lib/download.go
@@ -30,8 +30,10 @@ import (
 
 var tr = i18n.Tr
 
-// LibraryDownload FIXMEDOC
-func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downloadCB rpc.DownloadProgressCB) (*rpc.LibraryDownloadResponse, error) {
+// LibraryDownload executes the download of the library.
+// A DownloadProgressCB callback function must be passed to monitor download progress.
+// queryParameter is passed for analysis purposes.
+func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downloadCB rpc.DownloadProgressCB, queryParameter string) (*rpc.LibraryDownloadResponse, error) {
 	logrus.Info("Executing `arduino-cli lib download`")
 
 	lm := commands.GetLibraryManager(req)
@@ -46,22 +48,22 @@ func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downl
 		return nil, err
 	}
 
-	if err := downloadLibrary(lm, lib, downloadCB, func(*rpc.TaskProgress) {}); err != nil {
+	if err := downloadLibrary(lm, lib, downloadCB, func(*rpc.TaskProgress) {}, queryParameter); err != nil {
 		return nil, err
 	}
 
 	return &rpc.LibraryDownloadResponse{}, nil
 }
 
 func downloadLibrary(lm *librariesmanager.LibrariesManager, libRelease *librariesindex.Release,
-	downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error {
+	downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, queryParameter string) error {
 
 	taskCB(&rpc.TaskProgress{Name: tr("Downloading %s", libRelease)})
 	config, err := httpclient.GetDownloaderConfig()
 	if err != nil {
 		return &arduino.FailedDownloadError{Message: tr("Can't download library"), Cause: err}
 	}
-	if err := libRelease.Resource.Download(lm.DownloadsDir, config, libRelease.String(), downloadCB); err != nil {
+	if err := libRelease.Resource.Download(lm.DownloadsDir, config, libRelease.String(), downloadCB, queryParameter); err != nil {
 		return &arduino.FailedDownloadError{Message: tr("Can't download library"), Cause: err}
 	}
 	taskCB(&rpc.TaskProgress{Completed: true})
diff --git a/commands/lib/install.go b/commands/lib/install.go
@@ -30,8 +30,9 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// LibraryInstall FIXMEDOC
-func LibraryInstall(ctx context.Context, req *rpc.LibraryInstallRequest, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB) error {
+// LibraryInstall resolves the library dependencies, then downloads and installs the libraries into the install location.
+// queryParameter is passed for analysis purposes and forwarded to the called functions. It is set to "depends" when a dependency is installed
+func LibraryInstall(ctx context.Context, req *rpc.LibraryInstallRequest, downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, queryParameter string) error {
 	lm := commands.GetLibraryManager(req)
 	if lm == nil {
 		return &arduino.InvalidInstanceError{}
@@ -96,11 +97,21 @@ func LibraryInstall(ctx context.Context, req *rpc.LibraryInstallRequest, downloa
 	}
 
 	for libRelease, installTask := range libReleasesToInstall {
-		if err := downloadLibrary(lm, libRelease, downloadCB, taskCB); err != nil {
-			return err
-		}
-		if err := installLibrary(lm, libRelease, installTask, taskCB); err != nil {
-			return err
+		// Checks if libRelease is the requested library and not a dependency
+		if libRelease.GetName() == req.Name {
+			if err := downloadLibrary(lm, libRelease, downloadCB, taskCB, queryParameter); err != nil {
+				return err
+			}
+			if err := installLibrary(lm, libRelease, installTask, taskCB); err != nil {
+				return err
+			}
+		} else {
+			if err := downloadLibrary(lm, libRelease, downloadCB, taskCB, "depends"); err != nil {
+				return err
+			}
+			if err := installLibrary(lm, libRelease, installTask, taskCB); err != nil {
+				return err
+			}
 		}
 	}
 
diff --git a/commands/lib/upgrade.go b/commands/lib/upgrade.go
@@ -73,7 +73,7 @@ func upgrade(instance *rpc.Instance, libs []*installedLib, downloadCB rpc.Downlo
 			NoDeps:      false,
 			NoOverwrite: false,
 		}
-		err := LibraryInstall(context.Background(), libInstallReq, downloadCB, taskCB)
+		err := LibraryInstall(context.Background(), libInstallReq, downloadCB, taskCB, "upgrade")
 		if err != nil {
 			return err
 		}
diff --git a/internal/cli/lib/download.go b/internal/cli/lib/download.go
@@ -60,7 +60,7 @@ func runDownloadCommand(cmd *cobra.Command, args []string) {
 			Name:     library.Name,
 			Version:  library.Version,
 		}
-		_, err := lib.LibraryDownload(context.Background(), libraryDownloadRequest, feedback.ProgressBar())
+		_, err := lib.LibraryDownload(context.Background(), libraryDownloadRequest, feedback.ProgressBar(), "download")
 		if err != nil {
 			feedback.Fatal(tr("Error downloading %[1]s: %[2]v", library, err), feedback.ErrNetwork)
 		}
diff --git a/internal/cli/lib/install.go b/internal/cli/lib/install.go
@@ -130,7 +130,7 @@ func runInstallCommand(cmd *cobra.Command, args []string) {
 			NoDeps:      noDeps,
 			NoOverwrite: noOverwrite,
 		}
-		err := lib.LibraryInstall(context.Background(), libraryInstallRequest, feedback.ProgressBar(), feedback.TaskProgress())
+		err := lib.LibraryInstall(context.Background(), libraryInstallRequest, feedback.ProgressBar(), feedback.TaskProgress(), "install")
 		if err != nil {
 			feedback.Fatal(tr("Error installing %s: %v", libRef.Name, err), feedback.ErrGeneric)
 		}
diff --git a/internal/integrationtest/lib/lib_test.go b/internal/integrationtest/lib/lib_test.go
@@ -1480,3 +1480,43 @@ func TestInstallWithGitUrlLocalFileUri(t *testing.T) {
 	// Verifies library is installed
 	require.DirExists(t, libInstallDir.String())
 }
+
+func TestLibQueryParameters(t *testing.T) {
+	// This test should not use shared download directory because it needs to download the libraries from scratch
+	env := integrationtest.NewEnvironment(t)
+	cli := integrationtest.NewArduinoCliWithinEnvironment(env, &integrationtest.ArduinoCLIConfig{
+		ArduinoCLIPath: integrationtest.FindArduinoCLIPath(t),
+	})
+	defer env.CleanUp()
+
+	// Updates index for cores and libraries
+	_, _, err := cli.Run("core", "update-index")
+	require.NoError(t, err)
+	_, _, err = cli.Run("lib", "update-index")
+	require.NoError(t, err)
+
+	// Check query=install when a library is installed
+	stdout, _, err := cli.Run("lib", "install", "USBHost@1.0.0", "-v", "--log-level", "debug")
+	require.NoError(t, err)
+	require.Contains(t, string(stdout),
+		"Starting download                             \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/arduino-libraries/USBHost-1.0.0.zip?query=install\"\n")
+
+	// Check query=upgrade when a library is upgraded
+	stdout, _, err = cli.Run("lib", "upgrade", "USBHost", "-v", "--log-level", "debug")
+	require.NoError(t, err)
+	require.Contains(t, string(stdout),
+		"Starting download                             \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/arduino-libraries/USBHost-1.0.5.zip?query=upgrade\"\n")
+
+	// Check query=depends when a library dependency is installed
+	stdout, _, err = cli.Run("lib", "install", "MD_Parola@3.5.5", "-v", "--log-level", "debug")
+	require.NoError(t, err)
+	require.Contains(t, string(stdout),
+		"Starting download                             \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/MajicDesigns/MD_MAX72XX-3.3.1.zip?query=depends\"\n")
+
+	// Check query=download when a library is downloaded
+	stdout, _, err = cli.Run("lib", "download", "WiFi101@0.16.1", "-v", "--log-level", "debug")
+	require.NoError(t, err)
+	require.Contains(t, string(stdout),
+		"Starting download                             \x1b[36murl\x1b[0m=\"https://downloads.arduino.cc/libraries/github.com/arduino-libraries/WiFi101-0.16.1.zip?query=download\"\n")
+
+}

Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ func (pme Explorer) DownloadToolRelease(tool cores.ToolRelease, config *downlo`
`127`	`127`	`Message: tr("Error downloading tool %s", tool),`
`128`	`128`	`Cause: errors.New(tr("no versions available for the current OS"))}`
`129`	`129`	`}`
`130`		`- return resource.Download(pme.DownloadDir, config, tool.String(), progressCB)`
	`130`	`+ return resource.Download(pme.DownloadDir, config, tool.String(), progressCB, "")`
`131`	`131`	`}`
`132`	`132`
`133`	`133`	`// DownloadPlatformRelease downloads a PlatformRelease. If the platform is already downloaded a`
`@@ -136,5 +136,5 @@ func (pme Explorer) DownloadPlatformRelease(platform cores.PlatformRelease, co`
`136`	`136`	`if platform.Resource == nil {`
`137`	`137`	`return &arduino.PlatformNotFoundError{Platform: platform.String()}`
`138`	`138`	`}`
`139`		`- return platform.Resource.Download(pme.DownloadDir, config, platform.String(), progressCB)`
	`139`	`+ return platform.Resource.Download(pme.DownloadDir, config, platform.String(), progressCB, "")`
`140`	`140`	`}`
Original file line number	Diff line number	Diff line change
`@@ -176,7 +176,7 @@ func (pmb Builder) installMissingProfileTool(toolRelease cores.ToolRelease, de`
`176`	`176`	`return &arduino.InvalidVersionError{Cause: fmt.Errorf(tr("version %s not available for this operating system", toolRelease))}`
`177`	`177`	`}`
`178`	`178`	`taskCB(&rpc.TaskProgress{Name: tr("Downloading tool %s", toolRelease)})`
`179`		`- if err := toolResource.Download(pmb.DownloadDir, nil, toolRelease.String(), downloadCB); err != nil {`
	`179`	`+ if err := toolResource.Download(pmb.DownloadDir, nil, toolRelease.String(), downloadCB, ""); err != nil {`
`180`	`180`	`taskCB(&rpc.TaskProgress{Name: tr("Error downloading tool %s", toolRelease)})`
`181`	`181`	`return &arduino.FailedInstallError{Message: tr("Error installing tool %s", toolRelease), Cause: err}`
`182`	`182`	`}`
Original file line number	Diff line number	Diff line change
`@@ -415,7 +415,7 @@ func Init(req rpc.InitRequest, responseCallback func(r rpc.InitResponse)) erro`
`415`	`415`	`responseError(err.ToRPCStatus())`
`416`	`416`	`continue`
`417`	`417`	`}`
`418`		`- if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback); err != nil {`
	`418`	`+ if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback, ""); err != nil {`
`419`	`419`	`taskCallback(&rpc.TaskProgress{Name: tr("Error downloading library %s", libraryRef)})`
`420`	`420`	`e := &arduino.FailedLibraryInstallError{Cause: err}`
`421`	`421`	`responseError(e.ToRPCStatus())`