Repository: apigee/registry (go)

Issue:
Registry API: Supporting artifacts on resource revisions
This discussion started when we were thinking about supporting a history of Scores. As we are building more UI features, it is getting more and more difficult to keep track of which spec revision a particular artifact (conformance report, Score, scorecard) belongs to. This is a limitation in conveying the correct information.

Can we evaluate the option of attaching artifacts to a resource's revision?

**Motivation:**

In our case, we will attach artifacts to SpecRevisions and DeploymentRevisions and keep Apis, Versions, Project as is (resources with no revisions).
This goes in line with our data model, the artifacts that get generated for specs are usually properties of a particular revision (lint reports, scores, etc), same with deployments (security analysis of a particular deployed state). The changes to these artifacts are also usually governed by changes in the revision.
For other generic properties, like Related links, Source code repo, those are usually higher level properties outside the scope of a spec/deployment which will normally get attached to an API. For an API, spec becomes the implementation detail of that API (and deployment becomes an operational detail), hence we predict that most of the characteristics related artifacts will be attached at the API level since that is the unit that users are managing in the registry.
Such artifacts at API level will not have to worry about revisions, they will be one time artifacts which will be preserved whereas for Specs and Deployments, artifacts will need a regeneration every time a new revision gets created.
Following [AIP-162](https://google.aip.dev/162#child-resources), the implementation will look like this:

The example is for Specs here, deployments will also have a similar behavior:

- **Create/Update:** Users should create artifacts only under a SpecRevision.
   - The artifact path will look like this: 
   `projects/demo/locations/global/apis/petstore/versions/v1/specs/openapi.yaml@0000/artifacts/conformance-report`
   - The parent of the created artifact must always include the revision ID of the spec.
   - When a user creates an artifact without specifying the revision ID, it should always get attached to the latest revision.
   - The response from the API will always include the revision ID in the artifact path.
- **List/Read:** Users should be able to list artifacts under SpecRevision and for a particular Spec
   - A list request like `projects/demo/locations/global/versions/v1/specs/openapi.yaml@0000/artifacts/-` should list all artifacts 
   under the specified revision.
   - A list request without the revision ID, `projects/demo/locations/global/versions/v1/specs/openapi.yaml/artifacts/-` should list 
   artifacts under the latest revision of that spec.
   - A list request with wildcard, `projects/demo/locations/global/versions/v1/specs/openapi.yaml@-/artifacts/-` should list all artifacts under all revisions of that spec.
   - The response from the API will always include the revision ID in the artifact path.
 
The above implementation is inline with the second scenario explained [here](https://google.aip.dev/162#child-resources)

**Some caveats to call out:**

- Whenever a user attaches an artifact to a spec without specifying the revision, it will be attached to the latest revision, that might have been updated by someone else and might not be the same as the user had created before.
- Users can no longer create artifacts under a spec, they will always be attached to a specific revision.
- Users should be aware that when they create a new revision of spec, the list call projects/demo/locations/global/versions/v1/specs/openapi.yaml/artifacts/- will no longer return artifacts that were available on the previous revision, they will have to create new artifacts on the latest revision for the list call to return artifacts.

Pull request:
artifacts on deployment revisions
Fixes #774

Patch:
diff --git a/cmd/registry/cmd/list/list.go b/cmd/registry/cmd/list/list.go
index 48e4ba3..28607d8 100644
--- a/cmd/registry/cmd/list/list.go
+++ b/cmd/registry/cmd/list/list.go
@@ -17,7 +17,6 @@ package list
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/apigee/registry/cmd/registry/core"
 	"github.com/apigee/registry/log"
@@ -73,6 +72,8 @@ func matchAndHandleListCmd(
 		return core.ListAPIs(ctx, client, api, filter, core.PrintAPI)
 	} else if deployment, err := names.ParseDeploymentCollection(name); err == nil {
 		return core.ListDeployments(ctx, client, deployment, filter, core.PrintDeployment)
+	} else if rev, err := names.ParseDeploymentRevisionCollection(name); err == nil {
+		return core.ListDeploymentRevisions(ctx, client, rev, filter, core.PrintDeployment)
 	} else if version, err := names.ParseVersionCollection(name); err == nil {
 		return core.ListVersions(ctx, client, version, filter, core.PrintVersion)
 	} else if spec, err := names.ParseSpecCollection(name); err == nil {
@@ -90,6 +91,8 @@ func matchAndHandleListCmd(
 		return core.ListAPIs(ctx, client, api, filter, core.PrintAPI)
 	} else if deployment, err := names.ParseDeployment(name); err == nil {
 		return core.ListDeployments(ctx, client, deployment, filter, core.PrintDeployment)
+	} else if rev, err := names.ParseDeploymentRevision(name); err == nil {
+		return core.ListDeploymentRevisions(ctx, client, rev, filter, core.PrintDeployment)
 	} else if version, err := names.ParseVersion(name); err == nil {
 		return core.ListVersions(ctx, client, version, filter, core.PrintVersion)
 	} else if spec, err := names.ParseSpec(name); err == nil {
@@ -100,15 +103,6 @@ func matchAndHandleListCmd(
 		return core.ListArtifacts(ctx, client, artifact, filter, false, core.PrintArtifact)
 	}
 
-	// Then try to match resources with revisions.
-	// "@-" signals that we want to list revisions.
-	if strings.HasSuffix(name, "@-") {
-		name := strings.TrimSuffix(name, "@-")
-		if deployment, err := names.ParseDeployment(name); err == nil {
-			return core.ListDeploymentRevisions(ctx, client, deployment, core.PrintDeployment)
-		}
-	}
-
 	// If nothing matched, return an error.
 	return fmt.Errorf("unsupported argument: %s", name)
 }
diff --git a/cmd/registry/core/list.go b/cmd/registry/core/list.go
index d81967b..38fc2ce 100644
--- a/cmd/registry/core/list.go
+++ b/cmd/registry/core/list.go
@@ -109,7 +109,8 @@ func ListDeployments(ctx context.Context,
 
 func ListDeploymentRevisions(ctx context.Context,
 	client *gapic.RegistryClient,
-	name names.Deployment,
+	name names.DeploymentRevision,
+	filter string,
 	handler DeploymentHandler) error {
 	it := client.ListApiDeploymentRevisions(ctx, &rpc.ListApiDeploymentRevisionsRequest{
 		Name: name.String(),
diff --git a/server/registry/actions_artifacts.go b/server/registry/actions_artifacts.go
index 21ec5a6..aab721a 100644
--- a/server/registry/actions_artifacts.go
+++ b/server/registry/actions_artifacts.go
@@ -36,12 +36,14 @@ type artifactParent interface {
 func parseArtifactParent(name string) (artifactParent, error) {
 	if s, err := names.ParseSpec(name); err == nil {
 		return s, nil
-	} else if v, err := names.ParseSpecRevision(name); err == nil {
-		return v, nil
+	} else if s, err := names.ParseSpecRevision(name); err == nil {
+		return s, nil
 	} else if v, err := names.ParseVersion(name); err == nil {
 		return v, nil
 	} else if d, err := names.ParseDeployment(name); err == nil {
 		return d, nil
+	} else if d, err := names.ParseDeploymentRevision(name); err == nil {
+		return d, nil
 	} else if a, err := names.ParseApi(name); err == nil {
 		return a, nil
 	} else if p, err := names.ParseProjectWithLocation(name); err == nil {
@@ -83,7 +85,12 @@ func (s *RegistryServer) CreateArtifact(ctx context.Context, req *rpc.CreateArti
 		case names.SpecRevision:
 			_, err = db.LockSpecs(ctx).GetSpecRevision(ctx, typedParent)
 		case names.Deployment:
-			_, err = db.LockDeployments(ctx).GetDeployment(ctx, typedParent)
+			// assign to latest revision
+			var deployment *models.Deployment
+			deployment, err = db.LockDeployments(ctx).GetDeployment(ctx, typedParent)
+			if err == nil {
+				parent = parent.(names.Deployment).Revision(deployment.RevisionID)
+			}
 		}
 		if err != nil {
 			return err
@@ -250,6 +257,13 @@ func (s *RegistryServer) ListArtifacts(ctx context.Context, req *rpc.ListArtifac
 			Order:  req.GetOrderBy(),
 			Token:  req.GetPageToken(),
 		})
+	case names.DeploymentRevision:
+		listing, err = db.ListDeploymentRevisionArtifacts(ctx, parent, storage.PageOptions{
+			Size:   req.GetPageSize(),
+			Filter: req.GetFilter(),
+			Order:  req.GetOrderBy(),
+			Token:  req.GetPageToken(),
+		})
 	}
 	if err != nil {
 		return nil, err
diff --git a/server/registry/internal/storage/list.go b/server/registry/internal/storage/list.go
index fd79322..45762a8 100644
--- a/server/registry/internal/storage/list.go
+++ b/server/registry/internal/storage/list.go
@@ -1038,6 +1038,10 @@ func (c *Client) ListVersionArtifacts(ctx context.Context, parent names.Version,
 }
 
 func (c *Client) ListDeploymentArtifacts(ctx context.Context, parent names.Deployment, opts PageOptions) (ArtifactList, error) {
+	return c.ListDeploymentRevisionArtifacts(ctx, parent.Revision(""), opts)
+}
+
+func (c *Client) ListDeploymentRevisionArtifacts(ctx context.Context, parent names.DeploymentRevision, opts PageOptions) (ArtifactList, error) {
 	token, err := decodeToken(opts.Token)
 	if err != nil {
 		return ArtifactList{}, status.Errorf(codes.InvalidArgument, "invalid page token %q: %s", opts.Token, err.Error())
@@ -1055,38 +1059,50 @@ func (c *Client) ListDeploymentArtifacts(ctx context.Context, parent names.Deplo
 		token.Order = opts.Order
 	}
 
-	if parent.ProjectID != "-" && parent.ApiID != "-" && parent.DeploymentID != "-" {
-		if _, err := c.GetDeployment(ctx, parent); err != nil {
-			return ArtifactList{}, err
-		}
-	} else if parent.ProjectID != "-" && parent.ApiID != "-" && parent.DeploymentID == "-" {
-		if _, err := c.GetApi(ctx, parent.Api()); err != nil {
-			return ArtifactList{}, err
-		}
-	} else if parent.ProjectID != "-" && parent.ApiID == "-" && parent.DeploymentID == "-" {
-		if _, err := c.GetProject(ctx, parent.Project()); err != nil {
-			return ArtifactList{}, err
-		}
-	}
-
-	op := c.db.WithContext(ctx).
-		Where(`version_id = ''`).
-		Where(`spec_id = ''`)
+	op := c.db.WithContext(ctx)
 	if id := parent.ProjectID; id != "-" {
-		op = op.Where("project_id = ?", id)
+		op.Where("artifacts.project_id = ?", id)
 	}
 	if id := parent.ApiID; id != "-" {
-		op = op.Where("api_id = ?", id)
+		op = op.Where("artifacts.api_id = ?", id)
 	}
 	if id := parent.DeploymentID; id != "-" {
-		op = op.Where("deployment_id = ?", id)
+		op = op.Where("artifacts.deployment_id = ?", id)
+	}
+	if id := parent.RevisionID; id != "-" && id != "" { // select specific deployment revision
+		op = op.Where("artifacts.revision_id = ?", id)
+	}
+	if id := parent.RevisionID; id == "" { // select latest deployment revision
+		op = op.Model(&models.Artifact{}).
+			Where(`artifacts.spec_id = ''`).
+			Joins(`join (?) latest
+			ON artifacts.project_id = latest.project_id
+			AND artifacts.api_id = latest.api_id
+			AND artifacts.deployment_id = latest.deployment_id
+			AND artifacts.revision_id = latest.revision_id`, c.latestDeploymentRevisionsQuery(ctx))
 	}
 
 	return c.listArtifacts(ctx, op, opts, func(a *models.Artifact) bool {
-		return a.ProjectID != "" && a.ApiID != "" && a.DeploymentID != ""
+		return a.ProjectID != "" && a.ApiID != "" && a.DeploymentID != "" && a.RevisionID != ""
 	})
 }
 
+// This query only returns the most recent revision rows for each unique deployment from the
+// deployment table. Additional criteria may be added to restrict this query further and it
+// may be joined with dependant tables (eg. artifacts, blobs) to ensure that only the
+// rows in those tables that are associated with the more recent deployment revision are matched.
+func (c *Client) latestDeploymentRevisionsQuery(ctx context.Context) *gorm.DB {
+	return c.db.WithContext(ctx).
+		Table("deployments d").
+		Select("d.*").
+		Joins(`LEFT JOIN deployments d2
+		ON d.project_id = d2.project_id
+		AND d.api_id = d2.api_id
+		AND d.deployment_id = d2.deployment_id
+		AND d.revision_create_time < d2.revision_create_time`).
+		Where("d2.key IS NULL")
+}
+
 func (c *Client) ListApiArtifacts(ctx context.Context, parent names.Api, opts PageOptions) (ArtifactList, error) {
 	token, err := decodeToken(opts.Token)
 	if err != nil {
diff --git a/server/registry/internal/storage/models/artifact.go b/server/registry/internal/storage/models/artifact.go
index c248688..71131a1 100644
--- a/server/registry/internal/storage/models/artifact.go
+++ b/server/registry/internal/storage/models/artifact.go
@@ -85,8 +85,8 @@ func (artifact *Artifact) Name() string {
 		return fmt.Sprintf("projects/%s/locations/%s/apis/%s/versions/%s/artifacts/%s",
 			artifact.ProjectID, names.Location, artifact.ApiID, artifact.VersionID, artifact.ArtifactID)
 	case artifact.DeploymentID != "":
-		return fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s/artifacts/%s",
-			artifact.ProjectID, names.Location, artifact.ApiID, artifact.DeploymentID, artifact.ArtifactID)
+		return fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s@%s/artifacts/%s",
+			artifact.ProjectID, names.Location, artifact.ApiID, artifact.DeploymentID, artifact.RevisionID, artifact.ArtifactID)
 	case artifact.ApiID != "":
 		return fmt.Sprintf("projects/%s/locations/%s/apis/%s/artifacts/%s",
 			artifact.ProjectID, names.Location, artifact.ApiID, artifact.ArtifactID)
diff --git a/server/registry/names/artifact.go b/server/registry/names/artifact.go
index 6d7fc6b..480b753 100644
--- a/server/registry/names/artifact.go
+++ b/server/registry/names/artifact.go
@@ -24,13 +24,13 @@ var (
 	apiArtifactCollectionRegexp        = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/artifacts$", identifier, Location, identifier))
 	versionArtifactCollectionRegexp    = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/versions/%s/artifacts$", identifier, Location, identifier, identifier))
 	specArtifactCollectionRegexp       = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/versions/%s/specs/%s(?:@%s)?/artifacts$", identifier, Location, identifier, identifier, identifier, revisionTag))
-	deploymentArtifactCollectionRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s/artifacts$", identifier, Location, identifier, identifier))
+	deploymentArtifactCollectionRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s(?:@%s)?/artifacts$", identifier, Location, identifier, identifier, revisionTag))
 
 	projectArtifactRegexp    = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/artifacts/%s$", identifier, Location, identifier))
 	apiArtifactRegexp        = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/artifacts/%s$", identifier, Location, identifier, identifier))
 	versionArtifactRegexp    = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/versions/%s/artifacts/%s$", identifier, Location, identifier, identifier, identifier))
 	specArtifactRegexp       = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/versions/%s/specs/%s(?:@%s)?/artifacts/%s$", identifier, Location, identifier, identifier, identifier, revisionTag, identifier))
-	deploymentArtifactRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s/artifacts/%s$", identifier, Location, identifier, identifier, identifier))
+	deploymentArtifactRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s(?:@%s)?/artifacts/%s$", identifier, Location, identifier, identifier, revisionTag, identifier))
 )
 
 // Artifact represents a resource name for an artifact.
@@ -102,6 +102,8 @@ func (a Artifact) RevisionID() string {
 	switch name := a.name.(type) {
 	case specArtifact:
 		return name.RevisionID
+	case deploymentArtifact:
+		return name.RevisionID
 	default:
 		return ""
 	}
@@ -468,6 +470,7 @@ type deploymentArtifact struct {
 	ProjectID    string
 	ApiID        string
 	DeploymentID string
+	RevisionID   string
 	ArtifactID   string
 }
 
@@ -484,16 +487,22 @@ func (a deploymentArtifact) Validate() error {
 }
 
 func (a deploymentArtifact) Parent() string {
-	return Deployment{
+	return DeploymentRevision{
 		ProjectID:    a.ProjectID,
 		ApiID:        a.ApiID,
 		DeploymentID: a.DeploymentID,
+		RevisionID:   a.RevisionID,
 	}.String()
 }
 
 func (a deploymentArtifact) String() string {
-	return normalize(fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s/artifacts/%s",
-		a.ProjectID, Location, a.ApiID, a.DeploymentID, a.ArtifactID))
+	if a.RevisionID == "" { // use latest revision
+		return normalize(fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s/artifacts/%s",
+			a.ProjectID, Location, a.ApiID, a.DeploymentID, a.ArtifactID))
+	} else {
+		return normalize(fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s@%s/artifacts/%s",
+			a.ProjectID, Location, a.ApiID, a.DeploymentID, a.RevisionID, a.ArtifactID))
+	}
 }
 
 func parseDeploymentArtifact(name string) (deploymentArtifact, error) {
@@ -506,7 +515,8 @@ func parseDeploymentArtifact(name string) (deploymentArtifact, error) {
 		ProjectID:    m[1],
 		ApiID:        m[2],
 		DeploymentID: m[3],
-		ArtifactID:   m[4],
+		RevisionID:   m[4],
+		ArtifactID:   m[5],
 	}
 
 	return artifact, nil
@@ -522,6 +532,7 @@ func parseDeploymentArtifactCollection(name string) (deploymentArtifact, error)
 		ProjectID:    m[1],
 		ApiID:        m[2],
 		DeploymentID: m[3],
+		RevisionID:   m[4],
 		ArtifactID:   "",
 	}
 
diff --git a/server/registry/names/deployment.go b/server/registry/names/deployment.go
index 7add485..e2e4cf4 100644
--- a/server/registry/names/deployment.go
+++ b/server/registry/names/deployment.go
@@ -19,6 +19,19 @@ import (
 	"regexp"
 )
 
+// deploymentRegexp is a regular expression that matches a deployment resource name.
+var deploymentRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s(@%s)?$",
+	identifier, Location, identifier, identifier, revisionTag))
+
+// deploymentCollectionRegexp is a regular expression that matches a collection of deployments.
+var deploymentCollectionRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments$",
+	identifier, Location, identifier))
+
+// simpleDeploymentRegexp is the regex pattern for deployment resource names.
+// Notably, this differs from deploymentRegexp by not accepting revision IDs in the resource name.
+var simpleDeploymentRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s$",
+	identifier, Location, identifier, identifier))
+
 // Deployment represents a resource name for an API deployment.
 type Deployment struct {
 	ProjectID    string
@@ -33,7 +46,7 @@ func (d Deployment) Validate() error {
 		return err
 	}
 
-	r := deploymentRegexp()
+	r := deploymentRegexp
 	if name := d.String(); !r.MatchString(name) {
 		return fmt.Errorf("invalid deployment name %q: must match %q", name, r)
 	}
@@ -95,21 +108,9 @@ func (d Deployment) String() string {
 		d.ProjectID, Location, d.ApiID, d.DeploymentID))
 }
 
-// deploymentCollectionRegexp returns a regular expression that matches a collection of deployments.
-func deploymentCollectionRegexp() *regexp.Regexp {
-	return regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments$",
-		identifier, Location, identifier))
-}
-
-// deploymentRegexp returns a regular expression that matches a deployment resource name.
-func deploymentRegexp() *regexp.Regexp {
-	return regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s$",
-		identifier, Location, identifier, identifier))
-}
-
 // ParseDeployment parses the name of a deployment.
 func ParseDeployment(name string) (Deployment, error) {
-	r := deploymentRegexp()
+	r := simpleDeploymentRegexp
 	if !r.MatchString(name) {
 		return Deployment{}, fmt.Errorf("invalid deployment name %q: must match %q", name, r)
 	}
@@ -123,7 +124,7 @@ func ParseDeployment(name string) (Deployment, error) {
 }
 
 func ParseDeploymentCollection(name string) (Deployment, error) {
-	r := deploymentCollectionRegexp()
+	r := deploymentCollectionRegexp
 	if !r.MatchString(name) {
 		return Deployment{}, fmt.Errorf("invalid deployment collection name %q: must match %q", name, r)
 	}
diff --git a/server/registry/names/deployment_revision.go b/server/registry/names/deployment_revision.go
index 8f3bff4..8fdbacb 100644
--- a/server/registry/names/deployment_revision.go
+++ b/server/registry/names/deployment_revision.go
@@ -19,7 +19,11 @@ import (
 	"regexp"
 )
 
-var deploymentRevisionRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s@%s$", identifier, Location, identifier, identifier, revisionTag))
+var deploymentRevisionRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s(?:@%s)?$",
+	identifier, Location, identifier, identifier, revisionTag))
+
+var deploymentRevisionCollectionRegexp = regexp.MustCompile(fmt.Sprintf("^projects/%s/locations/%s/apis/%s/deployments/%s@",
+	identifier, Location, identifier, identifier))
 
 // DeploymentRevision represents a resource name for an API deployment revision.
 type DeploymentRevision struct {
@@ -38,9 +42,47 @@ func (s DeploymentRevision) Deployment() Deployment {
 	}
 }
 
+// Project returns the parent project for this resource.
+func (d DeploymentRevision) Project() Project {
+	return Project{
+		ProjectID: d.ProjectID,
+	}
+}
+
+// Api returns the parent API for this resource.
+func (d DeploymentRevision) Api() Api {
+	return Api{
+		ProjectID: d.ProjectID,
+		ApiID:     d.ApiID,
+	}
+}
+
+// Artifact returns an artifact with the provided ID and this resource as its parent.
+func (s DeploymentRevision) Artifact(id string) Artifact {
+	return Artifact{
+		name: deploymentArtifact{
+			ProjectID:    s.ProjectID,
+			ApiID:        s.ApiID,
+			DeploymentID: s.DeploymentID,
+			RevisionID:   s.RevisionID,
+			ArtifactID:   id,
+		},
+	}
+}
+
+// Parent returns this resource's parent API resource name.
+func (s DeploymentRevision) Parent() string {
+	return s.Deployment().Parent()
+}
+
 func (s DeploymentRevision) String() string {
-	return normalize(fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s@%s",
-		s.ProjectID, Location, s.ApiID, s.DeploymentID, s.RevisionID))
+	if s.RevisionID == "" { // use latest revision
+		return normalize(fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s",
+			s.ProjectID, Location, s.ApiID, s.DeploymentID))
+	} else {
+		return normalize(fmt.Sprintf("projects/%s/locations/%s/apis/%s/deployments/%s@%s",
+			s.ProjectID, Location, s.ApiID, s.DeploymentID, s.RevisionID))
+	}
 }
 
 // ParseDeploymentRevision parses the name of a deployment.
@@ -59,3 +101,18 @@ func ParseDeploymentRevision(name string) (DeploymentRevision, error) {
 
 	return revision, nil
 }
+
+func ParseDeploymentRevisionCollection(name string) (DeploymentRevision, error) {
+	r := deploymentRevisionCollectionRegexp
+	if !r.MatchString(name) {
+		return DeploymentRevision{}, fmt.Errorf("invalid deployment revision collection name %q: must match %q", name, r)
+	}
+
+	m := r.FindStringSubmatch(name)
+	return DeploymentRevision{
+		ProjectID:    m[1],
+		ApiID:        m[2],
+		DeploymentID: m[3],
+		RevisionID:   "-",
+	}, nil
+}


Test patch:
diff --git a/server/registry/actions_artifacts_test.go b/server/registry/actions_artifacts_test.go
index 222efcd..bb53ff6 100644
--- a/server/registry/actions_artifacts_test.go
+++ b/server/registry/actions_artifacts_test.go
@@ -100,26 +100,6 @@ func TestCreateArtifact(t *testing.T) {
 				Hash:      sha256hash(artifactContents),
 			},
 		},
-		{
-			desc: "create deployment artifact",
-			seed: &rpc.ApiDeployment{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment"},
-			req: &rpc.CreateArtifactRequest{
-				Parent:     "projects/my-project/locations/global/apis/my-api/deployments/my-deployment",
-				ArtifactId: "my-artifact",
-				Artifact: &rpc.Artifact{
-					MimeType:  "application/json",
-					SizeBytes: int32(len(artifactContents)),
-					Hash:      sha256hash(artifactContents),
-					Contents:  artifactContents,
-				},
-			},
-			want: &rpc.Artifact{
-				Name:      "projects/my-project/locations/global/apis/my-api/deployments/my-deployment/artifacts/my-artifact",
-				MimeType:  "application/json",
-				SizeBytes: int32(len(artifactContents)),
-				Hash:      sha256hash(artifactContents),
-			},
-		},
 	}
 
 	for _, test := range tests {
@@ -773,25 +753,6 @@ func TestListArtifacts(t *testing.T) {
 				},
 			},
 		},
-		{
-			desc: "artifacts owned by a deployment",
-			seed: []*rpc.Artifact{
-				{Name: "projects/my-project/locations/global/apis/my-api/deployments/d1/artifacts/artifact1"},
-				{Name: "projects/my-project/locations/global/apis/my-api/deployments/d1/artifacts/artifact2"},
-				{Name: "projects/my-project/locations/global/apis/my-api/deployments/d1/artifacts/artifact3"},
-				{Name: "projects/my-project/locations/global/apis/my-api/deployments/d2/artifacts/artifact4"},
-			},
-			req: &rpc.ListArtifactsRequest{
-				Parent: "projects/my-project/locations/global/apis/my-api/deployments/d1",
-			},
-			want: &rpc.ListArtifactsResponse{
-				Artifacts: []*rpc.Artifact{
-					{Name: "projects/my-project/locations/global/apis/my-api/deployments/d1/artifacts/artifact1"},
-					{Name: "projects/my-project/locations/global/apis/my-api/deployments/d1/artifacts/artifact2"},
-					{Name: "projects/my-project/locations/global/apis/my-api/deployments/d1/artifacts/artifact3"},
-				},
-			},
-		},
 		{
 			desc: "ordered by mime_type",
 			seed: []*rpc.Artifact{
@@ -1638,3 +1599,247 @@ func TestSpecRevisionArtifacts(t *testing.T) {
 		}
 	})
 }
+
+func TestDeploymentRevisionArtifacts(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 {
+		t.Fatalf("Setup/Seeding: Failed to seed registry: %s", err)
+	}
+
+	createDeployment := &rpc.CreateApiDeploymentRequest{
+		Parent:          "projects/my-project/locations/global/apis/my-api",
+		ApiDeploymentId: "my-deployment",
+		ApiDeployment: &rpc.ApiDeployment{
+			Description: "Empty First Revision",
+		},
+	}
+	deployment1r1, err := server.CreateApiDeployment(ctx, createDeployment)
+	if err != nil {
+		t.Fatalf("Setup: CreateApiDeployment(%+v) returned error: %s", createDeployment, err)
+	}
+
+	updateDeployment := &rpc.UpdateApiDeploymentRequest{
+		ApiDeployment: &rpc.ApiDeployment{
+			Name:        "projects/my-project/locations/global/apis/my-api/deployments/my-deployment",
+			Description: "Second revision",
+			EndpointUri: "updated",
+		},
+	}
+	deployment1r2, err := server.UpdateApiDeployment(ctx, updateDeployment)
+	if err != nil {
+		t.Fatalf("Setup: UpdateApiDeployment(%+v) returned error: %s", updateDeployment, err)
+	}
+
+	createDeployment2 := &rpc.CreateApiDeploymentRequest{
+		Parent:          "projects/my-project/locations/global/apis/my-api",
+		ApiDeploymentId: "my-deployment2",
+		ApiDeployment: &rpc.ApiDeployment{
+			Description: "Empty First Revision",
+		},
+	}
+	deployment2r1, err := server.CreateApiDeployment(ctx, createDeployment2)
+	if err != nil {
+		t.Fatalf("Setup: CreateApiDeployment(%+v) returned error: %s", createDeployment, err)
+	}
+
+	createArtifact := &rpc.CreateArtifactRequest{
+		Parent:     deployment2r1.Name,
+		ArtifactId: "my-artifact-r1",
+		Artifact: &rpc.Artifact{
+			MimeType:  "application/json",
+			SizeBytes: int32(len(artifactContents)),
+			Hash:      sha256hash(artifactContents),
+			Contents:  artifactContents,
+		},
+	}
+	_, err = server.CreateArtifact(ctx, createArtifact)
+	if err != nil {
+		t.Fatalf("CreateArtifact(%+v) returned error: %s", createArtifact, err)
+	}
+
+	t.Run("create under default revision", func(t *testing.T) {
+		createArtifact := &rpc.CreateArtifactRequest{
+			Parent:     "projects/my-project/locations/global/apis/my-api/deployments/my-deployment",
+			ArtifactId: "my-artifact-r2",
+			Artifact: &rpc.Artifact{
+				MimeType:  "application/json",
+				SizeBytes: int32(len(artifactContents)),
+				Hash:      sha256hash(artifactContents),
+				Contents:  artifactContents,
+			},
+		}
+		artifact, err := server.CreateArtifact(ctx, createArtifact)
+		if err != nil {
+			t.Fatalf("CreateArtifact(%+v) returned error: %s", createArtifact, err)
+		}
+
+		want := &rpc.Artifact{
+			Name:      fmt.Sprintf(createArtifact.Parent+"@%s/artifacts/my-artifact-r2", deployment1r2.RevisionId),
+			MimeType:  "application/json",
+			SizeBytes: int32(len(artifactContents)),
+			Hash:      sha256hash(artifactContents),
+		}
+
+		opts := cmp.Options{
+			protocmp.Transform(),
+			protocmp.IgnoreFields(new(rpc.Artifact), "create_time", "update_time"),
+		}
+
+		if !cmp.Equal(want, artifact, opts) {
+			t.Errorf("CreateArtifact(%+v) returned unexpected diff (-want +got):\n%s", createArtifact, cmp.Diff(want, artifact, opts))
+		}
+	})
+
+	t.Run("create under a specified revision", func(t *testing.T) {
+		createArtifact := &rpc.CreateArtifactRequest{
+			Parent:     "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId(),
+			ArtifactId: "my-artifact-r1",
+			Artifact: &rpc.Artifact{
+				MimeType:  "application/json",
+				SizeBytes: int32(len(artifactContents)),
+				Hash:      sha256hash(artifactContents),
+				Contents:  artifactContents,
+			},
+		}
+		artifact, err := server.CreateArtifact(ctx, createArtifact)
+		if err != nil {
+			t.Fatalf("CreateArtifact(%+v) returned error: %s", createArtifact, err)
+		}
+
+		want := &rpc.Artifact{
+			Name:      createArtifact.Parent + "/artifacts/my-artifact-r1",
+			MimeType:  "application/json",
+			SizeBytes: int32(len(artifactContents)),
+			Hash:      sha256hash(artifactContents),
+		}
+
+		opts := cmp.Options{
+			protocmp.Transform(),
+			protocmp.IgnoreFields(new(rpc.Artifact), "create_time", "update_time"),
+		}
+
+		if !cmp.Equal(want, artifact, opts) {
+			t.Errorf("CreateArtifact(%+v) returned unexpected diff (-want +got):\n%s", createArtifact, cmp.Diff(want, artifact, opts))
+		}
+	})
+
+	t.Run("list artifacts across", func(t *testing.T) {
+		tests := []struct {
+			desc string
+			req  *rpc.ListArtifactsRequest
+			want *rpc.ListArtifactsResponse
+		}{
+			{
+				desc: "specified deployment revision",
+				req: &rpc.ListArtifactsRequest{
+					Parent: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId(),
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+					},
+				},
+			},
+			{
+				desc: "latest deployment revision",
+				req: &rpc.ListArtifactsRequest{
+					Parent: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment",
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r2.GetRevisionId() + "/artifacts/my-artifact-r2"},
+					},
+				},
+			},
+			{
+				desc: "all deployment revisions",
+				req: &rpc.ListArtifactsRequest{
+					Parent:  "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@-",
+					OrderBy: "create_time",
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r2.GetRevisionId() + "/artifacts/my-artifact-r2"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+					},
+				},
+			},
+			{
+				desc: "latest revisions of all deployments",
+				req: &rpc.ListArtifactsRequest{
+					Parent:  "projects/my-project/locations/global/apis/my-api/deployments/-",
+					OrderBy: "create_time",
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment2@" + deployment2r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r2.GetRevisionId() + "/artifacts/my-artifact-r2"},
+					},
+				},
+			},
+			{
+				desc: "all revisions of all deployments",
+				req: &rpc.ListArtifactsRequest{
+					Parent:  "projects/my-project/locations/global/apis/my-api/deployments/-@-",
+					OrderBy: "create_time",
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment2@" + deployment2r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r2.GetRevisionId() + "/artifacts/my-artifact-r2"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+					},
+				},
+			},
+			{
+				desc: "all revisions in all apis",
+				req: &rpc.ListArtifactsRequest{
+					Parent:  "projects/my-project/locations/global/apis/-/deployments/-@-",
+					OrderBy: "create_time",
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment2@" + deployment2r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r2.GetRevisionId() + "/artifacts/my-artifact-r2"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+					},
+				},
+			},
+			{
+				desc: "all revisions in all projects",
+				req: &rpc.ListArtifactsRequest{
+					Parent:  "projects/-/locations/global/apis/-/deployments/-@-",
+					OrderBy: "create_time",
+				},
+				want: &rpc.ListArtifactsResponse{
+					Artifacts: []*rpc.Artifact{
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment2@" + deployment2r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r2.GetRevisionId() + "/artifacts/my-artifact-r2"},
+						{Name: "projects/my-project/locations/global/apis/my-api/deployments/my-deployment@" + deployment1r1.GetRevisionId() + "/artifacts/my-artifact-r1"},
+					},
+				},
+			},
+		}
+
+		for _, test := range tests {
+			t.Run(test.desc, func(t *testing.T) {
+				got, err := server.ListArtifacts(ctx, test.req)
+				if err != nil {
+					t.Fatalf("ListArtifacts(%+v) returned error: %s", test.req, err)
+				}
+
+				opts := cmp.Options{
+					protocmp.Transform(),
+					protocmp.IgnoreFields(new(rpc.ListArtifactsResponse), "next_page_token"),
+					protocmp.IgnoreFields(new(rpc.Artifact), "create_time", "update_time", "hash", "mime_type", "size_bytes"),
+				}
+
+				if !cmp.Equal(test.want, got, opts) {
+					t.Errorf("ListArtifacts(%+v) returned unexpected diff (-want +got):\n%s", test.req, cmp.Diff(test.want, got, opts))
+				}
+			})
+		}
+	})
+}
diff --git a/server/registry/names/names_test.go b/server/registry/names/names_test.go
index 2a12f3d..ec16e53 100644
--- a/server/registry/names/names_test.go
+++ b/server/registry/names/names_test.go
@@ -225,6 +225,10 @@ func TestResourceNames(t *testing.T) {
 				return err == nil
 			}, pass: []string{
 				"projects/google/locations/global/apis/sample/deployments/v1@1234567890abcdef",
+				"projects/google/locations/global/apis/sample/deployments/v1",
+				"projects/-/locations/global/apis/-/deployments/-",
+				"projects/123/locations/global/apis/abc/deployments/123",
+				"projects/1-2-3/locations/global/apis/abc/deployments/123",
 			},
 			fail: []string{
 				"-",
@@ -233,10 +237,6 @@ func TestResourceNames(t *testing.T) {
 				"projects/123/locations/global/apis/",
 				"projects/123/locations/global/invalid/123",
 				"projects/123/locations/global/apis/ 123",
-				"projects/google/locations/global/apis/sample/deployments/v1",
-				"projects/-/locations/global/apis/-/deployments/-",
-				"projects/123/locations/global/apis/abc/deployments/123",
-				"projects/1-2-3/locations/global/apis/abc/deployments/123",
 			},
 		},
 		{
@@ -275,15 +275,17 @@ func TestResourceNames(t *testing.T) {
 		},
 	}
 	for _, g := range groups {
-		for _, path := range g.pass {
-			if !g.check(path) {
-				t.Errorf("failed to match %s: %s", g.name, path)
+		t.Run(g.name, func(t *testing.T) {
+			for _, path := range g.pass {
+				if !g.check(path) {
+					t.Errorf("failed to match: %s", path)
+				}
 			}
-		}
-		for _, path := range g.fail {
-			if g.check(path) {
-				t.Errorf("false match %s: %s", g.name, path)
+			for _, path := range g.fail {
+				if g.check(path) {
+					t.Errorf("false match: %s", path)
+				}
 			}
-		}
+		})
 	}
 }