From c8a914a605ed8b0f4f2bad30fab46c67376c3157 Mon Sep 17 00:00:00 2001 From: Mike Eltsufin Date: Thu, 23 Oct 2025 02:44:52 +0000 Subject: [PATCH 1/3] feat(librariangen): generate grpc stubs and resource helpers --- internal/librariangen/generate/generator.go | 106 ++++++++++----- .../librariangen/generate/generator_test.go | 122 ++++++++---------- internal/librariangen/protoc/protoc.go | 17 ++- internal/librariangen/protoc/protoc_test.go | 19 ++- internal/librariangen/run-generate-library.sh | 5 + 5 files changed, 162 insertions(+), 107 deletions(-) diff --git a/internal/librariangen/generate/generator.go b/internal/librariangen/generate/generator.go index 8a1b00e521..31ae9698af 100644 --- a/internal/librariangen/generate/generator.go +++ b/internal/librariangen/generate/generator.go @@ -78,26 +78,25 @@ func Generate(ctx context.Context, cfg *Config) error { return fmt.Errorf("librariangen: invalid configuration: %w", err) } slog.Debug("librariangen: generate command started") - defer cleanupIntermediateFiles(cfg.OutputDir) + outputConfig := &protoc.OutputConfig{ + GAPICDir: filepath.Join(cfg.OutputDir, "gapic"), + GRPCDir: filepath.Join(cfg.OutputDir, "grpc"), + ProtoDir: filepath.Join(cfg.OutputDir, "proto"), + } + defer cleanupIntermediateFiles(outputConfig) generateReq, err := readGenerateReq(cfg.LibrarianDir) if err != nil { return fmt.Errorf("librariangen: failed to read request: %w", err) } - if err := invokeProtoc(ctx, cfg, generateReq); err != nil { + if err := invokeProtoc(ctx, cfg, generateReq, outputConfig); err != nil { return fmt.Errorf("librariangen: gapic generation failed: %w", err) } - // Unzip the generated zip file. - zipPath := filepath.Join(cfg.OutputDir, "java_gapic.zip") - if err := unzip(zipPath, cfg.OutputDir); err != nil { - return fmt.Errorf("librariangen: failed to unzip %s: %w", zipPath, err) - } - - // Unzip the inner temp-codegen.srcjar. - srcjarPath := filepath.Join(cfg.OutputDir, "temp-codegen.srcjar") - srcjarDest := filepath.Join(cfg.OutputDir, "java_gapic_srcjar") + // Unzip the temp-codegen.srcjar. + srcjarPath := filepath.Join(outputConfig.GAPICDir, "temp-codegen.srcjar") + srcjarDest := outputConfig.GAPICDir if err := unzip(srcjarPath, srcjarDest); err != nil { return fmt.Errorf("librariangen: failed to unzip %s: %w", srcjarPath, err) } @@ -113,7 +112,7 @@ func Generate(ctx context.Context, cfg *Config) error { // invokeProtoc handles the protoc GAPIC generation logic for the 'generate' CLI command. // It reads a request file, and for each API specified, it invokes protoc // to generate the client library. It returns the module path and the path to the service YAML. -func invokeProtoc(ctx context.Context, cfg *Config, generateReq *message.Library) error { +func invokeProtoc(ctx context.Context, cfg *Config, generateReq *message.Library, outputConfig *protoc.OutputConfig) error { for _, api := range generateReq.APIs { apiServiceDir := filepath.Join(cfg.SourceDir, api.Path) slog.Info("processing api", "service_dir", apiServiceDir) @@ -121,10 +120,18 @@ func invokeProtoc(ctx context.Context, cfg *Config, generateReq *message.Library if err != nil { return fmt.Errorf("librariangen: failed to parse BUILD.bazel for %s: %w", apiServiceDir, err) } - args, err := protocBuild(apiServiceDir, bazelConfig, cfg.SourceDir, cfg.OutputDir) + args, err := protocBuild(apiServiceDir, bazelConfig, cfg.SourceDir, outputConfig) if err != nil { return fmt.Errorf("librariangen: failed to build protoc command for api %q in library %q: %w", api.Path, generateReq.ID, err) } + + // Create protoc output directories. + for _, dir := range []string{outputConfig.ProtoDir, outputConfig.GRPCDir, outputConfig.GAPICDir} { + if err := os.MkdirAll(dir, 0755); err != nil { + return err + } + } + if err := execvRun(ctx, args, cfg.OutputDir); err != nil { return fmt.Errorf("librariangen: protoc failed for api %q in library %q: %w", api.Path, generateReq.ID, err) } @@ -168,29 +175,43 @@ func restructureOutput(outputDir, libraryID string) error { slog.Debug("librariangen: restructuring output directory", "dir", outputDir) // Define source and destination directories. - gapicSrcDir := filepath.Join(outputDir, "java_gapic_srcjar", "src", "main", "java") - gapicTestDir := filepath.Join(outputDir, "java_gapic_srcjar", "src", "test", "java") - protoSrcDir := filepath.Join(outputDir, "com") - samplesDir := filepath.Join(outputDir, "java_gapic_srcjar", "samples", "snippets") - + gapicSrcDir := filepath.Join(outputDir, "gapic", "src", "main", "java") + gapicTestDir := filepath.Join(outputDir, "gapic", "src", "test", "java") + protoSrcDir := filepath.Join(outputDir, "proto") + resourceNameSrcDir := filepath.Join(outputDir, "gapic", "proto", "src", "main", "java") + grpcSrcDir := filepath.Join(outputDir, "grpc") + samplesDir := filepath.Join(outputDir, "gapic", "samples", "snippets") + + // TODO(meltsufin): currently we assume we have a single API variant v1 gapicDestDir := filepath.Join(outputDir, fmt.Sprintf("google-cloud-%s", libraryID), "src", "main", "java") gapicTestDestDir := filepath.Join(outputDir, fmt.Sprintf("google-cloud-%s", libraryID), "src", "test", "java") protoDestDir := filepath.Join(outputDir, fmt.Sprintf("proto-google-cloud-%s-v1", libraryID), "src", "main", "java") + resourceNameDestDir := filepath.Join(outputDir, fmt.Sprintf("proto-google-cloud-%s-v1", libraryID), "src", "main", "java") + grpcDestDir := filepath.Join(outputDir, fmt.Sprintf("grpc-google-cloud-%s-v1", libraryID), "src", "main", "java") samplesDestDir := filepath.Join(outputDir, "samples", "snippets") // Create destination directories. - destDirs := []string{gapicDestDir, gapicTestDestDir, protoDestDir, samplesDestDir} + destDirs := []string{gapicDestDir, gapicTestDestDir, protoDestDir, samplesDestDir, grpcDestDir} for _, dir := range destDirs { if err := os.MkdirAll(dir, 0755); err != nil { return err } } - // Move files. + // The resource name directory is not created if there are no resource names + // to generate. We create it here to avoid errors later. + if _, err := os.Stat(resourceNameSrcDir); os.IsNotExist(err) { + if err := os.MkdirAll(resourceNameSrcDir, 0755); err != nil { + return err + } + } + + // Move files that won't have conflicts. moves := map[string]string{ gapicSrcDir: gapicDestDir, gapicTestDir: gapicTestDestDir, protoSrcDir: protoDestDir, + grpcSrcDir: grpcDestDir, samplesDir: samplesDestDir, } for src, dest := range moves { @@ -199,19 +220,46 @@ func restructureOutput(outputDir, libraryID string) error { } } + // Merge the resource name files into the proto destination. + if err := copyAndMerge(resourceNameSrcDir, resourceNameDestDir); err != nil { + return err + } + return nil } -func cleanupIntermediateFiles(outputDir string) { - slog.Debug("librariangen: cleaning up intermediate files", "dir", outputDir) - filesToRemove := []string{ - "java_gapic_srcjar", - "com", - "java_gapic.zip", - "temp-codegen.srcjar", +// copyAndMerge recursively copies the contents of src to dest, merging directories. +func copyAndMerge(src, dest string) error { + entries, err := os.ReadDir(src) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return err } - for _, file := range filesToRemove { - path := filepath.Join(outputDir, file) + + for _, entry := range entries { + srcPath := filepath.Join(src, entry.Name()) + destPath := filepath.Join(dest, entry.Name()) + if entry.IsDir() { + if err := os.MkdirAll(destPath, 0755); err != nil { + return err + } + if err := copyAndMerge(srcPath, destPath); err != nil { + return err + } + } else { + if err := os.Rename(srcPath, destPath); err != nil { + return fmt.Errorf("librariangen: failed to move %s to %s: %w", srcPath, destPath, err) + } + } + } + return nil +} + +func cleanupIntermediateFiles(outputConfig *protoc.OutputConfig) { + slog.Debug("librariangen: cleaning up intermediate files") + for _, path := range []string{outputConfig.GAPICDir, outputConfig.GRPCDir, outputConfig.ProtoDir} { if err := os.RemoveAll(path); err != nil { slog.Error("librariangen: failed to clean up intermediate file", "path", path, "error", err) } diff --git a/internal/librariangen/generate/generator_test.go b/internal/librariangen/generate/generator_test.go index 2e77903722..7c513b99d5 100644 --- a/internal/librariangen/generate/generator_test.go +++ b/internal/librariangen/generate/generator_test.go @@ -24,6 +24,8 @@ import ( "path/filepath" "strings" "testing" + + "cloud.google.com/java/internal/librariangen/protoc" ) // testEnv encapsulates a temporary test environment. @@ -110,40 +112,15 @@ func createFakeZip(t *testing.T, path string) { zipWriter := zip.NewWriter(newZipFile) defer zipWriter.Close() - // Create a temporary empty zip file to be included in the main zip file. - tmpfile, err := os.CreateTemp("", "temp-zip-*.zip") - if err != nil { - t.Fatalf("failed to create temp file: %v", err) - } - defer os.Remove(tmpfile.Name()) - - tempZipWriter := zip.NewWriter(tmpfile) - // Add the src/main/java directory to the inner zip file. - _, err = tempZipWriter.Create("src/main/java/") + // Add the src/main/java directory to the zip file. + _, err = zipWriter.Create("src/main/java/") if err != nil { t.Fatalf("failed to create directory in zip: %v", err) } - _, err = tempZipWriter.Create("src/test/java/") + _, err = zipWriter.Create("src/test/java/") if err != nil { t.Fatalf("failed to create directory in zip: %v", err) } - tempZipWriter.Close() - - // Read the content of the temporary zip file. - zipBytes, err := os.ReadFile(tmpfile.Name()) - if err != nil { - t.Fatalf("failed to read temp zip file: %v", err) - } - - // Add the temporary zip file to the main zip file as temp-codegen.srcjar. - w, err := zipWriter.Create("temp-codegen.srcjar") - if err != nil { - t.Fatalf("failed to create empty file in zip: %v", err) - } - _, err = w.Write(zipBytes) - if err != nil { - t.Fatalf("failed to write content to zip: %v", err) - } } func TestGenerate(t *testing.T) { @@ -255,12 +232,19 @@ java_gapic_library( } if tt.protocErr == nil && tt.name != "unzip fails" { // Simulate protoc creating the zip file. - createFakeZip(t, filepath.Join(e.outputDir, "java_gapic.zip")) + zipPath := filepath.Join(e.outputDir, "gapic", "temp-codegen.srcjar") + if err := os.MkdirAll(filepath.Dir(zipPath), 0755); err != nil { + t.Fatalf("failed to create directory: %v", err) + } + createFakeZip(t, zipPath) // Create the directory that is expected by restructureOutput. - if err := os.MkdirAll(filepath.Join(e.outputDir, "com"), 0755); err != nil { + if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "main", "java"), 0755); err != nil { + t.Fatalf("failed to create directory: %v", err) + } + if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "test", "java"), 0755); err != nil { t.Fatalf("failed to create directory: %v", err) } - if err := os.MkdirAll(filepath.Join(e.outputDir, "java_gapic_srcjar", "samples", "snippets"), 0755); err != nil { + if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "samples", "snippets"), 0755); err != nil { t.Fatalf("failed to create directory: %v", err) } } @@ -348,48 +332,45 @@ func TestConfig_Validate(t *testing.T) { func TestRestructureOutput(t *testing.T) { e := newTestEnv(t) defer e.cleanup(t) - // Create dummy files and directories to be restructured. - if err := os.MkdirAll(filepath.Join(e.outputDir, "java_gapic_srcjar", "src", "main", "java", "com"), 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(e.outputDir, "java_gapic_srcjar", "src", "main", "java", "com", "foo.java"), nil, 0644); err != nil { - t.Fatal(err) - } - if err := os.MkdirAll(filepath.Join(e.outputDir, "java_gapic_srcjar", "src", "test", "java", "com"), 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(e.outputDir, "java_gapic_srcjar", "src", "test", "java", "com", "foo_test.java"), nil, 0644); err != nil { - t.Fatal(err) - } - if err := os.MkdirAll(filepath.Join(e.outputDir, "com"), 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(e.outputDir, "com", "bar.proto"), nil, 0644); err != nil { - t.Fatal(err) - } - if err := os.MkdirAll(filepath.Join(e.outputDir, "java_gapic_srcjar", "samples", "snippets", "com"), 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(e.outputDir, "java_gapic_srcjar", "samples", "snippets", "com", "baz.java"), nil, 0644); err != nil { - t.Fatal(err) + + // 1. Setup: Create all the source directories and dummy files. + sourceFiles := map[string]string{ + "gapic/src/main/java/com/google/foo.java": "", + "gapic/src/test/java/com/google/foo_test.java": "", + "proto/com/google/bar.proto": "", + "grpc/com/google/bar_grpc.java": "", + "gapic/samples/snippets/com/google/baz.java": "", + "gapic/proto/src/main/java/com/google/resname.java": "", + } + for path, content := range sourceFiles { + fullPath := filepath.Join(e.outputDir, path) + if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil { + t.Fatalf("failed to create source directory for %s: %v", path, err) + } + if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { + t.Fatalf("failed to write source file for %s: %v", path, err) + } } + // 2. Execute: Call the function under test. if err := restructureOutput(e.outputDir, "my-library"); err != nil { t.Fatalf("restructureOutput() failed: %v", err) } - // Check that the files were moved to the correct locations. - if _, err := os.Stat(filepath.Join(e.outputDir, "google-cloud-my-library", "src", "main", "java", "com", "foo.java")); err != nil { - t.Errorf("file not moved to main: %v", err) - } - if _, err := os.Stat(filepath.Join(e.outputDir, "google-cloud-my-library", "src", "test", "java", "com", "foo_test.java")); err != nil { - t.Errorf("file not moved to test: %v", err) + // 3. Verify: Check that all files were moved to their expected destinations. + expectedFiles := []string{ + "google-cloud-my-library/src/main/java/com/google/foo.java", + "google-cloud-my-library/src/test/java/com/google/foo_test.java", + "proto-google-cloud-my-library-v1/src/main/java/com/google/bar.proto", + "grpc-google-cloud-my-library-v1/src/main/java/com/google/bar_grpc.java", + "samples/snippets/com/google/baz.java", + "proto-google-cloud-my-library-v1/src/main/java/com/google/resname.java", } - if _, err := os.Stat(filepath.Join(e.outputDir, "proto-google-cloud-my-library-v1", "src", "main", "java", "bar.proto")); err != nil { - t.Errorf("file not moved to proto: %v", err) - } - if _, err := os.Stat(filepath.Join(e.outputDir, "samples", "snippets", "com", "baz.java")); err != nil { - t.Errorf("file not moved to samples: %v", err) + for _, path := range expectedFiles { + fullPath := filepath.Join(e.outputDir, path) + if _, err := os.Stat(fullPath); err != nil { + t.Errorf("expected file not found at %s: %v", fullPath, err) + } } } @@ -502,7 +483,7 @@ func TestCleanupIntermediateFiles(t *testing.T) { defer e.cleanup(t) // Create a file that cannot be deleted. - protectedDir := filepath.Join(e.outputDir, "com") + protectedDir := filepath.Join(e.outputDir, "proto") if err := os.Mkdir(protectedDir, 0755); err != nil { t.Fatalf("failed to create protected dir: %v", err) } @@ -515,7 +496,12 @@ func TestCleanupIntermediateFiles(t *testing.T) { } defer os.Chmod(protectedDir, 0755) // Restore permissions for cleanup. - cleanupIntermediateFiles(e.outputDir) + outputConfig := &protoc.OutputConfig{ + GAPICDir: filepath.Join(e.outputDir, "gapic"), + GRPCDir: filepath.Join(e.outputDir, "grpc"), + ProtoDir: protectedDir, + } + cleanupIntermediateFiles(outputConfig) if !strings.Contains(buf.String(), "failed to clean up intermediate file") { t.Errorf("cleanupIntermediateFiles() should log an error on failure, but did not. Log: %s", buf.String()) diff --git a/internal/librariangen/protoc/protoc.go b/internal/librariangen/protoc/protoc.go index b7bf0ad19e..bd77d21a5c 100644 --- a/internal/librariangen/protoc/protoc.go +++ b/internal/librariangen/protoc/protoc.go @@ -19,7 +19,6 @@ import ( "os" "path/filepath" "strings" - ) // ConfigProvider is an interface that describes the configuration needed @@ -34,8 +33,15 @@ type ConfigProvider interface { HasGAPIC() bool } +// OutputConfig provides paths to directories to be used for protoc output. +type OutputConfig struct { + GAPICDir string + GRPCDir string + ProtoDir string +} + // Build constructs the full protoc command arguments for a given API. -func Build(apiServiceDir string, config ConfigProvider, sourceDir, outputDir string) ([]string, error) { +func Build(apiServiceDir string, config ConfigProvider, sourceDir string, outputConfig *OutputConfig) ([]string, error) { // Gather all .proto files in the API's source directory. entries, err := os.ReadDir(apiServiceDir) if err != nil { @@ -78,9 +84,12 @@ func Build(apiServiceDir string, config ConfigProvider, sourceDir, outputDir str "--experimental_allow_proto3_optional", } - args = append(args, fmt.Sprintf("--java_out=%s", outputDir)) + args = append(args, fmt.Sprintf("--java_out=%s", outputConfig.ProtoDir)) + if config.Transport() != "" && config.Transport() != "rest" { + args = append(args, fmt.Sprintf("--java_grpc_out=%s", outputConfig.GRPCDir)) + } if config.HasGAPIC() { - args = append(args, fmt.Sprintf("--java_gapic_out=metadata:%s", filepath.Join(outputDir, "java_gapic.zip"))) + args = append(args, fmt.Sprintf("--java_gapic_out=metadata:%s", outputConfig.GAPICDir)) if len(gapicOpts) > 0 { args = append(args, "--java_gapic_opt="+strings.Join(gapicOpts, ",")) diff --git a/internal/librariangen/protoc/protoc_test.go b/internal/librariangen/protoc/protoc_test.go index 7e7f62aec6..c35db57dde 100644 --- a/internal/librariangen/protoc/protoc_test.go +++ b/internal/librariangen/protoc/protoc_test.go @@ -66,8 +66,9 @@ func TestBuild(t *testing.T) { want: []string{ "protoc", "--experimental_allow_proto3_optional", - "--java_out=/output", - "--java_gapic_out=metadata:/output/java_gapic.zip", + "--java_out=/output/proto", + "--java_grpc_out=/output/grpc", + "--java_gapic_out=metadata:/output/gapic", "--java_gapic_opt=" + strings.Join([]string{ "api-service-config=" + filepath.Join(sourceDir, "google/cloud/workflows/v1/workflows_v1.yaml"), "gapic-config=" + filepath.Join(sourceDir, "google/cloud/workflows/v1/workflows_gapic.yaml"), @@ -92,8 +93,9 @@ func TestBuild(t *testing.T) { want: []string{ "protoc", "--experimental_allow_proto3_optional", - "--java_out=/output", - "--java_gapic_out=metadata:/output/java_gapic.zip", + "--java_out=/output/proto", + "--java_grpc_out=/output/grpc", + "--java_gapic_out=metadata:/output/gapic", "--java_gapic_opt=" + strings.Join([]string{ "api-service-config=" + filepath.Join(sourceDir, "google/cloud/secretmanager/v1beta2/secretmanager_v1beta2.yaml"), "grpc-service-config=" + filepath.Join(sourceDir, "google/cloud/secretmanager/v1beta2/secretmanager_grpc_service_config.json"), @@ -115,7 +117,7 @@ func TestBuild(t *testing.T) { want: []string{ "protoc", "--experimental_allow_proto3_optional", - "--java_out=/output", + "--java_out=/output/proto", "-I=" + sourceDir, filepath.Join(sourceDir, "google/cloud/secretmanager/v1beta2/secretmanager.proto"), }, @@ -124,7 +126,12 @@ func TestBuild(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := Build(filepath.Join(sourceDir, tt.apiPath), &tt.config, sourceDir, "/output") + outputConfig := &OutputConfig{ + GAPICDir: "/output/gapic", + GRPCDir: "/output/grpc", + ProtoDir: "/output/proto", + } + got, err := Build(filepath.Join(sourceDir, tt.apiPath), &tt.config, sourceDir, outputConfig) if err != nil { t.Fatalf("Build() failed: %v", err) } diff --git a/internal/librariangen/run-generate-library.sh b/internal/librariangen/run-generate-library.sh index e47d99d565..81c8317d23 100755 --- a/internal/librariangen/run-generate-library.sh +++ b/internal/librariangen/run-generate-library.sh @@ -36,6 +36,7 @@ WORKSPACE="$(pwd)/workspace" LIBRARIANGEN_GOOGLEAPIS_DIR=../../../googleapis LIBRARIANGEN_GOTOOLCHAIN=local GAPIC_GENERATOR_VERSION="2.62.3" +GRPC_PLUGIN_VERSION="1.65.0" LIBRARIANGEN_LOG="$WORKSPACE/librariangen.log" # --- Cleanup and Setup --- @@ -62,6 +63,10 @@ fi echo "Downloading gapic-generator-java version $GAPIC_GENERATOR_VERSION..." wget -q "https://repo1.maven.org/maven2/com/google/api/gapic-generator-java/$GAPIC_GENERATOR_VERSION/gapic-generator-java-$GAPIC_GENERATOR_VERSION.jar" -O "$WORKSPACE/gapic-generator-java.jar" +echo "Downloading protoc-gen-grpc-java..." +wget -q "https://repo1.maven.org/maven2/io/grpc/protoc-gen-grpc-java/$GRPC_PLUGIN_VERSION/protoc-gen-grpc-java-$GRPC_PLUGIN_VERSION-linux-x86_64.exe" -O "$WORKSPACE/protoc-gen-java_grpc" +chmod +x "$WORKSPACE/protoc-gen-java_grpc" + # Create wrapper script for protoc-gen-java_gapic echo "Creating protoc-gen-java_gapic wrapper..." cat > "$WORKSPACE/protoc-gen-java_gapic" << EOL From a86dbb81fbe87ccbfbade9d642c68b6e79361ccc Mon Sep 17 00:00:00 2001 From: Mike Eltsufin Date: Thu, 23 Oct 2025 02:57:31 +0000 Subject: [PATCH 2/3] test: improve coverage --- .../librariangen/generate/generator_test.go | 88 ++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/internal/librariangen/generate/generator_test.go b/internal/librariangen/generate/generator_test.go index 7c513b99d5..83cfde9e4c 100644 --- a/internal/librariangen/generate/generator_test.go +++ b/internal/librariangen/generate/generator_test.go @@ -374,11 +374,93 @@ func TestRestructureOutput(t *testing.T) { } } -func TestUnzip(t *testing.T) { +func TestCopyAndMerge(t *testing.T) { e := newTestEnv(t) defer e.cleanup(t) + // 1. Setup: Create source and destination directories with nested structures. + srcDir := filepath.Join(e.tmpDir, "src") + destDir := filepath.Join(e.tmpDir, "dest") + sourceFiles := map[string]string{ + "com/google/foo.java": "", + "com/google/bar/baz.java": "", + "com/google/bar/qux/quux.java": "", + } + destFiles := map[string]string{ + "com/google/existing.java": "", + "com/google/bar/another.java": "", + } + for path, content := range sourceFiles { + fullPath := filepath.Join(srcDir, path) + if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil { + t.Fatalf("failed to create source directory for %s: %v", path, err) + } + if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { + t.Fatalf("failed to write source file for %s: %v", path, err) + } + } + for path, content := range destFiles { + fullPath := filepath.Join(destDir, path) + if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil { + t.Fatalf("failed to create dest directory for %s: %v", path, err) + } + if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { + t.Fatalf("failed to write dest file for %s: %v", path, err) + } + } + + // 2. Execute: Call the function under test. + if err := copyAndMerge(srcDir, destDir); err != nil { + t.Fatalf("copyAndMerge() failed: %v", err) + } + + // 3. Verify: Check that all files were merged correctly. + for path := range sourceFiles { + fullPath := filepath.Join(destDir, path) + if _, err := os.Stat(fullPath); err != nil { + t.Errorf("source file not merged: %v", err) + } + } + for path := range destFiles { + fullPath := filepath.Join(destDir, path) + if _, err := os.Stat(fullPath); err != nil { + t.Errorf("destination file was deleted: %v", err) + } + } +} + +func TestUnzip(t *testing.T) { + t.Run("happy path", func(t *testing.T) { + e := newTestEnv(t) + defer e.cleanup(t) + // Create a valid zip file. + zipPath := filepath.Join(e.outputDir, "valid.zip") + f, err := os.Create(zipPath) + if err != nil { + t.Fatalf("failed to create zip file: %v", err) + } + defer f.Close() + zipWriter := zip.NewWriter(f) + if _, err := zipWriter.Create("file.txt"); err != nil { + t.Fatalf("failed to create file in zip: %v", err) + } + zipWriter.Close() + + // Unzip the file. + destDir := filepath.Join(e.outputDir, "unzip-dest") + if err := unzip(zipPath, destDir); err != nil { + t.Fatalf("unzip() failed: %v", err) + } + + // Check that the file was unzipped. + if _, err := os.Stat(filepath.Join(destDir, "file.txt")); err != nil { + t.Errorf("file not unzipped: %v", err) + } + }) + t.Run("invalid zip file", func(t *testing.T) { + e := newTestEnv(t) + defer e.cleanup(t) invalidZipPath := filepath.Join(e.outputDir, "invalid.zip") if err := os.WriteFile(invalidZipPath, []byte("not a zip file"), 0644); err != nil { t.Fatalf("failed to write invalid zip file: %v", err) @@ -389,6 +471,8 @@ func TestUnzip(t *testing.T) { }) t.Run("permission denied", func(t *testing.T) { + e := newTestEnv(t) + defer e.cleanup(t) // Create a valid zip file. validZipPath := filepath.Join(e.outputDir, "valid.zip") if err := os.WriteFile(validZipPath, []byte{}, 0644); err != nil { @@ -423,6 +507,8 @@ func TestUnzip(t *testing.T) { }) t.Run("zip slip vulnerability", func(t *testing.T) { + e := newTestEnv(t) + defer e.cleanup(t) // Create a zip file with a malicious file path. maliciousZipPath := filepath.Join(e.outputDir, "malicious.zip") f, err := os.Create(maliciousZipPath) From 10e2c5d0f872f153a4b6a4b15ed3da715ecf9e87 Mon Sep 17 00:00:00 2001 From: Mike Eltsufin Date: Thu, 23 Oct 2025 19:32:35 +0000 Subject: [PATCH 3/3] chore: address feedback --- internal/librariangen/bazel/parser.go | 1 + internal/librariangen/bazel/parser_test.go | 2 +- internal/librariangen/execv/execv.go | 2 +- internal/librariangen/execv/execv_test.go | 2 +- internal/librariangen/generate/generator.go | 19 ++++--- .../librariangen/generate/generator_test.go | 54 +++++++++---------- internal/librariangen/main_test.go | 2 +- internal/librariangen/protoc/protoc.go | 2 +- internal/librariangen/protoc/protoc_test.go | 12 ++--- 9 files changed, 49 insertions(+), 47 deletions(-) diff --git a/internal/librariangen/bazel/parser.go b/internal/librariangen/bazel/parser.go index e36bd464aa..f10eb8ed52 100644 --- a/internal/librariangen/bazel/parser.go +++ b/internal/librariangen/bazel/parser.go @@ -65,6 +65,7 @@ func (c *Config) Validate() error { } var javaGapicLibraryRE = regexp.MustCompile(`java_gapic_library\((?s:.)*?\)`) + // Parse reads a BUILD.bazel file from the given directory and extracts the // relevant configuration from the java_gapic_library rule. func Parse(dir string) (*Config, error) { diff --git a/internal/librariangen/bazel/parser_test.go b/internal/librariangen/bazel/parser_test.go index f0bf6f7b40..39e3eb12d8 100644 --- a/internal/librariangen/bazel/parser_test.go +++ b/internal/librariangen/bazel/parser_test.go @@ -256,4 +256,4 @@ func TestParse_noBuildFile(t *testing.T) { if err == nil { t.Error("Parse() succeeded; want error") } -} \ No newline at end of file +} diff --git a/internal/librariangen/execv/execv.go b/internal/librariangen/execv/execv.go index 3000cb761c..3a0289099c 100644 --- a/internal/librariangen/execv/execv.go +++ b/internal/librariangen/execv/execv.go @@ -48,4 +48,4 @@ func Run(ctx context.Context, args []string, workingDir string) error { return fmt.Errorf("librariangen: command failed: %w", err) } return nil -} \ No newline at end of file +} diff --git a/internal/librariangen/execv/execv_test.go b/internal/librariangen/execv/execv_test.go index b2f28be3c6..771213db4b 100644 --- a/internal/librariangen/execv/execv_test.go +++ b/internal/librariangen/execv/execv_test.go @@ -76,4 +76,4 @@ func TestRun(t *testing.T) { } }) } -} \ No newline at end of file +} diff --git a/internal/librariangen/generate/generator.go b/internal/librariangen/generate/generator.go index 31ae9698af..f4100a205b 100644 --- a/internal/librariangen/generate/generator.go +++ b/internal/librariangen/generate/generator.go @@ -83,7 +83,11 @@ func Generate(ctx context.Context, cfg *Config) error { GRPCDir: filepath.Join(cfg.OutputDir, "grpc"), ProtoDir: filepath.Join(cfg.OutputDir, "proto"), } - defer cleanupIntermediateFiles(outputConfig) + defer func() { + if err := cleanupIntermediateFiles(outputConfig); err != nil { + slog.Error("librariangen: failed to clean up intermediate files", "error", err) + } + }() generateReq, err := readGenerateReq(cfg.LibrarianDir) if err != nil { @@ -133,7 +137,7 @@ func invokeProtoc(ctx context.Context, cfg *Config, generateReq *message.Library } if err := execvRun(ctx, args, cfg.OutputDir); err != nil { - return fmt.Errorf("librariangen: protoc failed for api %q in library %q: %w", api.Path, generateReq.ID, err) + return fmt.Errorf("librariangen: protoc failed for api %q in library %q: %w, execvRun error: %v", api.Path, generateReq.ID, err, err) } } return nil @@ -165,7 +169,7 @@ func moveFiles(sourceDir, targetDir string) error { newPath := filepath.Join(targetDir, f.Name()) slog.Debug("librariangen: moving file", "from", oldPath, "to", newPath) if err := os.Rename(oldPath, newPath); err != nil { - return fmt.Errorf("librariangen: failed to move %s to %s: %w", oldPath, newPath, err) + return fmt.Errorf("librariangen: failed to move %s to %s: %w, os.Rename error: %v", oldPath, newPath, err, err) } } return nil @@ -211,7 +215,7 @@ func restructureOutput(outputDir, libraryID string) error { gapicSrcDir: gapicDestDir, gapicTestDir: gapicTestDestDir, protoSrcDir: protoDestDir, - grpcSrcDir: grpcDestDir, + grpcSrcDir: grpcDestDir, samplesDir: samplesDestDir, } for src, dest := range moves { @@ -250,20 +254,21 @@ func copyAndMerge(src, dest string) error { } } else { if err := os.Rename(srcPath, destPath); err != nil { - return fmt.Errorf("librariangen: failed to move %s to %s: %w", srcPath, destPath, err) + return fmt.Errorf("librariangen: failed to move %s to %s: %w, os.Rename error: %v", srcPath, destPath, err, err) } } } return nil } -func cleanupIntermediateFiles(outputConfig *protoc.OutputConfig) { +func cleanupIntermediateFiles(outputConfig *protoc.OutputConfig) error { slog.Debug("librariangen: cleaning up intermediate files") for _, path := range []string{outputConfig.GAPICDir, outputConfig.GRPCDir, outputConfig.ProtoDir} { if err := os.RemoveAll(path); err != nil { - slog.Error("librariangen: failed to clean up intermediate file", "path", path, "error", err) + return fmt.Errorf("failed to clean up intermediate file at %s: %w", path, err) } } + return nil } func unzip(src, dest string) error { diff --git a/internal/librariangen/generate/generator_test.go b/internal/librariangen/generate/generator_test.go index 83cfde9e4c..16c82f752c 100644 --- a/internal/librariangen/generate/generator_test.go +++ b/internal/librariangen/generate/generator_test.go @@ -16,13 +16,10 @@ package generate import ( "archive/zip" - "bytes" "context" "errors" - "log/slog" "os" "path/filepath" - "strings" "testing" "cloud.google.com/java/internal/librariangen/protoc" @@ -123,6 +120,25 @@ func createFakeZip(t *testing.T, path string) { } } +func setupFakeProtocOutput(t *testing.T, e *testEnv) { + // Simulate protoc creating the zip file. + zipPath := filepath.Join(e.outputDir, "gapic", "temp-codegen.srcjar") + if err := os.MkdirAll(filepath.Dir(zipPath), 0755); err != nil { + t.Fatalf("failed to create directory: %v", err) + } + createFakeZip(t, zipPath) + // Create the directory that is expected by restructureOutput. + if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "main", "java"), 0755); err != nil { + t.Fatalf("failed to create directory: %v", err) + } + if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "test", "java"), 0755); err != nil { + t.Fatalf("failed to create directory: %v", err) + } + if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "samples", "snippets"), 0755); err != nil { + t.Fatalf("failed to create directory: %v", err) + } +} + func TestGenerate(t *testing.T) { singleAPIRequest := `{"id": "foo", "apis": [{"path": "api/v1"}]}` validBazel := ` @@ -231,22 +247,7 @@ java_gapic_library( t.Errorf("protocRun called with %s; want %s", args[0], want) } if tt.protocErr == nil && tt.name != "unzip fails" { - // Simulate protoc creating the zip file. - zipPath := filepath.Join(e.outputDir, "gapic", "temp-codegen.srcjar") - if err := os.MkdirAll(filepath.Dir(zipPath), 0755); err != nil { - t.Fatalf("failed to create directory: %v", err) - } - createFakeZip(t, zipPath) - // Create the directory that is expected by restructureOutput. - if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "main", "java"), 0755); err != nil { - t.Fatalf("failed to create directory: %v", err) - } - if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "src", "test", "java"), 0755); err != nil { - t.Fatalf("failed to create directory: %v", err) - } - if err := os.MkdirAll(filepath.Join(e.outputDir, "gapic", "samples", "snippets"), 0755); err != nil { - t.Fatalf("failed to create directory: %v", err) - } + setupFakeProtocOutput(t, e) } protocRunCount++ return tt.protocErr @@ -382,12 +383,12 @@ func TestCopyAndMerge(t *testing.T) { srcDir := filepath.Join(e.tmpDir, "src") destDir := filepath.Join(e.tmpDir, "dest") sourceFiles := map[string]string{ - "com/google/foo.java": "", - "com/google/bar/baz.java": "", + "com/google/foo.java": "", + "com/google/bar/baz.java": "", "com/google/bar/qux/quux.java": "", } destFiles := map[string]string{ - "com/google/existing.java": "", + "com/google/existing.java": "", "com/google/bar/another.java": "", } for path, content := range sourceFiles { @@ -562,9 +563,6 @@ func TestMoveFiles(t *testing.T) { } func TestCleanupIntermediateFiles(t *testing.T) { - var buf bytes.Buffer - slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil))) - e := newTestEnv(t) defer e.cleanup(t) @@ -587,9 +585,7 @@ func TestCleanupIntermediateFiles(t *testing.T) { GRPCDir: filepath.Join(e.outputDir, "grpc"), ProtoDir: protectedDir, } - cleanupIntermediateFiles(outputConfig) - - if !strings.Contains(buf.String(), "failed to clean up intermediate file") { - t.Errorf("cleanupIntermediateFiles() should log an error on failure, but did not. Log: %s", buf.String()) + if err := cleanupIntermediateFiles(outputConfig); err == nil { + t.Error("cleanupIntermediateFiles() should return an error on failure, but did not") } } diff --git a/internal/librariangen/main_test.go b/internal/librariangen/main_test.go index 643c366123..7e722327f6 100644 --- a/internal/librariangen/main_test.go +++ b/internal/librariangen/main_test.go @@ -63,4 +63,4 @@ func TestParseLogLevel(t *testing.T) { } }) } -} \ No newline at end of file +} diff --git a/internal/librariangen/protoc/protoc.go b/internal/librariangen/protoc/protoc.go index bd77d21a5c..f6945221ee 100644 --- a/internal/librariangen/protoc/protoc.go +++ b/internal/librariangen/protoc/protoc.go @@ -106,4 +106,4 @@ func Build(apiServiceDir string, config ConfigProvider, sourceDir string, output args = append(args, protoFiles...) return args, nil -} \ No newline at end of file +} diff --git a/internal/librariangen/protoc/protoc_test.go b/internal/librariangen/protoc/protoc_test.go index c35db57dde..ffef840e38 100644 --- a/internal/librariangen/protoc/protoc_test.go +++ b/internal/librariangen/protoc/protoc_test.go @@ -47,10 +47,10 @@ func TestBuild(t *testing.T) { t.Fatalf("failed to get absolute path for sourceDir: %v", err) } tests := []struct { - name string - apiPath string - config mockConfigProvider - want []string + name string + apiPath string + config mockConfigProvider + want []string }{ { name: "java_grpc_library rule", @@ -112,7 +112,7 @@ func TestBuild(t *testing.T) { name: "proto-only", apiPath: "google/cloud/secretmanager/v1beta2", config: mockConfigProvider{ - hasGAPIC: false, + hasGAPIC: false, }, want: []string{ "protoc", @@ -141,4 +141,4 @@ func TestBuild(t *testing.T) { } }) } -} \ No newline at end of file +}