From ff65066567990880fe357a330c1d65cad4e7eae3 Mon Sep 17 00:00:00 2001 From: Prashant Varanasi Date: Thu, 5 Jan 2017 21:52:57 -0800 Subject: [PATCH] thrift-gen: Add "namespace go" support. Currently, if "a.thrift" includes "b.thrift" but "b.thrift" has a "namespace go" declaration, then the Apache Thrift generated code uses the "namespace go" value as the package name. thrift-gen was not respecting the "namespace go" and so the generated code would not compile. This will also help work around the following scenario. If we had: - b/b.thrift which includes: b/shared.thrift - c/c.thrift which includes c/shared.thrift If a.thrift includes "b/b.thrift", everything is OK. As soon as it tries to include "c/c.thrift", everything breaks since the "shared" package is overwritten by the last compiled "shared.thrift". We can work around this by adding a "namespace go" declaration to one of the shared.thrift file to use a unique package for each of the shared.thrift files, without having to rename the file (Which may break a lot of existing code). Fixes #101 --- thrift/thrift-gen/include.go | 13 +++++------ thrift/thrift-gen/main.go | 22 +++++++++++++++---- .../include_test/namespace/a/shared.thrift | 5 +++++ .../include_test/namespace/b/shared.thrift | 3 +++ .../include_test/namespace/namespace.thrift | 5 +++++ thrift/thrift-gen/typestate.go | 8 ++++++- 6 files changed, 44 insertions(+), 12 deletions(-) create mode 100644 thrift/thrift-gen/test_files/include_test/namespace/a/shared.thrift create mode 100644 thrift/thrift-gen/test_files/include_test/namespace/b/shared.thrift create mode 100644 thrift/thrift-gen/test_files/include_test/namespace/namespace.thrift diff --git a/thrift/thrift-gen/include.go b/thrift/thrift-gen/include.go index 73df32bb..43cdac83 100644 --- a/thrift/thrift-gen/include.go +++ b/thrift/thrift-gen/include.go @@ -20,16 +20,13 @@ package main -import ( - "strings" - - "github.com/samuel/go-thrift/parser" -) +import "github.com/samuel/go-thrift/parser" // Include represents a single include statement in the Thrift file. type Include struct { key string file string + pkg string } // Import returns the go import to use for this package. @@ -42,15 +39,17 @@ func (i *Include) Import() string { // Package returns the package selector for this package. func (i *Include) Package() string { - return strings.ToLower(i.key) + return i.pkg } -func createIncludes(parsed *parser.Thrift) map[string]*Include { +func createIncludes(parsed *parser.Thrift, all map[string]parseState) map[string]*Include { includes := make(map[string]*Include) for k, v := range parsed.Includes { + included := all[v] includes[k] = &Include{ key: k, file: v, + pkg: included.namespace, } } return includes diff --git a/thrift/thrift-gen/main.go b/thrift/thrift-gen/main.go index c7ec99fb..e9722d95 100644 --- a/thrift/thrift-gen/main.go +++ b/thrift/thrift-gen/main.go @@ -129,9 +129,10 @@ func processFile(opts processOptions) error { } type parseState struct { - ast *parser.Thrift - global *State - services []*Service + ast *parser.Thrift + namespace string + global *State + services []*Service } // parseTemplates returns a list of Templates that must be rendered given the template files. @@ -171,11 +172,24 @@ func parseFile(inputFile string) (map[string]parseState, error) { if err != nil { return nil, fmt.Errorf("wrap services failed: %v", err) } - allParsed[filename] = parseState{v, state, services} + + namespace := getNamespace(filename, v) + allParsed[filename] = parseState{v, namespace, state, services} } + setIncludes(allParsed) return allParsed, setExtends(allParsed) } +func getNamespace(filename string, v *parser.Thrift) string { + if ns, ok := v.Namespaces["go"]; ok { + return ns + } + + base := filepath.Base(filename) + base = strings.TrimSuffix(base, filepath.Ext(base)) + return strings.ToLower(base) +} + func generateCode(outputFile string, template *Template, pkg string, state parseState) error { if outputFile == "" { return fmt.Errorf("must speciy an output file") diff --git a/thrift/thrift-gen/test_files/include_test/namespace/a/shared.thrift b/thrift/thrift-gen/test_files/include_test/namespace/a/shared.thrift new file mode 100644 index 00000000..1541013e --- /dev/null +++ b/thrift/thrift-gen/test_files/include_test/namespace/a/shared.thrift @@ -0,0 +1,5 @@ +namespace go a_shared + +include "../b/shared.thrift" + +typedef shared.b_string a_string diff --git a/thrift/thrift-gen/test_files/include_test/namespace/b/shared.thrift b/thrift/thrift-gen/test_files/include_test/namespace/b/shared.thrift new file mode 100644 index 00000000..0e2da53f --- /dev/null +++ b/thrift/thrift-gen/test_files/include_test/namespace/b/shared.thrift @@ -0,0 +1,3 @@ +namespace go b_shared + +typedef string b_string diff --git a/thrift/thrift-gen/test_files/include_test/namespace/namespace.thrift b/thrift/thrift-gen/test_files/include_test/namespace/namespace.thrift new file mode 100644 index 00000000..8692adfa --- /dev/null +++ b/thrift/thrift-gen/test_files/include_test/namespace/namespace.thrift @@ -0,0 +1,5 @@ +include "a/shared.thrift" + +service Foo { + void Foo(1: shared.a_string str) +} diff --git a/thrift/thrift-gen/typestate.go b/thrift/thrift-gen/typestate.go index 7d96c6a2..719f730d 100644 --- a/thrift/thrift-gen/typestate.go +++ b/thrift/thrift-gen/typestate.go @@ -51,7 +51,13 @@ func newState(v *parser.Thrift, all map[string]parseState) *State { typedefs[k] = i64Type } - return &State{typedefs, createIncludes(v), all} + return &State{typedefs, nil, all} +} + +func setIncludes(all map[string]parseState) { + for _, v := range all { + v.global.includes = createIncludes(v.ast, all) + } } func (s *State) isBasicType(thriftType string) bool {