✨ Feature: Dependency-diff API optimize: var re-naming, removing unused JSON tags (#2090)

* save

* save

* Update dependencydiff_result.go

* save

* save

* save

dependencydiff/dependencydiff.go

@@ -17,12 +17,13 @@ package dependencydiff
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/checks"
 	"github.com/ossf/scorecard/v4/clients"
 	sce "github.com/ossf/scorecard/v4/errors"
-	"github.com/ossf/scorecard/v4/log"
+	sclog "github.com/ossf/scorecard/v4/log"
 	"github.com/ossf/scorecard/v4/pkg"
 	"github.com/ossf/scorecard/v4/policy"
 )
@@ -30,46 +31,54 @@ import (
 // Depdiff is the exported name for dependency-diff.
 const Depdiff = "Dependency-diff"
 
+// A private context struct used for GetDependencyCheckResults.
 type dependencydiffContext struct {
-	logger                                *log.Logger
-	ownerName, repoName, baseSHA, headSHA string
-	ctx                                   context.Context
-	ghRepo                                clients.Repo
-	ghRepoClient                          clients.RepoClient
-	ossFuzzClient                         clients.RepoClient
-	vulnsClient                           clients.VulnerabilitiesClient
-	ciiClient                             clients.CIIBestPracticesClient
-	changeTypesToCheck                    map[pkg.ChangeType]bool
-	checkNamesToRun                       []string
-	dependencydiffs                       []dependency
-	results                               []pkg.DependencyCheckResult
+	logger                          *sclog.Logger
+	ownerName, repoName, base, head string
+	ctx                             context.Context
+	ghRepo                          clients.Repo
+	ghRepoClient                    clients.RepoClient
+	ossFuzzClient                   clients.RepoClient
+	vulnsClient                     clients.VulnerabilitiesClient
+	ciiClient                       clients.CIIBestPracticesClient
+	changeTypesToCheck              map[pkg.ChangeType]bool
+	checkNamesToRun                 []string
+	dependencydiffs                 []dependency
+	results                         []pkg.DependencyCheckResult
 }
 
 // GetDependencyDiffResults gets dependency changes between two given code commits BASE and HEAD
 // along with the Scorecard check results of the dependencies, and returns a slice of DependencyCheckResult.
-// TO use this API, an access token must be set following https://github.com/ossf/scorecard#authentication.
+// TO use this API, an access token must be set. See https://github.com/ossf/scorecard#authentication.
 func GetDependencyDiffResults(
-	ctx context.Context, ownerName, repoName, baseSHA, headSHA string, scorecardChecksNames []string,
-	changeTypesToCheck map[pkg.ChangeType]bool) ([]pkg.DependencyCheckResult, error) {
-	// Fetch the raw dependency diffs.
+	ctx context.Context,
+	repoURI string, /* Use the format "ownerName/repoName" as the repo URI, such as "ossf/scorecard". */
+	base, head string, /* Two code commits base and head, can use either SHAs or branch names. */
+	checksToRun []string, /* A list of enabled check names to run. */
+	changeTypesToCheck map[pkg.ChangeType]bool, /* A list of change types for which to surface scorecard results. */
+) ([]pkg.DependencyCheckResult, error) {
+
+	logger := sclog.NewLogger(sclog.DefaultLevel)
+	ownerAndRepo := strings.Split(repoURI, "/")
+	if len(ownerAndRepo) != 2 {
+		return nil, fmt.Errorf("%w: repo uri input", errInvalid)
+	}
+	owner, repo := ownerAndRepo[0], ownerAndRepo[1]
 	dCtx := dependencydiffContext{
-		logger:             log.NewLogger(log.InfoLevel),
-		ownerName:          ownerName,
-		repoName:           repoName,
-		baseSHA:            baseSHA,
-		headSHA:            headSHA,
+		logger:             logger,
+		ownerName:          owner,
+		repoName:           repo,
+		base:               base,
+		head:               head,
 		ctx:                ctx,
 		changeTypesToCheck: changeTypesToCheck,
-		checkNamesToRun:    scorecardChecksNames,
+		checkNamesToRun:    checksToRun,
 	}
+	// Fetch the raw dependency diffs. This API will also handle error cases such as invalid base or head.
 	err := fetchRawDependencyDiffData(&dCtx)
 	if err != nil {
 		return nil, fmt.Errorf("error in fetchRawDependencyDiffData: %w", err)
 	}
-
-	if err != nil {
-		return nil, fmt.Errorf("error in initRepoAndClientByChecks: %w", err)
-	}
 	err = getScorecardCheckResults(&dCtx)
 	if err != nil {
 		return nil, fmt.Errorf("error getting scorecard check results: %w", err)
@@ -150,10 +159,12 @@ func getScorecardCheckResults(dCtx *dependencydiffContext) error {
 			// If the run fails, we leave the current dependency scorecard result empty and record the error
 			// rather than letting the entire API return nil since we still expect results for other dependencies.
 			if err != nil {
-				depCheckResult.ScorecardResultsWithError.Error = sce.WithMessage(sce.ErrScorecardInternal,
-					fmt.Sprintf("error running the scorecard checks: %v", err))
+				wrappedErr := sce.WithMessage(sce.ErrScorecardInternal,
+					fmt.Sprintf("scorecard running failed for %s: %v", d.Name, err))
+				dCtx.logger.Error(wrappedErr, "")
+				depCheckResult.ScorecardResultWithError.Error = wrappedErr
 			} else { // Otherwise, we record the scorecard check results for this dependency.
-				depCheckResult.ScorecardResultsWithError.ScorecardResults = &scorecardResult
+				depCheckResult.ScorecardResultWithError.ScorecardResult = &scorecardResult
 			}
 		}
 		dCtx.results = append(dCtx.results, depCheckResult)

dependencydiff/dependencydiff_test.go

@@ -40,8 +40,8 @@ func Test_fetchRawDependencyDiffData(t *testing.T) {
 				ctx:       context.Background(),
 				ownerName: "no_such_owner",
 				repoName:  "repo_not_exist",
-				baseSHA:   "base",
-				headSHA:   clients.HeadSHA,
+				base:      "main",
+				head:      clients.HeadSHA,
 			},
 			wantEmpty: true,
 			wantErr:   true,

dependencydiff/errors.go

@@ -0,0 +1,22 @@
+// Copyright 2022 Security Scorecard Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package dependencydiff
+
+import "errors"
+
+// static Errors for mapping
+var (
+	errInvalid = errors.New("invalid")
+)

dependencydiff/raw_dependencies.go

@@ -58,7 +58,7 @@ func fetchRawDependencyDiffData(dCtx *dependencydiffContext) error {
 	req, err := ghClient.NewRequest(
 		"GET",
 		path.Join("repos", dCtx.ownerName, dCtx.repoName,
-			"dependency-graph", "compare", dCtx.baseSHA+"..."+dCtx.headSHA),
+			"dependency-graph", "compare", dCtx.base+"..."+dCtx.head),
 		nil,
 	)
 	if err != nil {

e2e/dependencydiff_test.go

@@ -26,19 +26,16 @@ import (
 )
 
 const (
-	OWNER = "ossf-tests"
-	REPO  = "scorecard-depdiff"
-	BASE  = "fd2a82b3b735fffbc2d782ed5f50301b879ecc51"
-	HEAD  = "1989568f93e484f6a86f8b276b170e3d6962ce12"
+	repoURI = "ossf-tests/scorecard-depdiff"
+	base    = "fd2a82b3b735fffbc2d782ed5f50301b879ecc51"
+	head    = "1989568f93e484f6a86f8b276b170e3d6962ce12"
 )
 
 var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 	Context("E2E TEST:Validating use of the dependency-diff API", func() {
 		It("Should return a slice of dependency-diff checking results", func() {
 			ctx := context.Background()
-			ownerName, repoName := OWNER, REPO
-			baseSHA, headSHA := BASE, HEAD
-			scorecardChecksNames := []string{
+			checksToRun := []string{
 				checks.CheckBranchProtection,
 			}
 			changeTypesToCheck := map[pkg.ChangeType]bool{
@@ -46,8 +43,9 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 			}
 			results, err := dependencydiff.GetDependencyDiffResults(
 				ctx,
-				ownerName, repoName, baseSHA, headSHA,
-				scorecardChecksNames,
+				repoURI,
+				base, head,
+				checksToRun,
 				changeTypesToCheck,
 			)
 			Expect(err).Should(BeNil())
@@ -55,10 +53,7 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 		})
 		It("Should return a valid empty result", func() {
 			ctx := context.Background()
-			ownerName, repoName := OWNER, REPO
-			baseSHA, headSHA := BASE, BASE
-
-			scorecardChecksNames := []string{
+			checksToRun := []string{
 				checks.CheckBranchProtection,
 			}
 			changeTypesToCheck := map[pkg.ChangeType]bool{
@@ -66,8 +61,9 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 			}
 			results, err := dependencydiff.GetDependencyDiffResults(
 				ctx,
-				ownerName, repoName, baseSHA, headSHA,
-				scorecardChecksNames,
+				repoURI,
+				base, base,
+				checksToRun,
 				changeTypesToCheck,
 			)
 			Expect(err).Should(BeNil())
@@ -75,17 +71,17 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 		})
 		It("Should initialize clients corresponding to the checks to run and do not crash", func() {
 			ctx := context.Background()
-			ownerName, repoName := OWNER, REPO
-			baseSHA, headSHA := BASE, HEAD
-
-			scorecardChecksNames := []string{}
+			checksToRun := []string{
+				checks.CheckFuzzing,
+			}
 			changeTypesToCheck := map[pkg.ChangeType]bool{
 				pkg.Removed: true,
 			}
 			_, err := dependencydiff.GetDependencyDiffResults(
 				ctx,
-				ownerName, repoName, baseSHA, headSHA,
-				scorecardChecksNames,
+				repoURI,
+				base, head,
+				checksToRun,
 				changeTypesToCheck,
 			)
 			Expect(err).Should(BeNil())

pkg/dependencydiff_result.go

@@ -44,40 +44,41 @@ func (ct *ChangeType) IsValid() bool {
 	}
 }
 
-// ScorecardResultsWithError is used for the dependency-diff module to record scorecard results and their errors.
-type ScorecardResultsWithError struct {
-	// ScorecardResults is the scorecard result for the dependency repo.
-	ScorecardResults *ScorecardResult `json:"scorecardResults"`
+// ScorecardResultWithError is used for the dependency-diff module to record the scorecard result
+// and a potential error field if the Scorecard run fails.
+type ScorecardResultWithError struct {
+	// ScorecardResult is the scorecard result for the dependency repo.
+	ScorecardResult *ScorecardResult
 
 	// Error is an error returned when running the scorecard checks. A nil Error indicates the run succeeded.
-	Error error `json:"scorecardRunTimeError"`
+	Error error
 }
 
 // DependencyCheckResult is the dependency structure used in the returned results.
 type DependencyCheckResult struct {
 	// ChangeType indicates whether the dependency is added, updated, or removed.
-	ChangeType *ChangeType `json:"changeType"`
+	ChangeType *ChangeType
 
 	// Package URL is a short link for a package.
-	PackageURL *string `json:"packageUrl"`
+	PackageURL *string
 
 	// SourceRepository is the source repository URL of the dependency.
-	SourceRepository *string `json:"sourceRepository"`
+	SourceRepository *string
 
 	// ManifestPath is the path of the manifest file of the dependency, such as go.mod for Go.
-	ManifestPath *string `json:"manifestPath"`
+	ManifestPath *string
 
 	// Ecosystem is the name of the package management system, such as NPM, GO, PYPI.
-	Ecosystem *string `json:"ecosystem"`
+	Ecosystem *string
 
 	// Version is the package version of the dependency.
-	Version *string `json:"version"`
+	Version *string
 
-	// ScorecardResultsWithError is the scorecard checking results of the dependency.
-	ScorecardResultsWithError ScorecardResultsWithError `json:"scorecardResultsWithError"`
+	// ScorecardResultWithError is the scorecard checking results of the dependency.
+	ScorecardResultWithError ScorecardResultWithError
 
 	// Name is the name of the dependency.
-	Name string `json:"name"`
+	Name string
 }
 
 // AsJSON for DependencyCheckResult exports the DependencyCheckResult as a JSON object.