This is an automated email from the ASF dual-hosted git repository. dcelasun pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/thrift.git
The following commit(s) were added to refs/heads/master by this push: new d9019fc THRIFT-4797: Fix import collisions in Go d9019fc is described below commit d9019fc5a4a2cec110a9acd9f36a45ee34e3b7f2 Author: John Boiles <johnaboi...@gmail.com> AuthorDate: Fri Jun 28 23:07:10 2019 -0700 THRIFT-4797: Fix import collisions in Go Client: go This closes #1811. --- compiler/cpp/src/thrift/generate/t_go_generator.cc | 184 ++++++++++++--------- lib/go/test/ConflictNamespaceServiceTest.thrift | 25 +++ lib/go/test/ConflictNamespaceTestA.thrift | 23 +++ lib/go/test/ConflictNamespaceTestB.thrift | 23 +++ lib/go/test/ConflictNamespaceTestC.thrift | 23 +++ lib/go/test/ConflictNamespaceTestD.thrift | 23 +++ lib/go/test/ConflictNamespaceTestSuperThing.thrift | 29 ++++ lib/go/test/Makefile.am | 26 ++- 8 files changed, 271 insertions(+), 85 deletions(-) diff --git a/compiler/cpp/src/thrift/generate/t_go_generator.cc b/compiler/cpp/src/thrift/generate/t_go_generator.cc index c8187d8..33b7547 100644 --- a/compiler/cpp/src/thrift/generate/t_go_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_go_generator.cc @@ -29,6 +29,7 @@ #include <iostream> #include <limits> #include <string> +#include <unordered_map> #include <vector> #include <stdlib.h> @@ -249,6 +250,8 @@ public: std::string go_imports_end(); std::string render_includes(bool consts); std::string render_included_programs(string& unused_protection); + std::string render_program_import(const t_program* program, string& unused_protection); + std::string render_system_packages(std::vector<string> &system_packages); std::string render_import_protection(); std::string render_fastbinary_includes(); std::string declare_argument(t_field* tfield); @@ -298,6 +301,8 @@ private: std::string package_name_; std::string package_dir_; + std::unordered_map<std::string, std::string> package_identifiers_; + std::set<std::string> package_identifiers_set_; std::string read_method_name_; std::string write_method_name_; @@ -763,33 +768,81 @@ void t_go_generator::init_generator() { f_unused_prot_.close(); } - -string t_go_generator::render_included_programs(string& unused_protection) { +string t_go_generator::render_included_programs(string& unused_prot) { const vector<t_program*>& includes = program_->get_includes(); string result = ""; - - unused_protection = ""; - string local_namespace = program_->get_namespace("go"); for (auto include : includes) { if (!local_namespace.empty() && local_namespace == include->get_namespace("go")) { continue; } - string go_module = get_real_go_module(include); - size_t found = 0; - for (size_t j = 0; j < go_module.size(); j++) { - // Import statement uses slashes ('/') in namespace - if (go_module[j] == '.') { - go_module[j] = '/'; - found = j + 1; - } + result += render_program_import(include, unused_prot); + } + return result; +} + +string t_go_generator::render_program_import(const t_program* program, string& unused_protection) { + string result = ""; + + string go_module = get_real_go_module(program); + string go_path = go_module; + size_t found = 0; + for (size_t j = 0; j < go_module.size(); j++) { + // Import statement uses slashes ('/') in namespace + if (go_module[j] == '.') { + go_path[j] = '/'; + found = j + 1; } + } - result += "\t\"" + gen_package_prefix_ + go_module + "\"\n"; - unused_protection += "var _ = " + go_module.substr(found) + ".GoUnusedProtection__\n"; + auto it = package_identifiers_.find(go_module); + auto last_component = go_module.substr(found); + if (it == package_identifiers_.end()) { + auto value = last_component; + // This final path component has already been used, let's construct a more unique alias + if (package_identifiers_set_.find(value) != package_identifiers_set_.end()) { + // TODO: This would produce more readable code if it appended trailing go_module + // path components to generate a more readable name unique identifier (e.g. use + // packageacommon as the alias for packagea/common instead of common=). But just + // appending an integer is much simpler code + value = tmp(value); + } + package_identifiers_set_.insert(value); + it = package_identifiers_.emplace(go_module, std::move(value)).first; + } + auto const& package_identifier = it->second; + result += "\t"; + // if the package_identifier is different than final path component we need an alias + if (last_component.compare(package_identifier) != 0) { + result += package_identifier + " "; } + string s; + + for (auto const& e : package_identifiers_set_) + { + s += e; + s += ','; + } + + s.pop_back(); + + result += "\"" + gen_package_prefix_ + go_path + "\"\n"; + unused_protection += "var _ = " + package_identifier + ".GoUnusedProtection__\n"; + return result; +} + +string t_go_generator::render_system_packages(std::vector<string>& system_packages) { + string result = ""; + for (vector<string>::iterator iter = system_packages.begin(); iter != system_packages.end(); ++iter) { + string package = *iter; + result += "\t\""+ package +"\"\n"; + + // Reserve these package names in case the collide with imported Thrift packages + package_identifiers_set_.insert(package); + package_identifiers_.emplace(package, package); + } return result; } @@ -801,32 +854,14 @@ string t_go_generator::render_includes(bool consts) { const vector<t_program*>& includes = program_->get_includes(); string result = ""; string unused_prot = ""; - - string local_namespace = program_->get_namespace("go"); - for (auto include : includes) { - if (!local_namespace.empty() && local_namespace == include->get_namespace("go")) { - continue; - } - - string go_module = get_real_go_module(include); - size_t found = 0; - for (size_t j = 0; j < go_module.size(); j++) { - // Import statement uses slashes ('/') in namespace - if (go_module[j] == '.') { - go_module[j] = '/'; - found = j + 1; - } - } - - result += "\t\"" + gen_package_prefix_ + go_module + "\"\n"; - unused_prot += "var _ = " + go_module.substr(found) + ".GoUnusedProtection__\n"; - } + result += go_imports_begin(consts); + result += render_included_programs(unused_prot); if (includes.size() > 0) { result += "\n"; } - return go_imports_begin(consts) + result + go_imports_end() + unused_prot; + return result + go_imports_end() + unused_prot; } string t_go_generator::render_import_protection() { @@ -862,22 +897,18 @@ string t_go_generator::go_package() { * If consts include the additional imports. */ string t_go_generator::go_imports_begin(bool consts) { - string extra; - + std::vector<string> system_packages; + system_packages.push_back("bytes"); + system_packages.push_back("context"); + system_packages.push_back("reflect"); // If not writing constants, and there are enums, need extra imports. if (!consts && get_program()->get_enums().size() > 0) { - extra += - "\t\"database/sql/driver\"\n" - "\t\"errors\"\n"; + system_packages.push_back("database/sql/driver"); + system_packages.push_back("errors"); } - return string( - "import (\n" - "\t\"bytes\"\n" - "\t\"context\"\n" - "\t\"reflect\"\n" - + extra + - "\t\"fmt\"\n" - "\t\"" + gen_thrift_import_ + "\"\n"); + system_packages.push_back("fmt"); + system_packages.push_back(gen_thrift_import_); + return "import(\n" + render_system_packages(system_packages); } /** @@ -2113,35 +2144,27 @@ void t_go_generator::generate_service_remote(t_service* tservice) { + underscore(service_name_) + "-remote.go"; ofstream_with_content_based_conditional_update f_remote; f_remote.open(f_remote_name.c_str()); - string service_module = get_real_go_module(program_); - string::size_type loc; - - while ((loc = service_module.find(".")) != string::npos) { - service_module.replace(loc, 1, 1, '/'); - } - if (!gen_package_prefix_.empty()) { - service_module = gen_package_prefix_ + service_module; - } string unused_protection; - string ctxPackage = "context"; + std::vector<string> system_packages; + system_packages.push_back("context"); + system_packages.push_back("flag"); + system_packages.push_back("fmt"); + system_packages.push_back("math"); + system_packages.push_back("net"); + system_packages.push_back("net/url"); + system_packages.push_back("os"); + system_packages.push_back("strconv"); + system_packages.push_back("strings"); f_remote << go_autogen_comment(); f_remote << indent() << "package main" << endl << endl; f_remote << indent() << "import (" << endl; - f_remote << indent() << " \"" << ctxPackage << "\"" << endl; - f_remote << indent() << " \"flag\"" << endl; - f_remote << indent() << " \"fmt\"" << endl; - f_remote << indent() << " \"math\"" << endl; - f_remote << indent() << " \"net\"" << endl; - f_remote << indent() << " \"net/url\"" << endl; - f_remote << indent() << " \"os\"" << endl; - f_remote << indent() << " \"strconv\"" << endl; - f_remote << indent() << " \"strings\"" << endl; - f_remote << indent() << " \"" + gen_thrift_import_ + "\"" << endl; + f_remote << render_system_packages(system_packages); + f_remote << indent() << "\t\"" + gen_thrift_import_ + "\"" << endl; f_remote << indent() << render_included_programs(unused_protection); - f_remote << indent() << " \"" << service_module << "\"" << endl; + f_remote << render_program_import(program_, unused_protection); f_remote << indent() << ")" << endl; f_remote << indent() << endl; f_remote << indent() << unused_protection; // filled in render_included_programs() @@ -2153,6 +2176,8 @@ void t_go_generator::generate_service_remote(t_service* tservice) { f_remote << indent() << " flag.PrintDefaults()" << endl; f_remote << indent() << " fmt.Fprintln(os.Stderr, \"\\nFunctions:\")" << endl; + string package_name_aliased = package_identifiers_[get_real_go_module(program_)]; + for (f_iter = functions.begin(); f_iter != functions.end(); ++f_iter) { f_remote << " fmt.Fprintln(os.Stderr, \" " << (*f_iter)->get_returntype()->get_name() << " " << (*f_iter)->get_name() << "("; @@ -2299,7 +2324,7 @@ void t_go_generator::generate_service_remote(t_service* tservice) { f_remote << indent() << "}" << endl; f_remote << indent() << "iprot := protocolFactory.GetProtocol(trans)" << endl; f_remote << indent() << "oprot := protocolFactory.GetProtocol(trans)" << endl; - f_remote << indent() << "client := " << package_name_ << ".New" << publicize(service_name_) + f_remote << indent() << "client := " << package_name_aliased << ".New" << publicize(service_name_) << "Client(thrift.NewTStandardClient(iprot, oprot))" << endl; f_remote << indent() << "if err := trans.Open(); err != nil {" << endl; f_remote << indent() << " fmt.Fprintln(os.Stderr, \"Error opening socket to \", " @@ -2336,7 +2361,7 @@ void t_go_generator::generate_service_remote(t_service* tservice) { f_remote << indent() << " Usage()" << endl; f_remote << indent() << " return" << endl; f_remote << indent() << "}" << endl; - f_remote << indent() << "argvalue" << i << " := " << package_name_ << "." + f_remote << indent() << "argvalue" << i << " := " << package_name_aliased << "." << publicize(the_type->get_name()) << "(tmp" << i << ")" << endl; } else if (the_type2->is_base_type()) { t_base_type::t_base e = ((t_base_type*)the_type2)->get_base(); @@ -2424,7 +2449,7 @@ void t_go_generator::generate_service_remote(t_service* tservice) { std::string tstruct_name(publicize(the_type->get_name())); std::string tstruct_module( module_name(the_type)); if(tstruct_module.empty()) { - tstruct_module = package_name_; + tstruct_module = package_name_aliased; } f_remote << indent() << arg << " := flag.Arg(" << flagArg << ")" << endl; @@ -2468,7 +2493,7 @@ void t_go_generator::generate_service_remote(t_service* tservice) { f_remote << indent() << factory << " := thrift.NewTJSONProtocolFactory()" << endl; f_remote << indent() << jsProt << " := " << factory << ".GetProtocol(" << mbTrans << ")" << endl; - f_remote << indent() << "containerStruct" << i << " := " << package_name_ << ".New" + f_remote << indent() << "containerStruct" << i << " := " << package_name_aliased << ".New" << argumentsName << "()" << endl; f_remote << indent() << err2 << " := containerStruct" << i << ".ReadField" << (i + 1) << "(" << jsProt << ")" << endl; @@ -2483,9 +2508,9 @@ void t_go_generator::generate_service_remote(t_service* tservice) { } if (the_type->is_typedef()) { - std::string typedef_module( module_name(the_type)); + std::string typedef_module(module_name(the_type)); if(typedef_module.empty()) { - typedef_module = package_name_; + typedef_module = package_name_aliased; } f_remote << indent() << "value" << i << " := " << typedef_module << "." << publicize(the_type->get_name()) << "(argvalue" << i << ")" << endl; @@ -3504,12 +3529,7 @@ string t_go_generator::module_name(t_type* ttype) { program_->get_namespace("go").empty() || program->get_namespace("go") != program_->get_namespace("go")) { string module(get_real_go_module(program)); - // for namespaced includes, only keep part after dot. - size_t dot = module.rfind('.'); - if (dot != string::npos) { - module = module.substr(dot + 1); - } - return module; + return package_identifiers_[module]; } } diff --git a/lib/go/test/ConflictNamespaceServiceTest.thrift b/lib/go/test/ConflictNamespaceServiceTest.thrift new file mode 100644 index 0000000..aade3d7 --- /dev/null +++ b/lib/go/test/ConflictNamespaceServiceTest.thrift @@ -0,0 +1,25 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +namespace go conflict.context + +include "ConflictNamespaceTestD.thrift" + +service ConflictService { + ConflictNamespaceTestD.ThingD thingFunc() +} diff --git a/lib/go/test/ConflictNamespaceTestA.thrift b/lib/go/test/ConflictNamespaceTestA.thrift new file mode 100644 index 0000000..2749da9 --- /dev/null +++ b/lib/go/test/ConflictNamespaceTestA.thrift @@ -0,0 +1,23 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +namespace go conflicta.common + +struct ThingA { + 1: bool value +} diff --git a/lib/go/test/ConflictNamespaceTestB.thrift b/lib/go/test/ConflictNamespaceTestB.thrift new file mode 100644 index 0000000..b1940ff --- /dev/null +++ b/lib/go/test/ConflictNamespaceTestB.thrift @@ -0,0 +1,23 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +namespace go conflictb.common + +struct ThingB { + 1: bool value +} diff --git a/lib/go/test/ConflictNamespaceTestC.thrift b/lib/go/test/ConflictNamespaceTestC.thrift new file mode 100644 index 0000000..7d5ee25 --- /dev/null +++ b/lib/go/test/ConflictNamespaceTestC.thrift @@ -0,0 +1,23 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +namespace go common + +struct ThingC { + 1: bool value +} diff --git a/lib/go/test/ConflictNamespaceTestD.thrift b/lib/go/test/ConflictNamespaceTestD.thrift new file mode 100644 index 0000000..8fe7f5e --- /dev/null +++ b/lib/go/test/ConflictNamespaceTestD.thrift @@ -0,0 +1,23 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +namespace go conflictd.context + +struct ThingD { + 1: bool value +} diff --git a/lib/go/test/ConflictNamespaceTestSuperThing.thrift b/lib/go/test/ConflictNamespaceTestSuperThing.thrift new file mode 100644 index 0000000..2348a62 --- /dev/null +++ b/lib/go/test/ConflictNamespaceTestSuperThing.thrift @@ -0,0 +1,29 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +include "ConflictNamespaceTestA.thrift" +include "ConflictNamespaceTestB.thrift" +include "ConflictNamespaceTestC.thrift" +include "ConflictNamespaceTestD.thrift" + +struct SuperThing { + 1: ConflictNamespaceTestA.ThingA thing_a + 2: ConflictNamespaceTestB.ThingB thing_b + 3: ConflictNamespaceTestC.ThingC thing_c + 4: ConflictNamespaceTestD.ThingD thing_d +} diff --git a/lib/go/test/Makefile.am b/lib/go/test/Makefile.am index 78d4681..244ddff 100644 --- a/lib/go/test/Makefile.am +++ b/lib/go/test/Makefile.am @@ -39,7 +39,13 @@ gopath: $(THRIFT) $(THRIFTTEST) \ InitialismsTest.thrift \ DontExportRWTest.thrift \ dontexportrwtest/compile_test.go \ - IgnoreInitialismsTest.thrift + IgnoreInitialismsTest.thrift \ + ConflictNamespaceTestA.thrift \ + ConflictNamespaceTestB.thrift \ + ConflictNamespaceTestC.thrift \ + ConflictNamespaceTestD.thrift \ + ConflictNamespaceTestSuperThing.thrift \ + ConflictNamespaceServiceTest.thrift mkdir -p gopath/src grep -v list.*map.*list.*map $(THRIFTTEST) | grep -v 'set<Insanity>' > ThriftTest.thrift $(THRIFT) $(THRIFTARGS) -r IncludesTest.thrift @@ -59,6 +65,12 @@ gopath: $(THRIFT) $(THRIFTTEST) \ $(THRIFT) $(THRIFTARGS) InitialismsTest.thrift $(THRIFT) $(THRIFTARGS),read_write_private DontExportRWTest.thrift $(THRIFT) $(THRIFTARGS),ignore_initialisms IgnoreInitialismsTest.thrift + $(THRIFT) $(THRIFTARGS) ConflictNamespaceTestA.thrift + $(THRIFT) $(THRIFTARGS) ConflictNamespaceTestB.thrift + $(THRIFT) $(THRIFTARGS) ConflictNamespaceTestC.thrift + $(THRIFT) $(THRIFTARGS) ConflictNamespaceTestD.thrift + $(THRIFT) $(THRIFTARGS) ConflictNamespaceTestSuperThing.thrift + $(THRIFT) $(THRIFTARGS) ConflictNamespaceServiceTest.thrift GOPATH=`pwd`/gopath $(GO) get github.com/golang/mock/gomock || true sed -i 's/\"context\"/\"golang.org\/x\/net\/context\"/g' gopath/src/github.com/golang/mock/gomock/controller.go || true GOPATH=`pwd`/gopath $(GO) get github.com/golang/mock/gomock @@ -79,7 +91,9 @@ check: gopath initialismstest \ dontexportrwtest \ ignoreinitialismstest \ - unionbinarytest + unionbinarytest \ + conflictnamespacetestsuperthing \ + conflict/context/conflict_service-remote GOPATH=`pwd`/gopath $(GO) test thrift tests dontexportrwtest clean-local: @@ -108,4 +122,10 @@ EXTRA_DIST = \ NamesTest.thrift \ InitialismsTest.thrift \ DontExportRWTest.thrift \ - IgnoreInitialismsTest.thrift + IgnoreInitialismsTest.thrift \ + ConflictNamespaceTestA.thrift \ + ConflictNamespaceTestB.thrift \ + ConflictNamespaceTestC.thrift \ + ConflictNamespaceTestD.thrift \ + ConflictNamespaceTestSuperThing.thrift + ConflictNamespaceServiceTest.thrift