From 600cc5f810f3510b8e4f66ee2178bd679f1e9e85 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Thu, 17 Nov 2022 10:51:31 +0100 Subject: [PATCH 1/5] Slightly refactored apiByVidPid --- commands/board/list.go | 59 +++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/commands/board/list.go b/commands/board/list.go index 13f1dbd05ad..69252ed7eb0 100644 --- a/commands/board/list.go +++ b/commands/board/list.go @@ -19,7 +19,7 @@ import ( "context" "encoding/json" "fmt" - "io/ioutil" + "io" "net/http" "regexp" "sort" @@ -60,7 +60,6 @@ func apiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { } url := fmt.Sprintf("%s/%s/%s", vidPidURL, vid, pid) - retVal := []*rpc.BoardListItem{} req, _ := http.NewRequest("GET", url, nil) req.Header.Set("Content-Type", "application/json") @@ -72,39 +71,41 @@ func apiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { return nil, errors.Wrap(err, tr("failed to initialize http client")) } - if res, err := httpClient.Do(req); err == nil { - if res.StatusCode >= 400 { - if res.StatusCode == 404 { - return nil, ErrNotFound - } - return nil, errors.Errorf(tr("the server responded with status %s"), res.Status) - } - - body, _ := ioutil.ReadAll(res.Body) - res.Body.Close() - - var dat map[string]interface{} - err = json.Unmarshal(body, &dat) - if err != nil { - return nil, errors.Wrap(err, tr("error processing response from server")) + res, err := httpClient.Do(req) + if err != nil { + return nil, errors.Wrap(err, tr("error querying Arduino Cloud Api")) + } + if res.StatusCode >= 400 { + if res.StatusCode == 404 { + return nil, ErrNotFound } + return nil, errors.Errorf(tr("the server responded with status %s"), res.Status) + } - name, nameFound := dat["name"].(string) - fqbn, fbqnFound := dat["fqbn"].(string) + resp, err := io.ReadAll(res.Body) + if err != nil { + return nil, err + } + if err := res.Body.Close(); err != nil { + return nil, err + } - if !nameFound || !fbqnFound { - return nil, errors.New(tr("wrong format in server response")) - } + var dat map[string]interface{} + if err := json.Unmarshal(resp, &dat); err != nil { + return nil, errors.Wrap(err, tr("error processing response from server")) + } + name, nameFound := dat["name"].(string) + fqbn, fbqnFound := dat["fqbn"].(string) + if !nameFound || !fbqnFound { + return nil, errors.New(tr("wrong format in server response")) + } - retVal = append(retVal, &rpc.BoardListItem{ + return []*rpc.BoardListItem{ + { Name: name, Fqbn: fqbn, - }) - } else { - return nil, errors.Wrap(err, tr("error querying Arduino Cloud Api")) - } - - return retVal, nil + }, + }, nil } func identifyViaCloudAPI(port *discovery.Port) ([]*rpc.BoardListItem, error) { From 8095a7cbbb5042dc71b54bea1f4faf1d252b020a Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Thu, 17 Nov 2022 10:51:50 +0100 Subject: [PATCH 2/5] Cache cloud-api response for 24h to improve responsiveness --- commands/board/list.go | 36 +++++++++++++++++++++++++++++++++++- inventory/inventory.go | 6 ++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/commands/board/list.go b/commands/board/list.go index 69252ed7eb0..154647f170d 100644 --- a/commands/board/list.go +++ b/commands/board/list.go @@ -32,6 +32,7 @@ import ( "github.com/arduino/arduino-cli/arduino/discovery" "github.com/arduino/arduino-cli/arduino/httpclient" "github.com/arduino/arduino-cli/commands" + "github.com/arduino/arduino-cli/inventory" rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -50,6 +51,39 @@ var ( validVidPid = regexp.MustCompile(`0[xX][a-fA-F\d]{4}`) ) +func cachedApiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { + var resp []*rpc.BoardListItem + + cacheKey := fmt.Sprintf("cache.builder-api.v3/boards/byvid/pid/%s/%s", vid, pid) + if cachedResp := inventory.Store.GetString(cacheKey + ".data"); cachedResp != "" { + ts := inventory.Store.GetTime(cacheKey + ".ts") + if time.Since(ts) < time.Hour*24 { + // Use cached response + if cachedResp == "ErrNotFound" { + return nil, ErrNotFound + } + if err := json.Unmarshal([]byte(cachedResp), &resp); err == nil { + return resp, nil + } + } + } + + resp, err := apiByVidPid(vid, pid) // Perform API requrest + + if err == ErrNotFound { + inventory.Store.Set(cacheKey+".data", "ErrNotFound") + inventory.Store.Set(cacheKey+".ts", time.Now()) + inventory.WriteStore() + } else if err == nil { + if cachedResp, err := json.Marshal(resp); err == nil { + inventory.Store.Set(cacheKey+".data", string(cachedResp)) + inventory.Store.Set(cacheKey+".ts", time.Now()) + inventory.WriteStore() + } + } + return resp, err +} + func apiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { // ensure vid and pid are valid before hitting the API if !validVidPid.MatchString(vid) { @@ -116,7 +150,7 @@ func identifyViaCloudAPI(port *discovery.Port) ([]*rpc.BoardListItem, error) { } logrus.Debug("Querying builder API for board identification...") - return apiByVidPid(id.Get("vid"), id.Get("pid")) + return cachedApiByVidPid(id.Get("vid"), id.Get("pid")) } // identify returns a list of boards checking first the installed platforms or the Cloud API diff --git a/inventory/inventory.go b/inventory/inventory.go index fe86634b6ac..8411a58fc2c 100644 --- a/inventory/inventory.go +++ b/inventory/inventory.go @@ -19,6 +19,7 @@ import ( "fmt" "os" "path/filepath" + "sync" "github.com/arduino/arduino-cli/i18n" "github.com/gofrs/uuid" @@ -77,9 +78,14 @@ func generateInstallationData() error { return nil } +var writeStoreMux sync.Mutex + // WriteStore writes the current information from Store to configFilePath. // Returns err if it fails. func WriteStore() error { + writeStoreMux.Lock() + defer writeStoreMux.Unlock() + configPath := filepath.Dir(configFilePath) // Create config dir if not present, From 56d88e12f288a53f0ef1ef960e6e453d7af2bc6f Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Mon, 14 Nov 2022 22:33:22 +0100 Subject: [PATCH 3/5] Do not fail with errors in case of cloud-api is not available --- commands/board/list.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commands/board/list.go b/commands/board/list.go index 154647f170d..c8b71b08a13 100644 --- a/commands/board/list.go +++ b/commands/board/list.go @@ -185,8 +185,8 @@ func identify(pme *packagemanager.Explorer, port *discovery.Port) ([]*rpc.BoardL // the board couldn't be detected, print a warning logrus.Debug("Board not recognized") } else if err != nil { - // this is bad, bail out - return nil, &arduino.UnavailableError{Message: tr("Error getting board info from Arduino Cloud")} + // this is bad, but keep going + logrus.WithError(err).Debug("Error querying builder API") } // add a DetectedPort entry in any case: the `Boards` field will From 0b372c808fe6414a839cfb7071e0cd54f8e3f5f2 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Thu, 17 Nov 2022 10:58:55 +0100 Subject: [PATCH 4/5] Fixed linter warning... --- commands/board/list.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commands/board/list.go b/commands/board/list.go index c8b71b08a13..98274e80e15 100644 --- a/commands/board/list.go +++ b/commands/board/list.go @@ -51,7 +51,7 @@ var ( validVidPid = regexp.MustCompile(`0[xX][a-fA-F\d]{4}`) ) -func cachedApiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { +func cachedAPIByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { var resp []*rpc.BoardListItem cacheKey := fmt.Sprintf("cache.builder-api.v3/boards/byvid/pid/%s/%s", vid, pid) @@ -150,7 +150,7 @@ func identifyViaCloudAPI(port *discovery.Port) ([]*rpc.BoardListItem, error) { } logrus.Debug("Querying builder API for board identification...") - return cachedApiByVidPid(id.Get("vid"), id.Get("pid")) + return cachedAPIByVidPid(id.Get("vid"), id.Get("pid")) } // identify returns a list of boards checking first the installed platforms or the Cloud API From 9c0c122ca3cb1008bbea079d4c5f87509529dea0 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Fri, 18 Nov 2022 13:35:28 +0100 Subject: [PATCH 5/5] Removed useless ErrNotFound from `apiByVidPid` The `apiByVidPid` function now masks the odd behavior of the builder-api returning an HTTP 404 if the request succeed but the result is empty. --- commands/board/list.go | 35 +++++++---------------------------- commands/board/list_test.go | 7 +++---- 2 files changed, 10 insertions(+), 32 deletions(-) diff --git a/commands/board/list.go b/commands/board/list.go index 98274e80e15..6335fa69cb3 100644 --- a/commands/board/list.go +++ b/commands/board/list.go @@ -38,15 +38,7 @@ import ( "github.com/sirupsen/logrus" ) -type boardNotFoundError struct{} - -func (e *boardNotFoundError) Error() string { - return tr("board not found") -} - var ( - // ErrNotFound is returned when the API returns 404 - ErrNotFound = &boardNotFoundError{} vidPidURL = "https://builder.arduino.cc/v3/boards/byVidPid" validVidPid = regexp.MustCompile(`0[xX][a-fA-F\d]{4}`) ) @@ -59,9 +51,6 @@ func cachedAPIByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { ts := inventory.Store.GetTime(cacheKey + ".ts") if time.Since(ts) < time.Hour*24 { // Use cached response - if cachedResp == "ErrNotFound" { - return nil, ErrNotFound - } if err := json.Unmarshal([]byte(cachedResp), &resp); err == nil { return resp, nil } @@ -70,11 +59,7 @@ func cachedAPIByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { resp, err := apiByVidPid(vid, pid) // Perform API requrest - if err == ErrNotFound { - inventory.Store.Set(cacheKey+".data", "ErrNotFound") - inventory.Store.Set(cacheKey+".ts", time.Now()) - inventory.WriteStore() - } else if err == nil { + if err == nil { if cachedResp, err := json.Marshal(resp); err == nil { inventory.Store.Set(cacheKey+".data", string(cachedResp)) inventory.Store.Set(cacheKey+".ts", time.Now()) @@ -109,10 +94,11 @@ func apiByVidPid(vid, pid string) ([]*rpc.BoardListItem, error) { if err != nil { return nil, errors.Wrap(err, tr("error querying Arduino Cloud Api")) } + if res.StatusCode == 404 { + // This is not an error, it just means that the board is not recognized + return nil, nil + } if res.StatusCode >= 400 { - if res.StatusCode == 404 { - return nil, ErrNotFound - } return nil, errors.Errorf(tr("the server responded with status %s"), res.Status) } @@ -146,7 +132,7 @@ func identifyViaCloudAPI(port *discovery.Port) ([]*rpc.BoardListItem, error) { // If the port is not USB do not try identification via cloud id := port.Properties if !id.ContainsKey("vid") || !id.ContainsKey("pid") { - return nil, ErrNotFound + return nil, nil } logrus.Debug("Querying builder API for board identification...") @@ -181,17 +167,10 @@ func identify(pme *packagemanager.Explorer, port *discovery.Port) ([]*rpc.BoardL // the builder API if the board is a USB device port if len(boards) == 0 { items, err := identifyViaCloudAPI(port) - if errors.Is(err, ErrNotFound) { - // the board couldn't be detected, print a warning - logrus.Debug("Board not recognized") - } else if err != nil { + if err != nil { // this is bad, but keep going logrus.WithError(err).Debug("Error querying builder API") } - - // add a DetectedPort entry in any case: the `Boards` field will - // be empty but the port will be shown anyways (useful for 3rd party - // boards) boards = items } diff --git a/commands/board/list_test.go b/commands/board/list_test.go index 3d3314f9404..cf1dca37e4c 100644 --- a/commands/board/list_test.go +++ b/commands/board/list_test.go @@ -71,9 +71,8 @@ func TestGetByVidPidNotFound(t *testing.T) { vidPidURL = ts.URL res, err := apiByVidPid("0x0420", "0x0069") - require.NotNil(t, err) - require.Equal(t, "board not found", err.Error()) - require.Len(t, res, 0) + require.NoError(t, err) + require.Empty(t, res) } func TestGetByVidPid5xx(t *testing.T) { @@ -108,7 +107,7 @@ func TestBoardDetectionViaAPIWithNonUSBPort(t *testing.T) { Properties: properties.NewMap(), } items, err := identifyViaCloudAPI(port) - require.ErrorIs(t, err, ErrNotFound) + require.NoError(t, err) require.Empty(t, items) }