From 1c47496025db745e76bcbfaa9a9d479b588f24a1 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Wed, 9 Oct 2019 16:59:40 -0700 Subject: [PATCH 01/16] Hyphenated file support --- compile/compiler.go | 9 +++ compile/error.go | 8 +++ gen/generate.go | 47 +++++++++++++++- gen/generate_test.go | 56 +++++++++++++++++++ gen/golden_test.go | 2 +- gen/internal/tests/Makefile | 2 +- gen/internal/tests/thrift/abc-defs.thrift | 5 ++ .../nestedfiles_error/abc-defs-nested.thrift | 5 ++ .../nestedfiles_error/abc_defs_nested.thrift | 5 ++ .../include_hyphen_files.thrift | 5 ++ .../tests/thrift/non_hyphenated.thrift | 3 + 11 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 gen/internal/tests/thrift/abc-defs.thrift create mode 100644 gen/internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift create mode 100644 gen/internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift create mode 100644 gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift create mode 100644 gen/internal/tests/thrift/non_hyphenated.thrift diff --git a/compile/compiler.go b/compile/compiler.go index 66a4d71f..f864e48b 100644 --- a/compile/compiler.go +++ b/compile/compiler.go @@ -22,6 +22,7 @@ package compile import ( "path/filepath" + "strings" "go.uber.org/thriftrw/ast" "go.uber.org/thriftrw/idl" @@ -251,6 +252,14 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err } } + includeName := fileBaseName(include.Path) + if strings.Contains(includeName, "-") { + return nil, includeError{ + Include: include, + Reason: includingHyphenatedFileNotAllowed{}, + } + } + ipath := filepath.Join(filepath.Dir(m.ThriftPath), include.Path) incM, err := c.load(ipath) if err != nil { diff --git a/compile/error.go b/compile/error.go index 27c74b96..0c945dcf 100644 --- a/compile/error.go +++ b/compile/error.go @@ -64,6 +64,14 @@ func (e includeAsDisabledError) Error() string { return "include-as syntax is currently disabled" } +// includingHyphenatedFileNotAllowed is raised when the user attempts to include hyphenated thrift file which is not +// allowed +type includingHyphenatedFileNotAllowed struct{} + +func (e includingHyphenatedFileNotAllowed) Error() string { + return "including hyphenated thrift file is not allowed" +} + // includeError is raised when there is an error including another Thrift // file. type includeError struct { diff --git a/gen/generate.go b/gen/generate.go index d6b0053b..2aa49013 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -105,6 +105,10 @@ func Generate(m *compile.Module, o *Options) error { genBuilder := newGenerateServiceBuilder(importer) generate := func(m *compile.Module) error { + if err := isDuplicateFileAfterNormalization(m.ThriftPath); err != nil { + return err + } + path, contents, err := generateModule(m, importer, genBuilder, o) if err != nil { return generateError{Name: m.ThriftPath, Reason: err} @@ -165,6 +169,43 @@ func Generate(m *compile.Module, o *Options) error { return nil } +func isDuplicateFileAfterNormalization(f string) error { + absFileName, err := filepath.Abs(f) + if err != nil { + return generateError{Name: f, Reason: fmt.Errorf("File %q does not exist: %v", f, + err)} + } + normalizedFileName := filePathWithUnderscore(absFileName) + if fileNormalized(absFileName, normalizedFileName) && fileExists(normalizedFileName) { + return generateError{Name: f, + Reason: fmt.Errorf("File after normalization %q is colliding with existing file %q in the path, with error: %v", + normalizedFileName, absFileName, err)} + } + return nil +} + +func fileNormalized(f1, f2 string) bool { + return !(f1 == f2) +} + +// fileExists checks if a file exists and is not a directory before we +// try using it to prevent further errors. +func fileExists(filename string) bool { + info, err := os.Stat(filename) + if os.IsNotExist(err) { + return false + } + return !info.IsDir() +} + +func replaceHyphenWithUnderscore(str string) string { + return strings.Replace(str, "-", "_", -1) +} + +func filePathWithUnderscore(p string) string { + return filepath.Join(filepath.Dir(p), replaceHyphenWithUnderscore(filepath.Base(p))) +} + // ThriftPackageImporter determines import paths from a Thrift root. type ThriftPackageImporter interface { // RelativePackage returns the import path for the top-level package of the @@ -186,7 +227,7 @@ type thriftPackageImporter struct { } func (i thriftPackageImporter) RelativePackage(file string) (string, error) { - return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(file, ".thrift")) + return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(filePathWithUnderscore(file), ".thrift")) } func (i thriftPackageImporter) RelativeThriftFilePath(file string) (string, error) { @@ -226,12 +267,14 @@ func generateModule( builder *generateServiceBuilder, o *Options, ) (outputFilepath string, contents []byte, err error) { + // converts file from /home/abc/ab-def.thrift to /home/abc/ab_def.thrift for golang code generation + normalizedThriftPath := filePathWithUnderscore(m.ThriftPath) // packageRelPath is the path relative to outputDir into which we'll be // writing the package for this Thrift file. For $thriftRoot/foo/bar.thrift, // packageRelPath is foo/bar, and packageDir is $outputDir/foo/bar. All // files for bar.thrift will be written to the $outputDir/foo/bar/ tree. The // package will be importable via $importPrefix/foo/bar. - packageRelPath, err := i.RelativePackage(m.ThriftPath) + packageRelPath, err := i.RelativePackage(normalizedThriftPath) if err != nil { return "", nil, err } diff --git a/gen/generate_test.go b/gen/generate_test.go index aadb1e5e..8a4bc5a1 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -79,6 +79,62 @@ func TestGenerateWithRelativePaths(t *testing.T) { } } +func TestGenerateWithHyphenPaths(t *testing.T) { + outputDir, err := ioutil.TempDir("", "thriftrw-generate-test") + require.NoError(t, err) + defer os.RemoveAll(outputDir) + + thriftRoot, err := os.Getwd() + require.NoError(t, err) + + tests := []struct { + filepath string + generateError string + compileError string + }{ + { + filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", + compileError: "including hyphenated thrift file is not allowed", + }, + { + filepath: "internal/tests/thrift/abc-defs.thrift", + }, + { + filepath: "internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift", + generateError: "is colliding with existing file", + }, + { + filepath: "internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift", + }, + } + + for _, test := range tests { + module, err := compile.Compile(test.filepath) + if test.compileError != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), test.compileError) + } else { + assert.NoError(t, err) + } + if module == nil { + continue + } + + opt := &Options{ + OutputDir: outputDir, + PackagePrefix: "go.uber.org/thriftrw/gen", + ThriftRoot: thriftRoot, + } + err = Generate(module, opt) + if test.generateError != "" { + assert.Error(t, err, "expected code generation with filepath %v to fail", test.filepath) + assert.Contains(t, err.Error(), test.generateError) + } else { + assert.NoError(t, err) + } + } +} + func TestGenerate(t *testing.T) { var ( ts compile.TypeSpec = &compile.TypedefSpec{ diff --git a/gen/golden_test.go b/gen/golden_test.go index d1a85c8d..edb578df 100644 --- a/gen/golden_test.go +++ b/gen/golden_test.go @@ -58,7 +58,7 @@ func TestCodeIsUpToDate(t *testing.T) { defer os.RemoveAll(outputDir) for _, thriftFile := range thriftFiles { - pkgRelPath := strings.TrimSuffix(filepath.Base(thriftFile), ".thrift") + pkgRelPath := strings.TrimSuffix(filePathWithUnderscore(filepath.Base(thriftFile)), ".thrift") currentPackageDir := filepath.Join("internal/tests", pkgRelPath) newPackageDir := filepath.Join(outputDir, pkgRelPath) diff --git a/gen/internal/tests/Makefile b/gen/internal/tests/Makefile index 06dc8241..4831435d 100644 --- a/gen/internal/tests/Makefile +++ b/gen/internal/tests/Makefile @@ -18,4 +18,4 @@ nozap: thrift/nozap.thrift $(THRIFTRW) $(THRIFTRW) --no-recurse --no-zap $< %: thrift/%.thrift $(THRIFTRW) - $(THRIFTRW) --no-recurse $< + $(THRIFTRW) $< diff --git a/gen/internal/tests/thrift/abc-defs.thrift b/gen/internal/tests/thrift/abc-defs.thrift new file mode 100644 index 00000000..cd5d08bf --- /dev/null +++ b/gen/internal/tests/thrift/abc-defs.thrift @@ -0,0 +1,5 @@ +include "./non_hyphenated.thrift" + +struct DocumentStruct { + 1: required non_hyphenated.Second second +} \ No newline at end of file diff --git a/gen/internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift b/gen/internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift new file mode 100644 index 00000000..fcb4a7a0 --- /dev/null +++ b/gen/internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift @@ -0,0 +1,5 @@ +include "../non_hyphenated.thrift" + +struct DocumentStructure { + 1: required non_hyphenated.Second r2 +} \ No newline at end of file diff --git a/gen/internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift b/gen/internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift new file mode 100644 index 00000000..4c511477 --- /dev/null +++ b/gen/internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift @@ -0,0 +1,5 @@ +include "../non_hyphenated.thrift" + +struct DocumentStructure { + 1: required non_hyphenated.Second r1 +} \ No newline at end of file diff --git a/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift b/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift new file mode 100644 index 00000000..666d187f --- /dev/null +++ b/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift @@ -0,0 +1,5 @@ +include "../abc-defs.thrift" + +union DocumentUnion { + 1: string value +} \ No newline at end of file diff --git a/gen/internal/tests/thrift/non_hyphenated.thrift b/gen/internal/tests/thrift/non_hyphenated.thrift new file mode 100644 index 00000000..73681ca6 --- /dev/null +++ b/gen/internal/tests/thrift/non_hyphenated.thrift @@ -0,0 +1,3 @@ +struct First {} + +struct Second {} \ No newline at end of file From 6c0ab1d3507babb3606dc10eab451f570cd60936 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Thu, 10 Oct 2019 10:45:36 -0700 Subject: [PATCH 02/16] gen code --- gen/internal/tests/abc_defs/abc_defs.go | 173 +++++++++++++++ .../tests/non_hyphenated/non_hyphenated.go | 203 ++++++++++++++++++ 2 files changed, 376 insertions(+) create mode 100644 gen/internal/tests/abc_defs/abc_defs.go create mode 100644 gen/internal/tests/non_hyphenated/non_hyphenated.go diff --git a/gen/internal/tests/abc_defs/abc_defs.go b/gen/internal/tests/abc_defs/abc_defs.go new file mode 100644 index 00000000..f6b97059 --- /dev/null +++ b/gen/internal/tests/abc_defs/abc_defs.go @@ -0,0 +1,173 @@ +// Code generated by thriftrw v1.20.1-dev. DO NOT EDIT. +// @generated + +package abc_defs + +import ( + errors "errors" + fmt "fmt" + multierr "go.uber.org/multierr" + non_hyphenated "go.uber.org/thriftrw/gen/internal/tests/non_hyphenated" + thriftreflect "go.uber.org/thriftrw/thriftreflect" + wire "go.uber.org/thriftrw/wire" + zapcore "go.uber.org/zap/zapcore" + strings "strings" +) + +type DocumentStruct struct { + Second *non_hyphenated.Second `json:"second,required"` +} + +// ToWire translates a DocumentStruct struct into a Thrift-level intermediate +// representation. This intermediate representation may be serialized +// into bytes using a ThriftRW protocol implementation. +// +// An error is returned if the struct or any of its fields failed to +// validate. +// +// x, err := v.ToWire() +// if err != nil { +// return err +// } +// +// if err := binaryProtocol.Encode(x, writer); err != nil { +// return err +// } +func (v *DocumentStruct) ToWire() (wire.Value, error) { + var ( + fields [1]wire.Field + i int = 0 + w wire.Value + err error + ) + + if v.Second == nil { + return w, errors.New("field Second of DocumentStruct is required") + } + w, err = v.Second.ToWire() + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 1, Value: w} + i++ + + return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil +} + +func _Second_Read(w wire.Value) (*non_hyphenated.Second, error) { + var v non_hyphenated.Second + err := v.FromWire(w) + return &v, err +} + +// FromWire deserializes a DocumentStruct struct from its Thrift-level +// representation. The Thrift-level representation may be obtained +// from a ThriftRW protocol implementation. +// +// An error is returned if we were unable to build a DocumentStruct struct +// from the provided intermediate representation. +// +// x, err := binaryProtocol.Decode(reader, wire.TStruct) +// if err != nil { +// return nil, err +// } +// +// var v DocumentStruct +// if err := v.FromWire(x); err != nil { +// return nil, err +// } +// return &v, nil +func (v *DocumentStruct) FromWire(w wire.Value) error { + var err error + + secondIsSet := false + + for _, field := range w.GetStruct().Fields { + switch field.ID { + case 1: + if field.Value.Type() == wire.TStruct { + v.Second, err = _Second_Read(field.Value) + if err != nil { + return err + } + secondIsSet = true + } + } + } + + if !secondIsSet { + return errors.New("field Second of DocumentStruct is required") + } + + return nil +} + +// String returns a readable string representation of a DocumentStruct +// struct. +func (v *DocumentStruct) String() string { + if v == nil { + return "" + } + + var fields [1]string + i := 0 + fields[i] = fmt.Sprintf("Second: %v", v.Second) + i++ + + return fmt.Sprintf("DocumentStruct{%v}", strings.Join(fields[:i], ", ")) +} + +// Equals returns true if all the fields of this DocumentStruct match the +// provided DocumentStruct. +// +// This function performs a deep comparison. +func (v *DocumentStruct) Equals(rhs *DocumentStruct) bool { + if v == nil { + return rhs == nil + } else if rhs == nil { + return false + } + if !v.Second.Equals(rhs.Second) { + return false + } + + return true +} + +// MarshalLogObject implements zapcore.ObjectMarshaler, enabling +// fast logging of DocumentStruct. +func (v *DocumentStruct) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { + if v == nil { + return nil + } + err = multierr.Append(err, enc.AddObject("second", v.Second)) + return err +} + +// GetSecond returns the value of Second if it is set or its +// zero value if it is unset. +func (v *DocumentStruct) GetSecond() (o *non_hyphenated.Second) { + if v != nil { + o = v.Second + } + return +} + +// IsSetSecond returns true if Second is not nil. +func (v *DocumentStruct) IsSetSecond() bool { + return v != nil && v.Second != nil +} + +// ThriftModule represents the IDL file used to generate this package. +var ThriftModule = &thriftreflect.ThriftModule{ + Name: "abc-defs", + Package: "go.uber.org/thriftrw/gen/internal/tests/abc_defs", + FilePath: "abc-defs.thrift", + SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", + Includes: []*thriftreflect.ThriftModule{ + non_hyphenated.ThriftModule, + }, + Raw: rawIDL, +} + +const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStruct {\n 1: required non_hyphenated.Second second\n}" diff --git a/gen/internal/tests/non_hyphenated/non_hyphenated.go b/gen/internal/tests/non_hyphenated/non_hyphenated.go new file mode 100644 index 00000000..92673e9e --- /dev/null +++ b/gen/internal/tests/non_hyphenated/non_hyphenated.go @@ -0,0 +1,203 @@ +// Code generated by thriftrw v1.20.1-dev. DO NOT EDIT. +// @generated + +package non_hyphenated + +import ( + fmt "fmt" + thriftreflect "go.uber.org/thriftrw/thriftreflect" + wire "go.uber.org/thriftrw/wire" + zapcore "go.uber.org/zap/zapcore" + strings "strings" +) + +type First struct { +} + +// ToWire translates a First struct into a Thrift-level intermediate +// representation. This intermediate representation may be serialized +// into bytes using a ThriftRW protocol implementation. +// +// An error is returned if the struct or any of its fields failed to +// validate. +// +// x, err := v.ToWire() +// if err != nil { +// return err +// } +// +// if err := binaryProtocol.Encode(x, writer); err != nil { +// return err +// } +func (v *First) ToWire() (wire.Value, error) { + var ( + fields [0]wire.Field + i int = 0 + ) + + return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil +} + +// FromWire deserializes a First struct from its Thrift-level +// representation. The Thrift-level representation may be obtained +// from a ThriftRW protocol implementation. +// +// An error is returned if we were unable to build a First struct +// from the provided intermediate representation. +// +// x, err := binaryProtocol.Decode(reader, wire.TStruct) +// if err != nil { +// return nil, err +// } +// +// var v First +// if err := v.FromWire(x); err != nil { +// return nil, err +// } +// return &v, nil +func (v *First) FromWire(w wire.Value) error { + + for _, field := range w.GetStruct().Fields { + switch field.ID { + } + } + + return nil +} + +// String returns a readable string representation of a First +// struct. +func (v *First) String() string { + if v == nil { + return "" + } + + var fields [0]string + i := 0 + + return fmt.Sprintf("First{%v}", strings.Join(fields[:i], ", ")) +} + +// Equals returns true if all the fields of this First match the +// provided First. +// +// This function performs a deep comparison. +func (v *First) Equals(rhs *First) bool { + if v == nil { + return rhs == nil + } else if rhs == nil { + return false + } + + return true +} + +// MarshalLogObject implements zapcore.ObjectMarshaler, enabling +// fast logging of First. +func (v *First) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { + if v == nil { + return nil + } + return err +} + +type Second struct { +} + +// ToWire translates a Second struct into a Thrift-level intermediate +// representation. This intermediate representation may be serialized +// into bytes using a ThriftRW protocol implementation. +// +// An error is returned if the struct or any of its fields failed to +// validate. +// +// x, err := v.ToWire() +// if err != nil { +// return err +// } +// +// if err := binaryProtocol.Encode(x, writer); err != nil { +// return err +// } +func (v *Second) ToWire() (wire.Value, error) { + var ( + fields [0]wire.Field + i int = 0 + ) + + return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil +} + +// FromWire deserializes a Second struct from its Thrift-level +// representation. The Thrift-level representation may be obtained +// from a ThriftRW protocol implementation. +// +// An error is returned if we were unable to build a Second struct +// from the provided intermediate representation. +// +// x, err := binaryProtocol.Decode(reader, wire.TStruct) +// if err != nil { +// return nil, err +// } +// +// var v Second +// if err := v.FromWire(x); err != nil { +// return nil, err +// } +// return &v, nil +func (v *Second) FromWire(w wire.Value) error { + + for _, field := range w.GetStruct().Fields { + switch field.ID { + } + } + + return nil +} + +// String returns a readable string representation of a Second +// struct. +func (v *Second) String() string { + if v == nil { + return "" + } + + var fields [0]string + i := 0 + + return fmt.Sprintf("Second{%v}", strings.Join(fields[:i], ", ")) +} + +// Equals returns true if all the fields of this Second match the +// provided Second. +// +// This function performs a deep comparison. +func (v *Second) Equals(rhs *Second) bool { + if v == nil { + return rhs == nil + } else if rhs == nil { + return false + } + + return true +} + +// MarshalLogObject implements zapcore.ObjectMarshaler, enabling +// fast logging of Second. +func (v *Second) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { + if v == nil { + return nil + } + return err +} + +// ThriftModule represents the IDL file used to generate this package. +var ThriftModule = &thriftreflect.ThriftModule{ + Name: "non_hyphenated", + Package: "go.uber.org/thriftrw/gen/internal/tests/non_hyphenated", + FilePath: "non_hyphenated.thrift", + SHA1: "5cbe76400805b1c5a248631c4ad87e3dfe6f183e", + Raw: rawIDL, +} + +const rawIDL = "struct First {}\n\nstruct Second {}" From 61f08cd96f608410ee45f3723d77fef6d265eaa4 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Thu, 10 Oct 2019 21:15:47 -0700 Subject: [PATCH 03/16] review comments --- compile/compiler.go | 4 +- compile/error.go | 10 +- gen/generate.go | 33 ++-- gen/generate_test.go | 59 +++--- gen/golden_test.go | 2 +- gen/internal/tests/abc_defs/abc_defs.go | 173 ------------------ ...abc-defs.thrift => hyphenated-file.thrift} | 0 ...d.thrift => hyphenated-file-nested.thrift} | 0 ...d.thrift => hyphenated_file_nested.thrift} | 0 .../include_hyphen_files.thrift | 2 +- 10 files changed, 56 insertions(+), 227 deletions(-) delete mode 100644 gen/internal/tests/abc_defs/abc_defs.go rename gen/internal/tests/thrift/{abc-defs.thrift => hyphenated-file.thrift} (100%) rename gen/internal/tests/thrift/nestedfiles_error/{abc-defs-nested.thrift => hyphenated-file-nested.thrift} (100%) rename gen/internal/tests/thrift/nestedfiles_error/{abc_defs_nested.thrift => hyphenated_file_nested.thrift} (100%) diff --git a/compile/compiler.go b/compile/compiler.go index f864e48b..bdf7f47a 100644 --- a/compile/compiler.go +++ b/compile/compiler.go @@ -243,6 +243,8 @@ func (c compiler) gather(m *Module, prog *ast.Program) error { // include loads the file specified by the given include in the given Module. // // The path to the file is relative to the ThriftPath of the given module. +// Among other errors, one place it will error out if you attempt to include +// a file whose name starts with hyphen. func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, error) { if len(include.Name) > 0 { // TODO(abg): Add support for include-as flag somewhere. @@ -256,7 +258,7 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err if strings.Contains(includeName, "-") { return nil, includeError{ Include: include, - Reason: includingHyphenatedFileNotAllowed{}, + Reason: includeHyphenatedFileNameError{}, } } diff --git a/compile/error.go b/compile/error.go index 0c945dcf..0b5c2a53 100644 --- a/compile/error.go +++ b/compile/error.go @@ -64,12 +64,12 @@ func (e includeAsDisabledError) Error() string { return "include-as syntax is currently disabled" } -// includingHyphenatedFileNotAllowed is raised when the user attempts to include hyphenated thrift file which is not -// allowed -type includingHyphenatedFileNotAllowed struct{} +// includeHyphenatedFileNameError is raised when the user attempts to +// include hyphenated file names +type includeHyphenatedFileNameError struct{} -func (e includingHyphenatedFileNotAllowed) Error() string { - return "including hyphenated thrift file is not allowed" +func (e includeHyphenatedFileNameError) Error() string { + return "cannot include hyphenated Thrift files" } // includeError is raised when there is an error including another Thrift diff --git a/gen/generate.go b/gen/generate.go index 2aa49013..365f0891 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -105,7 +105,7 @@ func Generate(m *compile.Module, o *Options) error { genBuilder := newGenerateServiceBuilder(importer) generate := func(m *compile.Module) error { - if err := isDuplicateFileAfterNormalization(m.ThriftPath); err != nil { + if err := duplicateAfterNormalization(m.ThriftPath); err != nil { return err } @@ -169,27 +169,25 @@ func Generate(m *compile.Module, o *Options) error { return nil } -func isDuplicateFileAfterNormalization(f string) error { - absFileName, err := filepath.Abs(f) +func duplicateAfterNormalization(fileName string) error { + absFileName, err := filepath.Abs(fileName) if err != nil { - return generateError{Name: f, Reason: fmt.Errorf("File %q does not exist: %v", f, + return generateError{Name: fileName, Reason: fmt.Errorf("File %q does not exist: %v", fileName, err)} } - normalizedFileName := filePathWithUnderscore(absFileName) + normalizedFileName := normalizeFilePath(absFileName) if fileNormalized(absFileName, normalizedFileName) && fileExists(normalizedFileName) { - return generateError{Name: f, - Reason: fmt.Errorf("File after normalization %q is colliding with existing file %q in the path, with error: %v", - normalizedFileName, absFileName, err)} + return generateError{ + Name: fileName, + Reason: fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, absFileName)} } return nil } func fileNormalized(f1, f2 string) bool { - return !(f1 == f2) + return f1 != f2 } -// fileExists checks if a file exists and is not a directory before we -// try using it to prevent further errors. func fileExists(filename string) bool { info, err := os.Stat(filename) if os.IsNotExist(err) { @@ -198,12 +196,9 @@ func fileExists(filename string) bool { return !info.IsDir() } -func replaceHyphenWithUnderscore(str string) string { - return strings.Replace(str, "-", "_", -1) -} - -func filePathWithUnderscore(p string) string { - return filepath.Join(filepath.Dir(p), replaceHyphenWithUnderscore(filepath.Base(p))) +func normalizeFilePath(p string) string { + fileName := strings.Replace(filepath.Base(p), "-", "_", -1) + return filepath.Join(filepath.Dir(p), fileName) } // ThriftPackageImporter determines import paths from a Thrift root. @@ -227,7 +222,7 @@ type thriftPackageImporter struct { } func (i thriftPackageImporter) RelativePackage(file string) (string, error) { - return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(filePathWithUnderscore(file), ".thrift")) + return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(normalizeFilePath(file), ".thrift")) } func (i thriftPackageImporter) RelativeThriftFilePath(file string) (string, error) { @@ -268,7 +263,7 @@ func generateModule( o *Options, ) (outputFilepath string, contents []byte, err error) { // converts file from /home/abc/ab-def.thrift to /home/abc/ab_def.thrift for golang code generation - normalizedThriftPath := filePathWithUnderscore(m.ThriftPath) + normalizedThriftPath := normalizeFilePath(m.ThriftPath) // packageRelPath is the path relative to outputDir into which we'll be // writing the package for this Thrift file. For $thriftRoot/foo/bar.thrift, // packageRelPath is foo/bar, and packageDir is $outputDir/foo/bar. All diff --git a/gen/generate_test.go b/gen/generate_test.go index 8a4bc5a1..fbd0b7b0 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -80,7 +80,7 @@ func TestGenerateWithRelativePaths(t *testing.T) { } func TestGenerateWithHyphenPaths(t *testing.T) { - outputDir, err := ioutil.TempDir("", "thriftrw-generate-test") + outputDir, err := ioutil.TempDir("", "thriftrw-generate-tt") require.NoError(t, err) defer os.RemoveAll(outputDir) @@ -88,50 +88,55 @@ func TestGenerateWithHyphenPaths(t *testing.T) { require.NoError(t, err) tests := []struct { + name string filepath string generateError string compileError string }{ { + name: "hyphen files inclusion gives include error", filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", - compileError: "including hyphenated thrift file is not allowed", + compileError: "cannot include hyphenated Thrift files", }, { - filepath: "internal/tests/thrift/abc-defs.thrift", + name: "hyphen files as top level file allowed", + filepath: "internal/tests/thrift/hyphenated-file.thrift", }, { - filepath: "internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift", + name: "hyphen file post normalization code gen fails with non hyphen same name file", + filepath: "internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift", generateError: "is colliding with existing file", }, { - filepath: "internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift", + name: "non hyphen file code gen doesn't collide with same name file post normalization of hyphen file", + filepath: "internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift", }, } - for _, test := range tests { - module, err := compile.Compile(test.filepath) - if test.compileError != "" { - assert.Error(t, err) - assert.Contains(t, err.Error(), test.compileError) - } else { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + module, err := compile.Compile(tt.filepath) + if tt.compileError != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.compileError) + return + } assert.NoError(t, err) - } - if module == nil { - continue - } + require.NotNil(t, module) - opt := &Options{ - OutputDir: outputDir, - PackagePrefix: "go.uber.org/thriftrw/gen", - ThriftRoot: thriftRoot, - } - err = Generate(module, opt) - if test.generateError != "" { - assert.Error(t, err, "expected code generation with filepath %v to fail", test.filepath) - assert.Contains(t, err.Error(), test.generateError) - } else { - assert.NoError(t, err) - } + opt := &Options{ + OutputDir: outputDir, + PackagePrefix: "go.uber.org/thriftrw/gen", + ThriftRoot: thriftRoot, + } + err = Generate(module, opt) + if tt.generateError != "" { + require.Error(t, err, "expected code generation with filepath %v to fail", tt.filepath) + assert.Contains(t, err.Error(), tt.generateError) + } else { + assert.NoError(t, err) + } + }) } } diff --git a/gen/golden_test.go b/gen/golden_test.go index edb578df..7bdb325c 100644 --- a/gen/golden_test.go +++ b/gen/golden_test.go @@ -58,7 +58,7 @@ func TestCodeIsUpToDate(t *testing.T) { defer os.RemoveAll(outputDir) for _, thriftFile := range thriftFiles { - pkgRelPath := strings.TrimSuffix(filePathWithUnderscore(filepath.Base(thriftFile)), ".thrift") + pkgRelPath := strings.TrimSuffix(normalizeFilePath(filepath.Base(thriftFile)), ".thrift") currentPackageDir := filepath.Join("internal/tests", pkgRelPath) newPackageDir := filepath.Join(outputDir, pkgRelPath) diff --git a/gen/internal/tests/abc_defs/abc_defs.go b/gen/internal/tests/abc_defs/abc_defs.go deleted file mode 100644 index f6b97059..00000000 --- a/gen/internal/tests/abc_defs/abc_defs.go +++ /dev/null @@ -1,173 +0,0 @@ -// Code generated by thriftrw v1.20.1-dev. DO NOT EDIT. -// @generated - -package abc_defs - -import ( - errors "errors" - fmt "fmt" - multierr "go.uber.org/multierr" - non_hyphenated "go.uber.org/thriftrw/gen/internal/tests/non_hyphenated" - thriftreflect "go.uber.org/thriftrw/thriftreflect" - wire "go.uber.org/thriftrw/wire" - zapcore "go.uber.org/zap/zapcore" - strings "strings" -) - -type DocumentStruct struct { - Second *non_hyphenated.Second `json:"second,required"` -} - -// ToWire translates a DocumentStruct struct into a Thrift-level intermediate -// representation. This intermediate representation may be serialized -// into bytes using a ThriftRW protocol implementation. -// -// An error is returned if the struct or any of its fields failed to -// validate. -// -// x, err := v.ToWire() -// if err != nil { -// return err -// } -// -// if err := binaryProtocol.Encode(x, writer); err != nil { -// return err -// } -func (v *DocumentStruct) ToWire() (wire.Value, error) { - var ( - fields [1]wire.Field - i int = 0 - w wire.Value - err error - ) - - if v.Second == nil { - return w, errors.New("field Second of DocumentStruct is required") - } - w, err = v.Second.ToWire() - if err != nil { - return w, err - } - fields[i] = wire.Field{ID: 1, Value: w} - i++ - - return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil -} - -func _Second_Read(w wire.Value) (*non_hyphenated.Second, error) { - var v non_hyphenated.Second - err := v.FromWire(w) - return &v, err -} - -// FromWire deserializes a DocumentStruct struct from its Thrift-level -// representation. The Thrift-level representation may be obtained -// from a ThriftRW protocol implementation. -// -// An error is returned if we were unable to build a DocumentStruct struct -// from the provided intermediate representation. -// -// x, err := binaryProtocol.Decode(reader, wire.TStruct) -// if err != nil { -// return nil, err -// } -// -// var v DocumentStruct -// if err := v.FromWire(x); err != nil { -// return nil, err -// } -// return &v, nil -func (v *DocumentStruct) FromWire(w wire.Value) error { - var err error - - secondIsSet := false - - for _, field := range w.GetStruct().Fields { - switch field.ID { - case 1: - if field.Value.Type() == wire.TStruct { - v.Second, err = _Second_Read(field.Value) - if err != nil { - return err - } - secondIsSet = true - } - } - } - - if !secondIsSet { - return errors.New("field Second of DocumentStruct is required") - } - - return nil -} - -// String returns a readable string representation of a DocumentStruct -// struct. -func (v *DocumentStruct) String() string { - if v == nil { - return "" - } - - var fields [1]string - i := 0 - fields[i] = fmt.Sprintf("Second: %v", v.Second) - i++ - - return fmt.Sprintf("DocumentStruct{%v}", strings.Join(fields[:i], ", ")) -} - -// Equals returns true if all the fields of this DocumentStruct match the -// provided DocumentStruct. -// -// This function performs a deep comparison. -func (v *DocumentStruct) Equals(rhs *DocumentStruct) bool { - if v == nil { - return rhs == nil - } else if rhs == nil { - return false - } - if !v.Second.Equals(rhs.Second) { - return false - } - - return true -} - -// MarshalLogObject implements zapcore.ObjectMarshaler, enabling -// fast logging of DocumentStruct. -func (v *DocumentStruct) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { - if v == nil { - return nil - } - err = multierr.Append(err, enc.AddObject("second", v.Second)) - return err -} - -// GetSecond returns the value of Second if it is set or its -// zero value if it is unset. -func (v *DocumentStruct) GetSecond() (o *non_hyphenated.Second) { - if v != nil { - o = v.Second - } - return -} - -// IsSetSecond returns true if Second is not nil. -func (v *DocumentStruct) IsSetSecond() bool { - return v != nil && v.Second != nil -} - -// ThriftModule represents the IDL file used to generate this package. -var ThriftModule = &thriftreflect.ThriftModule{ - Name: "abc-defs", - Package: "go.uber.org/thriftrw/gen/internal/tests/abc_defs", - FilePath: "abc-defs.thrift", - SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", - Includes: []*thriftreflect.ThriftModule{ - non_hyphenated.ThriftModule, - }, - Raw: rawIDL, -} - -const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStruct {\n 1: required non_hyphenated.Second second\n}" diff --git a/gen/internal/tests/thrift/abc-defs.thrift b/gen/internal/tests/thrift/hyphenated-file.thrift similarity index 100% rename from gen/internal/tests/thrift/abc-defs.thrift rename to gen/internal/tests/thrift/hyphenated-file.thrift diff --git a/gen/internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift b/gen/internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift similarity index 100% rename from gen/internal/tests/thrift/nestedfiles_error/abc-defs-nested.thrift rename to gen/internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift diff --git a/gen/internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift b/gen/internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift similarity index 100% rename from gen/internal/tests/thrift/nestedfiles_error/abc_defs_nested.thrift rename to gen/internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift diff --git a/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift b/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift index 666d187f..cf321a3f 100644 --- a/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift +++ b/gen/internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift @@ -1,4 +1,4 @@ -include "../abc-defs.thrift" +include "../hyphenated-file.thrift" union DocumentUnion { 1: string value From 232c51a4566c12c058bd21bfd2ec100f994f33b6 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Thu, 10 Oct 2019 21:19:18 -0700 Subject: [PATCH 04/16] generated go code --- gen/generate_test.go | 2 +- .../tests/hyphenated_file/hyphenated_file.go | 173 ++++++++++++++++++ 2 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 gen/internal/tests/hyphenated_file/hyphenated_file.go diff --git a/gen/generate_test.go b/gen/generate_test.go index fbd0b7b0..18e35d49 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -80,7 +80,7 @@ func TestGenerateWithRelativePaths(t *testing.T) { } func TestGenerateWithHyphenPaths(t *testing.T) { - outputDir, err := ioutil.TempDir("", "thriftrw-generate-tt") + outputDir, err := ioutil.TempDir("", "thriftrw-generate-test") require.NoError(t, err) defer os.RemoveAll(outputDir) diff --git a/gen/internal/tests/hyphenated_file/hyphenated_file.go b/gen/internal/tests/hyphenated_file/hyphenated_file.go new file mode 100644 index 00000000..35e62ba0 --- /dev/null +++ b/gen/internal/tests/hyphenated_file/hyphenated_file.go @@ -0,0 +1,173 @@ +// Code generated by thriftrw v1.20.1-dev. DO NOT EDIT. +// @generated + +package hyphenated_file + +import ( + errors "errors" + fmt "fmt" + multierr "go.uber.org/multierr" + non_hyphenated "go.uber.org/thriftrw/gen/internal/tests/non_hyphenated" + thriftreflect "go.uber.org/thriftrw/thriftreflect" + wire "go.uber.org/thriftrw/wire" + zapcore "go.uber.org/zap/zapcore" + strings "strings" +) + +type DocumentStruct struct { + Second *non_hyphenated.Second `json:"second,required"` +} + +// ToWire translates a DocumentStruct struct into a Thrift-level intermediate +// representation. This intermediate representation may be serialized +// into bytes using a ThriftRW protocol implementation. +// +// An error is returned if the struct or any of its fields failed to +// validate. +// +// x, err := v.ToWire() +// if err != nil { +// return err +// } +// +// if err := binaryProtocol.Encode(x, writer); err != nil { +// return err +// } +func (v *DocumentStruct) ToWire() (wire.Value, error) { + var ( + fields [1]wire.Field + i int = 0 + w wire.Value + err error + ) + + if v.Second == nil { + return w, errors.New("field Second of DocumentStruct is required") + } + w, err = v.Second.ToWire() + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 1, Value: w} + i++ + + return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil +} + +func _Second_Read(w wire.Value) (*non_hyphenated.Second, error) { + var v non_hyphenated.Second + err := v.FromWire(w) + return &v, err +} + +// FromWire deserializes a DocumentStruct struct from its Thrift-level +// representation. The Thrift-level representation may be obtained +// from a ThriftRW protocol implementation. +// +// An error is returned if we were unable to build a DocumentStruct struct +// from the provided intermediate representation. +// +// x, err := binaryProtocol.Decode(reader, wire.TStruct) +// if err != nil { +// return nil, err +// } +// +// var v DocumentStruct +// if err := v.FromWire(x); err != nil { +// return nil, err +// } +// return &v, nil +func (v *DocumentStruct) FromWire(w wire.Value) error { + var err error + + secondIsSet := false + + for _, field := range w.GetStruct().Fields { + switch field.ID { + case 1: + if field.Value.Type() == wire.TStruct { + v.Second, err = _Second_Read(field.Value) + if err != nil { + return err + } + secondIsSet = true + } + } + } + + if !secondIsSet { + return errors.New("field Second of DocumentStruct is required") + } + + return nil +} + +// String returns a readable string representation of a DocumentStruct +// struct. +func (v *DocumentStruct) String() string { + if v == nil { + return "" + } + + var fields [1]string + i := 0 + fields[i] = fmt.Sprintf("Second: %v", v.Second) + i++ + + return fmt.Sprintf("DocumentStruct{%v}", strings.Join(fields[:i], ", ")) +} + +// Equals returns true if all the fields of this DocumentStruct match the +// provided DocumentStruct. +// +// This function performs a deep comparison. +func (v *DocumentStruct) Equals(rhs *DocumentStruct) bool { + if v == nil { + return rhs == nil + } else if rhs == nil { + return false + } + if !v.Second.Equals(rhs.Second) { + return false + } + + return true +} + +// MarshalLogObject implements zapcore.ObjectMarshaler, enabling +// fast logging of DocumentStruct. +func (v *DocumentStruct) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { + if v == nil { + return nil + } + err = multierr.Append(err, enc.AddObject("second", v.Second)) + return err +} + +// GetSecond returns the value of Second if it is set or its +// zero value if it is unset. +func (v *DocumentStruct) GetSecond() (o *non_hyphenated.Second) { + if v != nil { + o = v.Second + } + return +} + +// IsSetSecond returns true if Second is not nil. +func (v *DocumentStruct) IsSetSecond() bool { + return v != nil && v.Second != nil +} + +// ThriftModule represents the IDL file used to generate this package. +var ThriftModule = &thriftreflect.ThriftModule{ + Name: "hyphenated-file", + Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", + FilePath: "hyphenated-file.thrift", + SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", + Includes: []*thriftreflect.ThriftModule{ + non_hyphenated.ThriftModule, + }, + Raw: rawIDL, +} + +const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStruct {\n 1: required non_hyphenated.Second second\n}" From 68ba6db368e2f150c74094c33de423ea93a4fbc2 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Thu, 10 Oct 2019 21:23:31 -0700 Subject: [PATCH 05/16] test msg assert --- gen/generate_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen/generate_test.go b/gen/generate_test.go index 18e35d49..99837b18 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -105,7 +105,7 @@ func TestGenerateWithHyphenPaths(t *testing.T) { { name: "hyphen file post normalization code gen fails with non hyphen same name file", filepath: "internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift", - generateError: "is colliding with existing file", + generateError: "collides with existing file", }, { name: "non hyphen file code gen doesn't collide with same name file post normalization of hyphen file", From bc12fe234ce0c6094b762e2f1811f7063cec0821 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Fri, 11 Oct 2019 15:15:53 -0700 Subject: [PATCH 06/16] stylistic code comments --- compile/compiler.go | 6 ++---- compile/error.go | 2 +- gen/generate.go | 25 +++++++++++-------------- gen/generate_test.go | 8 ++++---- 4 files changed, 18 insertions(+), 23 deletions(-) diff --git a/compile/compiler.go b/compile/compiler.go index bdf7f47a..5fa4a14f 100644 --- a/compile/compiler.go +++ b/compile/compiler.go @@ -243,8 +243,7 @@ func (c compiler) gather(m *Module, prog *ast.Program) error { // include loads the file specified by the given include in the given Module. // // The path to the file is relative to the ThriftPath of the given module. -// Among other errors, one place it will error out if you attempt to include -// a file whose name starts with hyphen. +// Including hyphenated file names will error. func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, error) { if len(include.Name) > 0 { // TODO(abg): Add support for include-as flag somewhere. @@ -254,8 +253,7 @@ func (c compiler) include(m *Module, include *ast.Include) (*IncludedModule, err } } - includeName := fileBaseName(include.Path) - if strings.Contains(includeName, "-") { + if strings.Contains(fileBaseName(include.Path), "-") { return nil, includeError{ Include: include, Reason: includeHyphenatedFileNameError{}, diff --git a/compile/error.go b/compile/error.go index 0b5c2a53..13cb72ce 100644 --- a/compile/error.go +++ b/compile/error.go @@ -65,7 +65,7 @@ func (e includeAsDisabledError) Error() string { } // includeHyphenatedFileNameError is raised when the user attempts to -// include hyphenated file names +// include hyphenated file names. type includeHyphenatedFileNameError struct{} func (e includeHyphenatedFileNameError) Error() string { diff --git a/gen/generate.go b/gen/generate.go index 365f0891..fe998a9a 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -105,10 +105,6 @@ func Generate(m *compile.Module, o *Options) error { genBuilder := newGenerateServiceBuilder(importer) generate := func(m *compile.Module) error { - if err := duplicateAfterNormalization(m.ThriftPath); err != nil { - return err - } - path, contents, err := generateModule(m, importer, genBuilder, o) if err != nil { return generateError{Name: m.ThriftPath, Reason: err} @@ -172,22 +168,19 @@ func Generate(m *compile.Module, o *Options) error { func duplicateAfterNormalization(fileName string) error { absFileName, err := filepath.Abs(fileName) if err != nil { - return generateError{Name: fileName, Reason: fmt.Errorf("File %q does not exist: %v", fileName, - err)} - } - normalizedFileName := normalizeFilePath(absFileName) - if fileNormalized(absFileName, normalizedFileName) && fileExists(normalizedFileName) { return generateError{ Name: fileName, - Reason: fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, absFileName)} + Reason: fmt.Errorf("File %q does not exist: %v", fileName, err), + } + } + normalizedFileName := normalizeFilePath(absFileName) + fileNormalized := absFileName != normalizedFileName + if fileNormalized && fileExists(normalizedFileName) { + return fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, absFileName) } return nil } -func fileNormalized(f1, f2 string) bool { - return f1 != f2 -} - func fileExists(filename string) bool { info, err := os.Stat(filename) if os.IsNotExist(err) { @@ -196,6 +189,7 @@ func fileExists(filename string) bool { return !info.IsDir() } +// normalizeFilePath replaces hyphens in the file name with underscores. func normalizeFilePath(p string) string { fileName := strings.Replace(filepath.Base(p), "-", "_", -1) return filepath.Join(filepath.Dir(p), fileName) @@ -262,6 +256,9 @@ func generateModule( builder *generateServiceBuilder, o *Options, ) (outputFilepath string, contents []byte, err error) { + if err := duplicateAfterNormalization(m.ThriftPath); err != nil { + return "", nil, err + } // converts file from /home/abc/ab-def.thrift to /home/abc/ab_def.thrift for golang code generation normalizedThriftPath := normalizeFilePath(m.ThriftPath) // packageRelPath is the path relative to outputDir into which we'll be diff --git a/gen/generate_test.go b/gen/generate_test.go index 99837b18..a745f9fe 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -94,7 +94,7 @@ func TestGenerateWithHyphenPaths(t *testing.T) { compileError string }{ { - name: "hyphen files inclusion gives include error", + name: "include with hyphens error", filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", compileError: "cannot include hyphenated Thrift files", }, @@ -103,12 +103,12 @@ func TestGenerateWithHyphenPaths(t *testing.T) { filepath: "internal/tests/thrift/hyphenated-file.thrift", }, { - name: "hyphen file post normalization code gen fails with non hyphen same name file", + name: "normalization collision err", filepath: "internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift", generateError: "collides with existing file", }, { - name: "non hyphen file code gen doesn't collide with same name file post normalization of hyphen file", + name: "normalization success", filepath: "internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift", }, } @@ -121,7 +121,7 @@ func TestGenerateWithHyphenPaths(t *testing.T) { assert.Contains(t, err.Error(), tt.compileError) return } - assert.NoError(t, err) + require.NoError(t, err) require.NotNil(t, module) opt := &Options{ From ef2c62f52e10a85919b9d0b542060176225a140e Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Mon, 14 Oct 2019 12:38:59 -0700 Subject: [PATCH 07/16] code review comments --- gen/generate.go | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/gen/generate.go b/gen/generate.go index fe998a9a..5c89641d 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -165,28 +165,19 @@ func Generate(m *compile.Module, o *Options) error { return nil } -func duplicateAfterNormalization(fileName string) error { - absFileName, err := filepath.Abs(fileName) - if err != nil { - return generateError{ - Name: fileName, - Reason: fmt.Errorf("File %q does not exist: %v", fileName, err), - } - } - normalizedFileName := normalizeFilePath(absFileName) - fileNormalized := absFileName != normalizedFileName +func duplicateAfterNormalization(originalFileName, normalizedFileName string) error { + fileNormalized := originalFileName != normalizedFileName if fileNormalized && fileExists(normalizedFileName) { - return fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, absFileName) + return fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, originalFileName) } return nil } func fileExists(filename string) bool { - info, err := os.Stat(filename) - if os.IsNotExist(err) { - return false + if info, err := os.Stat(filename); err == nil { + return !info.IsDir() } - return !info.IsDir() + return false } // normalizeFilePath replaces hyphens in the file name with underscores. @@ -256,11 +247,11 @@ func generateModule( builder *generateServiceBuilder, o *Options, ) (outputFilepath string, contents []byte, err error) { - if err := duplicateAfterNormalization(m.ThriftPath); err != nil { - return "", nil, err - } // converts file from /home/abc/ab-def.thrift to /home/abc/ab_def.thrift for golang code generation normalizedThriftPath := normalizeFilePath(m.ThriftPath) + if err := duplicateAfterNormalization(m.ThriftPath, normalizedThriftPath); err != nil { + return "", nil, err + } // packageRelPath is the path relative to outputDir into which we'll be // writing the package for this Thrift file. For $thriftRoot/foo/bar.thrift, // packageRelPath is foo/bar, and packageDir is $outputDir/foo/bar. All From e508f241716c2c75f8f5ca7791a9820bd5418613 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Mon, 14 Oct 2019 14:20:34 -0700 Subject: [PATCH 08/16] addressing paths to be retained and just package to be renamed, i.e. path/to/file.thrift becomes path/to/file/*.go. Previously, file.thrift became package $file. Now it would be package $(normalize $file) --- gen/generate.go | 48 ++--- gen/generate_test.go | 25 +-- .../tests/hyphenated-file/hyphenated-file.go | 173 ++++++++++++++++++ .../tests/hyphenated_file/hyphenated_file.go | 74 ++++---- ...e-nested.thrift => hyphenated_file.thrift} | 2 +- .../hyphenated_file_nested.thrift | 5 - 6 files changed, 240 insertions(+), 87 deletions(-) create mode 100644 gen/internal/tests/hyphenated-file/hyphenated-file.go rename gen/internal/tests/thrift/{nestedfiles_error/hyphenated-file-nested.thrift => hyphenated_file.thrift} (65%) delete mode 100644 gen/internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift diff --git a/gen/generate.go b/gen/generate.go index 5c89641d..11cb8d73 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -165,25 +165,14 @@ func Generate(m *compile.Module, o *Options) error { return nil } -func duplicateAfterNormalization(originalFileName, normalizedFileName string) error { - fileNormalized := originalFileName != normalizedFileName - if fileNormalized && fileExists(normalizedFileName) { - return fmt.Errorf("normalized file %q collides with existing file %q", normalizedFileName, originalFileName) - } - return nil -} - -func fileExists(filename string) bool { - if info, err := os.Stat(filename); err == nil { - return !info.IsDir() - } - return false -} - // normalizeFilePath replaces hyphens in the file name with underscores. func normalizeFilePath(p string) string { - fileName := strings.Replace(filepath.Base(p), "-", "_", -1) - return filepath.Join(filepath.Dir(p), fileName) + return filepath.Join(filepath.Dir(p), normalizedName(p)) +} + +// normalizedName replaces hyphens in the file name with underscores. +func normalizedName(f string) string { + return strings.Replace(filepath.Base(f), "-", "_", -1) } // ThriftPackageImporter determines import paths from a Thrift root. @@ -247,30 +236,25 @@ func generateModule( builder *generateServiceBuilder, o *Options, ) (outputFilepath string, contents []byte, err error) { - // converts file from /home/abc/ab-def.thrift to /home/abc/ab_def.thrift for golang code generation - normalizedThriftPath := normalizeFilePath(m.ThriftPath) - if err := duplicateAfterNormalization(m.ThriftPath, normalizedThriftPath); err != nil { - return "", nil, err - } - // packageRelPath is the path relative to outputDir into which we'll be + // thriftRelPath is the path relative to outputDir into which we'll be // writing the package for this Thrift file. For $thriftRoot/foo/bar.thrift, // packageRelPath is foo/bar, and packageDir is $outputDir/foo/bar. All // files for bar.thrift will be written to the $outputDir/foo/bar/ tree. The // package will be importable via $importPrefix/foo/bar. - packageRelPath, err := i.RelativePackage(normalizedThriftPath) + thriftRelPath, err := i.RelativeThriftFilePath(m.ThriftPath) if err != nil { return "", nil, err } - + thriftRelPath = strings.TrimSuffix(thriftRelPath, ".thrift") // TODO(abg): Prefer top-level package name from `namespace go` directive. - packageName := filepath.Base(packageRelPath) + outputFilename := filepath.Base(thriftRelPath) // Output file name defaults to the package name. - outputFilename := packageName + ".go" + outputFilename = outputFilename + ".go" if len(o.OutputFile) > 0 { outputFilename = o.OutputFile } - outputFilepath = filepath.Join(packageRelPath, outputFilename) + outputFilepath = filepath.Join(thriftRelPath, outputFilename) // importPath is the full import path for the top-level package generated // for this Thrift file. @@ -279,10 +263,16 @@ func generateModule( return "", nil, err } + // converts package name from ab-def to ab_def for golang code generation + packageRelPath, err := i.RelativePackage(m.ThriftPath) + if err != nil { + return "", nil, err + } + normalizedPackageName := filepath.Base(packageRelPath) g := NewGenerator(&GeneratorOptions{ Importer: i, ImportPath: importPath, - PackageName: packageName, + PackageName: normalizedPackageName, NoZap: o.NoZap, }) diff --git a/gen/generate_test.go b/gen/generate_test.go index a745f9fe..9e457fce 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -93,24 +93,19 @@ func TestGenerateWithHyphenPaths(t *testing.T) { generateError string compileError string }{ + // { + // name: "include with hyphens error", + // filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", + // compileError: "cannot include hyphenated Thrift files", + // }, { - name: "include with hyphens error", - filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", - compileError: "cannot include hyphenated Thrift files", - }, - { - name: "hyphen files as top level file allowed", + name: "normalization - hyphen files as top level file allowed", filepath: "internal/tests/thrift/hyphenated-file.thrift", }, - { - name: "normalization collision err", - filepath: "internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift", - generateError: "collides with existing file", - }, - { - name: "normalization success", - filepath: "internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift", - }, + // { + // name: "normalization - non hyphen file with hyphen in the directory", + // filepath: "internal/tests/thrift/hyphenated_file.thrift", + // }, } for _, tt := range tests { diff --git a/gen/internal/tests/hyphenated-file/hyphenated-file.go b/gen/internal/tests/hyphenated-file/hyphenated-file.go new file mode 100644 index 00000000..35e62ba0 --- /dev/null +++ b/gen/internal/tests/hyphenated-file/hyphenated-file.go @@ -0,0 +1,173 @@ +// Code generated by thriftrw v1.20.1-dev. DO NOT EDIT. +// @generated + +package hyphenated_file + +import ( + errors "errors" + fmt "fmt" + multierr "go.uber.org/multierr" + non_hyphenated "go.uber.org/thriftrw/gen/internal/tests/non_hyphenated" + thriftreflect "go.uber.org/thriftrw/thriftreflect" + wire "go.uber.org/thriftrw/wire" + zapcore "go.uber.org/zap/zapcore" + strings "strings" +) + +type DocumentStruct struct { + Second *non_hyphenated.Second `json:"second,required"` +} + +// ToWire translates a DocumentStruct struct into a Thrift-level intermediate +// representation. This intermediate representation may be serialized +// into bytes using a ThriftRW protocol implementation. +// +// An error is returned if the struct or any of its fields failed to +// validate. +// +// x, err := v.ToWire() +// if err != nil { +// return err +// } +// +// if err := binaryProtocol.Encode(x, writer); err != nil { +// return err +// } +func (v *DocumentStruct) ToWire() (wire.Value, error) { + var ( + fields [1]wire.Field + i int = 0 + w wire.Value + err error + ) + + if v.Second == nil { + return w, errors.New("field Second of DocumentStruct is required") + } + w, err = v.Second.ToWire() + if err != nil { + return w, err + } + fields[i] = wire.Field{ID: 1, Value: w} + i++ + + return wire.NewValueStruct(wire.Struct{Fields: fields[:i]}), nil +} + +func _Second_Read(w wire.Value) (*non_hyphenated.Second, error) { + var v non_hyphenated.Second + err := v.FromWire(w) + return &v, err +} + +// FromWire deserializes a DocumentStruct struct from its Thrift-level +// representation. The Thrift-level representation may be obtained +// from a ThriftRW protocol implementation. +// +// An error is returned if we were unable to build a DocumentStruct struct +// from the provided intermediate representation. +// +// x, err := binaryProtocol.Decode(reader, wire.TStruct) +// if err != nil { +// return nil, err +// } +// +// var v DocumentStruct +// if err := v.FromWire(x); err != nil { +// return nil, err +// } +// return &v, nil +func (v *DocumentStruct) FromWire(w wire.Value) error { + var err error + + secondIsSet := false + + for _, field := range w.GetStruct().Fields { + switch field.ID { + case 1: + if field.Value.Type() == wire.TStruct { + v.Second, err = _Second_Read(field.Value) + if err != nil { + return err + } + secondIsSet = true + } + } + } + + if !secondIsSet { + return errors.New("field Second of DocumentStruct is required") + } + + return nil +} + +// String returns a readable string representation of a DocumentStruct +// struct. +func (v *DocumentStruct) String() string { + if v == nil { + return "" + } + + var fields [1]string + i := 0 + fields[i] = fmt.Sprintf("Second: %v", v.Second) + i++ + + return fmt.Sprintf("DocumentStruct{%v}", strings.Join(fields[:i], ", ")) +} + +// Equals returns true if all the fields of this DocumentStruct match the +// provided DocumentStruct. +// +// This function performs a deep comparison. +func (v *DocumentStruct) Equals(rhs *DocumentStruct) bool { + if v == nil { + return rhs == nil + } else if rhs == nil { + return false + } + if !v.Second.Equals(rhs.Second) { + return false + } + + return true +} + +// MarshalLogObject implements zapcore.ObjectMarshaler, enabling +// fast logging of DocumentStruct. +func (v *DocumentStruct) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { + if v == nil { + return nil + } + err = multierr.Append(err, enc.AddObject("second", v.Second)) + return err +} + +// GetSecond returns the value of Second if it is set or its +// zero value if it is unset. +func (v *DocumentStruct) GetSecond() (o *non_hyphenated.Second) { + if v != nil { + o = v.Second + } + return +} + +// IsSetSecond returns true if Second is not nil. +func (v *DocumentStruct) IsSetSecond() bool { + return v != nil && v.Second != nil +} + +// ThriftModule represents the IDL file used to generate this package. +var ThriftModule = &thriftreflect.ThriftModule{ + Name: "hyphenated-file", + Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", + FilePath: "hyphenated-file.thrift", + SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", + Includes: []*thriftreflect.ThriftModule{ + non_hyphenated.ThriftModule, + }, + Raw: rawIDL, +} + +const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStruct {\n 1: required non_hyphenated.Second second\n}" diff --git a/gen/internal/tests/hyphenated_file/hyphenated_file.go b/gen/internal/tests/hyphenated_file/hyphenated_file.go index 35e62ba0..3c1b29e1 100644 --- a/gen/internal/tests/hyphenated_file/hyphenated_file.go +++ b/gen/internal/tests/hyphenated_file/hyphenated_file.go @@ -14,11 +14,11 @@ import ( strings "strings" ) -type DocumentStruct struct { - Second *non_hyphenated.Second `json:"second,required"` +type DocumentStructure struct { + R2 *non_hyphenated.Second `json:"r2,required"` } -// ToWire translates a DocumentStruct struct into a Thrift-level intermediate +// ToWire translates a DocumentStructure struct into a Thrift-level intermediate // representation. This intermediate representation may be serialized // into bytes using a ThriftRW protocol implementation. // @@ -33,7 +33,7 @@ type DocumentStruct struct { // if err := binaryProtocol.Encode(x, writer); err != nil { // return err // } -func (v *DocumentStruct) ToWire() (wire.Value, error) { +func (v *DocumentStructure) ToWire() (wire.Value, error) { var ( fields [1]wire.Field i int = 0 @@ -41,10 +41,10 @@ func (v *DocumentStruct) ToWire() (wire.Value, error) { err error ) - if v.Second == nil { - return w, errors.New("field Second of DocumentStruct is required") + if v.R2 == nil { + return w, errors.New("field R2 of DocumentStructure is required") } - w, err = v.Second.ToWire() + w, err = v.R2.ToWire() if err != nil { return w, err } @@ -60,11 +60,11 @@ func _Second_Read(w wire.Value) (*non_hyphenated.Second, error) { return &v, err } -// FromWire deserializes a DocumentStruct struct from its Thrift-level +// FromWire deserializes a DocumentStructure struct from its Thrift-level // representation. The Thrift-level representation may be obtained // from a ThriftRW protocol implementation. // -// An error is returned if we were unable to build a DocumentStruct struct +// An error is returned if we were unable to build a DocumentStructure struct // from the provided intermediate representation. // // x, err := binaryProtocol.Decode(reader, wire.TStruct) @@ -72,62 +72,62 @@ func _Second_Read(w wire.Value) (*non_hyphenated.Second, error) { // return nil, err // } // -// var v DocumentStruct +// var v DocumentStructure // if err := v.FromWire(x); err != nil { // return nil, err // } // return &v, nil -func (v *DocumentStruct) FromWire(w wire.Value) error { +func (v *DocumentStructure) FromWire(w wire.Value) error { var err error - secondIsSet := false + r2IsSet := false for _, field := range w.GetStruct().Fields { switch field.ID { case 1: if field.Value.Type() == wire.TStruct { - v.Second, err = _Second_Read(field.Value) + v.R2, err = _Second_Read(field.Value) if err != nil { return err } - secondIsSet = true + r2IsSet = true } } } - if !secondIsSet { - return errors.New("field Second of DocumentStruct is required") + if !r2IsSet { + return errors.New("field R2 of DocumentStructure is required") } return nil } -// String returns a readable string representation of a DocumentStruct +// String returns a readable string representation of a DocumentStructure // struct. -func (v *DocumentStruct) String() string { +func (v *DocumentStructure) String() string { if v == nil { return "" } var fields [1]string i := 0 - fields[i] = fmt.Sprintf("Second: %v", v.Second) + fields[i] = fmt.Sprintf("R2: %v", v.R2) i++ - return fmt.Sprintf("DocumentStruct{%v}", strings.Join(fields[:i], ", ")) + return fmt.Sprintf("DocumentStructure{%v}", strings.Join(fields[:i], ", ")) } -// Equals returns true if all the fields of this DocumentStruct match the -// provided DocumentStruct. +// Equals returns true if all the fields of this DocumentStructure match the +// provided DocumentStructure. // // This function performs a deep comparison. -func (v *DocumentStruct) Equals(rhs *DocumentStruct) bool { +func (v *DocumentStructure) Equals(rhs *DocumentStructure) bool { if v == nil { return rhs == nil } else if rhs == nil { return false } - if !v.Second.Equals(rhs.Second) { + if !v.R2.Equals(rhs.R2) { return false } @@ -135,39 +135,39 @@ func (v *DocumentStruct) Equals(rhs *DocumentStruct) bool { } // MarshalLogObject implements zapcore.ObjectMarshaler, enabling -// fast logging of DocumentStruct. -func (v *DocumentStruct) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { +// fast logging of DocumentStructure. +func (v *DocumentStructure) MarshalLogObject(enc zapcore.ObjectEncoder) (err error) { if v == nil { return nil } - err = multierr.Append(err, enc.AddObject("second", v.Second)) + err = multierr.Append(err, enc.AddObject("r2", v.R2)) return err } -// GetSecond returns the value of Second if it is set or its +// GetR2 returns the value of R2 if it is set or its // zero value if it is unset. -func (v *DocumentStruct) GetSecond() (o *non_hyphenated.Second) { +func (v *DocumentStructure) GetR2() (o *non_hyphenated.Second) { if v != nil { - o = v.Second + o = v.R2 } return } -// IsSetSecond returns true if Second is not nil. -func (v *DocumentStruct) IsSetSecond() bool { - return v != nil && v.Second != nil +// IsSetR2 returns true if R2 is not nil. +func (v *DocumentStructure) IsSetR2() bool { + return v != nil && v.R2 != nil } // ThriftModule represents the IDL file used to generate this package. var ThriftModule = &thriftreflect.ThriftModule{ - Name: "hyphenated-file", + Name: "hyphenated_file", Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", - FilePath: "hyphenated-file.thrift", - SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", + FilePath: "hyphenated_file.thrift", + SHA1: "aa5208b3d76766ff2c0900a849b4ee966f3057dd", Includes: []*thriftreflect.ThriftModule{ non_hyphenated.ThriftModule, }, Raw: rawIDL, } -const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStruct {\n 1: required non_hyphenated.Second second\n}" +const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStructure {\n 1: required non_hyphenated.Second r2\n}" diff --git a/gen/internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift b/gen/internal/tests/thrift/hyphenated_file.thrift similarity index 65% rename from gen/internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift rename to gen/internal/tests/thrift/hyphenated_file.thrift index fcb4a7a0..2ce53bcd 100644 --- a/gen/internal/tests/thrift/nestedfiles_error/hyphenated-file-nested.thrift +++ b/gen/internal/tests/thrift/hyphenated_file.thrift @@ -1,4 +1,4 @@ -include "../non_hyphenated.thrift" +include "./non_hyphenated.thrift" struct DocumentStructure { 1: required non_hyphenated.Second r2 diff --git a/gen/internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift b/gen/internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift deleted file mode 100644 index 4c511477..00000000 --- a/gen/internal/tests/thrift/nestedfiles_error/hyphenated_file_nested.thrift +++ /dev/null @@ -1,5 +0,0 @@ -include "../non_hyphenated.thrift" - -struct DocumentStructure { - 1: required non_hyphenated.Second r1 -} \ No newline at end of file From 7add654143d77b4ecf4dced6a5670b92b89e303d Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Mon, 14 Oct 2019 14:24:00 -0700 Subject: [PATCH 09/16] fix test --- gen/generate.go | 8 ++------ gen/golden_test.go | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/gen/generate.go b/gen/generate.go index 11cb8d73..6c0d4010 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -167,12 +167,8 @@ func Generate(m *compile.Module, o *Options) error { // normalizeFilePath replaces hyphens in the file name with underscores. func normalizeFilePath(p string) string { - return filepath.Join(filepath.Dir(p), normalizedName(p)) -} - -// normalizedName replaces hyphens in the file name with underscores. -func normalizedName(f string) string { - return strings.Replace(filepath.Base(f), "-", "_", -1) + normalizedName := strings.Replace(filepath.Base(p), "-", "_", -1) + return filepath.Join(filepath.Dir(p), normalizedName) } // ThriftPackageImporter determines import paths from a Thrift root. diff --git a/gen/golden_test.go b/gen/golden_test.go index 7bdb325c..d1a85c8d 100644 --- a/gen/golden_test.go +++ b/gen/golden_test.go @@ -58,7 +58,7 @@ func TestCodeIsUpToDate(t *testing.T) { defer os.RemoveAll(outputDir) for _, thriftFile := range thriftFiles { - pkgRelPath := strings.TrimSuffix(normalizeFilePath(filepath.Base(thriftFile)), ".thrift") + pkgRelPath := strings.TrimSuffix(filepath.Base(thriftFile), ".thrift") currentPackageDir := filepath.Join("internal/tests", pkgRelPath) newPackageDir := filepath.Join(outputDir, pkgRelPath) From ac2027ffc4649d47cbf7e33b0580c8f481f1f165 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Mon, 14 Oct 2019 14:28:52 -0700 Subject: [PATCH 10/16] undoing accidental comment --- gen/generate_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/gen/generate_test.go b/gen/generate_test.go index 9e457fce..ecfcc394 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -93,19 +93,19 @@ func TestGenerateWithHyphenPaths(t *testing.T) { generateError string compileError string }{ - // { - // name: "include with hyphens error", - // filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", - // compileError: "cannot include hyphenated Thrift files", - // }, + { + name: "include with hyphens error", + filepath: "internal/tests/thrift/nestedfiles_error/include_hyphen_files.thrift", + compileError: "cannot include hyphenated Thrift files", + }, { name: "normalization - hyphen files as top level file allowed", filepath: "internal/tests/thrift/hyphenated-file.thrift", }, - // { - // name: "normalization - non hyphen file with hyphen in the directory", - // filepath: "internal/tests/thrift/hyphenated_file.thrift", - // }, + { + name: "normalization - non hyphen file with hyphen in the directory", + filepath: "internal/tests/thrift/hyphenated_file.thrift", + }, } for _, tt := range tests { From 53bdd8175908b939272193990273b6e7b7b9dd4b Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Mon, 14 Oct 2019 14:57:27 -0700 Subject: [PATCH 11/16] quick test - loading the generated types --- gen/quick_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/gen/quick_test.go b/gen/quick_test.go index 4c0f108e..bc35481d 100644 --- a/gen/quick_test.go +++ b/gen/quick_test.go @@ -32,11 +32,15 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap/zapcore" + tl "go.uber.org/thriftrw/gen/internal/tests/collision" tc "go.uber.org/thriftrw/gen/internal/tests/containers" tle "go.uber.org/thriftrw/gen/internal/tests/enum_conflict" te "go.uber.org/thriftrw/gen/internal/tests/enums" tx "go.uber.org/thriftrw/gen/internal/tests/exceptions" + ahf "go.uber.org/thriftrw/gen/internal/tests/hyphenated-file" + hf "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file" tz "go.uber.org/thriftrw/gen/internal/tests/nozap" tf "go.uber.org/thriftrw/gen/internal/tests/services" tss "go.uber.org/thriftrw/gen/internal/tests/set_to_slice" @@ -46,7 +50,6 @@ import ( tul "go.uber.org/thriftrw/gen/internal/tests/uuid_conflict" envex "go.uber.org/thriftrw/internal/envelope/exception" "go.uber.org/thriftrw/wire" - "go.uber.org/zap/zapcore" ) func thriftTypeIsValid(v thriftType) bool { @@ -284,6 +287,9 @@ func TestQuickSuite(t *testing.T) { {Sample: tf.KeyValue_DeleteValue_Args{}, Kind: thriftStruct}, {Sample: tf.KeyValue_DeleteValue_Result{}, Kind: thriftStruct}, {Sample: tf.KeyValue_GetManyValues_Args{}, Kind: thriftStruct}, + {Sample: ahf.DocumentStruct{}, Kind: thriftStruct}, + {Sample: hf.DocumentStructure{}, Kind: thriftStruct}, + { Sample: tf.KeyValue_GetManyValues_Result{}, Kind: thriftStruct, From c8cbaa13d0d5eeaca07e363346974ff2850041ae Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Tue, 15 Oct 2019 19:10:18 -0700 Subject: [PATCH 12/16] review comments --- gen/generate.go | 24 +++++++------------ .../tests/hyphenated-file/hyphenated-file.go | 2 +- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/gen/generate.go b/gen/generate.go index 6c0d4010..bde92590 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -165,10 +165,9 @@ func Generate(m *compile.Module, o *Options) error { return nil } -// normalizeFilePath replaces hyphens in the file name with underscores. -func normalizeFilePath(p string) string { - normalizedName := strings.Replace(filepath.Base(p), "-", "_", -1) - return filepath.Join(filepath.Dir(p), normalizedName) +// normalizePackageName replaces hyphens in the file name with underscores. +func normalizePackageName(p string) string { + return strings.Replace(filepath.Base(p), "-", "_", -1) } // ThriftPackageImporter determines import paths from a Thrift root. @@ -192,7 +191,7 @@ type thriftPackageImporter struct { } func (i thriftPackageImporter) RelativePackage(file string) (string, error) { - return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(normalizeFilePath(file), ".thrift")) + return filepath.Rel(i.ThriftRoot, strings.TrimSuffix(file, ".thrift")) } func (i thriftPackageImporter) RelativeThriftFilePath(file string) (string, error) { @@ -232,25 +231,24 @@ func generateModule( builder *generateServiceBuilder, o *Options, ) (outputFilepath string, contents []byte, err error) { - // thriftRelPath is the path relative to outputDir into which we'll be + // packageRelPath is the path relative to outputDir into which we'll be // writing the package for this Thrift file. For $thriftRoot/foo/bar.thrift, // packageRelPath is foo/bar, and packageDir is $outputDir/foo/bar. All // files for bar.thrift will be written to the $outputDir/foo/bar/ tree. The // package will be importable via $importPrefix/foo/bar. - thriftRelPath, err := i.RelativeThriftFilePath(m.ThriftPath) + packageRelPath, err := i.RelativePackage(m.ThriftPath) if err != nil { return "", nil, err } - thriftRelPath = strings.TrimSuffix(thriftRelPath, ".thrift") // TODO(abg): Prefer top-level package name from `namespace go` directive. - outputFilename := filepath.Base(thriftRelPath) + outputFilename := filepath.Base(packageRelPath) // Output file name defaults to the package name. outputFilename = outputFilename + ".go" if len(o.OutputFile) > 0 { outputFilename = o.OutputFile } - outputFilepath = filepath.Join(thriftRelPath, outputFilename) + outputFilepath = filepath.Join(packageRelPath, outputFilename) // importPath is the full import path for the top-level package generated // for this Thrift file. @@ -260,11 +258,7 @@ func generateModule( } // converts package name from ab-def to ab_def for golang code generation - packageRelPath, err := i.RelativePackage(m.ThriftPath) - if err != nil { - return "", nil, err - } - normalizedPackageName := filepath.Base(packageRelPath) + normalizedPackageName := normalizePackageName(filepath.Base(packageRelPath)) g := NewGenerator(&GeneratorOptions{ Importer: i, ImportPath: importPath, diff --git a/gen/internal/tests/hyphenated-file/hyphenated-file.go b/gen/internal/tests/hyphenated-file/hyphenated-file.go index 35e62ba0..ae8cc3d2 100644 --- a/gen/internal/tests/hyphenated-file/hyphenated-file.go +++ b/gen/internal/tests/hyphenated-file/hyphenated-file.go @@ -161,7 +161,7 @@ func (v *DocumentStruct) IsSetSecond() bool { // ThriftModule represents the IDL file used to generate this package. var ThriftModule = &thriftreflect.ThriftModule{ Name: "hyphenated-file", - Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", + Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated-file", FilePath: "hyphenated-file.thrift", SHA1: "8513913ac76a3ba1c6b2b3b6fb241462e162c446", Includes: []*thriftreflect.ThriftModule{ From 9f30c306cf916a5bf3f2513a19bbe8a344461537 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Wed, 16 Oct 2019 13:34:55 -0700 Subject: [PATCH 13/16] Update gen/generate_test.go Co-Authored-By: Abhinav Gupta --- gen/generate_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen/generate_test.go b/gen/generate_test.go index ecfcc394..bdd9dd1e 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -103,7 +103,7 @@ func TestGenerateWithHyphenPaths(t *testing.T) { filepath: "internal/tests/thrift/hyphenated-file.thrift", }, { - name: "normalization - non hyphen file with hyphen in the directory", + name: "normalization/non hyphen file with hyphen in the directory", filepath: "internal/tests/thrift/hyphenated_file.thrift", }, } From a6e531deae0cbd2a617a15ae61db1fe1aba712bd Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Wed, 16 Oct 2019 13:35:17 -0700 Subject: [PATCH 14/16] Update gen/internal/tests/thrift/hyphenated_file.thrift Co-Authored-By: Abhinav Gupta --- gen/internal/tests/thrift/hyphenated_file.thrift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gen/internal/tests/thrift/hyphenated_file.thrift b/gen/internal/tests/thrift/hyphenated_file.thrift index 2ce53bcd..b0f03be1 100644 --- a/gen/internal/tests/thrift/hyphenated_file.thrift +++ b/gen/internal/tests/thrift/hyphenated_file.thrift @@ -1,5 +1,8 @@ +// This file is named hyphenated_file to possibly conflict with the code +// generated from hyphenated-file. + include "./non_hyphenated.thrift" struct DocumentStructure { 1: required non_hyphenated.Second r2 -} \ No newline at end of file +} From 2187e998098a31ae0da9fdfae04d6f77e4f2b570 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Wed, 16 Oct 2019 14:05:10 -0700 Subject: [PATCH 15/16] Update gen/generate_test.go Co-Authored-By: Abhinav Gupta --- gen/generate_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gen/generate_test.go b/gen/generate_test.go index bdd9dd1e..eabb3b12 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -99,7 +99,7 @@ func TestGenerateWithHyphenPaths(t *testing.T) { compileError: "cannot include hyphenated Thrift files", }, { - name: "normalization - hyphen files as top level file allowed", + name: "normalization/hyphen files as top level file allowed", filepath: "internal/tests/thrift/hyphenated-file.thrift", }, { From cf2b8e2d4764d0b87686466fd4daf7130817d0d2 Mon Sep 17 00:00:00 2001 From: Abhishek Parwal Date: Wed, 16 Oct 2019 14:12:22 -0700 Subject: [PATCH 16/16] generated file --- gen/internal/tests/hyphenated_file/hyphenated_file.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gen/internal/tests/hyphenated_file/hyphenated_file.go b/gen/internal/tests/hyphenated_file/hyphenated_file.go index 3c1b29e1..c4938801 100644 --- a/gen/internal/tests/hyphenated_file/hyphenated_file.go +++ b/gen/internal/tests/hyphenated_file/hyphenated_file.go @@ -163,11 +163,11 @@ var ThriftModule = &thriftreflect.ThriftModule{ Name: "hyphenated_file", Package: "go.uber.org/thriftrw/gen/internal/tests/hyphenated_file", FilePath: "hyphenated_file.thrift", - SHA1: "aa5208b3d76766ff2c0900a849b4ee966f3057dd", + SHA1: "efdcd233efa65e3d451cdf36c518da9e2d0c40b1", Includes: []*thriftreflect.ThriftModule{ non_hyphenated.ThriftModule, }, Raw: rawIDL, } -const rawIDL = "include \"./non_hyphenated.thrift\"\n\nstruct DocumentStructure {\n 1: required non_hyphenated.Second r2\n}" +const rawIDL = "// This file is named hyphenated_file to possibly conflict with the code\n// generated from hyphenated-file.\n\ninclude \"./non_hyphenated.thrift\"\n\nstruct DocumentStructure {\n 1: required non_hyphenated.Second r2\n}\n"