Implement the behavior of "refresh" field in manifest definition (#607)

* Implement the behavior of "refresh" field in manifest definition * Fix test failures * Address feedback comments
apigee · Jun 16, 2022 · 0961ea5 · 0961ea5
1 parent 257aefd
commit 0961ea5
Show file tree

Hide file tree

Showing 10 changed files with 335 additions and 68 deletions.
diff --git a/cmd/registry/cmd/apply/testdata/sample/artifacts/manifest.yaml b/cmd/registry/cmd/apply/testdata/sample/artifacts/manifest.yaml
@@ -13,6 +13,7 @@ data:
         - pattern: $resource.spec
           filter: mime_type.contains('openapi')
       action: registry compute lint $resource.spec --linter spectral
+      refresh: null
     - pattern: apis/-/versions/-/specs/-/artifacts/lintstats-spectral
       filter: ""
       receipt: false
@@ -22,17 +23,20 @@ data:
         - pattern: $resource.spec/artifacts/complexity
           filter: ""
       action: registry compute lintstats $resource.spec --linter spectral
+      refresh: null
     - pattern: apis/-/versions/-/specs/-/artifacts/vocabulary
       filter: ""
       receipt: false
       dependencies:
         - pattern: $resource.spec
           filter: ""
       action: registry compute vocabulary $resource.spec
+      refresh: null
     - pattern: apis/-/versions/-/specs/-/artifacts/complexity
       filter: ""
       receipt: false
       dependencies:
         - pattern: $resource.spec
           filter: ""
       action: registry compute complexity $resource.spec
+      refresh: null
diff --git a/cmd/registry/cmd/upload/artifact_test.go b/cmd/registry/cmd/upload/artifact_test.go
@@ -221,7 +221,7 @@ func TestScoreDefinitionArtifactUpload(t *testing.T) {
 						Artifact: &rpc.ResourcePattern{
 							Pattern: "$resource.spec/artifacts/conformance-apihub-styleguide",
 						},
-						ScoreExpression: "sum(guidelineReportGroups[2].guidelineReports.map(r, has(r.ruleReportGroups[1].ruleReports) ? size(r.ruleReportGroups[1].ruleReports) : 0))",
+						ScoreExpression: "has(guidelineReportGroups[2].guidelineReports) ? sum(guidelineReportGroups[2].guidelineReports.map(r, has(r.ruleReportGroups[1].ruleReports) ? size(r.ruleReportGroups[1].ruleReports) : 0)) : 0",
 					},
 				},
 				Type: &rpc.ScoreDefinition_Integer{

diff --git a/cmd/registry/cmd/upload/testdata/score-definition.yaml b/cmd/registry/cmd/upload/testdata/score-definition.yaml
@@ -20,7 +20,7 @@ score_formula:
   artifact:
     pattern: "$resource.spec/artifacts/conformance-apihub-styleguide"
   # number of errors from conformance report
-  score_expression: "sum(guidelineReportGroups[2].guidelineReports.map(r, has(r.ruleReportGroups[1].ruleReports) ? size(r.ruleReportGroups[1].ruleReports) : 0))"
+  score_expression: "has(guidelineReportGroups[2].guidelineReports) ? sum(guidelineReportGroups[2].guidelineReports.map(r, has(r.ruleReportGroups[1].ruleReports) ? size(r.ruleReportGroups[1].ruleReports) : 0)) : 0"
 integer: 
   min_value: 0
   max_value: 100

diff --git a/cmd/registry/controller/controller.go b/cmd/registry/controller/controller.go
@@ -39,7 +39,7 @@ func ProcessManifest(
 	manifest *rpc.Manifest) []*Action {
 	var actions []*Action
 	//Check for errors in manifest
-	errs := ValidateManifest(fmt.Sprintf("projects/%s", projectID), manifest)
+	errs := ValidateManifest(fmt.Sprintf("projects/%s/locations/global", projectID), manifest)
 	if len(errs) > 0 {
 		for _, err := range errs {
 			log.FromContext(ctx).WithError(err).Debugf("Error in manifest")
@@ -71,8 +71,8 @@ func processManifestResource(
 	client connection.Client,
 	projectID string,
 	generatedResource *rpc.GeneratedResource) ([]*Action, error) {
-	// Generate dependency map
 	resourcePattern := fmt.Sprintf("projects/%s/locations/global/%s", projectID, generatedResource.Pattern)
+	// Generate dependency map
 	dependencyMaps := make([]map[string]time.Time, 0, len(generatedResource.Dependencies))
 	for _, dependency := range generatedResource.Dependencies {
 		dMap, err := generateDependencyMap(ctx, client, resourcePattern, dependency)
@@ -152,20 +152,17 @@ func generateActions(
 	generatedResource *rpc.GeneratedResource) []*Action {
 	actions := make([]*Action, 0)
 
-	// Calculate actions only if dependencies are non-empty
-	if len(dependencyMaps) > 0 {
-		updateActions, visited, err := generateUpdateActions(ctx, client, resourcePattern, filter, dependencyMaps, generatedResource)
-		if err != nil {
-			log.Errorf(ctx, "Error while generating UpdateActions: %s", err)
-		}
-		actions = append(actions, updateActions...)
+	updateActions, visited, err := generateUpdateActions(ctx, client, resourcePattern, filter, dependencyMaps, generatedResource)
+	if err != nil {
+		log.Errorf(ctx, "Error while generating UpdateActions: %s", err)
+	}
+	actions = append(actions, updateActions...)
 
-		createActions, err := generateCreateActions(ctx, client, resourcePattern, dependencyMaps, generatedResource, visited)
-		if err != nil {
-			log.Errorf(ctx, "Error while generating CreateActions: %s", err)
-		}
-		actions = append(actions, createActions...)
+	createActions, err := generateCreateActions(ctx, client, resourcePattern, dependencyMaps, generatedResource, visited)
+	if err != nil {
+		log.Errorf(ctx, "Error while generating CreateActions: %s", err)
 	}
+	actions = append(actions, createActions...)
 
 	return actions
 }
@@ -331,6 +328,11 @@ func needsUpdate(
 	targetResourceTime time.Time,
 	dependencyMaps []map[string]time.Time,
 	generatedResource *rpc.GeneratedResource) (bool, error) {
+	// Check "refresh" first to decide whether to take action or not.
+	if generatedResource.Refresh != nil && targetResourceTime.Add(generatedResource.Refresh.AsDuration()).Before(time.Now()) {
+		return true, nil
+	}
+	// Check for dependencies otherwise
 	for i, dependency := range generatedResource.Dependencies {
 		dMap := dependencyMaps[i]
 		// Get the entity to look for in dependencyMap
@@ -357,6 +359,11 @@ func needsCreate(
 	targetResourceName patterns.ResourceName,
 	dependencyMaps []map[string]time.Time,
 	generatedResource *rpc.GeneratedResource) (bool, error) {
+	// Take action if "refresh" is set and > 0
+	if generatedResource.Refresh != nil && generatedResource.Refresh.AsDuration().Seconds() > 0 {
+		return true, nil
+	}
+	// Check for dependencies otherwise
 	for i, dependency := range generatedResource.Dependencies {
 		dMap := dependencyMaps[i]
 		// Get the entity to look for in dependencyMap

diff --git a/cmd/registry/controller/controller_test.go b/cmd/registry/controller/controller_test.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/apigee/registry/cmd/registry/core"
 	"github.com/apigee/registry/connection"
@@ -28,6 +29,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/durationpb"
 )
 
 const gzipOpenAPIv3 = "application/x.openapi+gzip;version=3.0.0"
@@ -959,3 +961,159 @@ func TestMultipleEntitiesArtifacts(t *testing.T) {
 		})
 	}
 }
+
+func TestRefreshArtifacts(t *testing.T) {
+	tests := []struct {
+		desc string
+		seed []seeder.RegistryResource
+		want []*Action
+		wait time.Duration
+	}{
+		{
+			desc: "non-existing artifacts",
+			seed: []seeder.RegistryResource{
+				&rpc.ApiSpec{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
+				},
+				&rpc.ApiSpec{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml",
+				},
+				&rpc.ApiSpec{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml",
+				},
+			},
+			want: []*Action{
+				{
+					Command:           "registry compute score projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
+					GeneratedResource: "projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/score-receipt",
+					RequiresReceipt:   true,
+				},
+				{
+					Command:           "registry compute score projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml",
+					GeneratedResource: "projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml/artifacts/score-receipt",
+					RequiresReceipt:   true,
+				},
+				{
+					Command:           "registry compute score projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml",
+					GeneratedResource: "projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml/artifacts/score-receipt",
+					RequiresReceipt:   true,
+				},
+			},
+		},
+		{
+			desc: "existing valid artifacts",
+			seed: []seeder.RegistryResource{
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/score-receipt",
+				},
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml/artifacts/score-receipt",
+				},
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml/artifacts/score-receipt",
+				},
+			},
+			want: nil,
+		},
+		{
+			desc: "existing invalid artifacts",
+			seed: []seeder.RegistryResource{
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/score-receipt",
+				},
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml/artifacts/score-receipt",
+				},
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml/artifacts/score-receipt",
+				},
+			},
+			want: []*Action{
+				{
+					Command:           "registry compute score projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
+					GeneratedResource: "projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/score-receipt",
+					RequiresReceipt:   true,
+				},
+				{
+					Command:           "registry compute score projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml",
+					GeneratedResource: "projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml/artifacts/score-receipt",
+					RequiresReceipt:   true,
+				},
+				{
+					Command:           "registry compute score projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml",
+					GeneratedResource: "projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml/artifacts/score-receipt",
+					RequiresReceipt:   true,
+				},
+			},
+			wait: 5,
+		},
+		{
+			desc: "existing valid artifacts",
+			seed: []seeder.RegistryResource{
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/score-receipt",
+				},
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.0.1/specs/openapi.yaml/artifacts/score-receipt",
+				},
+				&rpc.Artifact{
+					Name: "projects/controller-test/locations/global/apis/petstore/versions/1.1.0/specs/openapi.yaml/artifacts/score-receipt",
+				},
+			},
+			want: nil,
+		},
+	}
+
+	const projectID = "controller-test"
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			ctx := context.Background()
+			registryClient, err := connection.NewClient(ctx)
+			if err != nil {
+				t.Fatalf("Failed to create client: %+v", err)
+			}
+			t.Cleanup(func() { registryClient.Close() })
+
+			adminClient, err := connection.NewAdminClient(ctx)
+			if err != nil {
+				t.Fatalf("Failed to create client: %+v", err)
+			}
+			t.Cleanup(func() { adminClient.Close() })
+
+			deleteProject(ctx, adminClient, t, "controller-test")
+			t.Cleanup(func() { deleteProject(ctx, adminClient, t, "controller-test") })
+
+			client := seeder.Client{
+				RegistryClient: registryClient,
+				AdminClient:    adminClient,
+			}
+
+			if err := seeder.SeedRegistry(ctx, client, test.seed...); err != nil {
+				t.Fatalf("Setup: failed to seed registry: %s", err)
+			}
+
+			time.Sleep(test.wait * time.Second)
+
+			manifest := &rpc.Manifest{
+				Id: "controller-test",
+				GeneratedResources: []*rpc.GeneratedResource{
+					{
+						Pattern: "apis/-/versions/-/specs/-/artifacts/score-receipt",
+						Receipt: true,
+						Refresh: &durationpb.Duration{
+							Seconds: 2,
+						},
+						Action: "registry compute score $resource.spec",
+					},
+				},
+			}
+			actions := ProcessManifest(ctx, registryClient, projectID, manifest)
+
+			if diff := cmp.Diff(test.want, actions, sortActions); diff != "" {
+				t.Errorf("ProcessManifest(%+v) returned unexpected diff (-want +got):\n%s", manifest, diff)
+			}
+
+			deleteProject(ctx, adminClient, t, "controller-test")
+		})
+	}
+}
diff --git a/cmd/registry/controller/parser.go b/cmd/registry/controller/parser.go
@@ -83,6 +83,16 @@ func validateGeneratedResourceEntry(parent string, generatedResource *rpc.Genera
 		}
 	}
 
+	// Check that either "dependencies" or "refresh" is set and "refresh > 0"
+	if len(generatedResource.Dependencies) == 0 && generatedResource.Refresh == nil {
+		errs = append(errs, fmt.Errorf("either 'dependencies' or 'refresh' must be set for generated resource: %v", generatedResource))
+	}
+
+	// Check that "refresh" > 0
+	if generatedResource.Refresh != nil && generatedResource.Refresh.AsDuration().Seconds() == 0 {
+		errs = append(errs, fmt.Errorf("'refresh' must be >0 for generated resource: %v", generatedResource))
+	}
+
 	//Validate that all the action References are valid
 	references, err := getReferencesFromAction(generatedResource.Action)
 	if err != nil {

diff --git a/cmd/registry/controller/parser_test.go b/cmd/registry/controller/parser_test.go
@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	"github.com/apigee/registry/rpc"
+	"google.golang.org/protobuf/types/known/durationpb"
 )
 
 func TestGenerateCommand(t *testing.T) {
@@ -173,6 +174,28 @@ func TestValidateGeneratedResourceEntry(t *testing.T) {
 				Action: "registry generate summary $resource.version",
 			},
 		},
+		{
+			desc: "refresh field with missing dependencies",
+			generatedResource: &rpc.GeneratedResource{
+				Pattern: "apis/-/versions/-/artifacts/score-receipt",
+				Receipt: true,
+				Refresh: &durationpb.Duration{
+					Seconds: 5,
+				},
+				Action: "registry generate summary $resource.version",
+			},
+		},
+		{
+			desc: "refresh field (in nanoseconds)",
+			generatedResource: &rpc.GeneratedResource{
+				Pattern: "apis/-/versions/-/artifacts/score-receipt",
+				Receipt: true,
+				Refresh: &durationpb.Duration{
+					Nanos: 5,
+				},
+				Action: "registry generate summary $resource.version",
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
@@ -247,6 +270,25 @@ func TestValidateGeneratedResourceEntryError(t *testing.T) {
 				Action: "registry generate summary $resource.spec", // Correct pattern: registry generate summary $resource.version
 			},
 		},
+		{
+			desc: "refresh field equal to 0",
+			generatedResource: &rpc.GeneratedResource{
+				Pattern: "apis/-/versions/-/artifacts/score-receipt",
+				Receipt: true,
+				Refresh: &durationpb.Duration{
+					Seconds: 0,
+				},
+				Action: "registry generate summary $resource.version",
+			},
+		},
+		{
+			desc: "missing dependencies and refresh",
+			generatedResource: &rpc.GeneratedResource{
+				Pattern: "apis/-/versions/-/artifacts/score-receipt",
+				Receipt: true,
+				Action:  "registry generate summary $resource.version",
+			},
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {