Fix \export_dashboard in omnisql

* Fix export_dashboard call in omnisql Add check to identify multiple dashboards with same name Accept owner_name as additional parameter Use get_dashboard call to fetch confirmed dashboard Comment on `get_dashboards` call to not update viewState there for payload reasons
yma11 · Apr 1, 2019 · 8f9d23b · 8f9d23b
1 parent 5d62658
commit 8f9d23b
Show file tree

Hide file tree

Showing 7 changed files with 116 additions and 31 deletions.
diff --git a/SQLFrontend/CommandFunctors.h b/SQLFrontend/CommandFunctors.h
@@ -247,7 +247,8 @@ StandardCommand(Help, {
   std::cout << "\\version Print OmniSci Server version.\n";
   std::cout << "\\copy <file path> <table> Copy data from file to table.\n";
   std::cout << "\\status Get status of the server and the leaf nodes.\n";
-  std::cout << "\\export_dashboard <dashboard name> <filename> Exports a dashboard to a "
+  std::cout << "\\export_dashboard <dashboard name> <filename> <optional: dashboard "
+               "owner name> Exports a dashboard to a "
                "file.\n";
   std::cout << "\\import_dashboard <dashboard name> <filename> Imports a dashboard from "
                "a file\n";
@@ -447,22 +448,67 @@ StandardCommand(ExportDashboard, {
   decltype(p[2])& filename(p[2]);
 
   cmdContext().view_name = dashboard_name;
+  cmdContext().dashboard_owner = p.size() == 4 ? p[3] : "";
 
-  thrift_op<kEXPORT_DASHBOARD>(
+  thrift_op<kGET_DASHBOARDS>(
       cmdContext(),
       [&](ContextType& lambda_context) {
-        output_stream << "Exporting dashboard " << lambda_context.view_name << " to file "
-                      << filename << std::endl;  // create file and dump string to it
-        std::ofstream dashfile;
-        dashfile.open(filename);
-        if (dashfile.is_open()) {
-          dashfile << lambda_context.view_name << std::endl;
-          dashfile << lambda_context.view_return.view_metadata << std::endl;
-          dashfile << decode64(lambda_context.view_return.view_state);
-          dashfile.close();
+        if (lambda_context.dash_names.size() == 0) {
+          output_stream << "No dashboards exist." << std::endl;
+          return;
+        }
+        std::vector<TDashboard> dashboards;
+        for (auto& dash : lambda_context.dash_names) {
+          if (dash.dashboard_name == lambda_context.view_name) {
+            dashboards.push_back(dash);
+          }
+        }
+        if (dashboards.size() == 0) {
+          output_stream << "Dashboard " + lambda_context.view_name + " does not exist"
+                        << std::endl;
+          return;
+        }
+        int view_id;
+        if (dashboards.size() > 1 && p.size() == 3) {
+          output_stream
+              << "Multiple dashboards with name '" + lambda_context.view_name +
+                     "' exist. To help disambiguate identical dashboards please try  "
+                     "\\export_dashboard <dash_name> <exportfile> <dash_owner>."
+              << std::endl;
+          return;
+        } else if (dashboards.size() > 1 && p.size() == 4) {
+          bool found = false;
+          for (auto& dash : dashboards) {
+            if (dash.dashboard_owner == lambda_context.dashboard_owner) {
+              view_id = dash.dashboard_id;
+              found = true;
+            }
+          }
+          if (!found) {
+            output_stream << "No dashboard with owner name '" +
+                                 lambda_context.dashboard_owner + "' found."
+                          << std::endl;
+            return;
+          }
         } else {
-          output_stream << "Could not open file `" << filename << "`" << std::endl;
+          view_id = dashboards[0].dashboard_id;
         }
+        cmdContext().dash_id = view_id;
+        thrift_op<kGET_DASHBOARD>(cmdContext(), [&](ContextType& lambda_context) {
+          output_stream << "Exporting dashboard "
+                        << lambda_context.dash_return.dashboard_name << " to file "
+                        << filename << std::endl;  // create file and dump string to it
+          std::ofstream dashfile;
+          dashfile.open(filename);
+          if (dashfile.is_open()) {
+            dashfile << lambda_context.dash_return.dashboard_name << std::endl;
+            dashfile << lambda_context.dash_return.dashboard_metadata << std::endl;
+            dashfile << decode64(lambda_context.dash_return.dashboard_state);
+            dashfile.close();
+          } else {
+            output_stream << "Could not open file `" << filename << "`" << std::endl;
+          }
+        });
       },
       [&](ContextType&) { output_stream << "Failed to export dashboard." << std::endl; });
 });

diff --git a/SQLFrontend/MetaClientContext.h b/SQLFrontend/MetaClientContext.h
@@ -42,9 +42,11 @@ struct MetaClientContext {
   TClusterHardwareInfo cluster_hardware_info;
   std::vector<TServerStatus> cluster_status;
   std::string view_name;
+  std::string dashboard_owner;
+  int dash_id;
   std::string view_state;
   std::string view_metadata;
-  TFrontendView view_return;
+  TDashboard dash_return;
   std::string privs_role_name;
   std::string privs_user_name;
   std::string privs_object_name;

diff --git a/SQLFrontend/ThriftService.h b/SQLFrontend/ThriftService.h
@@ -27,7 +27,6 @@ enum ThriftService {
   kGET_TABLE_EPOCH_BY_NAME,
   kGET_SERVER_STATUS,
   kIMPORT_DASHBOARD,
-  kEXPORT_DASHBOARD,
   kGET_ROLES,
   kGET_OBJECTS_FOR_GRANTEE,
   kGET_OBJECT_PRIVS,
@@ -36,7 +35,8 @@ enum ThriftService {
   kSET_LICENSE_KEY,
   kGET_LICENSE_CLAIMS,
   kGET_COMPLETION_HINTS,
-  kGET_DASHBOARDS
+  kGET_DASHBOARDS,
+  kGET_DASHBOARD
 };
 
 #endif
diff --git a/SQLFrontend/ThriftWithRetry.h b/SQLFrontend/ThriftWithRetry.h
@@ -115,10 +115,6 @@ bool thrift_with_retry(SERVICE_ENUM which_service,
                                             "",
                                             context.view_metadata);
         break;
-      case kEXPORT_DASHBOARD:
-        context.client.get_frontend_view(
-            context.view_return, context.session, context.view_name);
-        break;
       case kGET_ROLES:
         context.client.get_roles(context.role_names, context.session);
         break;
@@ -150,6 +146,10 @@ bool thrift_with_retry(SERVICE_ENUM which_service,
       case kGET_DASHBOARDS:
         context.client.get_dashboards(context.dash_names, context.session);
         break;
+      case kGET_DASHBOARD:
+        context.client.get_dashboard(
+            context.dash_return, context.session, context.dash_id);
+        break;
     }
   } catch (TMapDException& e) {
     std::cerr << e.error_msg << std::endl;

diff --git a/SQLFrontend/omnisql.cpp b/SQLFrontend/omnisql.cpp
@@ -1416,7 +1416,7 @@ int main(int argc, char** argv) {
         ( "\\copy", 3, 3, [&](Params const& p) { copy_table(p[1].c_str() /* filepath */, p[2].c_str() /* table */, context); } )
         ( "\\ste", 2, 2, [&](Params const& p) { set_table_epoch(context, p[1] /* table_details */); } )
         ( "\\gte", 2, 2, [&](Params const& p) { get_table_epoch(context, p[1] /* table_details */); } )
-        ( "\\export_dashboard", 3, 3, ExportDashboardCmd<>( context ), "Usage \\export_dashboard <dash name> <file name>" )
+        ( "\\export_dashboard", 3, 4, ExportDashboardCmd<>( context ), "Usage \\export_dashboard <dash name> <file name> <optional:dash_owner>" )
         ( "\\import_dashboard", 3, 3, ImportDashboardCmd<>( context ), "Usage \\import_dashboard <dash name> <file name>"  )
         ( "\\role_list", 2, 2, RoleListCmd<>(context), "Usage: \\role_list <userName>")
         ( "\\roles", 1, 1, RolesCmd<>(context))("\\set_license", 2, 2, [&](Params const& p ) { set_license_key(context, p[1]); })

diff --git a/Tests/OmniSQLCommandTest.cpp b/Tests/OmniSQLCommandTest.cpp
@@ -49,7 +49,6 @@ class CoreMockClient {
   RetValMockMethod(get_table_epoch_by_name, int32_t)
   MockMethod(get_status)
   MockMethod(create_frontend_view)
-  MockMethod(get_frontend_view)
   MockMethod(get_roles)
   MockMethod(get_db_objects_for_grantee)
   MockMethod(get_db_object_privs)
@@ -59,6 +58,7 @@ class CoreMockClient {
   MockMethod(get_completion_hints)
   MockMethod(get_dashboards)
   MockMethod(get_session_info)
+  MockMethod(get_dashboard)
 };
 // clang-format on
 
@@ -871,7 +871,7 @@ TEST(OmniSQLTest, ExportDashboardCommandTest_SimpleDashSimpleFilename) {
       CommandResolutionChain<>(fake_input.c_str(),
                                "\\export_dashboard",
                                3,
-                               3,
+                               4,
                                UnitTestExportDashboardCmd(unit_test_context),
                                test_capture_stream)
           .is_resolved();
@@ -883,9 +883,10 @@ TEST(OmniSQLTest, ExportDashboardCommandTest_SimpleDashSimpleFilename) {
   EXPECT_EQ(unit_test_context.view_name, "simpledash");
   EXPECT_EQ(unit_test_context.view_state, "");
   EXPECT_EQ(unit_test_context.view_metadata, "");
+  EXPECT_EQ(unit_test_context.dashboard_owner, "");
 
-  // Whether call succeeds or fails, the file name is in the output
-  EXPECT_NE(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
+  // The file name is not in the output
+  EXPECT_EQ(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
 
   unlink(test_filename);
 }
@@ -907,7 +908,7 @@ TEST(OmniSQLTest, ExportDashboardCommandTest_SimpleDashComplexFilename) {
       CommandResolutionChain<>(fake_input.c_str(),
                                "\\export_dashboard",
                                3,
-                               3,
+                               4,
                                UnitTestExportDashboardCmd(unit_test_context),
                                test_capture_stream)
           .is_resolved();
@@ -916,9 +917,9 @@ TEST(OmniSQLTest, ExportDashboardCommandTest_SimpleDashComplexFilename) {
   EXPECT_EQ(unit_test_context.view_name, "simpledash");
   EXPECT_EQ(unit_test_context.view_state, "");
   EXPECT_EQ(unit_test_context.view_metadata, "");
+  EXPECT_EQ(unit_test_context.dashboard_owner, "");
 
-  // Whether call succeeds or fails, the file name is in the output
-  EXPECT_NE(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
+  EXPECT_EQ(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
   unlink(test_filename);
 }
 
@@ -940,16 +941,17 @@ TEST(OmniSQLTest, ExportDashboardCommandTest_ComplexDashSimpleFilename) {
       CommandResolutionChain<>(fake_input.c_str(),
                                "\\export_dashboard",
                                3,
-                               3,
+                               4,
                                UnitTestExportDashboardCmd(unit_test_context),
                                test_capture_stream)
           .is_resolved();
   EXPECT_TRUE(resolution);
   EXPECT_EQ(unit_test_context.view_name, "\"Who uses spaces anyway?\"");
   EXPECT_EQ(unit_test_context.view_state, "");
   EXPECT_EQ(unit_test_context.view_metadata, "");
+  EXPECT_EQ(unit_test_context.dashboard_owner, "");
 
-  EXPECT_NE(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
+  EXPECT_EQ(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
   unlink(test_filename);
 }
 
@@ -971,16 +973,49 @@ TEST(OmniSQLTest, ExportDashboardCommandTest_ComplexDashComplexFilename) {
       CommandResolutionChain<>(fake_input.c_str(),
                                "\\export_dashboard",
                                3,
+                               4,
+                               UnitTestExportDashboardCmd(unit_test_context),
+                               test_capture_stream)
+          .is_resolved();
+  EXPECT_TRUE(resolution);
+  EXPECT_EQ(unit_test_context.view_name, "\"Who uses spaces anyway?\"");
+  EXPECT_EQ(unit_test_context.view_state, "");
+  EXPECT_EQ(unit_test_context.view_metadata, "");
+  EXPECT_EQ(unit_test_context.dashboard_owner, "");
+
+  EXPECT_EQ(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
+  unlink(test_filename);
+}
+
+TEST(OmniSQLTest, ExportDashboardCommandTest_ComplexDashComplexFilenameWithOwnerName) {
+  using Params = CommandResolutionChain<>::CommandTokenList;
+  using UnitTestExportDashboardCmd =
+      ExportDashboardCmd<ExportDashboardCommandContextOpsPolicy,
+                         ExportDashboardCommandMockupContextOperations>;
+  using TokenExtractor = UnitTestOutputTokenizer<std::vector<std::string>>;
+
+  static const char* test_filename = "Windows is Terrible.txt";
+  std::string fake_input =
+      std::string("\\export_dashboard \"\\\"Who uses spaces anyway?\\\"\" \"") +
+      test_filename + "\"" + " ChelseaF.C.";
+
+  std::ostringstream test_capture_stream;
+  ExportDashboardCommandMockupContext unit_test_context;
+  auto resolution =
+      CommandResolutionChain<>(fake_input.c_str(),
+                               "\\export_dashboard",
                                3,
+                               4,
                                UnitTestExportDashboardCmd(unit_test_context),
                                test_capture_stream)
           .is_resolved();
   EXPECT_TRUE(resolution);
   EXPECT_EQ(unit_test_context.view_name, "\"Who uses spaces anyway?\"");
   EXPECT_EQ(unit_test_context.view_state, "");
   EXPECT_EQ(unit_test_context.view_metadata, "");
+  EXPECT_EQ(unit_test_context.dashboard_owner, "ChelseaF.C.");
 
-  EXPECT_NE(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
+  EXPECT_EQ(std::strstr(test_capture_stream.str().c_str(), test_filename), nullptr);
   unlink(test_filename);
 }
 

diff --git a/ThriftHandler/MapDHandler.cpp b/ThriftHandler/MapDHandler.cpp
@@ -3027,7 +3027,9 @@ void MapDHandler::get_dashboards(std::vector<TDashboard>& dashboards,
       dash.dashboard_metadata = d->viewMetadata;
       dash.dashboard_id = d->viewId;
       dash.dashboard_owner = d->user;
-      // dash.is_dash_shared = !objects_list.empty();
+      // viewState is intentionally not populated here
+      // for payload reasons
+      // use get_dashboard call to get state
       if (objects_list.empty() ||
           (objects_list.size() == 1 && objects_list[0]->roleName == user_meta.userName)) {
         dash.is_dash_shared = false;