From 903d08e37ba8421b556275018bd9b167a26f39f9 Mon Sep 17 00:00:00 2001 From: Tim Burks Date: Wed, 25 Jan 2023 17:00:47 -0800 Subject: [PATCH] Allow uploads of CSV to hosted registries. (#972) * Allow uploads of CSV to hosted registries. * Update "export csv" usage to tool conventions. * Test a few `upload csv` error-handling situations. --- cmd/registry/cmd/upload/csv.go | 83 ++++++++++++++-------------- cmd/registry/cmd/upload/csv_test.go | 85 ++++++++++++++++++++++++++++- 2 files changed, 122 insertions(+), 46 deletions(-) diff --git a/cmd/registry/cmd/upload/csv.go b/cmd/registry/cmd/upload/csv.go index bab231673..9b98ee065 100644 --- a/cmd/registry/cmd/upload/csv.go +++ b/cmd/registry/cmd/upload/csv.go @@ -35,38 +35,33 @@ import ( func csvCommand() *cobra.Command { var ( - projectID string delimiter string jobs int ) cmd := &cobra.Command{ - Use: "csv file --project-id=value [--delimiter=value]", + Use: "csv FILE", Short: "Upload API descriptions from a CSV file", Args: cobra.ExactArgs(1), - Run: func(cmd *cobra.Command, args []string) { - ctx := cmd.Context() + RunE: func(cmd *cobra.Command, args []string) error { if len(delimiter) != 1 { - log.Fatalf(ctx, "Invalid delimiter %q: must be exactly one character", delimiter) + return fmt.Errorf("invalid delimiter %q: must be exactly one character", delimiter) } - - client, err := connection.NewRegistryClient(ctx) + ctx := cmd.Context() + parent, err := getParent(cmd) if err != nil { - log.FromContext(ctx).WithError(err).Fatal("Failed to get client") + return fmt.Errorf("failed to identify parent project (%s)", err) } - - adminClient, err := connection.NewAdminClient(ctx) + parentName, err := names.ParseProjectWithLocation(parent) if err != nil { - log.FromContext(ctx).WithError(err).Fatal("Failed to get client") + return fmt.Errorf("error parsing project name (%s)", err) } - - if _, err := adminClient.UpdateProject(ctx, &rpc.UpdateProjectRequest{ - Project: &rpc.Project{ - Name: names.Project{ProjectID: projectID}.String(), - }, - AllowMissing: true, - }); err != nil { - log.FromContext(ctx).WithError(err).Fatal("Failed to ensure project exists") + client, err := connection.NewRegistryClient(ctx) + if err != nil { + return fmt.Errorf("error getting client (%s)", err) + } + if err := core.VerifyLocation(ctx, client, parent); err != nil { + return fmt.Errorf("parent does not exist (%s)", err) } taskQueue, wait := core.WorkerPool(ctx, jobs) @@ -74,7 +69,7 @@ func csvCommand() *cobra.Command { file, err := os.Open(args[0]) if err != nil { - log.FromContext(ctx).WithError(err).Fatal("Failed to open file") + return fmt.Errorf("failed to open file (%s)", err) } defer file.Close() @@ -85,23 +80,22 @@ func csvCommand() *cobra.Command { for row, err := r.Read(); err != io.EOF; row, err = r.Read() { if err != nil { - log.FromContext(ctx).WithError(err).Fatal("Failed to read row from file") + return fmt.Errorf("failed to read row from file (%s)", err) } taskQueue <- &uploadSpecTask{ - client: client, - projectID: projectID, - apiID: row.ApiID, - versionID: row.VersionID, - specID: row.SpecID, - filepath: row.Filepath, + client: client, + parentName: parentName, + apiID: row.ApiID, + versionID: row.VersionID, + specID: row.SpecID, + filepath: row.Filepath, } } + return nil }, } - cmd.Flags().StringVar(&projectID, "project-id", "", "project ID to use for each upload") - _ = cmd.MarkFlagRequired("project-id") cmd.Flags().StringVar(&delimiter, "delimiter", ",", "field delimiter for the CSV file") cmd.Flags().IntVarP(&jobs, "jobs", "j", 10, "number of actions to perform concurrently") return cmd @@ -180,18 +174,19 @@ func (r *uploadCSVReader) buildColumnIndex(header []string) error { } type uploadSpecTask struct { - client connection.RegistryClient - projectID string - apiID string - versionID string - specID string - filepath string + client connection.RegistryClient + parentName names.Project + apiID string + versionID string + specID string + filepath string } func (t uploadSpecTask) Run(ctx context.Context) error { + apiName := t.parentName.Api(t.apiID) api, err := t.client.CreateApi(ctx, &rpc.CreateApiRequest{ - Parent: fmt.Sprintf("projects/%s/locations/global", t.projectID), - ApiId: t.apiID, + Parent: apiName.Parent(), + ApiId: apiName.ApiID, Api: &rpc.Api{}, }) @@ -200,15 +195,16 @@ func (t uploadSpecTask) Run(ctx context.Context) error { log.Debugf(ctx, "Created API: %s", api.GetName()) case codes.AlreadyExists: api = &rpc.Api{ - Name: fmt.Sprintf("projects/%s/locations/global/apis/%s", t.projectID, t.apiID), + Name: apiName.String(), } default: return fmt.Errorf("failed to ensure API exists: %s", err) } + versionName := apiName.Version(t.versionID) version, err := t.client.CreateApiVersion(ctx, &rpc.CreateApiVersionRequest{ - Parent: api.GetName(), - ApiVersionId: t.versionID, + Parent: versionName.Parent(), + ApiVersionId: versionName.VersionID, ApiVersion: &rpc.ApiVersion{}, }) @@ -217,7 +213,7 @@ func (t uploadSpecTask) Run(ctx context.Context) error { log.Debugf(ctx, "Created API version: %s", version.GetName()) case codes.AlreadyExists: version = &rpc.ApiVersion{ - Name: fmt.Sprintf("projects/%s/locations/global/apis/%s/versions/%s", t.projectID, t.apiID, t.versionID), + Name: versionName.String(), } default: return fmt.Errorf("failed to ensure API version exists: %s", err) @@ -233,9 +229,10 @@ func (t uploadSpecTask) Run(ctx context.Context) error { return err } + specName := versionName.Spec(t.specID) spec, err := t.client.CreateApiSpec(ctx, &rpc.CreateApiSpecRequest{ - Parent: version.GetName(), - ApiSpecId: t.specID, + Parent: specName.Parent(), + ApiSpecId: specName.SpecID, ApiSpec: &rpc.ApiSpec{ // TODO: How do we choose a mime type? MimeType: types.OpenAPIMimeType("+gzip", "3.0.0"), diff --git a/cmd/registry/cmd/upload/csv_test.go b/cmd/registry/cmd/upload/csv_test.go index 1ba6b02ba..4acec2b97 100644 --- a/cmd/registry/cmd/upload/csv_test.go +++ b/cmd/registry/cmd/upload/csv_test.go @@ -57,6 +57,7 @@ func TestUploadCSV(t *testing.T) { } const testProject = "csv-demo" + const testParent = "projects/" + testProject + "/locations/global" tests := []struct { desc string args []string @@ -66,7 +67,7 @@ func TestUploadCSV(t *testing.T) { desc: "multiple spec upload", args: []string{ filepath.Join("testdata", "csv", "multiple-specs.csv"), - "--project-id", testProject, + "--parent", testParent, }, want: []*rpc.ApiSpec{ { @@ -95,7 +96,7 @@ func TestUploadCSV(t *testing.T) { desc: "out of order columns", args: []string{ filepath.Join("testdata", "csv", "out-of-order-columns.csv"), - "--project-id", testProject, + "--parent", testParent, }, want: []*rpc.ApiSpec{ { @@ -109,7 +110,7 @@ func TestUploadCSV(t *testing.T) { desc: "empty sheet", args: []string{ filepath.Join("testdata", "csv", "empty-sheet.csv"), - "--project-id", testProject, + "--parent", testParent, }, want: []*rpc.ApiSpec{}, }, @@ -135,6 +136,17 @@ func TestUploadCSV(t *testing.T) { t.Fatalf("Setup: Failed to delete test project: %s", err) } + _, err = adminClient.CreateProject(ctx, &rpc.CreateProjectRequest{ + ProjectId: testProject, + Project: &rpc.Project{ + DisplayName: "Test", + Description: "A test catalog", + }, + }) + if err != nil { + t.Fatalf("Error creating project %s", err) + } + args := append([]string{"csv"}, test.args...) cmd := Command() @@ -179,3 +191,70 @@ func TestUploadCSV(t *testing.T) { }) } } + +func TestUploadCSVErrors(t *testing.T) { + const testProject = "csv-errors" + const testParent = "projects/" + testProject + "/locations/global" + tests := []struct { + desc string + args []string + }{ + { + desc: "invalid parent", + args: []string{ + filepath.Join("testdata", "csv", "multiple-specs.csv"), + "--parent", "invalid", + }, + }, + { + desc: "missing parent", + args: []string{ + filepath.Join("testdata", "csv", "out-of-order-columns.csv"), + "--parent", "projects/missing/locations/global", + }, + }, + { + desc: "missing input file", + args: []string{ + filepath.Join("testdata", "csv", "missing.csv"), + "--parent", testParent, + }, + }, + } + + ctx := context.Background() + adminClient, err := connection.NewAdminClient(ctx) + if err != nil { + t.Fatalf("Setup: Failed to create client: %s", err) + } + err = adminClient.DeleteProject(ctx, &rpc.DeleteProjectRequest{ + Name: "projects/" + testProject, + Force: true, + }) + if err != nil && status.Code(err) != codes.NotFound { + t.Fatalf("Setup: Failed to delete test project: %s", err) + } + _, err = adminClient.CreateProject(ctx, &rpc.CreateProjectRequest{ + ProjectId: testProject, + Project: &rpc.Project{ + DisplayName: "Test", + Description: "A test catalog", + }, + }) + if err != nil { + t.Fatalf("Error creating project %s", err) + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + args := append([]string{"csv"}, test.args...) + cmd := Command() + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs(args) + if err := cmd.Execute(); err == nil { + t.Fatalf("Execute() with args %v succeeded and should have failed", args) + } + }) + } +}