Skip to content
Snippets Groups Projects
Commit 0bcb7e36 authored by Cam Saul's avatar Cam Saul
Browse files

Remove the code relating to handling legacy conn_str DB connection details

parent 5d462aad
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 86 additions and 334 deletions
resources/frontend_client/app/admin/databases/databases.controllers.js
+ 3
− 3
View file @ 0bcb7e36
......@@ -42,7 +42,7 @@ DatabasesControllers.controller('DatabaseEdit', ['$scope', '$routeParams', '$loc
// update an existing Database
var update = function(database, details) {
$scope.$broadcast("form:reset");
database.details = $scope.ENGINES[database.engine].buildDetails(details);
database.details = details;
return Metabase.db_update(database).$promise.then(function(updated_database) {
$scope.database = updated_database;
$scope.$broadcast("form:api-success", "Successfully saved!");
......@@ -55,7 +55,7 @@ DatabasesControllers.controller('DatabaseEdit', ['$scope', '$routeParams', '$loc
// create a new Database
var create = function(database, details, redirectToDetail) {
$scope.$broadcast("form:reset");
database.details = $scope.ENGINES[database.engine].buildDetails(details);
database.details = details;
return Metabase.db_create(database).$promise.then(function(new_database) {
if (redirectToDetail) {
$location.path('/admin/databases/' + new_database.id);
......@@ -135,7 +135,7 @@ DatabasesControllers.controller('DatabaseEdit', ['$scope', '$routeParams', '$loc
}, function(database) {
$scope.hiddenFields = null;
$scope.database = database;
$scope.details = $scope.ENGINES[database.engine].parseDetails(database.details);
$scope.details = database.details;
}, function(error) {
console.log('error loading database', error);
if (error.status == 404) {
......
resources/frontend_client/app/services.js
+ 3
− 85
View file @ 0bcb7e36
......@@ -425,17 +425,9 @@ CorvusServices.service('CorvusCore', ['$resource', 'User', function($resource, U
//
// NOTE:
// A database's connection details is stored in a JSON map in the field database.details.
// Originially, this map was expected to contain a single key called 'conn_str' that combined all of a database's connection details.
// In real life, both the backend and frontend need access to the individual values, and have implemented complicated logic to parse conn_str.
// Thus, we are moving towards saving the connection details in a 'new-style' broken-out map, instead of as 'legacy' map containing just a combined conn_str.
//
// Until this is fully supported by the backend(s), we can save the connection details with both the 'new-style' broken-out values, and the combined conn_str
// to ensure backwards-compatibility. Until this transition is complete, however, we'll still need to handle legacy maps containing just 'conn_str'.
//
// ENGINE DICT FORMAT:
// * name - human-facing name to use for this DB engine
// * buildDetails - take a 'new-style' details map and add 'conn_str' for backwards compatibility, if needed
// * parseDetails - take a details map and parse 'conn_str' if it's a legacy map. Otherwise we can return the map as-is
// * fields - array of available fields to display when a user adds/edits a DB of this type. Each field should be a dict of the format below:
//
// FIELD DICT FORMAT:
......@@ -493,33 +485,7 @@ CorvusServices.service('CorvusCore', ['$resource', 'User', function($resource, U
value: false,
selectionAccent: 'danger'
}]
}],
parseDetails: function(details) {
// Check for new-style details
if (details.dbname) return details;
// Otherwise parse legacy details
var map = {
ssl: details.ssl
};
details.conn_str.split(' ').forEach(function(val) {
var split = val.split('=');
if (split.length === 2) {
map[split[0]] = split[1];
}
});
return map;
},
buildDetails: function(details) {
// add conn_str for backwards-compatibility
details.conn_str =
"host=" + details.host +
" port=" + details.port +
" dbname=" + details.dbname +
" user=" + details.user +
(details.pass ? (" password=" + details.pass) : '');
return details;
}
}]
},
h2: {
name: 'H2',
......@@ -527,21 +493,7 @@ CorvusServices.service('CorvusCore', ['$resource', 'User', function($resource, U
displayName: "Connection String",
fieldName: "db",
placeholder: "file:/Users/camsaul/bird_sightings/toucans;AUTO_SERVER=TRUE"
}],
parseDetails: function(details) {
// Check for new-style details
if (details.db) return details;
// Otherwise parse legacy details
return {
db: details.conn_str
};
},
buildDetails: function(details) {
// add conn_str for backwards-compatibility
details.conn_str = details.db;
return details;
}
}]
},
mongo: {
name: 'MongoDB',
......@@ -568,41 +520,7 @@ CorvusServices.service('CorvusCore', ['$resource', 'User', function($resource, U
displayName: "Database password",
fieldName: "pass",
placeholder: "******"
}],
parseDetails: function(details) {
// check for new-style details
if (details.dbname) return details;
// otherwise parse legacy details
var regex = /^mongodb:\/\/(?:([^@:]+)(?::([^@:]+))?@)?([^\/:@]+)(?::([\d]+))?\/([^\/]+)$/gm, // :scream:
matches = regex.exec(details.conn_str);
return {
user: matches[1],
pass: matches[2],
host: matches[3],
port: matches[4],
dbname: matches[5]
};
},
buildDetails: function(details) {
// add conn_str for backwards-compatibility
var connStr = "mongodb://";
if (details.user) {
connStr += details.user;
if (details.pass) {
connStr += ":" + details.pass;
}
connStr += "@";
}
connStr += details.host;
if (details.port) {
connStr += ":" + details.port;
}
connStr += "/" + details.dbname;
details.conn_str = connStr;
return details;
}
}]
}
};
......
src/metabase/db.clj
+ 10
− 15
View file @ 0bcb7e36
......@@ -61,18 +61,17 @@
;; ## CONNECTION
(defn metabase-db-conn-str
"A connection string that can be used when pretending the Metabase DB is itself a `Database`
(defn- metabase-db-connection-details
"Connection details that can be used when pretending the Metabase DB is itself a `Database`
(e.g., to use the Generic SQL driver functions on the Metabase DB itself)."
[]
(case (config/config-kw :mb-db-type)
:h2 (db-file)
:postgres (format "host=%s port=%d dbname=%s user=%s password=%s"
(config/config-str :mb-db-host)
(config/config-int :mb-db-port)
(config/config-str :mb-db-dbname)
(config/config-str :mb-db-user)
(config/config-str :mb-db-pass))))
:h2 {:db (db-file)}
:postgres {:host (config/config-str :mb-db-host)
:port (config/config-int :mb-db-port)
:dbname (config/config-str :mb-db-dbname)
:user (config/config-str :mb-db-user)
:password (config/config-str :mb-db-pass)}))
;; ## MIGRATE
......@@ -107,7 +106,7 @@
;; Test DB connection and throw exception if we have any troubles connecting
(log/info "Verifying Database Connection ...")
(assert (db-can-connect? {:engine (config/config-kw :mb-db-type)
:details {:conn_str (metabase-db-conn-str)}})
:details (metabase-db-connection-details)})
"Unable to connect to Metabase DB.")
(log/info "Verify Database Connection ... CHECK")
......@@ -127,11 +126,7 @@
(log/info "Database Migrations Current ... CHECK")
;; Establish our 'default' Korma DB Connection
(default-connection (create-db korma-db))
;; Perform ghetto "data migration" to update any old-style database connection strings. This is temporary until everyone's DBs are updated
;; Resolve at runtime to avoid circular deps
((u/runtime-resolved-fn 'metabase.db.migrate-details 'convert-legacy-database-connection-details))))
(default-connection (create-db korma-db))))
(defn setup-db-if-needed [& args]
(when-not @setup-db-has-been-called?
......
src/metabase/db/migrate_details.clj deleted 100644 → 0
+ 0
− 51
View file @ 5d462aad
(ns metabase.db.migrate-details
(:require [clojure.pprint :refer [pprint]]
[clojure.tools.logging :as log]
[medley.core :as m]
[metabase.db :refer :all]
(metabase.driver [postgres :as postgres])
[metabase.models.database :refer :all]))
;; # Convert old-style DB connection details to new-style ones
(def ^:private ^:const is-legacy-connection-details?
{:postgres postgres/is-legacy-conn-details?
:h2 (fn [details]
(not (:db details)))
:mongo (fn [details]
(not (:dbname details)))})
(def ^:private ^:const convert-legacy-connection-details
{:postgres (fn [details]
(merge details (postgres/parse-legacy-conn-str (:conn_str details))))
:h2 (fn [details]
(assoc details :db (:conn_str details)))
:mongo (fn [details]
(let [[_ user pass host port dbname] (re-matches #"^mongodb:\/\/(?:([^@:]+)(?::([^@:]+))?@)?([^\/:@]+)(?::([\d]+))?\/([^\/]+)$" (:conn_str details))]
(m/filter-vals identity (assoc details
:user user
:pass pass
:host host
:port (when port
(Integer/parseInt port))
:dbname dbname))))})
(defn convert-details-when-legacy
"When DETAILS are legacy, return updated map, otherwise return `nil`."
[engine details]
{:pre [(keyword? engine)
(map? details)]}
(when ((is-legacy-connection-details? engine) details)
((convert-legacy-connection-details engine) details)))
(defn convert-legacy-database-connection-details
"Convert the connection `details` of databases in the DB to [backwards-compatible] new-style ones when applicable."
[]
(doseq [{:keys [id details engine]} (sel :many :fields [Database :id :details :engine])]
(when-let [updated-details (convert-details-when-legacy engine details)]
(log/info (format "Database %d (%s) has legacy connection details: " id engine)
(with-out-str (clojure.pprint/pprint details))
"Updated these to: "
(with-out-str (clojure.pprint/pprint updated-details)))
(upd Database id :details updated-details))))
src/metabase/driver/h2.clj
+ 1
− 2
View file @ 0bcb7e36
......@@ -11,8 +11,7 @@
:make-pool? false)))
(defn- database->connection-details [{:keys [details]}]
{:db (or (:db details) ; new-style connection details call it 'db'
(:conn_str details))}) ; legacy instead calls is 'conn_str'
details)
;; ## SYNCING
......
src/metabase/driver/mongo/util.clj
+ 2
− 2
View file @ 0bcb7e36
......@@ -20,7 +20,8 @@
(when-not (s/blank? (str port))
(str ":" port))
"/"
dbname))
dbname
"?connectTimeoutMS=250")) ; timeout after 250 ms instead of 10s so can-connect? doesn't take forever
(def ^:dynamic *mongo-connection*
"Connection to a Mongo database.
......@@ -35,7 +36,6 @@
(string? database) database
(:dbname (:details database)) (details-map->connection-string (:details database)) ; new-style -- entire Database obj
(:dbname database) (details-map->connection-string database) ; new-style -- connection details map only
(:conn_str (:details database)) (:conn_str (:details database)) ; legacy
:else (throw (Exception. (str "with-mongo-connection failed: bad connection details:" (:details database)))))
{conn :conn mongo-connection :db} (mg/connect-via-uri connection-string)]
(log/debug (color/cyan "<< OPENED NEW MONGODB CONNECTION >>"))
......
src/metabase/driver/postgres.clj
+ 1
− 20
View file @ 0bcb7e36
......@@ -90,28 +90,9 @@
(rename-keys {:dbname :db})
kdb/postgres))
(defn is-legacy-conn-details?
"Is DETAILS-MAP a legacy map (i.e., does it only contain `conn_str`)?"
[details-map]
{:pre [(map? details-map)]}
(not (:dbname details-map)))
(defn parse-legacy-conn-str
"Parse a legacy `database.details.conn_str` CONNECTION-STRING and return a new-style map."
[connection-string]
{:pre [(string? connection-string)]}
(let [{:keys [port] :as details} (-<>> connection-string
(s/split <> #" ") ; split into k=v pairs
(map (fn [pair] ; convert to {:k v} pairs
(let [[k v] (s/split pair #"=")]
{(keyword k) v})))
(reduce conj {}))]
(cond-> details
(string? port) (assoc :port (Integer/parseInt port)))))
(defn- database->connection-details [{:keys [details]}]
(let [{:keys [host port] :as details} (if (is-legacy-conn-details? details) (parse-legacy-conn-str (:conn_str details))
details)]
(let [{:keys [host port]} details]
(-> details
(assoc :host host
:make-pool? false
......
test/metabase/api/meta/db_test.clj
+ 54
− 51
View file @ 0bcb7e36
......@@ -13,9 +13,12 @@
;; HELPER FNS
(defn create-db [db-name]
((user->client :crowberto) :post 200 "meta/db" {:engine :postgres
:name db-name
:details {:conn_str "host=localhost port=5432 dbname=fakedb user=cam"}}))
((user->client :crowberto) :post 200 "meta/db" {:engine :postgres
:name db-name
:details {:host "localhost"
:port 5432
:dbname "fakedb"
:user "cam"}}))
;; # FORM INPUT
......@@ -54,14 +57,14 @@
(let [db-name (random-name)]
(expect-eval-actual-first
(match-$ (sel :one Database :name db-name)
{:created_at $
:engine "postgres" ; string because it's coming back from API instead of DB
:id $
:details {:conn_str "host=localhost port=5432 dbname=fakedb user=cam"}
:updated_at $
:name db-name
{:created_at $
:engine "postgres" ; string because it's coming back from API instead of DB
:id $
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
:updated_at $
:name db-name
:organization_id nil
:description nil})
:description nil})
(create-db db-name)))
;; ## DELETE /api/meta/db/:id
......@@ -80,20 +83,20 @@
(expect-let [[old-name new-name] (repeatedly 2 random-name)
{db-id :id} (create-db old-name)
sel-db (fn [] (sel :one :fields [Database :name :engine :details] :id db-id))]
[{:details {:conn_str "host=localhost port=5432 dbname=fakedb user=cam"}
:engine :postgres
:name old-name}
{:details {:conn_str "host=localhost port=5432 dbname=fakedb user=rastacan"}
:engine :h2
:name new-name}
{:details {:conn_str "host=localhost port=5432 dbname=fakedb user=rastacan"}
:engine :h2
:name old-name}]
[{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
:engine :postgres
:name old-name}
{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
:engine :h2
:name new-name}
{:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}
:engine :h2
:name old-name}]
[(sel-db)
;; Check that we can update all the fields
(do ((user->client :crowberto) :put 200 (format "meta/db/%d" db-id) {:name new-name
:engine "h2"
:details {:conn_str "host=localhost port=5432 dbname=fakedb user=rastacan"}})
(do ((user->client :crowberto) :put 200 (format "meta/db/%d" db-id) {:name new-name
:engine "h2"
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "rastacan"}})
(sel-db))
;; Check that we can update just a single field
(do ((user->client :crowberto) :put 200 (format "meta/db/%d" db-id) {:name old-name})
......@@ -105,36 +108,36 @@
;; Test that we can get all the DBs for an Org, ordered by name
(let [db-name (str "A" (random-name))] ; make sure this name comes before "Test Database"
(expect-eval-actual-first
(filter identity
[(datasets/when-testing-dataset :generic-sql
(match-$ (sel :one Database :name db-name)
{:created_at $
:engine "postgres"
:id $
:details {:conn_str "host=localhost port=5432 dbname=fakedb user=cam"}
:updated_at $
:name $
(filter identity
[(datasets/when-testing-dataset :generic-sql
(match-$ (sel :one Database :name db-name)
{:created_at $
:engine "postgres"
:id $
:details {:host "localhost", :port 5432, :dbname "fakedb", :user "cam"}
:updated_at $
:name $
:organization_id nil
:description nil}))
(datasets/when-testing-dataset :mongo
(match-$ @mongo-test-data/mongo-test-db
{:created_at $
:engine "mongo"
:id $
:details $
:updated_at $
:name "Test Database"
:organization_id nil
:description nil}))
(match-$ (db)
{:created_at $
:engine "h2"
:id $
:details $
:updated_at $
:name "Test Database"
:organization_id nil
:description nil}))
(datasets/when-testing-dataset :mongo
(match-$ @mongo-test-data/mongo-test-db
{:created_at $
:engine "mongo"
:id $
:details $
:updated_at $
:name "Test Database"
:organization_id nil
:description nil}))
(match-$ (db)
{:created_at $
:engine "h2"
:id $
:details $
:updated_at $
:name "Test Database"
:organization_id nil
:description nil})])
:description nil})])
(do
;; Delete all the randomly created Databases we've made so far
(cascade-delete Database :id [not-in (set (filter identity
......
test/metabase/db/migrate_details_test.clj deleted 100644 → 0
+ 0
− 76
View file @ 5d462aad
(ns metabase.db.migrate-details-test
(:require [expectations :refer :all]
[metabase.db.migrate-details :refer :all]))
;; # tests for convert-details-when-legacy
;; ## legacy postgres
(expect {:user "cam"
:dbname "fakedb"
:port 5432
:host "localhost"
:timezone "US/Pacific"
:conn_str "host=localhost port=5432 dbname=fakedb user=cam"}
(convert-details-when-legacy
:postgres
{:conn_str "host=localhost port=5432 dbname=fakedb user=cam"
:timezone "US/Pacific"}))
;; ## new-style postgres
(expect nil
(convert-details-when-legacy
:postgres
{:ssl false
:host "localhost"
:port 5432
:dbname "ryde"
:user "cam"
:conn_str "host=localhost port=5432 dbname=ryde user=cam"}))
;; ## legacy h2
(expect {:timezone "US/Pacific",
:db "host=localhost port=5432 dbname=fakedb user=rastacan"
:conn_str "host=localhost port=5432 dbname=fakedb user=rastacan"}
(convert-details-when-legacy
:h2
{:timezone "US/Pacific"
:conn_str "host=localhost port=5432 dbname=fakedb user=rastacan"}))
;; ## new-style h2
(expect nil
(convert-details-when-legacy
:h2
{:timezone "US/Pacific"
:db "host=localhost port=5432 dbname=fakedb user=rastacan"
:conn_str "host=localhost port=5432 dbname=fakedb user=rastacan"}))
;; ## legacy mongo
(expect {:timezone "US/Pacific"
:conn_str "mongodb://localhost:27017/test"
:port 27017
:dbname "test"
:host "localhost"}
(convert-details-when-legacy
:mongo
{:timezone "US/Pacific", :conn_str "mongodb://localhost:27017/test"}))
(expect {:timezone "US/Pacific"
:conn_str "mongodb://cam:password@localhost:27017/test"
:port 27017
:dbname "test"
:host "localhost"
:pass "password"
:user "cam"}
(convert-details-when-legacy
:mongo
{:timezone "US/Pacific",
:conn_str "mongodb://cam:password@localhost:27017/test"}))
(expect nil
(convert-details-when-legacy
:mongo
{:timezone "US/Pacific"
:conn_str "mongodb://localhost:27017/test"
:port 27017
:dbname "test"
:host "localhost"}))
test/metabase/driver/generic_sql/connection_test.clj
+ 4
− 4
View file @ 0bcb7e36
......@@ -17,10 +17,10 @@
;; Random made-up DBs should fail
(expect false
(driver/can-connect? {:engine :postgres
:details {:conn_str "host=localhost port=5432 dbname=ABCDEFGHIJKLMNOP user=rasta"}}))
(driver/can-connect? {:engine :postgres
:details {:host "localhost", :port 5432, :dbname "ABCDEFGHIJKLMNOP", :user "rasta"}}))
;; Things that you can connect to, but are not DBs, should fail
(expect false
(driver/can-connect? {:engine :postgres
:details {:conn_str "host=google.com port=80"}}))
(driver/can-connect? {:engine :postgres
:details {:host "google.com", :port 80}}))
test/metabase/driver/h2_test.clj
+ 1
− 5
View file @ 0bcb7e36
......@@ -5,12 +5,8 @@
(resolve-private-fns metabase.driver.h2 database->connection-details)
;; # Check that database->connection-details works with both new-style and legacy details
;; # Check that database->connection-details works
;; ## new-style
(expect {:db
"file:/Users/cam/birdly/bird_sightings.db;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1"}
(database->connection-details {:details {:db "file:/Users/cam/birdly/bird_sightings.db;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1"}}))
;; ## legacy
(expect {:db "file:/Users/cam/birdly/bird_sightings.db;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1"}
(database->connection-details {:details {:conn_str "file:/Users/cam/birdly/bird_sightings.db;AUTO_SERVER=TRUE;DB_CLOSE_DELAY=-1"}}))
test/metabase/driver/mongo_test.clj
+ 5
− 8
View file @ 0bcb7e36
......@@ -64,12 +64,6 @@
;; ## Tests for connection functions
;; legacy
(expect-when-testing-mongo true
(driver/can-connect? {:engine :mongo
:details {:conn_str "mongodb://localhost:27017/metabase-test"}}))
;; new-style
(expect-when-testing-mongo true
(driver/can-connect? {:engine :mongo
:details {:host "localhost"
......@@ -78,11 +72,14 @@
(expect-when-testing-mongo false
(driver/can-connect? {:engine :mongo
:details {:conn_str "mongodb://123.4.5.6/bad-db-name?connectTimeoutMS=50"}})) ; timeout after 50ms instead of 10s so test's don't take forever
:details {:host "123.4.5.6"
:dbname "bad-db-name"}}))
(expect-when-testing-mongo false
(driver/can-connect? {:engine :mongo
:details {:conn_str "mongodb://localhost:3000/bad-db-name?connectTimeoutMS=50"}}))
:details {:host "localhost"
:port 3000
:dbname "bad-db-name"}}))
(expect-when-testing-mongo false
(driver/can-connect-with-details? :mongo {}))
......
test/metabase/driver/postgres_test.clj
+ 2
− 12
View file @ 0bcb7e36
......@@ -18,20 +18,10 @@
:user "camsaul"}
(database->connection-details {:details {:ssl false
:host "localhost"
:port "5432"
:port 5432
:dbname "bird_sightings"
:user "camsaul"}}))
;; ## legacy
(expect {:db "bird_sightings"
:ssl nil
:db-type :postgres
:make-pool? false
:user "camsaul"
:port 5432
:host "localhost"}
(database->connection-details {:details {:conn_str "host=localhost port=5432 dbname=bird_sightings user=camsaul"}}))
;; # Check that SSL params get added the connection details in the way we'd like
;; ## no SSL -- this should *not* include the key :ssl (regardless of its value) since that will cause the PG driver to use SSL anyway
......@@ -43,7 +33,7 @@
:make-pool? true}
(connection-details->connection-spec {:ssl false
:host "localhost"
:port "5432"
:port 5432
:dbname "bird_sightings"
:user "camsaul"}))
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment