Handle partial repo list warnings

Amp-Thread-ID: https://ampcode.com/threads/T-019d6793-b619-7488-8a51-ed232d6d82a3
Co-authored-by: Amp <amp@ampcode.com>

Author: burmudar

cmd/src/repos.go

@@ -84,7 +84,7 @@ type Repository struct {
 	CreatedAt           time.Time          `json:"createdAt"`
 	UpdatedAt           *time.Time         `json:"updatedAt"`
 	ExternalRepository  ExternalRepository `json:"externalRepository"`
-	DefaultBranch       GitRef             `json:"defaultBranch"`
+	DefaultBranch       *GitRef            `json:"defaultBranch"`
 	ViewerCanAdminister bool               `json:"viewerCanAdminister"`
 	KeyValuePairs       []KeyValuePair     `json:"keyValuePairs"`
 }

cmd/src/repos_list.go

@@ -82,37 +82,12 @@ func listRepositories(ctx context.Context, client api.Client, params reposListOp
 
 	errors := api.NewGraphQlErrors(result.Errors)
 	if len(repos) > 0 {
-		return filterRepositoriesWithErrors(repos, errors), errors, nil
+		return repos, errors, nil
 	}
 
 	return nil, nil, errors
 }
 
-func filterRepositoriesWithErrors(repos []Repository, errors api.GraphQlErrors) []Repository {
-	if len(errors) == 0 || len(repos) == 0 {
-		return repos
-	}
-
-	skip := make(map[int]struct{}, len(errors))
-	for _, graphQLError := range errors {
-		index, ok := gqlRepositoryErrorIndex(graphQLError)
-		if !ok || index >= len(repos) {
-			continue
-		}
-		skip[index] = struct{}{}
-	}
-
-	filtered := make([]Repository, 0, len(repos))
-	for i, repo := range repos {
-		if _, ok := skip[i]; ok {
-			continue
-		}
-		filtered = append(filtered, repo)
-	}
-
-	return filtered
-}
-
 func gqlErrorPathString(pathSegment any) (string, bool) {
 	value, ok := pathSegment.(string)
 	return value, ok
@@ -130,22 +105,52 @@ func gqlErrorIndex(pathSegment any) (int, bool) {
 	}
 }
 
-func gqlRepositoryErrorIndex(graphQLError *api.GraphQlError) (int, bool) {
+func gqlWarningPath(graphQLError *api.GraphQlError) string {
 	path, err := graphQLError.Path()
-	if err != nil || len(path) < 3 {
-		return 0, false
+	if err != nil || len(path) == 0 {
+		return ""
 	}
 
-	pathRoot, ok := gqlErrorPathString(path[0])
-	if !ok || pathRoot != "repositories" {
-		return 0, false
+	var b strings.Builder
+	for _, pathSegment := range path {
+		if segment, ok := gqlErrorPathString(pathSegment); ok {
+			if b.Len() > 0 {
+				b.WriteByte('.')
+			}
+			b.WriteString(segment)
+			continue
+		}
+
+		if index, ok := gqlErrorIndex(pathSegment); ok {
+			fmt.Fprintf(&b, "[%d]", index)
+		}
 	}
-	pathCollection, ok := gqlErrorPathString(path[1])
-	if !ok || pathCollection != "nodes" {
-		return 0, false
+
+	return b.String()
+}
+
+func gqlWarningMessage(graphQLError *api.GraphQlError) string {
+	message, err := graphQLError.Message()
+	if err != nil || message == "" {
+		return graphQLError.Error()
 	}
+	return message
+}
 
-	return gqlErrorIndex(path[2])
+func formatRepositoryListWarnings(warnings api.GraphQlErrors) string {
+	var b strings.Builder
+	fmt.Fprintf(&b, "warnings: %d errors during listing\n", len(warnings))
+	for _, warning := range warnings {
+		path := gqlWarningPath(warning)
+		message := gqlWarningMessage(warning)
+		if path != "" {
+			fmt.Fprintf(&b, "%s - %s\n", path, message)
+		} else {
+			fmt.Fprintf(&b, "%s\n", message)
+		}
+		fmt.Fprintf(&b, "%s\n", warning.Error())
+	}
+	return b.String()
 }
 
 func init() {
@@ -241,10 +246,7 @@ Examples:
 		}
 		if len(warnings) > 0 {
 			if *verbose {
-				fmt.Fprintf(flagSet.Output(), "warning: %d errors during listing:\n", len(warnings))
-				for _, warning := range warnings {
-					fmt.Fprintln(flagSet.Output(), warning.Error())
-				}
+				fmt.Fprint(flagSet.Output(), formatRepositoryListWarnings(warnings))
 			} else {
 				fmt.Fprintf(flagSet.Output(), "warning: %d errors during listing; rerun with -v to inspect them\n", len(warnings))
 			}

cmd/src/repos_list_test.go

@@ -2,15 +2,17 @@ package main
 
 import (
 	"context"
+	"encoding/json"
 	"strings"
 	"testing"
 
+	"github.com/sourcegraph/src-cli/internal/api"
 	mockapi "github.com/sourcegraph/src-cli/internal/api/mock"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
-func TestListRepositoriesSkipsRepositoryWhenDefaultBranchCannotBeResolved(t *testing.T) {
+func TestListRepositoriesPreservesRepositoryWhenDefaultBranchCannotBeResolved(t *testing.T) {
 	client := new(mockapi.Client)
 	request := &mockapi.Request{Response: `{
 		"data": {
@@ -92,15 +94,18 @@ func TestListRepositoriesSkipsRepositoryWhenDefaultBranchCannotBeResolved(t *tes
 		orderBy:    "REPOSITORY_NAME",
 	})
 	require.NoError(t, err)
-	require.Len(t, repos, 1)
+	require.Len(t, repos, 2)
 	require.Len(t, warnings, 1)
 	require.Equal(t, "github.com/sourcegraph/ok", repos[0].Name)
+	require.NotNil(t, repos[0].DefaultBranch)
+	require.Equal(t, "github.com/sourcegraph/broken", repos[1].Name)
+	require.Nil(t, repos[1].DefaultBranch)
 	require.ErrorContains(t, warnings[0], "failed to resolve HEAD for github.com/sourcegraph/broken")
 	client.AssertExpectations(t)
 	request.AssertExpectations(t)
 }
 
-func TestListRepositoriesFiltersNodeScopedFieldErrors(t *testing.T) {
+func TestListRepositoriesPreservesRepositoriesForNodeScopedFieldErrors(t *testing.T) {
 	client := new(mockapi.Client)
 	request := &mockapi.Request{Response: `{
 		"data": {
@@ -158,8 +163,9 @@ func TestListRepositoriesFiltersNodeScopedFieldErrors(t *testing.T) {
 		orderBy:    "REPOSITORY_NAME",
 	})
 	require.NoError(t, err)
-	require.Empty(t, repos)
+	require.Len(t, repos, 1)
 	require.Len(t, warnings, 1)
+	require.Equal(t, "github.com/sourcegraph/ok", repos[0].Name)
 	require.ErrorContains(t, warnings[0], "viewer permissions unavailable")
 	client.AssertExpectations(t)
 	request.AssertExpectations(t)
@@ -230,3 +236,36 @@ func TestListRepositoriesReturnsWarningsWithDataForNonNodeErrors(t *testing.T) {
 	client.AssertExpectations(t)
 	request.AssertExpectations(t)
 }
+
+func TestFormatRepositoryListWarnings(t *testing.T) {
+	warnings := api.NewGraphQlErrors([]json.RawMessage{json.RawMessage(`{
+		"message": "failed to resolve HEAD for github.com/sourcegraph/broken",
+		"path": ["repositories", "nodes", 1, "defaultBranch"]
+	}`)})
+
+	require.Equal(t, `warnings: 1 errors during listing
+repositories.nodes[1].defaultBranch - failed to resolve HEAD for github.com/sourcegraph/broken
+{
+  "message": "failed to resolve HEAD for github.com/sourcegraph/broken",
+  "path": [
+    "repositories",
+    "nodes",
+    1,
+    "defaultBranch"
+  ]
+}
+`, formatRepositoryListWarnings(warnings))
+}
+
+func TestFormatRepositoryListWarningsWithoutPath(t *testing.T) {
+	warnings := api.NewGraphQlErrors([]json.RawMessage{json.RawMessage(`{
+		"message": "listing timed out"
+	}`)})
+
+	require.Equal(t, `warnings: 1 errors during listing
+listing timed out
+{
+  "message": "listing timed out"
+}
+`, formatRepositoryListWarnings(warnings))
+}

internal/api/errors.go

@@ -43,6 +43,23 @@ func (gg GraphQlErrors) Error() string {
 // GraphQlError wraps a raw JSON error returned from a GraphQL endpoint.
 type GraphQlError struct{ v any }
 
+// Message returns the GraphQL error message, if one was set on the error.
+func (g *GraphQlError) Message() (string, error) {
+	e, ok := g.v.(map[string]any)
+	if !ok {
+		return "", errors.Errorf("unexpected GraphQL error of type %T", g.v)
+	}
+
+	if e["message"] == nil {
+		return "", nil
+	}
+	message, ok := e["message"].(string)
+	if !ok {
+		return "", errors.Errorf("unexpected message of type %T", e["message"])
+	}
+	return message, nil
+}
+
 // Code returns the GraphQL error code, if one was set on the error.
 func (g *GraphQlError) Code() (string, error) {
 	ext, err := g.Extensions()