✨ Enhancement: Dependency-diff API optimization - changing the input param changeType from a map to an array (#2111)

* save

* save

* save

* save

* save

* save

* save

dependencydiff/dependencydiff.go

@@ -41,7 +41,7 @@ type dependencydiffContext struct {
 	ossFuzzClient                   clients.RepoClient
 	vulnsClient                     clients.VulnerabilitiesClient
 	ciiClient                       clients.CIIBestPracticesClient
-	changeTypesToCheck              map[pkg.ChangeType]bool
+	changeTypesToCheck              []string
 	checkNamesToRun                 []string
 	dependencydiffs                 []dependency
 	results                         []pkg.DependencyCheckResult
@@ -55,7 +55,7 @@ func GetDependencyDiffResults(
 	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. */
+	changeTypes []string, /* A list of dependency change types for which we surface scorecard results. */
 ) ([]pkg.DependencyCheckResult, error) {
 
 	logger := sclog.NewLogger(sclog.DefaultLevel)
@@ -71,7 +71,7 @@ func GetDependencyDiffResults(
 		base:               base,
 		head:               head,
 		ctx:                ctx,
-		changeTypesToCheck: changeTypesToCheck,
+		changeTypesToCheck: changeTypes,
 		checkNamesToRun:    checksToRun,
 	}
 	// Fetch the raw dependency diffs. This API will also handle error cases such as invalid base or head.
@@ -133,13 +133,21 @@ func getScorecardCheckResults(dCtx *dependencydiffContext) error {
 			Ecosystem:        d.Ecosystem,
 			Version:          d.Version,
 			Name:             d.Name,
+			/* The scorecard check result is nil now. */
 		}
-		// Run the checks on all types if (1) the type is found in changeTypesToCheck or (2) no types are specified.
-		TypeFoundOrNoneGiven := dCtx.changeTypesToCheck[*d.ChangeType] ||
-			(dCtx.changeTypesToCheck == nil || len(dCtx.changeTypesToCheck) == 0)
+		if d.ChangeType == nil {
+			// Since we allow a dependency having a nil change type, so we also
+			// give such a dependency a nil scorecard result.
+			dCtx.results = append(dCtx.results, depCheckResult)
+			continue
+		}
+		// (1) If no change types are specified, run the checks on all types of dependencies.
+		// (2) If there are change types specified by the user, run the checks on the specified types.
+		noneGivenOrIsSpecified := len(dCtx.changeTypesToCheck) == 0 || /* None specified.*/
+			isSpecifiedByUser(*d.ChangeType, dCtx.changeTypesToCheck) /* Specified by the user.*/
 		// For now we skip those without source repo urls.
 		// TODO (#2063): use the BigQuery dataset to supplement null source repo URLs to fetch the Scorecard results for them.
-		if d.SourceRepository != nil && TypeFoundOrNoneGiven {
+		if d.SourceRepository != nil && noneGivenOrIsSpecified {
 			// Initialize the repo and client(s) corresponding to the checks to run.
 			err = initRepoAndClientByChecks(dCtx, *d.SourceRepository)
 			if err != nil {
@@ -176,3 +184,15 @@ func getScorecardCheckResults(dCtx *dependencydiffContext) error {
 	}
 	return nil
 }
+
+func isSpecifiedByUser(ct pkg.ChangeType, changeTypes []string) bool {
+	if len(changeTypes) == 0 {
+		return false
+	}
+	for _, ctByUser := range changeTypes {
+		if string(ct) == ctByUser {
+			return true
+		}
+	}
+	return false
+}

dependencydiff/dependencydiff_test.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/ossf/scorecard/v4/clients"
 	sclog "github.com/ossf/scorecard/v4/log"
+	"github.com/ossf/scorecard/v4/pkg"
 )
 
 // Test_fetchRawDependencyDiffData is a test function for fetchRawDependencyDiffData.
@@ -226,3 +227,53 @@ func Test_mapDependencyEcosystemNaming(t *testing.T) {
 		})
 	}
 }
+
+func Test_isSpecifiedByUser(t *testing.T) {
+	t.Parallel()
+	//nolint
+	tests := []struct {
+		name               string
+		ct                 pkg.ChangeType
+		changeTypesToCheck []string
+		resultWanted       bool
+	}{
+		{
+			name: "error invalid github ecosystem",
+		},
+		{
+			name:               "added",
+			ct:                 pkg.ChangeType("added"),
+			changeTypesToCheck: nil,
+			resultWanted:       false,
+		},
+		{
+			name:               "ct is added but not specified",
+			ct:                 pkg.ChangeType("added"),
+			changeTypesToCheck: []string{"removed"},
+			resultWanted:       false,
+		},
+		{
+			name:               "removed",
+			ct:                 pkg.ChangeType("added"),
+			changeTypesToCheck: []string{"added", "removed"},
+			resultWanted:       true,
+		},
+		{
+			name:               "not_supported",
+			ct:                 pkg.ChangeType("not_supported"),
+			changeTypesToCheck: []string{"added", "removed"},
+			resultWanted:       false,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			result := isSpecifiedByUser(tt.ct, tt.changeTypesToCheck)
+			if result != tt.resultWanted {
+				t.Errorf("result (%v) != result wanted (%v)", result, tt.resultWanted)
+				return
+			}
+		})
+	}
+}

e2e/dependencydiff_test.go

@@ -22,7 +22,6 @@ import (
 
 	"github.com/ossf/scorecard/v4/checks"
 	"github.com/ossf/scorecard/v4/dependencydiff"
-	"github.com/ossf/scorecard/v4/pkg"
 )
 
 const (
@@ -38,8 +37,8 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 			checksToRun := []string{
 				checks.CheckBranchProtection,
 			}
-			changeTypesToCheck := map[pkg.ChangeType]bool{
-				pkg.Removed: true, // Only checking those removed ones will make this test faster.
+			changeTypesToCheck := []string{
+				"removed", // Only checking those removed ones will make this test faster.
 			}
 			results, err := dependencydiff.GetDependencyDiffResults(
 				ctx,
@@ -56,8 +55,8 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 			checksToRun := []string{
 				checks.CheckBranchProtection,
 			}
-			changeTypesToCheck := map[pkg.ChangeType]bool{
-				pkg.Removed: true,
+			changeTypesToCheck := []string{
+				"removed",
 			}
 			results, err := dependencydiff.GetDependencyDiffResults(
 				ctx,
@@ -74,8 +73,8 @@ var _ = Describe("E2E TEST:"+dependencydiff.Depdiff, func() {
 			checksToRun := []string{
 				checks.CheckFuzzing,
 			}
-			changeTypesToCheck := map[pkg.ChangeType]bool{
-				pkg.Removed: true,
+			changeTypesToCheck := []string{
+				"removed",
 			}
 			_, err := dependencydiff.GetDependencyDiffResults(
 				ctx,