[PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Stefano Rodighiero]

[-- Attachment #1.1: Type: text/plain, Size: 1370 bytes --]

Hi,

[This is the first patch I ever submitted.  I hope it complies with
your standards: if it does not, I'll be happy to work on it until it's
fine.  I am not sure it qualifies as a tiny change.]

Org-babel allows SQL snippets to be run on a database connection that
can be specified in the source block header using parameters such as
:dbhost, :dbuser, :dbpassword and so forth.

This is very useful, but I'd also like to be able to use symbolic
references to connections defined elsewhere, so that for example one
does not have to specify the password every time, interactively or,
worse, in the .org file itself.

I am also a user of sql.el, that provides a custom variable
`sql-connection-alist', where users can define a mapping between
connection names and connection details.

The patch I'm submitting extends the behavior of
org-babel-execute:sql so that it's possible to specify a new param
:dbconnection containing a connection name, used for looking up
`sql-connection-alist'.

For example, if `sql-connection-alist' contains something like:

  (("mydb" (sql-product 'postgres)
     (sql-post 5432)
   (sql-server "mydb.server")
   (sql-user "stefano")
   (sql-password "supersecret")))

Then it's possible to use:

  #+begin_src sql :engine postgresql :dbconnection mydb
    select foo, bar, baz from mytable
  #+end_src

s.


-- 
www.stefanorodighiero.net

[-- Attachment #1.2: Type: text/html, Size: 2230 bytes --]

[-- Attachment #2: 0001-ob-sql.el-Option-to-reference-connections-in-sql-con.patch --]
[-- Type: text/x-patch, Size: 3051 bytes --]

From 176cf648f9f38de1b9c35bf1fae06c23187db8b7 Mon Sep 17 00:00:00 2001
From: Stefano Rodighiero <stefano.rodighiero@gmail.com>
Date: Sun, 24 Mar 2019 11:35:21 +0100
Subject: [PATCH] ob-sql.el: Option to reference connections in
 `sql-connection-alist'

* ob-sql.el: Provide a new param called :dbconnection, that can be
used to reference connections defined in `sql-connection-alist', a
custom variable defined in sql.el.
---
 lisp/ob-sql.el | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-sql.el b/lisp/ob-sql.el
index 2a58188..0e1d4f0 100644
--- a/lisp/ob-sql.el
+++ b/lisp/ob-sql.el
@@ -39,6 +39,7 @@
 ;; - dbport
 ;; - dbuser
 ;; - dbpassword
+;; - dbconnection (to reference connections in sql-connection-alist)
 ;; - database
 ;; - colnames (default, nil, means "yes")
 ;; - result-params
@@ -174,16 +175,36 @@ Otherwise, use Emacs' standard conversion function."
 	((string= "windows-nt" system-type) file)
 	(t (format "%S" (convert-standard-filename file)))))
 
+(defun org-babel-find-db-connection-param (params name)
+  "Return db connection param NAME.
+Given a param NAME, if :dbconnection is defined in PARAMS then
+look for the param into the corresponding connection defined in
+`sql-connection-alist`, otherwise look into PARAMS.  Look
+`sql-connection-alist` (part of SQL mode) for how to define
+database connections."
+  (if (assq :dbconnection params)
+      (let* ((dbconnection (cdr (assq :dbconnection params)))
+             (name-mapping '((dbhost . sql-server)
+                             (dbport . sql-port)
+                             (dbuser . sql-user)
+                             (dbpassword . sql-password)
+                             (database . sql-database)))
+             (mapped-name (cdr (assq name name-mapping))))
+        (cadr (assq mapped-name
+                    (cdr (assoc dbconnection
+                                sql-connection-alist)))))
+    (cdr (assq name params))))
+
 (defun org-babel-execute:sql (body params)
   "Execute a block of Sql code with Babel.
 This function is called by `org-babel-execute-src-block'."
   (let* ((result-params (cdr (assq :result-params params)))
          (cmdline (cdr (assq :cmdline params)))
-         (dbhost (cdr (assq :dbhost params)))
-         (dbport (cdr (assq :dbport params)))
-         (dbuser (cdr (assq :dbuser params)))
-         (dbpassword (cdr (assq :dbpassword params)))
-         (database (cdr (assq :database params)))
+         (dbhost (org-babel-find-db-connection-param params 'dbhost))
+         (dbport (org-babel-find-db-connection-param params 'dbport))
+         (dbuser (org-babel-find-db-connection-param params 'dbuser))
+         (dbpassword (org-babel-find-db-connection-param params 'dbpassword))
+         (database (org-babel-find-db-connection-param params 'database))
          (engine (cdr (assq :engine params)))
          (colnames-p (not (equal "no" (cdr (assq :colnames params)))))
          (in-file (org-babel-temp-file "sql-in-"))
-- 
2.7.4

Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Nicolas Goaziou]

Hello,

Stefano Rodighiero <stefano.rodighiero@gmail.com> writes:

> [This is the first patch I ever submitted.

Thank you. Some comments follow.

> I hope it complies with
> your standards: if it does not, I'll be happy to work on it until it's
> fine.  I am not sure it qualifies as a tiny change.]

It does.

> Subject: [PATCH] ob-sql.el: Option to reference connections in
>  `sql-connection-alist'
>
> * ob-sql.el: Provide a new param called :dbconnection, that can be
> used to reference connections defined in `sql-connection-alist', a
> custom variable defined in sql.el.

You need to spell out the new function in the commit message:

* ob-sql.el (org-babel-find-db-connection-param): new function.

This patch provides a new parameter :dbconnection, ...

> +(defun org-babel-find-db-connection-param (params name)
> +  "Return db connection param NAME.

database, parameter

> +Given a param NAME, if :dbconnection is defined in PARAMS then
> +look for the param into the corresponding connection defined in
> +`sql-connection-alist`, otherwise look into PARAMS.  Look
> +`sql-connection-alist` (part of SQL mode) for how to define
> +database connections."
> +  (if (assq :dbconnection params)
> +      (let* ((dbconnection (cdr (assq :dbconnection params)))
> +             (name-mapping '((dbhost . sql-server)
> +                             (dbport . sql-port)
> +                             (dbuser . sql-user)
> +                             (dbpassword . sql-password)
> +                             (database . sql-database)))
> +             (mapped-name (cdr (assq name name-mapping))))
> +        (cadr (assq mapped-name
> +                    (cdr (assoc dbconnection
> +                                sql-connection-alist)))))
> +    (cdr (assq name params))))

Isn't there a type mismatch here? 
> +
>  (defun org-babel-execute:sql (body params)
>    "Execute a block of Sql code with Babel.
>  This function is called by `org-babel-execute-src-block'."
>    (let* ((result-params (cdr (assq :result-params params)))
>           (cmdline (cdr (assq :cmdline params)))
> -         (dbhost (cdr (assq :dbhost params)))
> -         (dbport (cdr (assq :dbport params)))
> -         (dbuser (cdr (assq :dbuser params)))
> -         (dbpassword (cdr (assq :dbpassword params)))
> -         (database (cdr (assq :database params)))
> +         (dbhost (org-babel-find-db-connection-param params 'dbhost))
> +         (dbport (org-babel-find-db-connection-param params 'dbport))
> +         (dbuser (org-babel-find-db-connection-param params 'dbuser))
> +         (dbpassword (org-babel-find-db-connection-param params 'dbpassword))
> +         (database (org-babel-find-db-connection-param params 'database))
>           (engine (cdr (assq :engine params)))

The change from keywords to plain symbols is slightly confusing. Could
`org-babel-find-db-connection-param' use keywords instead? This is the
usual format for parameters in Babel.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Stefano Rodighiero]

[-- Attachment #1.1: Type: text/plain, Size: 1863 bytes --]

On Sun, Apr 7, 2019 at 9:24 AM Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:

Thank you for your review.
I am attaching a new patch that should address your remarks.
Also, see comments below.


> > +Given a param NAME, if :dbconnection is defined in PARAMS then
> > +look for the param into the corresponding connection defined in
> > +`sql-connection-alist`, otherwise look into PARAMS.  Look
> > +`sql-connection-alist` (part of SQL mode) for how to define
> > +database connections."
> > +  (if (assq :dbconnection params)
> > +      (let* ((dbconnection (cdr (assq :dbconnection params)))
> > +             (name-mapping '((dbhost . sql-server)
> > +                             (dbport . sql-port)
> > +                             (dbuser . sql-user)
> > +                             (dbpassword . sql-password)
> > +                             (database . sql-database)))
> > +             (mapped-name (cdr (assq name name-mapping))))
> > +        (cadr (assq mapped-name
> > +                    (cdr (assoc dbconnection
> > +                                sql-connection-alist)))))
> > +    (cdr (assq name params))))
>
> Isn't there a type mismatch here?
>

I think you are talking about the asymmetry of `cdr' and `cadr'
in the two branches of `if'.
Unfortunately, it's what I need to access `sql-connection-alist'.
This custom's name is IMO a bit misleading. Here a passage
from its documentation:

'''
Each element of the alist is as follows:

  (CONNECTION \(SQL-VARIABLE VALUE) ...)
'''

And here a piece of code from `sql-connect' as defined
in sql.el (here, connect-set is the set of options attached
to a particular connection):

'''
(dolist (vv connect-set)
                  (let ((var (car vv))
                        (val (cadr vv)))
                    (set-default var (eval val))))
'''

regards,
s.


-- 
www.stefanorodighiero.net

[-- Attachment #1.2: Type: text/html, Size: 2996 bytes --]

[-- Attachment #2: 0001-ob-sql.el-Option-to-reference-connections-in-sql-con.patch --]
[-- Type: text/x-patch, Size: 3108 bytes --]

From e810b3b4b9259b708dd37ac04339e2f71e76478a Mon Sep 17 00:00:00 2001
From: Stefano Rodighiero <stefano.rodighiero@gmail.com>
Date: Sun, 24 Mar 2019 11:35:21 +0100
Subject: [PATCH] ob-sql.el: Option to reference connections in
 `sql-connection-alist'

* lisp/ob-sql.el (org-babel-find-db-connection-param): new function.

Provide a new param called :dbconnection, that can be used to
reference connections defined in `sql-connection-alist', a custom
variable defined in sql.el.

TINYCHANGE
---
 lisp/ob-sql.el | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/lisp/ob-sql.el b/lisp/ob-sql.el
index 2a58188..1007e74 100644
--- a/lisp/ob-sql.el
+++ b/lisp/ob-sql.el
@@ -39,6 +39,7 @@
 ;; - dbport
 ;; - dbuser
 ;; - dbpassword
+;; - dbconnection (to reference connections in sql-connection-alist)
 ;; - database
 ;; - colnames (default, nil, means "yes")
 ;; - result-params
@@ -174,16 +175,35 @@ Otherwise, use Emacs' standard conversion function."
 	((string= "windows-nt" system-type) file)
 	(t (format "%S" (convert-standard-filename file)))))
 
+(defun org-babel-find-db-connection-param (params name)
+  "Return database connection parameter NAME.
+Given a parameter NAME, if :dbconnection is defined in PARAMS
+then look for the parameter into the corresponding connection
+defined in `sql-connection-alist`, otherwise look into PARAMS.
+Look `sql-connection-alist` (part of SQL mode) for how to define
+database connections."
+  (if (assq :dbconnection params)
+      (let* ((dbconnection (cdr (assq :dbconnection params)))
+             (name-mapping '((:dbhost . sql-server)
+                             (:dbport . sql-port)
+                             (:dbuser . sql-user)
+                             (:dbpassword . sql-password)
+                             (:database . sql-database)))
+             (mapped-name (cdr (assq name name-mapping))))
+        (cadr (assq mapped-name
+                    (cdr (assoc dbconnection sql-connection-alist)))))
+    (cdr (assq name params))))
+
 (defun org-babel-execute:sql (body params)
   "Execute a block of Sql code with Babel.
 This function is called by `org-babel-execute-src-block'."
   (let* ((result-params (cdr (assq :result-params params)))
          (cmdline (cdr (assq :cmdline params)))
-         (dbhost (cdr (assq :dbhost params)))
-         (dbport (cdr (assq :dbport params)))
-         (dbuser (cdr (assq :dbuser params)))
-         (dbpassword (cdr (assq :dbpassword params)))
-         (database (cdr (assq :database params)))
+         (dbhost (org-babel-find-db-connection-param params :dbhost))
+         (dbport (org-babel-find-db-connection-param params :dbport))
+         (dbuser (org-babel-find-db-connection-param params :dbuser))
+         (dbpassword (org-babel-find-db-connection-param params :dbpassword))
+         (database (org-babel-find-db-connection-param params :database))
          (engine (cdr (assq :engine params)))
          (colnames-p (not (equal "no" (cdr (assq :colnames params)))))
          (in-file (org-babel-temp-file "sql-in-"))
-- 
2.7.4

Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Nicolas Goaziou]

Hello,

Stefano Rodighiero <stefano.rodighiero@gmail.com> writes:

> I am attaching a new patch that should address your remarks.
> Also, see comments below.

Thank you. I applied your patch.

Could you also provide an entry in ORG-NEWS?

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Stefano Rodighiero]

[-- Attachment #1.1: Type: text/plain, Size: 264 bytes --]

On Mon, Apr 15, 2019 at 6:11 PM Nicolas Goaziou <mail@nicolasgoaziou.fr>
wrote:


> Thank you. I applied your patch.
>

Great, thank you again for your help.


> Could you also provide an entry in ORG-NEWS?
>

Attached.

regards,
s.

-- 
www.stefanorodighiero.net

[-- Attachment #1.2: Type: text/html, Size: 898 bytes --]

[-- Attachment #2: 0001-ORG-NEWS-Mention-parameter-dbconnection-for-org-babe.patch --]
[-- Type: text/x-patch, Size: 938 bytes --]

From 802cbc9c8e631c4457934c4ad00b3c6ac2a62105 Mon Sep 17 00:00:00 2001
From: Stefano Rodighiero <stefano.rodighiero@gmail.com>
Date: Mon, 15 Apr 2019 22:14:07 +0200
Subject: [PATCH] ORG-NEWS: Mention parameter :dbconnection for
 org-babel-execute:sql

---
 etc/ORG-NEWS | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index a2e9057..79659ea 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -188,6 +188,10 @@ days in the past.
 =<C-u K>= (~org-habit-toggle-display-in-agenda~) in an agenda toggles
 the display of all habits to those which are undone and scheduled.
 This is a function for convenience.
+*** New paramater for SQL Babel blocks: ~:dbconnection~
+The new paramater ~:dbconnection~ allows to specify a connection name
+in a SQL block header: this name is used to look up connection
+parameters in ~sql-connection-alist~.
 ** New functions
 *** ~org-dynamic-block-insert-dblock~
 
-- 
2.7.4

Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[Nicolas Goaziou]

Hello,

Stefano Rodighiero <stefano.rodighiero@gmail.com> writes:

>> Could you also provide an entry in ORG-NEWS?
>>
>
> Attached.

Applied. Thank you.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] ob-sql.el: Option to reference connections in `sql-connection-alist'

[stardiviner]

Stefano Rodighiero <stefano.rodighiero@gmail.com> writes:

> On Mon, Apr 15, 2019 at 6:11 PM Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
>  
>  Thank you. I applied your patch.
>
> Great, thank you again for your help. 
>  
>  Could you also provide an entry in ORG-NEWS?
>
> Attached. 
>
> regards,
> s.

Now this patch is applied. I'm curious about a simple ob-sql source block
example of using the ~:dbconnection~ as a proof of code?

-- 
[ stardiviner ]
       I try to make every word tell the meaning what I want to express.

       Blog: https://stardiviner.github.io/
       IRC(freenode): stardiviner, Matrix: stardiviner
       GPG: F09F650D7D674819892591401B5DF1C95AE89AC3