Repository: apigee/registry (go)

Issue:
Support nested queries of resource revisions
Right now you can query lists of nested resources using `-`s in the parent name. For example, you can get a list of all versions from all APIs in the system by setting `parent` to `projects/my-project/locations/global/apis/-` in the `ListApiVersionRequest`.

It would be nice to have the ability to query a list of all `ApiSpec` revisions in a similar way to prevent the need for multiple nested calls to get a full revision history of an API.

Pull request:
Support listing revisions across collections
Fixes #500 

Patch:
diff --git a/server/registry/internal/storage/list.go b/server/registry/internal/storage/list.go
index e3f35d5..a9134e4 100644
--- a/server/registry/internal/storage/list.go
+++ b/server/registry/internal/storage/list.go
@@ -515,20 +515,49 @@ func (c *Client) ListSpecRevisions(ctx context.Context, parent names.Spec, opts
 		return SpecList{}, status.Errorf(codes.InvalidArgument, "invalid page token %q: %s", opts.Token, err.Error())
 	}
 
+	// Check existence of the deepest fully specified resource in the parent name.
+	if parent.ProjectID != "-" && parent.ApiID != "-" && parent.VersionID != "-" && parent.SpecID != "-" {
+		if _, err := c.GetSpec(ctx, parent); err != nil {
+			return SpecList{}, err
+		}
+	} else if parent.ProjectID != "-" && parent.ApiID != "-" && parent.VersionID != "-" && parent.SpecID == "-" {
+		if _, err := c.GetVersion(ctx, parent.Version()); err != nil {
+			return SpecList{}, err
+		}
+	} else if parent.ProjectID != "-" && parent.ApiID != "-" && parent.VersionID == "-" && parent.SpecID == "-" {
+		if _, err := c.GetApi(ctx, parent.Api()); err != nil {
+			return SpecList{}, err
+		}
+	} else if parent.ProjectID != "-" && parent.ApiID == "-" && parent.VersionID == "-" && parent.SpecID == "-" {
+		if _, err := c.GetProject(ctx, parent.Project()); err != nil {
+			return SpecList{}, err
+		}
+	}
+
+	op := c.db.
+		Order("revision_create_time desc").
+		Offset(token.Offset).
+		Limit(int(opts.Size) + 1)
+
+	if id := parent.ProjectID; id != "-" {
+		op = op.Where("project_id = ?", id)
+	}
+	if id := parent.ApiID; id != "-" {
+		op = op.Where("api_id = ?", id)
+	}
+	if id := parent.VersionID; id != "-" {
+		op = op.Where("version_id = ?", id)
+	}
+	if id := parent.SpecID; id != "-" {
+		op = op.Where("spec_id = ?", id)
+	}
+
 	response := SpecList{
 		Specs: make([]models.Spec, 0, opts.Size),
 	}
 
 	lock()
-	err = c.db.
-		Where("project_id = ?", parent.ProjectID).
-		Where("api_id = ?", parent.ApiID).
-		Where("version_id = ?", parent.VersionID).
-		Where("spec_id = ?", parent.SpecID).
-		Order("revision_create_time desc").
-		Offset(token.Offset).
-		Limit(int(opts.Size) + 1).
-		Find(&response.Specs).Error
+	err = op.Find(&response.Specs).Error
 	unlock()
 
 	if err != nil {
@@ -699,19 +728,41 @@ func (c *Client) ListDeploymentRevisions(ctx context.Context, parent names.Deplo
 		return DeploymentList{}, status.Errorf(codes.InvalidArgument, "invalid page token %q: %s", opts.Token, err.Error())
 	}
 
+	if parent.ProjectID != "-" && parent.ApiID != "-" && parent.DeploymentID != "-" {
+		if _, err := c.GetDeployment(ctx, parent); err != nil {
+			return DeploymentList{}, err
+		}
+	} else if parent.ProjectID != "-" && parent.ApiID != "-" && parent.DeploymentID == "-" {
+		if _, err := c.GetApi(ctx, parent.Api()); err != nil {
+			return DeploymentList{}, err
+		}
+	} else if parent.ProjectID != "-" && parent.ApiID == "-" && parent.DeploymentID == "-" {
+		if _, err := c.GetProject(ctx, parent.Project()); err != nil {
+			return DeploymentList{}, err
+		}
+	}
+
+	op := c.db.
+		Order("revision_create_time desc").
+		Offset(token.Offset).
+		Limit(int(opts.Size) + 1)
+
+	if id := parent.ProjectID; id != "-" {
+		op = op.Where("project_id = ?", id)
+	}
+	if id := parent.ApiID; id != "-" {
+		op = op.Where("api_id = ?", id)
+	}
+	if id := parent.DeploymentID; id != "-" {
+		op = op.Where("deployment_id = ?", id)
+	}
+
 	response := DeploymentList{
 		Deployments: make([]models.Deployment, 0, opts.Size),
 	}
 
 	lock()
-	err = c.db.
-		Where("project_id = ?", parent.ProjectID).
-		Where("api_id = ?", parent.ApiID).
-		Where("deployment_id = ?", parent.DeploymentID).
-		Order("revision_create_time desc").
-		Offset(token.Offset).
-		Limit(int(opts.Size) + 1).
-		Find(&response.Deployments).Error
+	err = op.Find(&response.Deployments).Error
 	unlock()
 
 	if err != nil {


Test patch:
diff --git a/server/registry/actions_deployment_revisions_test.go b/server/registry/actions_deployment_revisions_test.go
index b2a3d5f..1f2d83f 100644
--- a/server/registry/actions_deployment_revisions_test.go
+++ b/server/registry/actions_deployment_revisions_test.go
@@ -17,6 +17,7 @@ package registry
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/apigee/registry/rpc"
@@ -285,6 +286,160 @@ func TestDeleteApiDeploymentRevision(t *testing.T) {
 }
 
 func TestListApiDeploymentRevisions(t *testing.T) {
+	tests := []struct {
+		desc      string
+		seed      []*rpc.ApiDeployment
+		req       *rpc.ListApiDeploymentRevisionsRequest
+		want      *rpc.ListApiDeploymentRevisionsResponse
+		wantToken bool
+	}{
+		{
+			desc: "single deployment",
+			seed: []*rpc.ApiDeployment{
+				{
+					Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+				},
+				{
+					Name:        "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+					EndpointUri: "updated",
+				},
+				{
+					Name: "projects/my-project/locations/global/apis/my-api/deployments/other-dep",
+				},
+			},
+			req: &rpc.ListApiDeploymentRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+			},
+			want: &rpc.ListApiDeploymentRevisionsResponse{
+				ApiDeployments: []*rpc.ApiDeployment{
+					{
+						Name:        "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+						EndpointUri: "updated",
+					},
+					{
+						Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+					},
+				},
+			},
+		},
+		{
+			desc: "across multiple deployments",
+			seed: []*rpc.ApiDeployment{
+				{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep"},
+				{Name: "projects/my-project/locations/global/apis/my-api/deployments/other-dep"},
+			},
+			req: &rpc.ListApiDeploymentRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/my-api/deployments/-",
+			},
+			want: &rpc.ListApiDeploymentRevisionsResponse{
+				ApiDeployments: []*rpc.ApiDeployment{
+					{Name: "projects/my-project/locations/global/apis/my-api/deployments/other-dep"},
+					{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep"},
+				},
+			},
+		},
+		{
+			desc: "across multiple apis",
+			seed: []*rpc.ApiDeployment{
+				{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep"},
+				{Name: "projects/my-project/locations/global/apis/other-api/deployments/my-dep"},
+			},
+			req: &rpc.ListApiDeploymentRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/-/deployments/my-dep",
+			},
+			want: &rpc.ListApiDeploymentRevisionsResponse{
+				ApiDeployments: []*rpc.ApiDeployment{
+					{Name: "projects/my-project/locations/global/apis/other-api/deployments/my-dep"},
+					{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep"},
+				},
+			},
+		},
+		{
+			desc: "across multiple projects",
+			seed: []*rpc.ApiDeployment{
+				{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep"},
+				{Name: "projects/other-project/locations/global/apis/my-api/deployments/my-dep"},
+			},
+			req: &rpc.ListApiDeploymentRevisionsRequest{
+				Name: "projects/-/locations/global/apis/my-api/deployments/my-dep",
+			},
+			want: &rpc.ListApiDeploymentRevisionsResponse{
+				ApiDeployments: []*rpc.ApiDeployment{
+					{Name: "projects/other-project/locations/global/apis/my-api/deployments/my-dep"},
+					{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep"},
+				},
+			},
+		},
+		{
+			desc: "custom page size",
+			seed: []*rpc.ApiDeployment{
+				{
+					Name: "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+				},
+				{
+					Name:        "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+					EndpointUri: "updated",
+				},
+			},
+			req: &rpc.ListApiDeploymentRevisionsRequest{
+				Name:     "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+				PageSize: 1,
+			},
+			want: &rpc.ListApiDeploymentRevisionsResponse{
+				ApiDeployments: []*rpc.ApiDeployment{
+					{
+						Name:        "projects/my-project/locations/global/apis/my-api/deployments/my-dep",
+						EndpointUri: "updated",
+					},
+				},
+			},
+			wantToken: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			ctx := context.Background()
+			server := defaultTestServer(t)
+			if err := seeder.SeedDeployments(ctx, server, test.seed...); err != nil {
+				t.Fatalf("Setup/Seeding: Failed to seed registry: %s", err)
+			}
+
+			got, err := server.ListApiDeploymentRevisions(ctx, test.req)
+			if err != nil {
+				t.Fatalf("ListApiDeploymentRevisions(%+v) returned error: %s", test.req, err)
+			}
+
+			opts := cmp.Options{
+				protocmp.Transform(),
+				protocmp.IgnoreFields(new(rpc.ListApiDeploymentRevisionsResponse), "next_page_token"),
+				protocmp.IgnoreFields(new(rpc.ApiDeployment), "name", "revision_id", "create_time", "revision_create_time", "revision_update_time"),
+			}
+
+			if !cmp.Equal(test.want, got, opts) {
+				t.Errorf("ListApiDeploymentRevisions(%+v) returned unexpected diff (-want +got):\n%s", test.req, cmp.Diff(test.want, got, opts))
+			}
+
+			if test.wantToken && got.NextPageToken == "" {
+				t.Errorf("ListApiDeploymentRevisions(%+v) returned empty next_page_token, expected non-empty next_page_token", test.req)
+			} else if !test.wantToken && got.NextPageToken != "" {
+				t.Errorf("ListApiDeploymentRevisions(%+v) returned non-empty next_page_token, expected empty next_page_token: %s", test.req, got.GetNextPageToken())
+			}
+
+			if len(got.ApiDeployments) != len(test.want.ApiDeployments) {
+				t.Fatalf("ListApiDeploymentRevisions(%+v) returned unexpected number of revisions: got %d, want %d", test.req, len(got.ApiDeployments), len(test.want.ApiDeployments))
+			}
+
+			for i, got := range got.ApiDeployments {
+				if want := test.want.ApiDeployments[i]; !strings.HasPrefix(got.GetName(), want.GetName()) {
+					t.Errorf("ListApiDeploymentRevisions(%+v) returned unexpected revision: got %q, want %q", test.req, got.GetName(), want.GetName())
+				}
+			}
+		})
+	}
+}
+
+func TestListApiDeploymentRevisionsSequence(t *testing.T) {
 	ctx := context.Background()
 	server := defaultTestServer(t)
 	if err := seeder.SeedApis(ctx, server, &rpc.Api{Name: "projects/my-project/locations/global/apis/my-api"}); err != nil {
diff --git a/server/registry/actions_spec_revisions_test.go b/server/registry/actions_spec_revisions_test.go
index 5c6e500..6e7669d 100644
--- a/server/registry/actions_spec_revisions_test.go
+++ b/server/registry/actions_spec_revisions_test.go
@@ -17,6 +17,7 @@ package registry
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/apigee/registry/rpc"
@@ -281,6 +282,178 @@ func TestDeleteApiSpecRevision(t *testing.T) {
 }
 
 func TestListApiSpecRevisions(t *testing.T) {
+	tests := []struct {
+		desc      string
+		seed      []*rpc.ApiSpec
+		req       *rpc.ListApiSpecRevisionsRequest
+		want      *rpc.ListApiSpecRevisionsResponse
+		wantToken bool
+	}{
+		{
+			desc: "single spec",
+			seed: []*rpc.ApiSpec{
+				{
+					Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+				},
+				{
+					Name:     "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+					Contents: specContents,
+				},
+				{
+					Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/other-spec",
+				},
+			},
+			req: &rpc.ListApiSpecRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+			},
+			want: &rpc.ListApiSpecRevisionsResponse{
+				ApiSpecs: []*rpc.ApiSpec{
+					{
+						Name:      "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+						Hash:      sha256hash(specContents),
+						SizeBytes: int32(len(specContents)),
+					},
+					{
+						Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+					},
+				},
+			},
+		},
+		{
+			desc: "across multiple specs",
+			seed: []*rpc.ApiSpec{
+				{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/other-spec"},
+			},
+			req: &rpc.ListApiSpecRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/-",
+			},
+			want: &rpc.ListApiSpecRevisionsResponse{
+				ApiSpecs: []*rpc.ApiSpec{
+					{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/other-spec"},
+					{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				},
+			},
+		},
+		{
+			desc: "across multiple versions",
+			seed: []*rpc.ApiSpec{
+				{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				{Name: "projects/my-project/locations/global/apis/my-api/versions/v2/specs/my-spec"},
+			},
+			req: &rpc.ListApiSpecRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/my-api/versions/-/specs/my-spec",
+			},
+			want: &rpc.ListApiSpecRevisionsResponse{
+				ApiSpecs: []*rpc.ApiSpec{
+					{Name: "projects/my-project/locations/global/apis/my-api/versions/v2/specs/my-spec"},
+					{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				},
+			},
+		},
+		{
+			desc: "across multiple apis",
+			seed: []*rpc.ApiSpec{
+				{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				{Name: "projects/my-project/locations/global/apis/other-api/versions/v1/specs/my-spec"},
+			},
+			req: &rpc.ListApiSpecRevisionsRequest{
+				Name: "projects/my-project/locations/global/apis/-/versions/v1/specs/my-spec",
+			},
+			want: &rpc.ListApiSpecRevisionsResponse{
+				ApiSpecs: []*rpc.ApiSpec{
+					{Name: "projects/my-project/locations/global/apis/other-api/versions/v1/specs/my-spec"},
+					{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				},
+			},
+		},
+		{
+			desc: "across multiple projects",
+			seed: []*rpc.ApiSpec{
+				{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				{Name: "projects/other-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+			},
+			req: &rpc.ListApiSpecRevisionsRequest{
+				Name: "projects/-/locations/global/apis/my-api/versions/v1/specs/my-spec",
+			},
+			want: &rpc.ListApiSpecRevisionsResponse{
+				ApiSpecs: []*rpc.ApiSpec{
+					{Name: "projects/other-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+					{Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec"},
+				},
+			},
+		},
+		{
+			desc: "custom page size",
+			seed: []*rpc.ApiSpec{
+				{
+					Name: "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+				},
+				{
+					Name:     "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+					Contents: specContents,
+				},
+			},
+			req: &rpc.ListApiSpecRevisionsRequest{
+				Name:     "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+				PageSize: 1,
+			},
+			want: &rpc.ListApiSpecRevisionsResponse{
+				ApiSpecs: []*rpc.ApiSpec{
+					{
+						Name:      "projects/my-project/locations/global/apis/my-api/versions/v1/specs/my-spec",
+						Hash:      sha256hash(specContents),
+						SizeBytes: int32(len(specContents)),
+					},
+				},
+			},
+			wantToken: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			ctx := context.Background()
+			server := defaultTestServer(t)
+			if err := seeder.SeedSpecs(ctx, server, test.seed...); err != nil {
+				t.Fatalf("Setup/Seeding: Failed to seed registry: %s", err)
+			}
+
+			got, err := server.ListApiSpecRevisions(ctx, test.req)
+			if err != nil {
+				t.Fatalf("ListApiSpecRevisions(%+v) returned error: %s", test.req, err)
+			}
+
+			opts := cmp.Options{
+				protocmp.Transform(),
+				protocmp.IgnoreFields(new(rpc.ListApiSpecRevisionsResponse), "next_page_token"),
+				protocmp.IgnoreFields(new(rpc.ApiSpec), "name", "revision_id", "create_time", "revision_create_time", "revision_update_time"),
+			}
+
+			if !cmp.Equal(test.want, got, opts) {
+				t.Errorf("ListApiSpecRevisions(%+v) returned unexpected diff (-want +got):\n%s", test.req, cmp.Diff(test.want, got, opts))
+			}
+
+			if test.wantToken && got.NextPageToken == "" {
+				t.Errorf("ListApiSpecRevisions(%+v) returned empty next_page_token, expected non-empty next_page_token", test.req)
+			} else if !test.wantToken && got.NextPageToken != "" {
+				t.Errorf("ListApiSpecRevisions(%+v) returned non-empty next_page_token, expected empty next_page_token: %s", test.req, got.GetNextPageToken())
+			}
+
+			if len(got.ApiSpecs) != len(test.want.ApiSpecs) {
+				t.Fatalf("ListApiSpecRevisions(%+v) returned unexpected number of revisions: got %d, want %d", test.req, len(got.ApiSpecs), len(test.want.ApiSpecs))
+			}
+
+			for i, got := range got.ApiSpecs {
+				if want := test.want.ApiSpecs[i]; !strings.HasPrefix(got.GetName(), want.GetName()) {
+					t.Errorf("ListApiSpecRevisions(%+v) returned unexpected revision: got %q, want %q", test.req, got.GetName(), want.GetName())
+				}
+			}
+		})
+	}
+}
+
+func TestListApiSpecRevisionsSequence(t *testing.T) {
 	ctx := context.Background()
 	server := defaultTestServer(t)
 	if err := seeder.SeedVersions(ctx, server, &rpc.ApiVersion{Name: "projects/my-project/locations/global/apis/my-api/versions/v1"}); err != nil {