BUG#29928485 PYTHON PLUGINS: FAILS LOADING PLUGINS HAVING INNER PACKAGE WITH THE SAME NAME

This bug describes an unexpected behavior when two plugins contain an inner package with
the same name, it results in the second plugin not being loaded because the definitions
on the affected package are not available.

This is due to a name clashing on package names caused because the plugin logic adds both
the plugin path (and in the case of plugin groups, the parent plugin path) into sys path,
allowing the inner packages to be imported using import <package_name>.

This patch fixes this problem by replacing the paths to the plugin/plugin group with the
plugins folder from which the plugins are being loaded. As a consecuense the following
restrictions are added when defining plugins with inner packages or shared code:

- The main plugin folder must be a valid package name according to python standards (PEP-8)
- Each folder to be considered a package must contain a file named __init__.py

The imports will have to be done with fully specified package names, i.e.

from <plugin_name>[.<package_name>]* import <target_import>

This resolves the package name clashing and so the reported bug.

Change-Id: I9bcc29a237839556d2a03527fe23c657250c30c1

Author: rennox

mysqlshdk/scripting/python_context.cc

@@ -1145,13 +1145,19 @@ bool Python_context::load_plugin(const Plugin_definition &plugin) {
 
   PyObject *plugin_file = nullptr;
   PyObject *plugin_path = nullptr;
-  PyObject *parent_plugin_path = nullptr;
   PyObject *result = nullptr;
   PyObject *globals = nullptr;
   PyObject *sys_path_backup = nullptr;
   FILE *file = nullptr;
   bool executed = false;
   std::string file_name = plugin.file;
+
+  // The path to the plugins folder needs to be in sys.path while the plugin is
+  // being loaded
+  std::string plugin_dir = shcore::path::dirname(file_name);
+  if (!plugin.main) plugin_dir = shcore::path::dirname(plugin_dir);
+  plugin_dir = shcore::path::dirname(plugin_dir);
+
   try {
     file = fopen(file_name.c_str(), "r");
     if (nullptr == file) {
@@ -1164,21 +1170,11 @@ bool Python_context::load_plugin(const Plugin_definition &plugin) {
       throw std::runtime_error("Unable to get plugin file");
     }
 
-    std::string plugin_dir = shcore::path::dirname(file_name);
     plugin_path = PyString_FromString(plugin_dir.c_str());
     if (nullptr == plugin_path) {
       throw std::runtime_error("Unable to get plugin path");
     }
 
-    std::string parent_plugin_dir;
-    if (!plugin.main) {
-      parent_plugin_dir = shcore::path::dirname(plugin_dir);
-      parent_plugin_path = PyString_FromString(parent_plugin_dir.c_str());
-      if (nullptr == parent_plugin_path) {
-        throw std::runtime_error("Unable to get the parent plugin path");
-      }
-    }
-
     PyObject *sys_path = PySys_GetObject(const_cast<char *>("path"));
     if (nullptr == sys_path) {
       throw std::runtime_error("Failed getting sys.path");
@@ -1205,13 +1201,6 @@ bool Python_context::load_plugin(const Plugin_definition &plugin) {
       throw std::runtime_error("Failed adding plugin path to sys.path");
     }
 
-    if (!plugin.main) {
-      if (PyList_Insert(sys_path, 0, parent_plugin_path)) {
-        throw std::runtime_error(
-            "Failed adding parent plugin path to sys.path");
-      }
-    }
-
     // PyRun_FileEx() will close the file
     // plugins are loaded in sandboxed environment, we're not using
     // the compiler flags here
@@ -1220,8 +1209,8 @@ bool Python_context::load_plugin(const Plugin_definition &plugin) {
 
     executed = true;
 
-    // In case of an error, gets the description and clears the error condition
-    // in python to be able to restore the sys.path
+    // In case of an error, gets the description and clears the error
+    // condition in python to be able to restore the sys.path
     std::string error;
     if (nullptr == result) {
       error = fetch_and_clear_exception();

unittest/mysqlsh_plugins_t.cc

@@ -720,15 +720,15 @@ shell.register_report('first_py', 'print', report);
 // in subfolders, while only the function registration resides
 // in the init.py file
 TEST_F(Mysqlsh_plugin_test, py_plugin_with_imports) {
-  // Creates complex-py plugin initialization file
-  write_user_plugin("complex-py", R"(
+  // Creates complex_py plugin initialization file
+  write_user_plugin("complex_py", R"(
 print("Plugin File Path: {0}".format(__file__))
 
 # loads a function definition from src package
-from src import definition
+from complex_py.src import definition
 
 # loads a sibling script
-import sibling
+from complex_py import sibling
 
 
 # Executes the plugin registration
@@ -739,23 +739,28 @@ shell.register_global('pyObject', obj);
 )", ".py");
 
   auto user_plugins = get_user_plugin_folder();
-  auto src_package = join_path(user_plugins, "complex-py", "src");
+  auto main_package = join_path(user_plugins, "complex_py");
+  auto src_package = join_path(main_package, "src");
+
+  // Creates __init__.py to make the main plugin a package
+  shcore::create_file(join_path(main_package, "__init__.py"), "");
+
   // Creates a sibling script with a function definition
-  shcore::create_file(join_path(user_plugins, "complex-py", "sibling.py"),
+  shcore::create_file(join_path(main_package, "sibling.py"),
                       R"(def my_function():
-  print("Function at complex-py/sibling.py"))");
+  print("Function at complex_py/sibling.py"))");
 
   // Creates the src sub-package with a function definition
   shcore::create_directory(src_package);
   shcore::create_file(join_path(src_package, "__init__.py"), "");
   shcore::create_file(join_path(src_package, "definition.py"),
                       R"(def my_function():
-  print("Function at complex-py/src/definition.py"))");
+  print("Function at complex_py/src/definition.py"))");
 
   add_py_test("pyObject.test_package()",
-              "Function at complex-py/src/definition.py");
+              "Function at complex_py/src/definition.py");
 
-  add_py_test("pyObject.test_sibling()", "Function at complex-py/sibling.py");
+  add_py_test("pyObject.test_sibling()", "Function at complex_py/sibling.py");
 
   add_expected_py_log(
       "The 'test_package' function has been registered into an unregistered "
@@ -774,36 +779,40 @@ shell.register_global('pyObject', obj);
   // check the output
   MY_EXPECT_CMD_OUTPUT_NOT_CONTAINS("WARNING: Found errors loading plugins");
   std::string expected("Plugin File Path: ");
-  expected += join_path(get_user_plugin_folder(), "complex-py", "init.py");
+  expected += join_path(get_user_plugin_folder(), "complex_py", "init.py");
   MY_EXPECT_CMD_OUTPUT_CONTAINS(expected.c_str());
   MY_EXPECT_CMD_OUTPUT_CONTAINS(expected_output().c_str());
 
   validate_log();
 
   wipe_out();
 
-  delete_user_plugin("complex-py");
+  delete_user_plugin("complex_py");
 }
 
 // This test emulates a folder in plugins that contains subfolders
 // with plugins
 TEST_F(Mysqlsh_plugin_test, py_multi_plugins) {
-  // Creates complex-py plugin initialization file
+  // Creates multi_plugins plugin initialization file
   auto user_plugins = get_user_plugin_folder();
-  auto multi_plugins = join_path(user_plugins, "multi-plugins");
-  shcore::create_directory(multi_plugins);
-  shcore::create_file(join_path(multi_plugins, "plugin_common.py"),
+  auto main_package = join_path(user_plugins, "multi_plugins");
+
+  // Creates __init__.py to make the main plugin a package
+  shcore::create_directory(main_package);
+  shcore::create_file(join_path(main_package, "__init__.py"), "");
+
+  shcore::create_file(join_path(main_package, "plugin_common.py"),
                       R"(import mysqlsh
 
 def global_function(caller):
   print("Global function called from {0}".format(caller)))");
 
-  auto create_plugin = [multi_plugins](const std::string &name,
-                                       const std::string &greeting) {
-    auto plugin = join_path(multi_plugins, name);
+  auto create_plugin = [main_package](const std::string &name,
+                                      const std::string &greeting) {
+    auto plugin = join_path(main_package, name);
     shcore::create_directory(plugin);
     std::string code =
-        R"(import plugin_common
+        R"(from multi_plugins import plugin_common
 
 # The implementation of the function to be added to demo
 def hello_function():
@@ -893,13 +902,13 @@ shell.add_extension_object_member(global_obj, "common_%s", global_function)
 
   wipe_out();
 
-  delete_user_plugin("multi-plugins");
+  delete_user_plugin("multi_plugins");
 }
 
 #ifdef WITH_OCI
 // This test emulates a plugins that uses the oci
 TEST_F(Mysqlsh_plugin_test, oci_and_paramiko_plugin) {
-  // Creates complex-py plugin initialization file
+  // Creates oci-paramiko plugin initialization file
   write_user_plugin("oci-paramiko", R"(import oci
 import paramiko