Repository: apigee/registry
Pull request #540 (2022-04-21 23:24:35)

Validation function for ScoreCardDefinition

codecov[bot]: # [Codecov](https://codecov.io/gh/apigee/registry/pull/540?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) Report
> Merging [#540](https://codecov.io/gh/apigee/registry/pull/540?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (8a0a4db) into [main](https://codecov.io/gh/apigee/registry/commit/d11444289d6e9f244d1f2b18be2529ad850cc326?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (d114442) will **increase** coverage by `0.19%`.
> The diff coverage is `84.90%`.

> :exclamation: Current head 8a0a4db differs from pull request most recent head d4be6ba. Consider uploading reports for the commit d4be6ba to get more accurate results

```diff
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
+ Coverage   41.26%   41.46%   +0.19%     
==========================================
  Files          69       69              
  Lines        8191     8232      +41     
==========================================
+ Hits         3380     3413      +33     
- Misses       4404     4410       +6     
- Partials      407      409       +2     
```


| [Impacted Files](https://codecov.io/gh/apigee/registry/pull/540?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) | Coverage Δ | |
|---|---|---|
| [cmd/registry/cmd/upload/artifact.go](https://codecov.io/gh/apigee/registry/pull/540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC91cGxvYWQvYXJ0aWZhY3QuZ28=) | `36.94% <37.50%> (-0.62%)` | :arrow_down: |
| [cmd/registry/scoring/parser.go](https://codecov.io/gh/apigee/registry/pull/540/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L3Njb3JpbmcvcGFyc2VyLmdv) | `95.29% <93.33%> (-1.01%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apigee/registry/pull/540?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apigee/registry/pull/540?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee). Last update [d114442...d4be6ba](https://codecov.io/gh/apigee/registry/pull/540?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee).


Patch:
diff --git a/cmd/registry/cmd/upload/artifact.go b/cmd/registry/cmd/upload/artifact.go
index a874619..6b5aa12 100644
--- a/cmd/registry/cmd/upload/artifact.go
+++ b/cmd/registry/cmd/upload/artifact.go
@@ -96,7 +96,7 @@ func buildArtifact(ctx context.Context, parent string, filename string) (*rpc.Ar
 	case "ScoreCard", patch.ScoreCardMimeType:
 		artifact, err = buildScoreCardArtifact(ctx, jsonBytes)
 	case "ScoreCardDefinition", patch.ScoreCardDefinitionMimeType:
-		artifact, err = buildScoreCardDefinitionArtifact(ctx, jsonBytes)
+		artifact, err = buildScoreCardDefinitionArtifact(ctx, parent, jsonBytes)
 	case "ScoreDefinition", patch.ScoreDefinitionMimeType:
 		artifact, err = buildScoreDefinitionArtifact(ctx, parent, jsonBytes)
 	case "TaxonomyList", patch.TaxonomyListMimeType:
@@ -218,11 +218,19 @@ func buildScoreDefinitionArtifact(ctx context.Context, parent string, jsonBytes
 	}, nil
 }
 
-func buildScoreCardDefinitionArtifact(ctx context.Context, jsonBytes []byte) (*rpc.Artifact, error) {
+func buildScoreCardDefinitionArtifact(ctx context.Context, parent string, jsonBytes []byte) (*rpc.Artifact, error) {
 	m := &rpc.ScoreCardDefinition{}
 	if err := protojson.Unmarshal(jsonBytes, m); err != nil {
 		return nil, err
 	}
+	errs := scoring.ValidateScoreCardDefinition(ctx, parent, m)
+	if count := len(errs); count > 0 {
+		for _, err := range errs {
+			log.FromContext(ctx).WithError(err).Error("ScoreCardDefinition error")
+		}
+		return nil, fmt.Errorf("ScoreCardDefinition contains %d error(s): see logs for details", count)
+	}
+
 	artifactBytes, err := proto.Marshal(m)
 	if err != nil {
 		return nil, err
diff --git a/cmd/registry/scoring/parser.go b/cmd/registry/scoring/parser.go
index 0c14533..85e76a6 100644
--- a/cmd/registry/scoring/parser.go
+++ b/cmd/registry/scoring/parser.go
@@ -67,26 +67,71 @@ func ValidateScoreDefinition(ctx context.Context, parent string, scoreDefinition
 	return totalErrs
 }
 
-func validateScoreFormula(targetName patterns.ResourceName, scoreFormula *rpc.ScoreFormula) []error {
-	errs := make([]error, 0)
+func ValidateScoreCardDefinition(ctx context.Context, parent string, scoreCardDefinition *rpc.ScoreCardDefinition) []error {
+	totalErrs := make([]error, 0)
 
-	// Validation checks for score_formula.artifact.pattern
-	pattern := scoreFormula.GetArtifact().GetPattern()
+	// target_resource.pattern should be a valid resource pattern
+	targetName, err := patterns.ParseResourcePattern(fmt.Sprintf("%s/%s", parent, scoreCardDefinition.GetTargetResource().GetPattern()))
+	if err != nil {
+		totalErrs = append(totalErrs, err)
+	}
 
-	// pattern should have a valid $resource pattern
+	scorePatterns := scoreCardDefinition.GetScorePatterns()
+
+	// Check if score_patterns are set
+	if len(scorePatterns) == 0 {
+		totalErrs = append(totalErrs, fmt.Errorf("missing score_patterns"))
+		return totalErrs
+	}
+
+	// Validate score_patterns only if target_resource.pattern is valid
+	if len(totalErrs) == 0 {
+		for _, pattern := range scorePatterns {
+			// Should have valid $resource references
+			errs := validateReferencesInPattern(targetName, pattern)
+			totalErrs = append(totalErrs, errs...)
+
+			// Should not end with a "-"
+			if strings.HasSuffix(pattern, "/-") {
+				totalErrs = append(totalErrs, fmt.Errorf("invalid score_pattern : %q, it should end with a resourceID and not a \"-\"", pattern))
+			}
+
+		}
+	}
+
+	return totalErrs
+}
+
+func validateReferencesInPattern(targetName patterns.ResourceName, pattern string) []error {
+	errs := make([]error, 0)
+
+	// pattern should have a valid $resource reference
 	_, entityType, err := patterns.GetReferenceEntityType(pattern)
 	if err != nil {
-		errs = append(errs, fmt.Errorf("invalid score_formula.artifact.pattern: %s", err))
+		errs = append(errs, fmt.Errorf("invalid pattern: %q, %s", pattern, err))
 	} else if entityType == "default" {
 		// pattern should always start with a $resource reference
-		errs = append(errs, fmt.Errorf("invalid score_formula.artifact.pattern: %q, must always start with '$resource.(api|version|spec|artifact)'", pattern))
+		errs = append(errs, fmt.Errorf("invalid pattern: %q, must always start with '$resource.(api|version|spec|artifact)'", pattern))
 	} else if _, err = patterns.GetReferenceEntityValue(pattern, targetName); err != nil {
 		// $resource should have valid entity reference wrt target_resource
-		errs = append(errs, fmt.Errorf("invalid $resource reference in score_formula.artifact.pattern: %s", err))
+		errs = append(errs, fmt.Errorf("invalid pattern %q, invalid $resource reference in pattern: %s", pattern, err))
 	}
 
-	// score_formula.pattern.pattern should not end with a "-"
-	if strings.HasSuffix(scoreFormula.GetArtifact().GetPattern(), "/-") {
+	return errs
+}
+
+func validateScoreFormula(targetName patterns.ResourceName, scoreFormula *rpc.ScoreFormula) []error {
+	errs := make([]error, 0)
+
+	// Validation checks for score_formula.artifact.pattern
+	pattern := scoreFormula.GetArtifact().GetPattern()
+
+	// Should have valid $resource references
+	patternErrs := validateReferencesInPattern(targetName, pattern)
+	errs = append(errs, patternErrs...)
+
+	// Should not end with a "-"
+	if strings.HasSuffix(pattern, "/-") {
 		errs = append(errs, fmt.Errorf("invalid score_formula.artifact.pattern : %q, it should end with a resourceID and not a \"-\"", pattern))
 	}
 


Test patch:
diff --git a/cmd/registry/scoring/parser_test.go b/cmd/registry/scoring/parser_test.go
index 1e52aa0..0e68966 100644
--- a/cmd/registry/scoring/parser_test.go
+++ b/cmd/registry/scoring/parser_test.go
@@ -600,7 +600,52 @@ func TestValidateScoreDefinition(t *testing.T) {
 			ctx := context.Background()
 			gotErrs := ValidateScoreDefinition(ctx, test.parent, test.scoreDefinition)
 			if len(gotErrs) != test.wantNumErr {
-				t.Errorf("ValidateScoreDefinition() returned unexpected no. of errors: want %d, got %s", test.wantNumErr, gotErrs)
+				t.Errorf("ValidateScoreDefinition(%s, %v) returned unexpected no. of errors: want %d, got %s", test.parent, test.scoreDefinition, test.wantNumErr, gotErrs)
+			}
+		})
+	}
+}
+
+func TestValidateReferencesInPattern(t *testing.T) {
+	tests := []struct {
+		desc          string
+		targetPattern string
+		pattern       string
+		wantNumErr    int
+	}{
+		// No errors
+		{
+			desc:          "score formula",
+			targetPattern: "projects/demo/locations/global/apis/-/versions/-/specs/-",
+			pattern:       "$resource.spec/artifacts/conformance-report",
+		},
+		// errors
+		{
+			desc:          "invalid $resource reference",
+			targetPattern: "projects/demo/locations/global/apis/-/versions/-/specs/-",
+			pattern:       "$resource.specs/artifacts/conformance-report", //error
+			wantNumErr:    1,
+		},
+		{
+			desc:          "no $resource reference",
+			targetPattern: "projects/demo/locations/global/apis/-/versions/-/specs/-",
+			pattern:       "apis/-/versions/-/specs/-/artifacts/conformance-report", //error
+			wantNumErr:    1,
+		},
+		{
+			desc:          "invalid $resource wrt targetName",
+			targetPattern: "projects/demo/locations/global/apis/-/versions/-",
+			pattern:       "$resource.spec/artifacts/conformance-report", //error
+			wantNumErr:    1,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			targetName, _ := patterns.ParseResourcePattern(test.targetPattern)
+			gotErrs := validateReferencesInPattern(targetName, test.pattern)
+			if len(gotErrs) != test.wantNumErr {
+				t.Errorf("validateReferencesInPattern(%s, %s) returned unexpected no. of errors: want %d, got %s", targetName, test.pattern, test.wantNumErr, gotErrs)
 			}
 		})
 	}
@@ -628,20 +673,7 @@ func TestValidateScoreFormula(t *testing.T) {
 		},
 		// Single errors
 		{
-			desc: "no $resource reference",
-			targetPattern: &rpc.ResourcePattern{
-				Pattern: "projects/demo/locations/global/apis/-/versions/-/specs/-",
-			},
-			scoreFormula: &rpc.ScoreFormula{
-				Artifact: &rpc.ResourcePattern{
-					Pattern: "apis/-/versions/-/specs/-/artifacts/conformance-report", //error
-				},
-				ScoreExpression: "count(errors)",
-			},
-			wantNumErr: 1,
-		},
-		{
-			desc: "invalid $resource reference",
+			desc: "$resource error",
 			targetPattern: &rpc.ResourcePattern{
 				Pattern: "projects/demo/locations/global/apis/-/versions/-/specs/-",
 			},
@@ -653,19 +685,6 @@ func TestValidateScoreFormula(t *testing.T) {
 			},
 			wantNumErr: 1,
 		},
-		{
-			desc: "invalid $resource wrt targetName",
-			targetPattern: &rpc.ResourcePattern{
-				Pattern: "projects/demo/locations/global/apis/-/versions/-",
-			},
-			scoreFormula: &rpc.ScoreFormula{
-				Artifact: &rpc.ResourcePattern{
-					Pattern: "$resource.spec/artifacts/conformance-report", //error
-				},
-				ScoreExpression: "count(errors)",
-			},
-			wantNumErr: 1,
-		},
 		{
 			desc: "missing artifact name",
 			targetPattern: &rpc.ResourcePattern{
@@ -725,7 +744,7 @@ func TestValidateScoreFormula(t *testing.T) {
 			targetName, _ := patterns.ParseResourcePattern(test.targetPattern.GetPattern())
 			gotErrs := validateScoreFormula(targetName, test.scoreFormula)
 			if len(gotErrs) != test.wantNumErr {
-				t.Errorf("validateScoreFormula() returned unexpected no. of errors: want %d, got %s", test.wantNumErr, gotErrs)
+				t.Errorf("validateScoreFormula(%s, %v) returned unexpected no. of errors: want %d, got %s", targetName, test.scoreFormula, test.wantNumErr, gotErrs)
 			}
 		})
 	}
@@ -1285,7 +1304,7 @@ func TestValidateNumberThresholds(t *testing.T) {
 		t.Run(test.desc, func(t *testing.T) {
 			gotErrs := validateNumberThresholds(test.thresholds, test.minValue, test.maxValue)
 			if len(gotErrs) != test.wantNumErr {
-				t.Errorf("validateNumberThresholds() returned unexpected no. of errors: want %d, got %s", test.wantNumErr, gotErrs)
+				t.Errorf("validateNumberThresholds(%v, %d, %d) returned unexpected no. of errors: want %d, got %s", test.thresholds, test.minValue, test.maxValue, test.wantNumErr, gotErrs)
 			}
 		})
 	}
@@ -1384,8 +1403,143 @@ func TestValidateBooleanThresholds(t *testing.T) {
 		t.Run(test.desc, func(t *testing.T) {
 			gotErrs := validateBooleanThresholds(test.thresholds)
 			if len(gotErrs) != test.wantNumErr {
-				t.Errorf("validateBooleanThresholds() returned unexpected no. of errors: want %d, got %s", test.wantNumErr, gotErrs)
+				t.Errorf("validateBooleanThresholds(%v) returned unexpected no. of errors: want %d, got %s", test.thresholds, test.wantNumErr, gotErrs)
+			}
+		})
+	}
+}
+
+func TestValidateScoreCardDefinition(t *testing.T) {
+	tests := []struct {
+		desc                string
+		parent              string
+		scoreCardDefinition *rpc.ScoreCardDefinition
+		wantNumErr          int
+	}{
+		// No errors
+		{
+			desc:   "simple scorecard definition",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/-/specs/-",
+				},
+				ScorePatterns: []string{
+					"$resource.spec/artifacts/score-lint-error",
+					"$resource.spec/artifacts/score-lang-reuse",
+					"$resource.spec/artifacts/score-security-audit",
+					"$resource.spec/artifacts/score-accuracy",
+				},
+			},
+		},
+		// errors
+		{
+			desc:   "invalid target_resource",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/specs/-", //error
+				},
+				ScorePatterns: []string{
+					"$resource.spec/artifacts/score-lint-error",
+					"$resource.spec/artifacts/score-lang-reuse",
+					"$resource.spec/artifacts/score-security-audit",
+					"$resource.spec/artifacts/score-accuracy",
+				},
+			},
+			wantNumErr: 1,
+		},
+		{
+			desc:   "missing score_patterns",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/-/specs/-",
+				},
+				// error
+			},
+			wantNumErr: 1,
+		},
+		{
+			desc:   "invalid target_resource and missing score_patterns",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/specs/-", //error
+				},
+				//error
+			},
+			wantNumErr: 2,
+		},
+		{
+			desc:   "invalid score_pattern $resource",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/-/specs/-",
+				},
+				ScorePatterns: []string{
+					"$resource.specs/artifacts/score-lint-error", //error
+					"$resource.spec/artifacts/score-lang-reuse",
+					"$resource.spec/artifacts/score-security-audit",
+					"$resource.spec/artifacts/score-accuracy",
+				},
+			},
+			wantNumErr: 1,
+		},
+		{
+			desc:   "invalid score_pattern no artifactID",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/-/specs/-",
+				},
+				ScorePatterns: []string{
+					"$resource.spec/artifacts/-", //error
+				},
+			},
+			wantNumErr: 1,
+		},
+		{
+			desc:   "multiple invalid score_pattern",
+			parent: "projects/demo/locations/global",
+			scoreCardDefinition: &rpc.ScoreCardDefinition{
+				Id:   "test-scorecard-definition",
+				Kind: "ScoreCardDefinition",
+				TargetResource: &rpc.ResourcePattern{
+					Pattern: "apis/-/versions/-/specs/-",
+				},
+				ScorePatterns: []string{
+					"$resource.specs/artifacts/score-lint-error",     //error
+					"$resource.specs/artifacts/score-lang-reuse",     //error
+					"$resource.specs/artifacts/score-security-audit", //error
+					"$resource.specs/artifacts/score-accuracy",       //error
+				},
+			},
+			wantNumErr: 4,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			ctx := context.Background()
+			gotErrs := ValidateScoreCardDefinition(ctx, test.parent, test.scoreCardDefinition)
+			if len(gotErrs) != test.wantNumErr {
+				t.Errorf("ValidateScoreCardDefinition(%s, %v) returned unexpected no. of errors: want %d, got %s", test.parent, test.scoreCardDefinition, test.wantNumErr, gotErrs)
 			}
 		})
 	}
+
 }