You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@thrift.apache.org by dc...@apache.org on 2019/06/29 06:07:19 UTC
[thrift] branch master updated: THRIFT-4797: Fix import collisions
in Go
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 <jo...@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