Repository: apigee/registry
Pull request #728 (2022-09-08 02:50:48)

Upload dependencies of protos in uploaded APIs.
This makes proto-based API specs fully self-contained. Before uploading protos, the uploader uses `protoc` to compile them and collect a list of referenced files, repeating until all transitive inclusions are listed, and then all of these files are included in the zip archive, making the uploaded spec "fully resolved". The resulting zip archives can be unzipped in a fresh directory and compiled with `protoc` for use in any other desired applications.

A new `--protoc-root` option allows the bulk uploader to upload individual protobuf specs. When `--protoc-root` is used to specify the proto compilation root, the `PATH` argument can point to a subdirectory of that root that includes a directory containing a single API. For example, the registry API can be individually uploaded with the following invocation:
```
registry upload bulk protos ~/googleapis/google/cloud/apigeeregistry/v1 --protoc-root ~/googleapis --project-id googleapis
```
An accompanying change updates the api-linter plugin to work correctly with these uploaded specs. It excludes common protos from linting and removes the prior (ugly) workarounds for incomplete specs (downloading [api-common-protos](https://github.com/googleapis/api-common-protos) and [googleapis](https://github.com/googleapis/googleapis) to resolve missing dependencies).

Here's a simple demonstration that uploads Google API protos and runs [api-linter](https://linter.aip.dev/)-based conformance checks:
```
# Create a sample project.
registry rpc admin create-project --project_id googleapis

# Load the protos for Google APIs.
# This assumes github.com/googleapis/googleapis is cloned to your home directory.
# Proto specs are uploaded as zip files that contain all the protos needed to compile the API spec.
registry upload bulk protos ~/googleapis --project-id googleapis

# Load the AIP-based style guide (get it from https://github.com/apigee/registry-experimental/pull/97).
registry apply -f apihub-styleguide.yaml --parent projects/googleapis/locations/global

# Compute conformance for all APIs. This uses the AIP style guide for all proto-based APIs.
registry compute conformance projects/googleapis/locations/global/apis/-/versions/-/specs/-

# Get one of the conformance reports.
registry get projects/googleapis/locations/global/apis/apigeeregistry/versions/v1/specs/google-cloud-apigeeregistry-v1.zip/artifacts/conformance-apihub-styleguide --print

# Count the conformance reports and specs. The numbers should match.
registry list projects/googleapis/locations/global/apis/-/versions/-/specs/-/artifacts/conformance-apihub-styleguide | wc -l
registry list projects/googleapis/locations/global/apis/-/versions/-/specs/- | wc -l
```

Known problems:
- No tests: #739

codecov[bot]: # [Codecov](https://codecov.io/gh/apigee/registry/pull/728?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) Report
> Merging [#728](https://codecov.io/gh/apigee/registry/pull/728?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (3c1600a) into [main](https://codecov.io/gh/apigee/registry/commit/86315f3d19fec7f2cdc55a8c1ca244319bde1ffb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (86315f3) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.

```diff
@@            Coverage Diff             @@
##             main     #728      +/-   ##
==========================================
- Coverage   57.21%   57.17%   -0.04%     
==========================================
  Files          93       93              
  Lines        8005     8005              
==========================================
- Hits         4580     4577       -3     
- Misses       2999     3001       +2     
- Partials      426      427       +1     
```


| [Impacted Files](https://codecov.io/gh/apigee/registry/pull/728?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) | Coverage Δ | |
|---|---|---|
| [server/registry/actions\_specs.go](https://codecov.io/gh/apigee/registry/pull/728/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-c2VydmVyL3JlZ2lzdHJ5L2FjdGlvbnNfc3BlY3MuZ28=) | `78.37% <0.00%> (-1.36%)` | :arrow_down: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee)

shrutiparabgoogle: Looks good to me. 

At some point, should we also provide some option to do this through the UI? If a user uploads a proto through the UI without resolving dependencies, the api linter will fail to lint such specs.
timburks: @theganyo Thanks - the option that I meant is "--protoc-root" (I renamed it). I've updated the description to show that. 
theganyo: Just curious: Would it ever possibly make sense to not gather and include all transitive dependencies or to limit them in any way?
timburks: > Just curious: Would it ever possibly make sense to not gather and include all transitive dependencies or to limit them in any way?

Maybe? That was what I originally thought that we should do -- upload just what is in a directory and then when compilation is needed, trace all the dependencies and unbundle them together. This requires putting the common protos in the registry as "APIs" and building all the dependency tracing. Meanwhile, @earth2marsh has been preferring that we store "fully resolved specs" in the registry with no dependencies -- here we're registering a snapshot of an API as a user would consume it, and changes to the dependencies could change user experience... i.e. dependencies should be frozen, which is what "fully resolving" here does.

So this is an opinionated upload, but I think that's good for where we are right now. It simplifies downstream tooling and matches what @earth2marsh was wanting all along.

Patch:
diff --git a/cmd/registry/cmd/upload/bulk/protos.go b/cmd/registry/cmd/upload/bulk/protos.go
index 0f11c21..c9ef518 100644
--- a/cmd/registry/cmd/upload/bulk/protos.go
+++ b/cmd/registry/cmd/upload/bulk/protos.go
@@ -19,9 +19,11 @@ import (
 	"fmt"
 	"io/fs"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/apigee/registry/cmd/registry/core"
@@ -31,6 +33,8 @@ import (
 	"github.com/spf13/cobra"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/descriptorpb"
 	"gopkg.in/yaml.v3"
 )
 
@@ -49,10 +53,11 @@ type ServiceConfig struct {
 
 func protosCommand() *cobra.Command {
 	var baseURI string
+	var root string
 	cmd := &cobra.Command{
-		Use:   "protos",
+		Use:   "protos PATH",
 		Short: "Bulk-upload Protocol Buffer descriptions from a directory of specs",
-		Args:  cobra.MinimumNArgs(1),
+		Args:  cobra.ExactArgs(1),
 		Run: func(cmd *cobra.Command, args []string) {
 			ctx := cmd.Context()
 			projectID, err := cmd.Flags().GetString("project-id")
@@ -79,19 +84,29 @@ func protosCommand() *cobra.Command {
 					log.FromContext(ctx).WithError(err).Fatal("Invalid path")
 				}
 
-				if err := scanDirectoryForProtos(client, projectID, baseURI, path, taskQueue); err != nil {
+				if root == "" {
+					root = path
+				}
+				root, err := filepath.Abs(root)
+				if err != nil {
+					log.FromContext(ctx).WithError(err).Fatal("Invalid path")
+				}
+
+				if err := scanDirectoryForProtos(client, projectID, baseURI, path, root, taskQueue); err != nil {
 					log.FromContext(ctx).WithError(err).Debug("Failed to walk directory")
 				}
 			}
 		},
 	}
 
+	cmd.Flags().StringVar(&root, "protoc-root", "", "Root directory to use for proto compilation, defaults to PATH")
+
 	cmd.Flags().StringVar(&baseURI, "base-uri", "", "Prefix to use for the source_uri field of each proto upload")
 	return cmd
 }
 
-func scanDirectoryForProtos(client connection.RegistryClient, projectID, baseURI, root string, taskQueue chan<- core.Task) error {
-	return filepath.Walk(root, func(filepath string, info os.FileInfo, err error) error {
+func scanDirectoryForProtos(client connection.RegistryClient, projectID, baseURI, start, root string, taskQueue chan<- core.Task) error {
+	return filepath.Walk(start, func(filepath string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
@@ -144,6 +159,7 @@ type uploadProtoTask struct {
 	apiDescription string
 	versionID      string // computed at runtime
 	specID         string // computed at runtime
+	contents       []byte // computed at runtime
 }
 
 func (task *uploadProtoTask) String() string {
@@ -155,6 +171,11 @@ func (task *uploadProtoTask) Run(ctx context.Context) error {
 	task.populateFields()
 	log.Infof(ctx, "Uploading apis/%s/versions/%s/specs/%s", task.apiID, task.versionID, task.specID)
 
+	// Zip up the protos first; if that fails, skip the API.
+	var err error
+	if task.contents, err = task.zipContents(); err != nil {
+		return err
+	}
 	// If the API does not exist, create it.
 	if err := task.createAPI(ctx); err != nil {
 		return err
@@ -223,17 +244,12 @@ func (task *uploadProtoTask) createVersion(ctx context.Context) error {
 }
 
 func (task *uploadProtoTask) createOrUpdateSpec(ctx context.Context) error {
-	contents, err := task.zipContents()
-	if err != nil {
-		return err
-	}
-
 	// Use the spec size and hash to avoid unnecessary uploads.
 	spec, err := task.client.GetApiSpec(ctx, &rpc.GetApiSpecRequest{
 		Name: task.specName(),
 	})
 
-	if err == nil && int(spec.GetSizeBytes()) == len(contents) && spec.GetHash() == hashForBytes(contents) {
+	if err == nil && int(spec.GetSizeBytes()) == len(task.contents) && spec.GetHash() == hashForBytes(task.contents) {
 		log.Debugf(ctx, "Matched already uploaded spec %s", task.specName())
 		return nil
 	}
@@ -243,7 +259,7 @@ func (task *uploadProtoTask) createOrUpdateSpec(ctx context.Context) error {
 			Name:     task.specName(),
 			MimeType: core.ProtobufMimeType("+zip"),
 			Filename: task.fileName(),
-			Contents: contents,
+			Contents: task.contents,
 		},
 		AllowMissing: true,
 	}
@@ -253,7 +269,7 @@ func (task *uploadProtoTask) createOrUpdateSpec(ctx context.Context) error {
 
 	response, err := task.client.UpdateApiSpec(ctx, request)
 	if err != nil {
-		log.FromContext(ctx).WithError(err).Errorf("Error %s [contents-length: %d]", task.specName(), len(contents))
+		log.FromContext(ctx).WithError(err).Errorf("Error %s [contents-length: %d]", task.specName(), len(task.contents))
 	} else {
 		log.Debugf(ctx, "Updated %s", response.Name)
 	}
@@ -289,10 +305,107 @@ func (task *uploadProtoTask) fileName() string {
 
 func (task *uploadProtoTask) zipContents() ([]byte, error) {
 	prefix := task.directory + "/"
-	contents, err := core.ZipArchiveOfPath(task.path, prefix, true)
+
+	// Get the proto files in the main directory.
+	protos, err := localProtos(task.path, prefix)
+	if err != nil {
+		return nil, err
+	}
+
+	// Compile the listed protos to get their dependencies.
+	if len(protos) > 0 {
+		protos, err = referencedProtos(protos, task.directory)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// Get the metadata files in the main directory.
+	metadata, err := localMetadata(task.path, prefix)
+	if err != nil {
+		return nil, err
+	}
+
+	// Zip the listed files.
+	contents, err := core.ZipArchiveOfFiles(append(metadata, protos...), prefix, true)
 	if err != nil {
 		return nil, err
 	}
 
 	return contents.Bytes(), nil
 }
+
+// Collect the names of all metadata files in a source directory, stripping the prefix.
+func localMetadata(source, prefix string) ([]string, error) {
+	return localFiles(source, prefix, func(name string) bool {
+		return strings.HasSuffix(name, ".json") || strings.HasSuffix(name, ".yaml")
+	})
+}
+
+// Collect the names of all proto files in a source directory, stripping the prefix.
+func localProtos(source, prefix string) ([]string, error) {
+	return localFiles(source, prefix, func(name string) bool {
+		return strings.HasSuffix(name, ".proto")
+	})
+}
+
+// Collect the names of all matching files in a source directory, stripping the prefix.
+func localFiles(source, prefix string, match func(string) bool) ([]string, error) {
+	filenames := make([]string, 0)
+	err := filepath.WalkDir(source, func(p string, entry fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		} else if entry.IsDir() {
+			return nil // Do nothing for the directory, but still walk its contents.
+		} else if match(p) {
+			filenames = append(filenames, strings.TrimPrefix(p, prefix))
+		}
+		return nil
+	})
+	return filenames, err
+}
+
+// Get all the protos that are referenced in the compilation of a list of protos.
+func referencedProtos(protos []string, root string) ([]string, error) {
+	tempDir, err := os.MkdirTemp("", "proto-import-")
+	if err != nil {
+		return nil, err
+	}
+	defer os.RemoveAll(tempDir)
+	args := []string{"-o", tempDir + "/proto.pb"}
+	args = append(args, protos...)
+	cmd := exec.Command("protoc", args...)
+	cmd.Dir = root
+	if err := cmd.Run(); err != nil {
+		return nil, fmt.Errorf("failed to compile protos with protoc: %s", err)
+	}
+	return protosFromFileDescriptorSet(tempDir + "/proto.pb")
+}
+
+// Get all the protos listed as dependencies in a file descriptor set.
+func protosFromFileDescriptorSet(filename string) ([]string, error) {
+	bytes, err := os.ReadFile(filename)
+	if err != nil {
+		return nil, err
+	}
+	fds := &descriptorpb.FileDescriptorSet{}
+	err = proto.Unmarshal(bytes, fds)
+	if err != nil {
+		return nil, err
+	}
+	filenameset := make(map[string]bool)
+	for _, file := range fds.File {
+		filenameset[*file.Name] = true
+		for _, dependency := range file.Dependency {
+			if !strings.HasPrefix(dependency, "google/protobuf/") {
+				filenameset[dependency] = true
+			}
+		}
+	}
+	filenames := make([]string, 0)
+	for k := range filenameset {
+		filenames = append(filenames, k)
+	}
+	sort.Strings(filenames)
+	return filenames, nil
+}
diff --git a/cmd/registry/core/zip.go b/cmd/registry/core/zip.go
index 3f607a5..bbf8e2d 100644
--- a/cmd/registry/core/zip.go
+++ b/cmd/registry/core/zip.go
@@ -122,6 +122,20 @@ func ZipArchiveOfPath(path, prefix string, recursive bool) (buf bytes.Buffer, er
 	return buf, err
 }
 
+// ZipArchiveOfFiles stores a list of files in a zip archive.
+// The specified prefix is stripped from file names in the archive.
+func ZipArchiveOfFiles(files []string, prefix string, recursive bool) (buf bytes.Buffer, err error) {
+	zipWriter := zip.NewWriter(&buf)
+	defer zipWriter.Close()
+
+	for _, filename := range files {
+		if err = addFileToZip(zipWriter, prefix+filename, prefix); err != nil {
+			return buf, err
+		}
+	}
+	return buf, err
+}
+
 func addFileToZip(zipWriter *zip.Writer, filename, prefix string) error {
 	fileToZip, err := os.Open(filename)
 	if err != nil {
diff --git a/cmd/registry/plugins/registry-lint-api-linter/main.go b/cmd/registry/plugins/registry-lint-api-linter/main.go
index b32ad34..f8e64af 100644
--- a/cmd/registry/plugins/registry-lint-api-linter/main.go
+++ b/cmd/registry/plugins/registry-lint-api-linter/main.go
@@ -27,7 +27,7 @@ import (
 )
 
 // Runs the API linter with a provided spec path
-type runLinter func(specPath string) ([]*rpc.LintProblem, error)
+type runLinter func(specPath, specDir string) ([]*rpc.LintProblem, error)
 
 // apiLinterRunner implements the LinterRunner interface for the API linter.
 type apiLinterRunner struct{}
@@ -42,6 +42,7 @@ func (linter *apiLinterRunner) RunImpl(
 ) (*rpc.LinterResponse, error) {
 	lintFiles := make([]*rpc.LintFile, 0)
 
+	//log.Infof(context.TODO(), "REQUEST %+v", req)
 	// Traverse the files in the directory
 	err := filepath.Walk(req.GetSpecDirectory(), func(path string, info os.FileInfo, err error) error {
 		if err != nil {
@@ -53,8 +54,16 @@ func (linter *apiLinterRunner) RunImpl(
 			return nil
 		}
 
+		protoPath := strings.TrimPrefix(path, req.GetSpecDirectory()+"/")
+		if strings.HasPrefix(protoPath, "google/api/") ||
+			strings.HasPrefix(protoPath, "google/longrunning/") ||
+			strings.HasPrefix(protoPath, "google/rpc/") {
+			// Skip common includes.
+			return nil
+		}
+
 		// Execute the API linter.
-		lintProblems, err := runLinter(path)
+		lintProblems, err := runLinter(protoPath, req.GetSpecDirectory())
 		if err != nil {
 			return err
 		}
@@ -82,73 +91,22 @@ func (linter *apiLinterRunner) RunImpl(
 	}, nil
 }
 
-func runApiLinter(specPath string) ([]*rpc.LintProblem, error) {
-	// API-linter necessitates being ran on specs in the CWD to avoid many import errors,
-	// so we change the directory of the command to the directory of the spec.
-	specDirectory := filepath.Dir(specPath)
-	specName := filepath.Base(specPath)
-
-	data, err := createAndRunApiLinterCommand(specDirectory, specName)
-	if err == nil {
-		return parseLinterOutput(data)
-	}
-
+func runApiLinter(specPath, specDirectory string) ([]*rpc.LintProblem, error) {
 	// TODO: Replace this new instance with a logger inherited from the context.
 	logger := log.NewLogger()
+	logger.Infof("Running api-linter on %s", specPath)
 
-	// Unpack api-common-protos and try again if failure occurred
-	logger.Info("API-linter failed due to an import error, unpacking API common protos and retrying.")
-	if err = unpackApiCommonProtos(specDirectory); err == nil {
-		data, err = createAndRunApiLinterCommand(specDirectory, specName)
-		if err == nil {
-			return parseLinterOutput(data)
-		}
-	}
-
-	logger.Info("API-linter failed due to an import error, unpacking GoogleAPIs and retrying.")
-	if err = unpackGoogleApisProtos(specDirectory); err == nil {
-		data, err = createAndRunApiLinterCommand(specDirectory, specName)
-		if err == nil {
-			return parseLinterOutput(data)
-		}
+	data, err := createAndRunApiLinterCommand(specDirectory, specPath)
+	if err != nil {
+		return nil, err
 	}
-
-	return []*rpc.LintProblem{}, nil
+	return parseLinterOutput(data)
 }
 
 func main() {
 	lint.Main(&apiLinterRunner{})
 }
 
-func unpackGoogleApisProtos(rootDir string) error {
-	// Curl the entire folder as a zipped archive from Github (much faster than git checkout).
-	curlCmd := exec.Command("curl", "-L", "https://github.com/googleapis/googleapis/archive/refs/heads/master.zip", "-O")
-	curlCmd.Dir = rootDir
-	err := curlCmd.Run()
-	if err != nil {
-		return err
-	}
-
-	// Unzip the contents of the zipped archive.
-	unzipCmd := exec.Command("unzip", "-q", "master.zip")
-	unzipCmd.Dir = rootDir
-	err = unzipCmd.Run()
-	if err != nil {
-		return err
-	}
-
-	// Move up the google/ directory (the one we're interested in) into the cwd.
-	mvCmd := exec.Command("mv", "googleapis-master/google", "google")
-	mvCmd.Dir = rootDir
-	return mvCmd.Run()
-}
-
-func unpackApiCommonProtos(rootDir string) error {
-	cmd := exec.Command("git", "clone", "https://github.com/googleapis/api-common-protos")
-	cmd.Dir = rootDir
-	return cmd.Run()
-}
-
 func (linter *apiLinterRunner) filterProblems(
 	problems []*rpc.LintProblem,
 	rules []string,
@@ -177,8 +135,7 @@ func (linter *apiLinterRunner) filterProblems(
 func createAndRunApiLinterCommand(specDirectory, specName string) ([]byte, error) {
 	cmd := exec.Command("api-linter",
 		specName,
-		"-I", "google",
-		"-I", "api-common-protos",
+		"-I", ".",
 		"--output-format", "json",
 	)
 	cmd.Dir = specDirectory


Test patch:
diff --git a/cmd/registry/plugins/registry-lint-api-linter/main_test.go b/cmd/registry/plugins/registry-lint-api-linter/main_test.go
index 3d61653..d8c4504 100644
--- a/cmd/registry/plugins/registry-lint-api-linter/main_test.go
+++ b/cmd/registry/plugins/registry-lint-api-linter/main_test.go
@@ -50,7 +50,7 @@ func TestApiLinterPluginLintSpec(t *testing.T) {
 	}{
 		{
 			&apiLinterRunner{},
-			func(specPath string) ([]*rpc.LintProblem, error) {
+			func(specPath, specDir string) ([]*rpc.LintProblem, error) {
 				return []*rpc.LintProblem{
 						{
 							Message: "test",
@@ -103,7 +103,7 @@ func TestApiLinterPluginLintSpec(t *testing.T) {
 		},
 		{
 			&apiLinterRunner{},
-			func(specPath string) ([]*rpc.LintProblem, error) {
+			func(specPath, specDir string) ([]*rpc.LintProblem, error) {
 				return []*rpc.LintProblem{}, errors.New("test")
 			},
 			&rpc.LinterRequest{