Validation function for ScoreDefinition (#536)

* Add artifact upload support for score(card) definitions * Fix rename in tests: 'target' to 'target_resource' * Add license info to new files * Add a validation function for ScoreDefinition * Fix lint errors * Rename 'pattern' to 'artifact' in ScoreFormula * Bug fix and modified tests to check numErrs * Add conditions and tests for missing 'formula' and 'type' * Simplified validateNumberThresholds() * Added a testcase for missing range * Refactored validateNumberThreshold() * refactor error slices in ValidateScoreDefinition() * Fix err message -> range.max to range
apigee · Apr 21, 2022 · b0715f5 · b0715f5
1 parent 6119e13
commit b0715f5
Show file tree

Hide file tree

Showing 8 changed files with 1,652 additions and 27 deletions.
diff --git a/cmd/registry/cmd/upload/artifact.go b/cmd/registry/cmd/upload/artifact.go
@@ -22,6 +22,7 @@ import (
 	"github.com/apigee/registry/cmd/registry/controller"
 	"github.com/apigee/registry/cmd/registry/core"
 	"github.com/apigee/registry/cmd/registry/patch"
+	"github.com/apigee/registry/cmd/registry/scoring"
 	"github.com/apigee/registry/connection"
 	"github.com/apigee/registry/log"
 	"github.com/apigee/registry/rpc"
@@ -97,7 +98,7 @@ func buildArtifact(ctx context.Context, parent string, filename string) (*rpc.Ar
 	case "ScoreCardDefinition", patch.ScoreCardDefinitionMimeType:
 		artifact, err = buildScoreCardDefinitionArtifact(ctx, jsonBytes)
 	case "ScoreDefinition", patch.ScoreDefinitionMimeType:
-		artifact, err = buildScoreDefinitionArtifact(ctx, jsonBytes)
+		artifact, err = buildScoreDefinitionArtifact(ctx, parent, jsonBytes)
 	case "TaxonomyList", patch.TaxonomyListMimeType:
 		artifact, err = buildTaxonomyListArtifact(ctx, jsonBytes)
 	default:
@@ -194,11 +195,19 @@ func buildTaxonomyListArtifact(ctx context.Context, jsonBytes []byte) (*rpc.Arti
 	}, nil
 }
 
-func buildScoreDefinitionArtifact(ctx context.Context, jsonBytes []byte) (*rpc.Artifact, error) {
+func buildScoreDefinitionArtifact(ctx context.Context, parent string, jsonBytes []byte) (*rpc.Artifact, error) {
 	m := &rpc.ScoreDefinition{}
 	if err := protojson.Unmarshal(jsonBytes, m); err != nil {
 		return nil, err
 	}
+	errs := scoring.ValidateScoreDefinition(ctx, parent, m)
+	if count := len(errs); count > 0 {
+		for _, err := range errs {
+			log.FromContext(ctx).WithError(err).Error("ScoreDefinition error")
+		}
+		return nil, fmt.Errorf("ScoreDefinition 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/cmd/upload/artifact_test.go b/cmd/registry/cmd/upload/artifact_test.go
@@ -163,17 +163,17 @@ func TestScoreDefinitionArtifactUpload(t *testing.T) {
 						MaxValue: 100,
 						Thresholds: []*rpc.NumberThreshold{
 							{
-								Severity: rpc.Severity_ALERT,
+								Severity: rpc.Severity_OK,
 								Range: &rpc.NumberThreshold_NumberRange{
-									Min: 60,
-									Max: 100,
+									Min: 0,
+									Max: 59,
 								},
 							},
 							{
-								Severity: rpc.Severity_OK,
+								Severity: rpc.Severity_ALERT,
 								Range: &rpc.NumberThreshold_NumberRange{
-									Min: 0,
-									Max: 59,
+									Min: 60,
+									Max: 100,
 								},
 							},
 						},

diff --git a/cmd/registry/controller/controller.go b/cmd/registry/controller/controller.go
@@ -76,7 +76,7 @@ func processManifestResource(
 	resourcePattern := fmt.Sprintf("projects/%s/locations/global/%s", projectID, generatedResource.Pattern)
 	dependencyMaps := make([]map[string]time.Time, 0, len(generatedResource.Dependencies))
 	for _, dependency := range generatedResource.Dependencies {
-		dMap, err := generateDependencyMap(ctx, client, resourcePattern, dependency, projectID)
+		dMap, err := generateDependencyMap(ctx, client, resourcePattern, dependency)
 		if err != nil {
 			return nil, fmt.Errorf("error while generating dependency map for %v: %s", dependency, err)
 		}
@@ -94,8 +94,7 @@ func generateDependencyMap(
 	ctx context.Context,
 	client connection.Client,
 	resourcePattern string,
-	dependency *rpc.Dependency,
-	projectID string) (map[string]time.Time, error) {
+	dependency *rpc.Dependency) (map[string]time.Time, error) {
 	// Creates a map of the resources to group them into corresponding buckets
 	// of match pattern which store the maxTimestamp
 	// An example entry will look like this:
@@ -114,13 +113,13 @@ func generateDependencyMap(
 	}
 
 	// Extend the dependency pattern if it contains $resource.api like pattern
-	extDependencyQuery, err := patterns.SubstituteReferenceEntity(dependency.Pattern, resourceName, projectID)
+	extDependencyName, err := patterns.SubstituteReferenceEntity(dependency.Pattern, resourceName)
 	if err != nil {
 		return nil, err
 	}
 
 	// Fetch resources using the extDependencyQuery
-	sourceList, err := listResources(ctx, client, extDependencyQuery, dependency.Filter)
+	sourceList, err := listResources(ctx, client, extDependencyName.String(), dependency.Filter)
 	if err != nil {
 		return nil, err
 	}
@@ -139,7 +138,7 @@ func generateDependencyMap(
 	}
 
 	if len(sourceMap) == 0 {
-		return nil, fmt.Errorf("no resources found for pattern: %s, filer: %s", extDependencyQuery, dependency.Filter)
+		return nil, fmt.Errorf("no resources found for pattern: %s, filer: %s", extDependencyName.String(), dependency.Filter)
 	}
 
 	return sourceMap, nil

diff --git a/cmd/registry/patterns/parser.go b/cmd/registry/patterns/parser.go
@@ -61,7 +61,7 @@ func ParseResourcePattern(resourcePattern string) (ResourceName, error) {
 	return nil, fmt.Errorf("invalid resourcePattern: %s", resourcePattern)
 }
 
-func SubstituteReferenceEntity(resourcePattern string, referred ResourceName, projectID string) (string, error) {
+func SubstituteReferenceEntity(resourcePattern string, referred ResourceName) (ResourceName, error) {
 	// Extends the resource pattern by replacing references to $resource
 	// Example:
 	// referred: "projects/demo/locations/global/apis/-/versions/-/specs/-/artifacts/-"
@@ -74,21 +74,29 @@ func SubstituteReferenceEntity(resourcePattern string, referred ResourceName, pr
 
 	entity, entityType, err := GetReferenceEntityType(resourcePattern)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
 	// no $resource reference present
 	// simply prepend the projectname and return full resource name
 	if entityType == "default" {
-		return fmt.Sprintf("projects/%s/locations/global/%s", projectID, resourcePattern), nil
+		resourceName, err := ParseResourcePattern(fmt.Sprintf("%s/locations/global/%s", referred.Project(), resourcePattern))
+		if err != nil {
+			return nil, err
+		}
+		return resourceName, nil
 	}
 
 	entityVal, err := GetReferenceEntityValue(resourcePattern, referred)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
 
-	return strings.Replace(resourcePattern, entity, entityVal, 1), nil
+	extendedName, err := ParseResourcePattern(strings.Replace(resourcePattern, entity, entityVal, 1))
+	if err != nil {
+		return nil, err
+	}
+	return extendedName, nil
 
 }
 

diff --git a/cmd/registry/patterns/parser_test.go b/cmd/registry/patterns/parser_test.go
@@ -33,7 +33,7 @@ func generateArtifact(t *testing.T, artifactName string) names.Artifact {
 	return artifact
 }
 
-func TestSubstitueReferenceEntity(t *testing.T) {
+func TestSubstituteReferenceEntity(t *testing.T) {
 	tests := []struct {
 		desc              string
 		resourcePattern   string
@@ -72,25 +72,24 @@ func TestSubstitueReferenceEntity(t *testing.T) {
 		},
 	}
 
-	const projectID = "demo"
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
 			resourceName, err := ParseResourcePattern(test.resourcePattern)
 			if err != nil {
 				t.Fatalf("Error in parsing, %s", err)
 			}
-			got, err := SubstituteReferenceEntity(test.dependencyPattern, resourceName, projectID)
+			got, err := SubstituteReferenceEntity(test.dependencyPattern, resourceName)
 			if err != nil {
 				t.Errorf("SubstituteReferenceEntity returned unexpected error: %s", err)
 			}
-			if got != test.want {
+			if got.String() != test.want {
 				t.Errorf("SubstituteReferenceEntity returned unexpected value want: %q got:%q", test.want, got)
 			}
 		})
 	}
 }
 
-func TestSubstitueReferenceEntityError(t *testing.T) {
+func TestSubstituteReferenceEntityError(t *testing.T) {
 	tests := []struct {
 		desc              string
 		resourcePattern   string
@@ -108,16 +107,15 @@ func TestSubstitueReferenceEntityError(t *testing.T) {
 		},
 	}
 
-	const projectID = "demo"
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
 			resourceName, err := ParseResourcePattern(test.resourcePattern)
 			if err != nil {
 				t.Fatalf("Error in parsing, %s", err)
 			}
-			got, err := SubstituteReferenceEntity(test.dependencyPattern, resourceName, projectID)
+			got, err := SubstituteReferenceEntity(test.dependencyPattern, resourceName)
 			if err == nil {
-				t.Errorf("expected SubstituteReferenceEntity to return error, got: %q", got)
+				t.Errorf("expected SubstituteReferenceEntity to return error, got: %q", got.String())
 			}
 		})
 	}

diff --git a/cmd/registry/patterns/resources.go b/cmd/registry/patterns/resources.go
@@ -29,6 +29,7 @@ type ResourceName interface {
 	Spec() string
 	Version() string
 	Api() string
+	Project() string
 	String() string
 	ParentName() ResourceName
 }
@@ -53,6 +54,10 @@ func (s SpecName) Api() string {
 	return s.Name.Api().String()
 }
 
+func (s SpecName) Project() string {
+	return s.Name.Project().String()
+}
+
 func (s SpecName) String() string {
 	return s.Name.String()
 }
@@ -91,6 +96,10 @@ func (v VersionName) Api() string {
 	return v.Name.Api().String()
 }
 
+func (v VersionName) Project() string {
+	return v.Name.Project().String()
+}
+
 func (v VersionName) String() string {
 	return v.Name.String()
 }
@@ -130,6 +139,10 @@ func (a ApiName) Api() string {
 	return a.Name.String()
 }
 
+func (a ApiName) Project() string {
+	return a.Name.Project().String()
+}
+
 func (a ApiName) String() string {
 	return a.Name.String()
 }
@@ -173,6 +186,10 @@ func (p ProjectName) Api() string {
 	return ""
 }
 
+func (p ProjectName) Project() string {
+	return p.Name.String()
+}
+
 func (p ProjectName) String() string {
 	return p.Name.String()
 }
@@ -238,6 +255,20 @@ func (ar ArtifactName) Api() string {
 	return ""
 }
 
+func (ar ArtifactName) Project() string {
+	projectPattern := names.Project{
+		ProjectID: ar.Name.ProjectID(),
+	}
+	// Validate the generated name
+	if _, err := names.ParseProject(projectPattern.String()); err == nil {
+		return projectPattern.String()
+	} else if _, err := names.ParseProjectCollection(projectPattern.String()); err == nil {
+		return projectPattern.String()
+	}
+
+	return ""
+}
+
 func (ar ArtifactName) String() string {
 	return ar.Name.String()
 }