Add support for driver deprecation (#17028)

# Backend changes

Introducing new `superseded-by` property to plugin manifest YAML, which will indicate the driver that is to eventually replace this one (and will drive UI/UX behavior).  If a driver declares this property, then it's considered to be deprecated in favor of the specified one.

Adding top level `test_modules` directory (with the same structure as modules) for the sole purpose of module/plugin testing of YAML files, which will not be included with the driver build

Updating `driver-plugin-manifest` to look for the new `test_modules` directory in addition to `modules`, when loading the driver manifest

# Frontend changes

Calculate `supersededBy` and supersedes maps from the "superseded-by" property for each engine

Change the options for the engine field to use a function to dynamically show the legacy driver if allowed by rules (either the new driver is selected, or the legacy driver was already selected for an existing DB, or the driver is not superseded by anything)

Add new `DriverWarning` component to show these warnings based on supersede status

Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>

frontend/src/metabase/admin/databases/containers/DatabaseEditApp.jsx

@@ -15,6 +15,7 @@ import ActionButton from "metabase/components/ActionButton";
 import AddDatabaseHelpCard from "metabase/components/AddDatabaseHelpCard";
 import Button from "metabase/components/Button";
 import Breadcrumbs from "metabase/components/Breadcrumbs";
+import DriverWarning from "metabase/components/DriverWarning";
 import Radio from "metabase/components/Radio";
 import ModalWithTrigger from "metabase/components/ModalWithTrigger";
 
@@ -210,15 +211,20 @@ export default class DatabaseEditApp extends Component {
                     )}
                   </LoadingAndErrorWrapper>
                 </Box>
-                {addingNewDatabase && (
-                  <Box>
+                <Box>
+                  <DriverWarning
+                    engine={selectedEngine}
+                    ml={26}
+                    data-testid="database-setup-driver-warning"
+                  />
+                  {addingNewDatabase && (
                     <AddDatabaseHelpCard
                       engine={selectedEngine}
                       ml={26}
                       data-testid="database-setup-help-card"
                     />
-                  </Box>
-                )}
+                  )}
+                </Box>
               </Flex>
             </div>
           </Box>

frontend/src/metabase/components/DriverWarning/DriverWarning.jsx

+import React from "react";
+import PropTypes from "prop-types";
+
+import { t } from "ttag";
+
+import {
+  allEngines,
+  engineSupersedesMap,
+} from "metabase/entities/databases/forms";
+
+import Warnings from "metabase/query_builder/components/Warnings";
+
+import {
+  CardContent,
+  DriverWarningContainer,
+  IconContainer,
+} from "./DriverWarning.styled";
+import ExternalLink from "metabase/components/ExternalLink";
+import MetabaseSettings from "metabase/lib/settings";
+
+const propTypes = {
+  engine: PropTypes.string.isRequired,
+};
+
+const driverUpgradeHelpLink = MetabaseSettings.docsUrl(
+  "administration-guide/01-managing-databases",
+);
+
+function getSupersedesWarningContent(newDriver, supersedesDriver) {
+  return (
+    <div>
+      <p className="text-medium m0">
+        {t`This is our new ${
+          allEngines[newDriver]["driver-name"]
+        } driver, which is faster and more reliable.`}
+      </p>
+      <p>{t`The old driver has been deprecated and will be removed in the next release. If you really
+      need to use it, you can select ${
+        allEngines[supersedesDriver]["driver-name"]
+      } now.`}</p>
+    </div>
+  );
+}
+
+function getSupersededByWarningContent(engine) {
+  return (
+    <div>
+      <p className="text-medium m0">
+        {t`This driver has been deprecated and will be removed in the next release.`}
+      </p>
+      <p className="text-medium m0">
+        {t`We recommend that you upgrade to the new ${
+          allEngines[engine]["driver-name"]
+        } driver, which is faster and more reliable.`}
+      </p>
+      <ExternalLink
+        href={driverUpgradeHelpLink}
+        className="text-brand text-bold"
+      >
+        {t`How to upgrade a driver`}
+      </ExternalLink>
+    </div>
+  );
+}
+
+function DriverWarning({ engine, ...props }) {
+  const supersededBy = engineSupersedesMap["superseded_by"][engine];
+  const supersedes = engineSupersedesMap["supersedes"][engine];
+
+  if (!supersedes && !supersededBy) {
+    return null;
+  }
+
+  const tooltipWarning = supersedes ? t`New driver` : t`Driver deprecated`;
+  const warningContent = supersedes
+    ? getSupersedesWarningContent(engine, supersedes)
+    : getSupersededByWarningContent(supersededBy);
+
+  return (
+    <DriverWarningContainer p={2} {...props}>
+      <IconContainer>
+        <Warnings
+          className="mx2 text-gold"
+          warnings={[tooltipWarning]}
+          size={20}
+        />
+      </IconContainer>
+      <CardContent flexDirection="column" justify="center" className="ml2">
+        {warningContent}
+      </CardContent>
+    </DriverWarningContainer>
+  );
+}
+
+DriverWarning.propTypes = propTypes;
+
+export default DriverWarning;

frontend/src/metabase/components/DriverWarning/DriverWarning.styled.js

+import styled from "styled-components";
+import { Flex } from "grid-styled";
+
+export const CardContent = styled(Flex)``;
+
+export const IconContainer = styled.div`
+  display: flex;
+  justify-content: center;
+  align-items: center;
+`;
+
+export const DriverWarningContainer = styled(Flex)`
+  background-color: #f9fbfb;
+  border-radius: 10px;
+  width: 300px;
+  margin-bottom: 8px;
+`;

frontend/src/metabase/components/DriverWarning/index.js

+export { default } from "./DriverWarning";

frontend/src/metabase/entities/databases/forms.js

@@ -278,13 +278,51 @@ function getEngineFormFields(engine, details, id) {
     });
 }
 
-const ENGINE_OPTIONS = Object.entries(MetabaseSettings.get("engines") || {})
+const ENGINES = MetabaseSettings.get("engines", {});
+const ENGINE_OPTIONS = Object.entries(ENGINES)
   .map(([engine, info]) => ({
     name: info["driver-name"],
     value: engine,
   }))
   .sort((a, b) => a.name.localeCompare(b.name));
 
+// use top level constant for engines so we only need to compute these maps once
+const ENGINE_SUPERSEDES_MAPS = Object.keys(ENGINES).reduce(
+  (acc, engine) => {
+    const newEngine = ENGINES[engine]["superseded-by"];
+    if (newEngine) {
+      acc.supersedes[newEngine] = engine;
+      acc.superseded_by[engine] = newEngine;
+    }
+    return acc;
+  },
+  { supersedes: {}, superseded_by: {} },
+);
+
+/**
+ * Returns the options to show in the engines selection widget. An engine is available to be selected if either
+ *  - it is not superseded by any other engine
+ *  - it is the selected engine (i.e. someone is already using it)
+ *  - it is superseded by some engine, which happens to be the currently selected one
+ *
+ * The idea behind this behavior is to only show someone a "legacy" driver if they have at least selected the one that
+ * will replace it first, at which point they can "fall back" on the legacy one if needed.
+ *
+ * @param currentEngine the current (selected engine)
+ * @returns the filtered engine options to be shown in the selection widget
+ */
+function getEngineOptions(currentEngine) {
+  return ENGINE_OPTIONS.filter(engine => {
+    const engineName = engine.value;
+    const newDriver = ENGINE_SUPERSEDES_MAPS["superseded_by"][engineName];
+    return (
+      typeof newDriver === "undefined" ||
+      newDriver === currentEngine ||
+      engineName === currentEngine
+    );
+  });
+}
+
 const forms = {
   details: {
     fields: ({ id, engine, details = {} } = {}) => [
@@ -292,7 +330,7 @@ const forms = {
         name: "engine",
         title: t`Database type`,
         type: "select",
-        options: ENGINE_OPTIONS,
+        options: getEngineOptions(engine),
         placeholder: t`Select a database`,
       },
       {
@@ -382,3 +420,5 @@ const SCHEDULING_FIELDS = new Set([
 ]);
 
 export default forms;
+export const engineSupersedesMap = ENGINE_SUPERSEDES_MAPS;
+export const allEngines = ENGINES;

frontend/src/metabase/setup/components/Setup.jsx

@@ -8,6 +8,7 @@ import MetabaseAnalytics from "metabase/lib/analytics";
 import MetabaseSettings from "metabase/lib/settings";
 
 import AddDatabaseHelpCard from "metabase/components/AddDatabaseHelpCard";
+import DriverWarning from "metabase/components/DriverWarning";
 import ExternalLink from "metabase/components/ExternalLink";
 import LogoIcon from "metabase/components/LogoIcon";
 import NewsletterForm from "metabase/components/NewsletterForm";
@@ -203,6 +204,11 @@ export default class Setup extends Component {
           </div>
 
           <AddDatabaseHelpCardHolder isVisible={isDatabaseHelpCardVisible}>
+            <DriverWarning
+              engine={selectedDatabaseEngine}
+              ml={26}
+              data-testid="database-setup-driver-warning"
+            />
             <AddDatabaseHelpCard
               engine={selectedDatabaseEngine}
               hasCircle={false}

src/metabase/driver.clj

@@ -617,3 +617,19 @@
   [_ db-details]
   ;; no normalization by default
   db-details)
+
+(defmulti superseded-by
+  "Returns the driver that supersedes the given `driver`.  A non-nil return value means that the given `driver` is
+  deprecated in Metabase and will eventually be replaced by the returned driver, in some future version (at which point
+  any databases using it will be migrated to the new one).
+
+  This is currently only used on the frontend for the purpose of showing/hiding deprecated drivers. A driver can make
+  use of this facility by adding a top-level `superseded-by` key to its plugin manifest YAML file, or (less preferred)
+  overriding this multimethod directly."
+  {:added "0.41.0" :arglists '([driver])}
+  dispatch-on-uninitialized-driver
+  :hierarchy #'hierarchy)
+
+(defmethod superseded-by :default
+  [_]
+  nil)

src/metabase/driver/util.clj

@@ -100,7 +100,8 @@
                  :when  props]
              ;; TODO - maybe we should rename `details-fields` -> `connection-properties` on the FE as well?
              [driver {:details-fields props
-                      :driver-name    (driver/display-name driver)}])))
+                      :driver-name    (driver/display-name driver)
+                      :superseded-by  (driver/superseded-by driver)}])))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                             TLS Helpers                                                        |

src/metabase/plugins/initialize.clj

@@ -45,7 +45,7 @@
   (@initialized-plugin-names plugin-name))
 
 (s/defn init-plugin-with-info!
-  "Initiaize plugin using parsed info from a plugin maifest. Returns truthy if plugin was successfully initialized;
+  "Initialize plugin using parsed info from a plugin manifest. Returns truthy if plugin was successfully initialized;
   falsey otherwise."
   [info :- {:info     {:name s/Str, :version s/Str, s/Keyword s/Any}
             s/Keyword s/Any}]

src/metabase/plugins/lazy_loaded_driver.clj

@@ -63,6 +63,7 @@
   "Register a basic shell of a Metabase driver using the information from its Metabase plugin"
   [{:keys                                                                                            [add-to-classpath!]
     init-steps                                                                                       :init
+    superseded-by                                                                                    :superseded-by
     {driver-name :name, :keys [abstract display-name parent], :or {abstract false}, :as driver-info} :driver}]
   {:pre [(map? driver-info)]}
   (let [driver           (keyword driver-name)
@@ -81,7 +82,8 @@
     (doseq [[^MultiFn multifn, f]
             {driver/initialize!           (make-initialize! driver add-to-classpath! init-steps)
              driver/display-name          (when display-name (constantly display-name))
-             driver/connection-properties (constantly connection-props)}]
+             driver/connection-properties (constantly connection-props)
+             driver/superseded-by         (constantly (keyword superseded-by))}]
       (when f
         (.addMethod multifn driver f)))
     ;; finally, register the Metabase driver

test/metabase/driver/driver_deprecation_test_legacy.clj

+(ns metabase.driver.driver-deprecation-test-legacy
+  "Dummy driver for driver deprecation testing (legacy driver)"
+  (:require [metabase.driver :as driver]))
+
+(driver/register! :driver-deprecation-test-legacy, :parent :sql)

test/metabase/driver/driver_deprecation_test_new.clj

+(ns metabase.driver.driver-deprecation-test-new
+  "Dummy driver for driver deprecation testing (new driver)"
+  (:require [metabase.driver :as driver]))
+
+(driver/register! :driver-deprecation-test-new, :parent :sql)

test/metabase/plugins/driver_deprecation_test.clj

+(ns metabase.plugins.driver-deprecation-test
+  (:require [clojure.test :refer :all]
+            [metabase.models.setting :as setting]
+            [metabase.test :as mt]
+            [metabase.test.fixtures :as fixtures]
+            [metabase.util.i18n :as i18n :refer [tru]]))
+
+(use-fixtures :once (fixtures/initialize :plugins))
+
+(deftest driver-deprecation-test
+  (mt/test-driver :driver-deprecation-test-legacy
+    (is (= :driver-deprecation-test-new
+           (get-in (setting/properties :public) [:engines :driver-deprecation-test-legacy :superseded-by])))))

test/metabase/test/data/driver_deprecation_test_legacy.clj

+(ns metabase.test.data.driver-deprecation-test-legacy
+  "Dummy namespace for driver deprecation testing \"legacy\" driver, test data."
+  (:require [metabase.test.data.sql :as sql.tx]))
+
+(sql.tx/add-test-extensions! :driver-deprecation-test-legacy)

test/metabase/test/data/driver_deprecation_test_new.clj

+(ns metabase.test.data.driver-deprecation-test-new
+  "Dummy namespace for driver deprecation testing \"new\" driver, test data."
+  (:require [metabase.test.data.sql :as sql.tx]))
+
+(sql.tx/add-test-extensions! :driver-deprecation-test-new)

test/metabase/test/initialize/plugins.clj

@@ -8,9 +8,23 @@
             [yaml.core :as yaml]))
 
 (defn- driver-plugin-manifest [driver]
-  (let [manifest (io/file (format "modules/drivers/%s/resources/metabase-plugin.yaml" (name driver)))]
-    (when (.exists manifest)
-      (yaml/parse-string (slurp manifest)))))
+  (let [nm    (name driver)
+        paths (mapv
+                #(format "%s/drivers/%s/resources/metabase-plugin.yaml" % nm)
+                ;; look for driver definition in both the regular modules directory, as well as in a top-level
+                ;; test_modules directory, specifically designed for test driver definitions
+                ["modules" "test_modules"])]
+    (first (filter some?
+                   (for [path paths
+                         :let [manifest (io/file path)]
+                         :when (.exists manifest)]
+                     (do
+                       (println (u/format-color
+                                  'green
+                                  "Loading plugin manifest (from %s) for driver as if it were a real plugin: %s"
+                                  path
+                                  nm))
+                       (yaml/parse-string (slurp manifest))))))))
 
 (defn- driver-parents [driver]
   (let [parents-file (io/file (format "modules/drivers/%s/parents" (name driver)))]
@@ -31,7 +45,6 @@
    (doseq [driver drivers
            :let   [info (driver-plugin-manifest driver)]
            :when  info]
-     (println (u/format-color 'green "Loading plugin manifest for driver as if it were a real plugin: %s" driver))
      (plugins.init/init-plugin-with-info! info)
      ;; ok, now we need to make sure we load any depenencies for those drivers as well (!)
      (load-plugin-manifests! (driver-parents driver)))))

test_modules/drivers/driver-deprecation-test-legacy/resources/metabase-plugin.yaml

+info:
+  name: Legacy Driver
+  version: 0.0.0-SNAPSHOT
+  description: Test plugin for driver deprecation (legacy)
+driver:
+  name: driver-deprecation-test-legacy
+  display-name: Driver Deprecation Test Plugin (Legacy)
+  lazy-load: true
+  parent: sql
+  connection-properties:
+    - host
+    - port
+superseded-by: driver-deprecation-test-new
+init:
+  - step: load-namespace
+    namespace: metabase.driver.driver-deprecation-test-legacy

test_modules/drivers/driver-deprecation-test-new/resources/metabase-plugin.yaml

+info:
+  name: New Driver
+  version: 0.0.0-SNAPSHOT
+  description: Test plugin for driver deprecation (new)
+driver:
+  name: driver-deprecation-test-new
+  display-name: Driver Deprecation Test Plugin (New)
+  lazy-load: true
+  parent: sql
+  connection-properties:
+    - host
+    - port
+init:
+  - step: load-namespace
+    namespace: metabase.driver.driver-deprecation-test-new