Repository: apigee/registry
Pull request #471 (2022-03-04 01:11:05)

Controller: Use parent to generate names for non-existent target resources.
Partially Fixes https://github.com/apigee/registry/issues/464

The validation pattern will be implemented in upcoming PRs (https://github.com/apigee/registry/pull/477)

Summary of changes:
* Change logic to use parent while generating names for non-existent target resources.
* Some refactoring in `generateActions`.

shrutiparabgoogle: > Some preliminary comments. I haven't looked much at the tests or thought much about potential bugs. I don't really understand what's happening with the "visited" set.
> 
> Is it skipping the check for missing artifacts if we update any artifacts under the same parent? I can imagine a scenario where a resource needs some artifacts updated and others created.

Since we have now decided (to be implemented in next PR) to fix the resource name for `generatedResource` and not allow `-`, each manifest entry will create/update only one artifact for a parent. Example:
```
generated_resources:
- pattern: "apis/-/versions/-/specs/-/artifacts/complexity"
   dependencies:
   - pattern: "$resource.spec"
   action: "registry compute complexity $resource.spec"
- pattern: "apis/-/versions/-/specs/-/artifacts/spectral-lint"
   dependencies:
   - pattern: "$resource.spec"
   action: "registry compute lint $resource.spec --linter spectral"
```

In the above example, the first entry is looking at only `complexity` artifacts whereas the second entry is looking for only `spectral-lint` artifacts under a spec. As for the visited map, the visited parents map is maintained per manifest entry (each entry in generated_resources). Hence, the parents which are marked as visited in the first entry will not affect the parents in the second entry. Thus, even if a `complexity` artifact was generated for a spec `apis/petstore/versions/1.0.0/specs/openapi.yaml`, this spec will still be checked for `lint` artifacts when the controller starts processing the next entry.  



Patch:
diff --git a/cmd/registry/controller/controller.go b/cmd/registry/controller/controller.go
index cd35e03..cd1c756 100644
--- a/cmd/registry/controller/controller.go
+++ b/cmd/registry/controller/controller.go
@@ -17,6 +17,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/apigee/registry/connection"
@@ -61,11 +62,11 @@ func processManifestResource(
 	ctx context.Context,
 	client connection.Client,
 	projectID string,
-	resource *rpc.GeneratedResource) ([]*Action, error) {
+	generatedResource *rpc.GeneratedResource) ([]*Action, error) {
 	// Generate dependency map
-	resourcePattern := fmt.Sprintf("projects/%s/locations/global/%s", projectID, resource.Pattern)
-	dependencyMaps := make([]map[string]time.Time, 0, len(resource.Dependencies))
-	for _, dependency := range resource.Dependencies {
+	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)
 		if err != nil {
 			return nil, fmt.Errorf("error while generating dependency map for %v: %s", dependency, err)
@@ -73,18 +74,9 @@ func processManifestResource(
 		dependencyMaps = append(dependencyMaps, dMap)
 	}
 
-	// Generate resource list
-	resourceList, err := ListResources(ctx, client, resourcePattern, resource.Filter)
-	if err != nil {
-		return nil, err
-	}
-
-	// Generate actions to update target resources
-	actions, err := generateActions(
-		ctx, client, resourcePattern, resourceList, dependencyMaps, resource)
-	if err != nil {
-		return nil, err
-	}
+	// Generate actions to create and update target resources
+	actions := generateActions(
+		ctx, client, resourcePattern, generatedResource.Filter, dependencyMaps, generatedResource)
 
 	return actions, nil
 }
@@ -120,7 +112,7 @@ func generateDependencyMap(
 	}
 
 	for _, source := range sourceList {
-		group, err := getGroupKey(dependency.Pattern, source)
+		group, err := getEntityKey(dependency.Pattern, source.GetResourceName())
 		if err != nil {
 			return nil, err
 		}
@@ -144,107 +136,240 @@ func generateActions(
 	ctx context.Context,
 	client connection.Client,
 	resourcePattern string,
-	resourceList []ResourceInstance,
+	filter string,
 	dependencyMaps []map[string]time.Time,
-	generatedResource *rpc.GeneratedResource) ([]*Action, error) {
+	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...)
+
+		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
+
+}
+
+// Go over the list of existing target resources to figure out which ones need an update.
+func generateUpdateActions(
+	ctx context.Context,
+	client connection.Client,
+	resourcePattern string,
+	filter string,
+	dependencyMaps []map[string]time.Time,
+	generatedResource *rpc.GeneratedResource) ([]*Action, map[string]bool, error) {
 
+	// Visited tracks the parents of target resources which were already generated.
 	visited := make(map[string]bool)
 	actions := make([]*Action, 0)
 
-	for _, resource := range resourceList {
-		resourceTime := resource.GetUpdateTimestamp()
+	// Generate resource list
+	resourceList, err := ListResources(ctx, client, resourcePattern, filter)
+	if err != nil {
+		return nil, nil, err
+	}
 
-		takeAction := false
+	// Iterate over a list of existing target resources to generate update actions
+	for _, targetResource := range resourceList {
+		visited[targetResource.GetResourceName().GetParent()] = true
 
-		// Evaluate this resource against each dependency source pattern
-		for i, dependency := range generatedResource.Dependencies {
-			dMap := dependencyMaps[i]
-			// Get the group to look for in dependencyMap
-			group, err := getGroupKey(dependency.Pattern, resource)
-			if err != nil {
-				return nil, fmt.Errorf("cannot match resource with dependency. Error: %s", err.Error())
-			}
+		takeAction, err := needsUpdate(
+			targetResource.GetResourceName(),
+			targetResource.GetUpdateTimestamp(),
+			dependencyMaps,
+			generatedResource,
+			false,
+		)
 
-			if maxUpdateTime, ok := dMap[group]; ok {
-				// Take action if dependency timestamp is later than resource timestamp
-				if maxUpdateTime.After(resourceTime) {
-					takeAction = true
-				}
-				visited[group] = true
-			} else {
-				// For a given resource, each of it's defined dependency group should be present.
-				// If any one of the dependency groups is missing, avoid calculating any action for the resource
-				takeAction = false
-				break
-			}
+		if err != nil {
+			log.Errorf(ctx, "%s", err)
+			continue
 		}
 
 		if takeAction {
-			cmd, err := generateCommand(generatedResource.Action, resource.GetName())
+			cmd, err := generateCommand(generatedResource.Action, targetResource.GetResourceName().String())
 			if err != nil {
-				return nil, err
+				return nil, nil, fmt.Errorf("Cannot generate command: %s", err)
 			}
-			action := &Action{
+			a := &Action{
 				Command:           cmd,
-				GeneratedResource: resource.GetName(),
+				GeneratedResource: targetResource.GetResourceName().String(),
 				RequiresReceipt:   generatedResource.Receipt,
 			}
-			actions = append(actions, action)
+			actions = append(actions, a)
 		}
+
 	}
 
-	// Check patterns where resources do not exist in the registry. Here new resources will be generated
-	// for the dependencies which were not visited in the above loop.
-	// Iterate over first dependency source and evaluate that against remaining dependencies
-	if len(dependencyMaps) > 0 {
-		dMap0 := dependencyMaps[0]
-		for groupKey := range dMap0 {
-			takeAction := true
-			if _, ok := visited[groupKey]; !ok {
-				for _, dMap := range dependencyMaps {
-					// in a particular group resources for each dependencyPattern must exist
-					// for the controller to generate new targeted resource.
-					// Example:
-					// generated_resource: apis/-/artifacts/summary
-					// - dependencies:
-					//   - pattern: $resource.api/artifacts/complexity
-					//   - pattern: $resource.api/artifacts/vocabulary
-					// for a particular api group, "petstore"
-					// both apis/petstore/artifacts/complexity and apis/petstore/artifacts/vocabulary should be present
-					_, dependencyExists := dMap[groupKey]
-					if !dependencyExists {
-						takeAction = false
-						break
-					}
-				}
-			} else {
-				takeAction = false
-			}
+	return actions, visited, nil
+}
 
-			if takeAction {
-				// Since the GeneratedResource is non-existent here,
-				// we will have to derive the exact name of the target resourc
-				resourceName, err := resourceNameFromGroupKey(
-					resourcePattern, groupKey)
-				if err != nil {
-					log.Debugf(ctx, "skipping entry for %q, cannot derive target resource name for pattern: %q", groupKey, resourcePattern)
-					continue
-				}
-				cmd, err := generateCommand(generatedResource.Action, resourceName)
-				if err != nil {
-					return nil, err
-				}
-
-				action := &Action{
-					Command:           cmd,
-					GeneratedResource: resourceName,
-					RequiresReceipt:   generatedResource.Receipt,
-				}
-				actions = append(actions, action)
-			}
+// For the target resources which do not exist in the registry yet,
+//we will use the parent resources to derive which new target resources should be created.
+func generateCreateActions(
+	ctx context.Context,
+	client connection.Client,
+	resourcePattern string,
+	dependencyMaps []map[string]time.Time,
+	generatedResource *rpc.GeneratedResource,
+	visited map[string]bool) ([]*Action, error) {
 
+	parsedResourcePattern, err := parseResourcePattern(resourcePattern)
+	if err != nil {
+		return nil, err
+	}
+	parentName := parsedResourcePattern.GetParent()
+
+	// If parent is a project, we can't list projects since this is registry client command.
+	// Since the manifest definition is scoped  only for a particular project,
+	// there will be only one target resource in this case.
+	// There are two cases where this might happen:
+	// 1. Target resource is a project level artifact "projects/demo/locations/global/artifacts/serach-index"
+	//    extracted parent will be "projects/demo/locations/global"
+	// 1. Target resource is an api "projects/demo/locations/global/apis/petstore"
+	//    extracted parent will be "projects/demo/locations/global"
+	if strings.HasSuffix(parentName, "locations/global") {
+		// Return if this parent was already visited.
+		if visited[parentName] {
+			return nil, nil
+		}
+
+		// Since the GeneratedResource is non-existent here,
+		// we will have to derive the exact name of the target resource.
+		targetResourceName, err := resourceNameFromParent(resourcePattern, parentName)
+		if err != nil {
+			return nil, fmt.Errorf("Cannot generate target resourceName to be created. Error: %s", err)
 		}
+
+		takeAction, err := needsCreate(
+			targetResourceName,
+			dependencyMaps,
+			generatedResource,
+		)
+
+		if err != nil {
+			return nil, err
+		} else if !takeAction {
+			return nil, nil
+		}
+
+		cmd, err := generateCommand(generatedResource.Action, targetResourceName.String())
+		if err != nil {
+			return nil, fmt.Errorf("Cannot generate command: %s", err)
+		}
+
+		return []*Action{
+			{
+				Command:           cmd,
+				GeneratedResource: targetResourceName.String(),
+				RequiresReceipt:   generatedResource.Receipt,
+			},
+		}, nil
+	}
+
+	// If parent resource is not a project, then go through all the non-visited parents.
+	// We don't pass the filter here because the filter is for the target resource and not it's parent.
+	parentList, err := ListResources(ctx, client, parentName, "")
+	if err != nil {
+		return nil, err
+	}
+
+	actions := make([]*Action, 0)
+
+	for _, parent := range parentList {
+		// Skip if this parent was already visited.
+		if visited[parent.GetResourceName().String()] {
+			continue
+		}
+		// Since the GeneratedResource is non-existent here,
+		// we will have to derive the exact name of the target resource
+		targetResourceName, err := resourceNameFromParent(resourcePattern, parent.GetResourceName().String())
+		if err != nil {
+			return nil, err
+		}
+
+		takeAction, err := needsCreate(
+			targetResourceName,
+			dependencyMaps,
+			generatedResource,
+		)
+
+		if err != nil {
+			return nil, err
+		} else if !takeAction {
+			continue
+		}
+
+		cmd, err := generateCommand(generatedResource.Action, targetResourceName.String())
+		if err != nil {
+			return nil, fmt.Errorf("Cannot generate command: %s", err)
+		}
+		a := &Action{
+			Command:           cmd,
+			GeneratedResource: targetResourceName.String(),
+			RequiresReceipt:   generatedResource.Receipt,
+		}
+		actions = append(actions, a)
 	}
 
 	return actions, nil
 }
+
+func needsUpdate(
+	targetResourceName ResourceName,
+	targetResourceTime time.Time,
+	dependencyMaps []map[string]time.Time,
+	generatedResource *rpc.GeneratedResource,
+	createMode bool) (bool, error) {
+	for i, dependency := range generatedResource.Dependencies {
+		dMap := dependencyMaps[i]
+		// Get the entity to look for in dependencyMap
+		entityKey, err := getEntityKey(dependency.Pattern, targetResourceName)
+		if err != nil {
+			// This means that there is error in the pattern definition, hence return
+			return false, fmt.Errorf("cannot match resource with dependency. Error: %s", err.Error())
+		}
+
+		// All the dependencies should be present to generate an action.
+		maxUpdateTime, ok := dMap[entityKey]
+		if !ok {
+			return false, nil
+		}
+
+		if maxUpdateTime.After(targetResourceTime) {
+			return true, nil // Take action if atleast one dependency timestamp is later than resource timestamp
+		}
+	}
+	return false, nil
+}
+
+func needsCreate(
+	targetResourceName ResourceName,
+	dependencyMaps []map[string]time.Time,
+	generatedResource *rpc.GeneratedResource) (bool, error) {
+	for i, dependency := range generatedResource.Dependencies {
+		dMap := dependencyMaps[i]
+		// Get the entity to look for in dependencyMap
+		entityKey, err := getEntityKey(dependency.Pattern, targetResourceName)
+		if err != nil {
+			// This means that there is error in the pattern definition, hence return
+			return false, fmt.Errorf("cannot match resource with dependency. Error: %s", err.Error())
+		}
+
+		// All the dependencies should be present to generate an action.
+		if _, ok := dMap[entityKey]; !ok {
+			return false, nil
+		}
+	}
+	return true, nil
+}
diff --git a/cmd/registry/controller/parser.go b/cmd/registry/controller/parser.go
index 791593b..8d96205 100644
--- a/cmd/registry/controller/parser.go
+++ b/cmd/registry/controller/parser.go
@@ -128,64 +128,33 @@ func extendDependencyPattern(
 
 }
 
-// This function is used in case of receipt artifacts where the name of the target resource to create is not known
-func resourceNameFromGroupKey(
+func resourceNameFromParent(
 	resourcePattern string,
-	groupKey string) (string, error) {
-	// Derives the resource name from the provided resourcePattern and groupKey.
+	parent string) (ResourceName, error) {
+	// Derives the resource name from the provided resourcePattern and it's parent.
 	// Example:
-	// 1) resourcePattern: projects/demo/locations/global/apis/-/versions/-/specs/-
-	//    groupKey: projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/complexity
+	// 1) resourcePattern: projects/demo/locations/global/apis/-/versions/-/specs/openapi.yaml
+	//    parent: projects/demo/locations/global/apis/petstore/versions/1.0.0
 	//    returns projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml
 	// 2) resourcePattern: projects/demo/locations/global/apis/petstore/versions/-/specs/-/artifacts/custom-artifact
-	//    groupKey: projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/complexity
+	//    parent: projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml
 	//    returns projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/custom-artifact
 
-	groupName, err := parseResource(groupKey)
-	resourceName := resourcePattern
-
-	if err == nil {
-		// Replace `apis/-` pattern with the corresponding name.
-		// groupName.GetApi() returns the full api name projects/demo/locations/global/apis/petstore
-		// We use stringsSplit()[-1] to extract only the API name
-		apiName := strings.Split(groupName.GetApi(), "/")
-		if len(apiName) > 0 {
-			resourceName = strings.ReplaceAll(resourceName, "/apis/-",
-				fmt.Sprintf("/apis/%s", apiName[len(apiName)-1]))
-		}
-
-		versionName := strings.Split(groupName.GetVersion(), "/")
-		if len(versionName) > 0 {
-			resourceName = strings.ReplaceAll(resourceName, "/versions/-",
-				fmt.Sprintf("/versions/%s", versionName[len(versionName)-1]))
-		}
+	parsedResourcePattern, err := parseResourcePattern(resourcePattern)
+	if err != nil {
+		return nil, fmt.Errorf("Invalid target Pattern: %s", err)
+	}
 
-		specName := strings.Split(groupName.GetSpec(), "/")
-		if len(specName) > 0 {
-			resourceName = strings.ReplaceAll(resourceName, "/specs/-",
-				fmt.Sprintf("/specs/%s", specName[len(specName)-1]))
-		}
+	// Replace the parent pattern in the resourcePattern with the supplied pattern name
+	resourceName := strings.Replace(resourcePattern, parsedResourcePattern.GetParent(), parent, 1)
 
-		artifactName := strings.Split(groupName.GetArtifact(), "/")
-		if len(artifactName) > 0 {
-			resourceName = strings.ReplaceAll(resourceName, "/artifacts/-",
-				fmt.Sprintf("/artifacts/%s", artifactName[len(artifactName)-1]))
-		}
-	}
 	//Validate generated resourceName
-	if _, err := names.ParseProject(resourceName); err == nil {
-		return resourceName, nil
-	} else if _, err := names.ParseApi(resourceName); err == nil {
-		return resourceName, nil
-	} else if _, err := names.ParseVersion(resourceName); err == nil {
-		return resourceName, nil
-	} else if _, err := names.ParseSpec(resourceName); err == nil {
-		return resourceName, nil
-	} else if _, err := names.ParseArtifact(resourceName); err == nil {
-		return resourceName, nil
+	resource, err := parseResource(resourceName)
+	if err != nil {
+		return nil, fmt.Errorf("invalid pattern: %q cannot derive GeneratedResource name from parent %s", resourcePattern, parent)
 	}
 
-	return "", fmt.Errorf("invalid pattern: %q cannot derive GeneratedResource name from groupKey %s", resourcePattern, groupKey)
+	return resource, nil
 }
 
 func ValidateResourceEntry(resource *rpc.GeneratedResource) error {
@@ -193,7 +162,7 @@ func ValidateResourceEntry(resource *rpc.GeneratedResource) error {
 
 	for _, dependency := range resource.Dependencies {
 		// Validate that all the dependencies are grouped at the same level
-		groupEntity, err := getGroupEntity(dependency.Pattern)
+		groupEntity, err := getEntityType(dependency.Pattern)
 		if err != nil {
 			return err
 		}
@@ -223,13 +192,13 @@ func ValidateResourceEntry(resource *rpc.GeneratedResource) error {
 	return nil
 }
 
-func getGroupEntity(pattern string) (string, error) {
-	// Reads the sourcePattern, finds out group by entity type
+func getEntityType(sourcePattern string) (string, error) {
+	// Reads the sourcePattern, finds out entity type in the $resource refernce
 	// Example:
 	// pattern: $resource.api/versions/-/specs/-
-	// returns "api", default if no group is present
+	// returns "api", default if no reference is present
 
-	if !strings.HasPrefix(pattern, resourceKW) {
+	if !strings.HasPrefix(sourcePattern, resourceKW) {
 		return "default", nil
 	}
 
@@ -239,22 +208,22 @@ func getGroupEntity(pattern string) (string, error) {
 	// ["$resource.api", "api"]
 	re := regexp.MustCompile(fmt.Sprintf(`\%s\.(api|version|spec|artifact)(/|$)`, resourceKW))
 
-	matches := re.FindStringSubmatch(pattern)
+	matches := re.FindStringSubmatch(sourcePattern)
 	if len(matches) <= 1 {
-		return "", fmt.Errorf("invalid pattern: Cannot extract group from pattern %s", pattern)
+		return "", fmt.Errorf("invalid pattern: Cannot extract entityType from pattern %s", sourcePattern)
 	}
 
 	return matches[1], nil
 }
 
-func getGroupKey(pattern string, resource ResourceInstance) (string, error) {
-	// Reads the pattern and returns the group value for the resource
+func getEntityKey(sourcePattern string, resource ResourceName) (string, error) {
+	// Reads the sourcePattern and returns the entity value for the $resource reference
 	// Example:
 	// pattern: $resource.api/versions/-/specs/-
 	// resource: "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml"
 	// returns "projects/demo/locations/global/apis/petstore"
 
-	entityType, err := getGroupEntity(pattern)
+	entityType, err := getEntityType(sourcePattern)
 	if err != nil {
 		return "", err
 	}
@@ -271,7 +240,7 @@ func getGroupKey(pattern string, resource ResourceInstance) (string, error) {
 	case "default":
 		return "default", nil
 	default:
-		return "", fmt.Errorf("invalid pattern: Cannot extract group from pattern %s", pattern)
+		return "", fmt.Errorf("invalid pattern: Cannot extract entity from sourcePattern %s", sourcePattern)
 	}
 
 }
diff --git a/cmd/registry/controller/resource-name.go b/cmd/registry/controller/resource-name.go
index dfe9e1b..7def94c 100644
--- a/cmd/registry/controller/resource-name.go
+++ b/cmd/registry/controller/resource-name.go
@@ -27,7 +27,8 @@ type ResourceName interface {
 	GetSpec() string
 	GetVersion() string
 	GetApi() string
-	GetName() string
+	String() string
+	GetParent() string
 }
 
 type SpecName struct {
@@ -50,10 +51,14 @@ func (s SpecName) GetApi() string {
 	return s.Spec.Api().String()
 }
 
-func (s SpecName) GetName() string {
+func (s SpecName) String() string {
 	return s.Spec.String()
 }
 
+func (s SpecName) GetParent() string {
+	return s.Spec.Parent()
+}
+
 type VersionName struct {
 	Version names.Version
 }
@@ -74,10 +79,14 @@ func (v VersionName) GetApi() string {
 	return v.Version.Api().String()
 }
 
-func (v VersionName) GetName() string {
+func (v VersionName) String() string {
 	return v.Version.String()
 }
 
+func (v VersionName) GetParent() string {
+	return v.Version.Parent()
+}
+
 type ApiName struct {
 	Api names.Api
 }
@@ -98,10 +107,14 @@ func (a ApiName) GetApi() string {
 	return a.Api.String()
 }
 
-func (a ApiName) GetName() string {
+func (a ApiName) String() string {
 	return a.Api.String()
 }
 
+func (a ApiName) GetParent() string {
+	return a.Api.Parent()
+}
+
 type ArtifactName struct {
 	Artifact names.Artifact
 }
@@ -159,6 +172,10 @@ func (ar ArtifactName) GetApi() string {
 	return ""
 }
 
-func (ar ArtifactName) GetName() string {
+func (ar ArtifactName) String() string {
 	return ar.Artifact.String()
 }
+
+func (ar ArtifactName) GetParent() string {
+	return ar.Artifact.Parent()
+}
diff --git a/cmd/registry/controller/resource.go b/cmd/registry/controller/resource.go
index 3f00e11..14631b1 100644
--- a/cmd/registry/controller/resource.go
+++ b/cmd/registry/controller/resource.go
@@ -21,12 +21,12 @@ import (
 // This interface is used to describe resource instances
 // ResourceName is embeded, the only additional field is the UpdateTimestamp
 type ResourceInstance interface {
-	ResourceName
+	GetResourceName() ResourceName
 	GetUpdateTimestamp() time.Time
 }
 
 type SpecResource struct {
-	SpecName
+	SpecName        ResourceName
 	UpdateTimestamp time.Time
 }
 
@@ -34,8 +34,12 @@ func (s SpecResource) GetUpdateTimestamp() time.Time {
 	return s.UpdateTimestamp
 }
 
+func (s SpecResource) GetResourceName() ResourceName {
+	return s.SpecName
+}
+
 type VersionResource struct {
-	VersionName
+	VersionName     ResourceName
 	UpdateTimestamp time.Time
 }
 
@@ -43,8 +47,12 @@ func (v VersionResource) GetUpdateTimestamp() time.Time {
 	return v.UpdateTimestamp
 }
 
+func (v VersionResource) GetResourceName() ResourceName {
+	return v.VersionName
+}
+
 type ApiResource struct {
-	ApiName
+	ApiName         ResourceName
 	UpdateTimestamp time.Time
 }
 
@@ -52,11 +60,19 @@ func (a ApiResource) GetUpdateTimestamp() time.Time {
 	return a.UpdateTimestamp
 }
 
+func (a ApiResource) GetResourceName() ResourceName {
+	return a.ApiName
+}
+
 type ArtifactResource struct {
-	ArtifactName
+	ArtifactName    ResourceName
 	UpdateTimestamp time.Time
 }
 
 func (ar ArtifactResource) GetUpdateTimestamp() time.Time {
 	return ar.UpdateTimestamp
 }
+
+func (ar ArtifactResource) GetResourceName() ResourceName {
+	return ar.ArtifactName
+}


Test patch:
diff --git a/cmd/registry/controller/parser_test.go b/cmd/registry/controller/parser_test.go
index a39f2ca..7ede80e 100644
--- a/cmd/registry/controller/parser_test.go
+++ b/cmd/registry/controller/parser_test.go
@@ -18,7 +18,6 @@ import (
 	"fmt"
 	"testing"
 
-	// "github.com/apigee/registry/rpc"
 	"github.com/apigee/registry/server/registry/names"
 )
 
@@ -31,6 +30,15 @@ func generateSpec(t *testing.T, specName string) names.Spec {
 	return spec
 }
 
+func generateVersion(t *testing.T, versionName string) names.Version {
+	t.Helper()
+	version, err := names.ParseVersion(versionName)
+	if err != nil {
+		t.Fatalf("Failed generateSpec(%s): %s", versionName, err.Error())
+	}
+	return version
+}
+
 func generateArtifact(t *testing.T, artifactName string) names.Artifact {
 	t.Helper()
 	artifact, err := names.ParseArtifact(artifactName)
@@ -127,83 +135,83 @@ func TestExtendDependencyPatternError(t *testing.T) {
 	}
 }
 
-func TestResourceNameFromGroupKey(t *testing.T) {
+func TestResourceNameFromParent(t *testing.T) {
 	tests := []struct {
 		desc            string
 		resourcePattern string
-		groupKey        string
-		want            string
+		parent          string
+		want            ResourceName
 	}{
-		{
-			desc:            "api pattern",
-			resourcePattern: "projects/demo/locations/global/apis/-",
-			groupKey:        "projects/demo/locations/global/apis/petstore",
-			want:            "projects/demo/locations/global/apis/petstore",
-		},
 		{
 			desc:            "version pattern",
-			resourcePattern: "projects/demo/locations/global/apis/-/versions/-",
-			groupKey:        "projects/demo/locations/global/apis/petstore/versions/1.0.0",
-			want:            "projects/demo/locations/global/apis/petstore/versions/1.0.0",
+			resourcePattern: "projects/demo/locations/global/apis/-/versions/1.0.0",
+			parent:          "projects/demo/locations/global/apis/petstore",
+			want: VersionName{
+				Version: generateVersion(t, "projects/demo/locations/global/apis/petstore/versions/1.0.0"),
+			},
 		},
 		{
 			desc:            "spec pattern",
-			resourcePattern: "projects/demo/locations/global/apis/petstore/versions/-/specs/-/artifacts/complexity",
-			groupKey:        "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
-			want:            "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/complexity",
+			resourcePattern: "projects/demo/locations/global/apis/-/versions/-/specs/openapi.yaml",
+			parent:          "projects/demo/locations/global/apis/petstore/versions/1.0.0",
+			want: SpecName{
+				Spec: generateSpec(t, "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml"),
+			},
 		},
 		{
 			desc:            "artifact pattern",
-			resourcePattern: "projects/demo/locations/global/apis/-/versions/-/specs/-/artifacts/-",
-			groupKey:        "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/complexity",
-			want:            "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/complexity",
+			resourcePattern: "projects/demo/locations/global/apis/-/versions/-/specs/-/artifacts/complexity",
+			parent:          "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
+			want: ArtifactName{
+				Artifact: generateArtifact(t, "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml/artifacts/complexity"),
+			},
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			got, err := resourceNameFromGroupKey(test.resourcePattern, test.groupKey)
+			got, err := resourceNameFromParent(test.resourcePattern, test.parent)
 			if err != nil {
-				t.Errorf("resourceNameFromGroupKey returned unexpected error: %s", err)
+				t.Errorf("resourceNameFromEntityKey returned unexpected error: %s", err)
 			}
 			if got != test.want {
-				t.Errorf("resourceNameFromGroupKey returned unexpected value want: %q got:%q", test.want, got)
+				t.Errorf("resourceNameFromEntityKey returned unexpected value want: %q got:%q", test.want, got)
 			}
 		})
 	}
 
 }
 
-func TestResourceNameFromGroupKeyError(t *testing.T) {
+func TestResourceNameFromParentError(t *testing.T) {
 	tests := []struct {
 		desc            string
 		resourcePattern string
-		groupKey        string
+		parent          string
 	}{
 		{
 			desc:            "incorrect keywords",
 			resourcePattern: "projects/demo/locations/global/apis/-/versions/-/apispecs/-",
-			groupKey:        "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
+			parent:          "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
 		},
 		{
 			desc:            "incorrect pattern",
 			resourcePattern: "projects/demo/locations/global/apis/-/specs/-",
-			groupKey:        "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
+			parent:          "projects/demo/locations/global/apis/petstore/versions/1.0.0/specs/openapi.yaml",
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			got, err := resourceNameFromGroupKey(test.resourcePattern, test.groupKey)
+			got, err := resourceNameFromParent(test.resourcePattern, test.parent)
 			if err == nil {
-				t.Errorf("expected resourceNameFromGroupKey to return error, got: %q", got)
+				t.Errorf("expected resourceNameFromEntityKey to return error, got: %q", got)
 			}
 		})
 	}
 
 }
 
-func TestGetGroupKey(t *testing.T) {
+func TestGetEntityKey(t *testing.T) {
 	tests := []struct {
 		desc     string
 		pattern  string
@@ -254,18 +262,18 @@ func TestGetGroupKey(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			got, err := getGroupKey(test.pattern, test.resource)
+			got, err := getEntityKey(test.pattern, test.resource.GetResourceName())
 			if err != nil {
-				t.Errorf("getGroupKey returned unexpected error: %s", err)
+				t.Errorf("getEntityKey returned unexpected error: %s", err)
 			}
 			if got != test.want {
-				t.Errorf("getGroupKey returned unexpected value want: %q got:%q", test.want, got)
+				t.Errorf("getEntityKey returned unexpected value want: %q got:%q", test.want, got)
 			}
 		})
 	}
 }
 
-func TestGetGroupKeyError(t *testing.T) {
+func TestGetEntityKeyError(t *testing.T) {
 	tests := []struct {
 		desc     string
 		pattern  string
@@ -296,9 +304,9 @@ func TestGetGroupKeyError(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			got, err := getGroupKey(test.pattern, test.resource)
+			got, err := getEntityKey(test.pattern, test.resource.GetResourceName())
 			if err == nil {
-				t.Errorf("expected getGroupKey to return error, got: %q", got)
+				t.Errorf("expected getEntityKey to return error, got: %q", got)
 			}
 		})
 	}