From 7cbaf43d15a7a1da75d0abc0490ccd956eb4d89d Mon Sep 17 00:00:00 2001 From: kaialang <52937472+kaialang@users.noreply.github.com> Date: Wed, 2 Aug 2023 21:53:43 +0000 Subject: [PATCH 1/3] Allow proto files to have a separate configurable idl root --- codegen/client.go | 2 +- codegen/package.go | 63 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/codegen/client.go b/codegen/client.go index 56dbf6f53..b9bc4383e 100644 --- a/codegen/client.go +++ b/codegen/client.go @@ -252,7 +252,7 @@ func newGRPCClientSpec( instance *ModuleInstance, h *PackageHelper, ) (*ClientSpec, error) { - protoFile := filepath.Join(h.IdlPath(), h.GetModuleIdlSubDir(false), config.IDLFile) + protoFile := filepath.Join(h.ProtoIdlPath(), h.GetModuleIdlSubDir(false), config.IDLFile) protoSpec, err := NewProtoModuleSpec(protoFile, false, h) if err != nil { return nil, errors.Wrapf( diff --git a/codegen/package.go b/codegen/package.go index c2e6a7fb8..594864396 100644 --- a/codegen/package.go +++ b/codegen/package.go @@ -1,4 +1,4 @@ -// Copyright (c) 2022 Uber Technologies, Inc. +// Copyright (c) 2023 Uber Technologies, Inc. // // Permission is hereby granted, free of charge, to any person obtaining a copy // of this software and associated documentation files (the "Software"), to deal @@ -45,6 +45,8 @@ type PackageHelper struct { configRoot string // The absolute root directory containing idl files idlRootDir string + // The absolute root directory containing proto idl files + protoIdlRootDir string // moduleIdlSubDir defines subdir for idl per module moduleIdlSubDir map[string]string // The map of idl type to go package name of where the generated structs are @@ -86,6 +88,8 @@ func NewDefaultPackageHelperOptions() *PackageHelperOptions { type PackageHelperOptions struct { // relative path to the idl dir, defaults to "./idl" RelIdlRootDir string + // relative path to the proto idl dir, defaults to RelIdlRootDir, if neither of them is set, defaults to "./idl" + ProtoRelIdlRootDir string // subdir for idl per module ModuleIdlSubDir map[string]string // relative path to the target dir that will contain generated code, defaults to "./build" @@ -138,6 +142,13 @@ func (p *PackageHelperOptions) relIdlRootDir() string { return "./idl" } +func (p *PackageHelperOptions) protoRelIdlRootDir() string { + if p.ProtoRelIdlRootDir != "" { + return p.ProtoRelIdlRootDir + } + return p.relIdlRootDir() +} + func (p *PackageHelperOptions) moduleIdlSubDir() map[string]string { return p.ModuleIdlSubDir } @@ -232,6 +243,7 @@ func NewPackageHelper( packageRoot: packageRoot, configRoot: absConfigRoot, idlRootDir: filepath.Join(absConfigRoot, options.relIdlRootDir()), + protoIdlRootDir: filepath.Join(absConfigRoot, options.protoRelIdlRootDir()), genCodePackage: options.genCodePackage(packageRoot), goGatewayNamespace: goGatewayNamespace, targetGenDir: filepath.Join(absConfigRoot, options.relTargetGenDir()), @@ -279,6 +291,7 @@ func (p PackageHelper) TypeImportPath(idlFile string) (string, error) { if !strings.HasSuffix(idlFile, thriftExtension) && !strings.HasSuffix(idlFile, protoExtension) { return "", errors.Errorf("idl file %s is not %s or %s", idlFile, thriftExtension, protoExtension) } + var suffix string // for a filepath: a/b/c.(thrift|proto) // - thrift generates code in path: a/b/c/c.go @@ -290,10 +303,11 @@ func (p PackageHelper) TypeImportPath(idlFile string) (string, error) { } idx := strings.Index(idlFile, p.idlRootDir) - if idx == -1 { + idxProto := strings.Index(idlFile, p.protoIdlRootDir) + if idx == -1 && idxProto == -1 { return "", errors.Errorf( - "file %s is not in IDL dir (%s)", - idlFile, p.idlRootDir, + "file %s is not in IDL dir (%s) or (%s)", + idlFile, p.idlRootDir, p.protoIdlRootDir, ) } @@ -302,6 +316,14 @@ func (p PackageHelper) TypeImportPath(idlFile string) (string, error) { if !ok { return "", errors.Errorf("genCodePackage for %q idl file is not configured in build.yaml", ext) } + + if strings.HasSuffix(idlFile, protoExtension){ + return path.Join( + genCodePkg, + idlFile[idxProto+len(p.protoIdlRootDir):len(suffix)], + ), nil + } + return path.Join( genCodePkg, idlFile[idx+len(p.idlRootDir):len(suffix)], @@ -318,6 +340,11 @@ func (p PackageHelper) IdlPath() string { return p.idlRootDir } +// ProtoIdlPath returns the file path to the proto idl folder +func (p PackageHelper) ProtoIdlPath() string { + return p.protoIdlRootDir +} + // CodeGenTargetPath returns the file path where the code should // be generated. func (p PackageHelper) CodeGenTargetPath() string { @@ -330,22 +357,26 @@ func (p PackageHelper) TypePackageName(idlFile string) (string, error) { return "", errors.Errorf("file %s is not %s or %s", idlFile, thriftExtension, protoExtension) } idx := strings.Index(idlFile, p.idlRootDir) - if idx == -1 { + idxProto := strings.Index(idlFile, p.protoIdlRootDir) + if idx == -1 && idxProto == -1 { return "", errors.Errorf( - "file %s is not in IDL dir (%s)", - idlFile, p.idlRootDir, + "file %s is not in IDL dir (%s) or (%s)", + idlFile, p.idlRootDir, p.protoIdlRootDir, ) } + var prefix string + var segment string if strings.HasSuffix(idlFile, thriftExtension) { prefix = strings.TrimSuffix(idlFile, thriftExtension) + // Strip the leading / and the trailing .thrift suffix. + segment = idlFile[idx+len(p.idlRootDir)+1 : len(prefix)] } else { prefix = strings.TrimSuffix(idlFile, protoExtension) + // Strip the leading / and the trailing .proto suffix. + segment = idlFile[idxProto+len(p.protoIdlRootDir)+1 : len(prefix)] } - // Strip the leading / and the trailing .thrift/.proto suffix. - segment := idlFile[idx+len(p.idlRootDir)+1 : len(prefix)] - packageName := strings.Replace(segment, "/", "_", -1) return CamelCase(packageName), nil } @@ -355,12 +386,18 @@ func (p PackageHelper) getRelativeFileName(idlFile string) (string, error) { return "", errors.Errorf("file %s is not %s or %s", idlFile, thriftExtension, protoExtension) } idx := strings.Index(idlFile, p.idlRootDir) - if idx == -1 { + idxProto := strings.Index(idlFile, p.protoIdlRootDir) + if idx == -1 && idxProto == -1 { return "", errors.Errorf( - "file %s is not in IDL dir (%s)", - idlFile, p.idlRootDir, + "file %s is not in IDL dir (%s) or (%s)", + idlFile, p.idlRootDir, p.protoIdlRootDir, ) } + + if strings.HasSuffix(idlFile, protoExtension) { + return idlFile[idxProto+len(p.protoIdlRootDir):], nil + } + return idlFile[idx+len(p.idlRootDir):], nil } From b7add15693a3091f31f3176af8616e49bb0059c7 Mon Sep 17 00:00:00 2001 From: kaialang <52937472+kaialang@users.noreply.github.com> Date: Wed, 2 Aug 2023 21:53:43 +0000 Subject: [PATCH 2/3] Allow proto files to have a separate configurable idl root - fix lint --- codegen/package.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codegen/package.go b/codegen/package.go index 594864396..50a05119a 100644 --- a/codegen/package.go +++ b/codegen/package.go @@ -243,7 +243,7 @@ func NewPackageHelper( packageRoot: packageRoot, configRoot: absConfigRoot, idlRootDir: filepath.Join(absConfigRoot, options.relIdlRootDir()), - protoIdlRootDir: filepath.Join(absConfigRoot, options.protoRelIdlRootDir()), + protoIdlRootDir: filepath.Join(absConfigRoot, options.protoRelIdlRootDir()), genCodePackage: options.genCodePackage(packageRoot), goGatewayNamespace: goGatewayNamespace, targetGenDir: filepath.Join(absConfigRoot, options.relTargetGenDir()), @@ -317,7 +317,7 @@ func (p PackageHelper) TypeImportPath(idlFile string) (string, error) { return "", errors.Errorf("genCodePackage for %q idl file is not configured in build.yaml", ext) } - if strings.HasSuffix(idlFile, protoExtension){ + if strings.HasSuffix(idlFile, protoExtension) { return path.Join( genCodePkg, idlFile[idxProto+len(p.protoIdlRootDir):len(suffix)], From b9e3a700d6ee73c4026d042dcb8fe9bce085d872 Mon Sep 17 00:00:00 2001 From: kaialang <52937472+kaialang@users.noreply.github.com> Date: Wed, 2 Aug 2023 21:53:43 +0000 Subject: [PATCH 3/3] Allow proto files to have a separate configurable idl root - fix lint --- codegen/client_test.go | 5 ++- codegen/package.go | 1 + codegen/package_test.go | 37 +++++++++++++++++++ codegen/service_test.go | 4 +- .../idl/clients-idl/clients/echo/echo.proto | 0 examples/example-gateway/build.yaml | 1 + 6 files changed, 44 insertions(+), 4 deletions(-) rename examples/example-gateway/{ => bazel-out}/idl/clients-idl/clients/echo/echo.proto (100%) diff --git a/codegen/client_test.go b/codegen/client_test.go index 3e612de59..61a0d5850 100644 --- a/codegen/client_test.go +++ b/codegen/client_test.go @@ -438,6 +438,7 @@ func newTestPackageHelper(t *testing.T) *PackageHelper { }, TraceKey: "trace-key", ModuleIdlSubDir: map[string]string{"endpoints": "endpoints-idl", "default": "clients-idl"}, + ProtoRelIdlRootDir: "./bazel-out/idl", } h, err := NewPackageHelper( @@ -540,8 +541,8 @@ func TestGRPCClientNewClientSpec(t *testing.T) { }, } h := newTestPackageHelper(t) - - idlFile := filepath.Join(h.IdlPath(), h.GetModuleIdlSubDir(false), "clients/echo/echo.proto") + assert.Equal(t, "/go/src/github.com/uber/zanzibar/examples/example-gateway/bazel-out/idl", h.ProtoIdlPath()) + idlFile := filepath.Join(h.ProtoIdlPath(), h.GetModuleIdlSubDir(false), "clients/echo/echo.proto") expectedSpec := &ClientSpec{ ModuleSpec: nil, YAMLFile: instance.YAMLFileName, diff --git a/codegen/package.go b/codegen/package.go index 50a05119a..b6073bc4a 100644 --- a/codegen/package.go +++ b/codegen/package.go @@ -146,6 +146,7 @@ func (p *PackageHelperOptions) protoRelIdlRootDir() string { if p.ProtoRelIdlRootDir != "" { return p.ProtoRelIdlRootDir } + return p.relIdlRootDir() } diff --git a/codegen/package_test.go b/codegen/package_test.go index b44744cbf..579bcde6c 100644 --- a/codegen/package_test.go +++ b/codegen/package_test.go @@ -38,6 +38,12 @@ var fooThrift = filepath.Join( "examples/example-gateway/idl/", "clients-idl/clients/foo/foo.thrift") +var fooProto = filepath.Join( + os.Getenv("GOPATH"), + "/src/github.com/uber/zanzibar/", + "examples/example-gateway/bazel-out/idl/", + "clients-idl/clients/foo/foo.proto") + var testCopyrightHeader = `// Copyright (c) 2018 Uber Technologies, Inc. // // Permission is hereby granted, free of charge, to any person obtaining a copy @@ -74,6 +80,7 @@ func newPackageHelper(t *testing.T) *codegen.PackageHelper { ".proto": packageRoot + "/build/gen-code", }, TraceKey: "trace-key", + ProtoRelIdlRootDir: "./bazel-out/idl", } h, err := codegen.NewPackageHelper( @@ -87,14 +94,39 @@ func newPackageHelper(t *testing.T) *codegen.PackageHelper { return h } +func TestProtoRelIdlRootDirInitializeToIDL(t *testing.T) { + relativeGatewayPath := "../examples/example-gateway" + absGatewayPath, err := filepath.Abs(relativeGatewayPath) + assert.NoError(t, err) + + packageRoot := "foo/" + options := &codegen.PackageHelperOptions{} + + h, err := codegen.NewPackageHelper( + packageRoot, + absGatewayPath, + options, + ) + + assert.NoError(t, err) + assert.Equal(t, "/go/src/github.com/uber/zanzibar/examples/example-gateway/idl", h.ProtoIdlPath()) +} + func TestImportPath(t *testing.T) { h := newPackageHelper(t) p, err := h.TypeImportPath(fooThrift) assert.Nil(t, err, "should not return error") assert.Equal(t, "github.com/uber/zanzibar/examples/example-gateway/build/gen-code/clients-idl/clients/foo/foo", p, "wrong type import path") + + p, err = h.TypeImportPath(fooProto) + assert.Nil(t, err, "should not return error") + assert.Equal(t, "github.com/uber/zanzibar/examples/example-gateway/build/gen-code/clients-idl/clients/foo", + p, "wrong type import path") + _, err = h.TypeImportPath("/Users/xxx/go/src/github.com/uber/zanzibar/examples/example-gateway/build/idl/github.com/uber/zanzibar/clients/foo/foo.go") assert.Error(t, err, "should return error for not a thrift file") + _, err = h.TypeImportPath("/Users/xxx/go/src/github.com/uber/zanzibar/examples/example-gateway/build/zanzibar/clients/foo/foo.thrift") assert.Error(t, err, "should return error for not in IDL dir") } @@ -104,6 +136,11 @@ func TestTypePackageName(t *testing.T) { packageName, err := h.TypePackageName(fooThrift) assert.Nil(t, err, "should not return error") assert.Equal(t, "clientsIDlClientsFooFoo", packageName, "wrong package name") + + packageName, err = h.TypePackageName(fooProto) + assert.Nil(t, err, "should not return error") + assert.Equal(t, "clientsIDlClientsFooFoo", packageName, "wrong package name") + _, err = h.TypeImportPath("/Users/xxx/go/src/github.com/uber/zanzibar/examples/example-gateway/build/idl/github.com/uber/zanzibar/clients/foo/foo.txt") assert.Error(t, err, "should return error for not a thrift file") } diff --git a/codegen/service_test.go b/codegen/service_test.go index b50a9e2bf..20d73fc3d 100644 --- a/codegen/service_test.go +++ b/codegen/service_test.go @@ -43,13 +43,13 @@ func TestModuleSpecNoThriftExist(t *testing.T) { } func TestProtoModuleSpec(t *testing.T) { - echoProto := "../examples/example-gateway/idl/clients-idl/clients/echo/echo.proto" + echoProto := "../examples/example-gateway/bazel-out/idl/clients-idl/clients/echo/echo.proto" _, err := codegen.NewProtoModuleSpec(echoProto, false, newPackageHelper(t)) assert.NoError(t, err, "unable to parse the proto file") } func TestProtoModuleSpecParseError(t *testing.T) { - tmpFile, err := ioutil.TempFile("../examples/example-gateway/idl/clients-idl/clients/", "temp*.proto") + tmpFile, err := ioutil.TempFile("../examples/example-gateway/bazel-out/idl/clients-idl/clients/", "temp*.proto") assert.NoError(t, err, "failed to create temp file") defer func(name string) { _ = os.Remove(name) diff --git a/examples/example-gateway/idl/clients-idl/clients/echo/echo.proto b/examples/example-gateway/bazel-out/idl/clients-idl/clients/echo/echo.proto similarity index 100% rename from examples/example-gateway/idl/clients-idl/clients/echo/echo.proto rename to examples/example-gateway/bazel-out/idl/clients-idl/clients/echo/echo.proto diff --git a/examples/example-gateway/build.yaml b/examples/example-gateway/build.yaml index a7832da49..c75b098e3 100644 --- a/examples/example-gateway/build.yaml +++ b/examples/example-gateway/build.yaml @@ -14,6 +14,7 @@ defaultHeaders: packageRoot: github.com/uber/zanzibar/examples/example-gateway targetGenDir: ./build idlRootDir: ./idl +protoIdlRootDir: ./bazel-out/idl moduleIdlSubDir: endpoints: endpoints-idl default: clients-idl