diff mbox series

[bug#69292,3/6] store: database: Inline SQL to where it's used.

Message ID 5abcabc4cb65ea63db7c4f046700116e4882b287.1708457946.git.mail@cbaines.net
State New
Headers show
Series Prepare the database code for use in the daemon | expand

Commit Message

Christopher Baines Feb. 20, 2024, 7:39 p.m. UTC 
This makes the code easier to read, as you don't have to keep jumping between
the two places.

* guix/store/database.scm (path-id-sql, update-sql, insert-sql,
add-reference-sql): Remove variables.
(path-id, update-or-insert, add-references): Include SQL.

Change-Id: I53b4ab973be8d0cd10a0f35ba25972f1c9680353
---
 guix/store/database.scm | 45 ++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 18 deletions(-)

Comments

Ludovic Courtès Feb. 23, 2024, 4:40 p.m. UTC | #1 
Christopher Baines <mail@cbaines.net> skribis:

> This makes the code easier to read, as you don't have to keep jumping between
> the two places.
>
> * guix/store/database.scm (path-id-sql, update-sql, insert-sql,
> add-reference-sql): Remove variables.
> (path-id, update-or-insert, add-references): Include SQL.
>
> Change-Id: I53b4ab973be8d0cd10a0f35ba25972f1c9680353

LGTM.

>    (let ((id (path-id db path)))
>      (if id
> -        (let ((stmt (sqlite-prepare db update-sql #:cache? #t)))
> +        (let ((stmt (sqlite-prepare
> +                     db
> +                     "
> +UPDATE ValidPaths
> +SET hash = :hash,
> +    registrationTime = :time,
> +    deriver = :deriver,
> +    narSize = :size
> +WHERE id = :id"
> +                     #:cache? #t)))

I think we can make it a bit more dense (3 or 4 lines in total for the
statement).  :-)

In the future, we should probably add a macro to sqlite3 like that one
from Cuirass:

--8<---------------cut here---------------start------------->8---
(define-syntax-rule (exec-query/bind db query args ...)
  "Execute the specific QUERY with the given ARGS.  Uses of 'exec-query/bind'
typically look like this:

  (exec-query/bind db \"SELECT * FROM Foo WHERE x = \" x \"AND Y=\" y \";\")

References to variables 'x' and 'y' here are replaced by $1 and $2 in the
SQL query.

This ensures that (1) SQL injection is impossible, and (2) the number of
parameters matches the number of arguments to bind."
  (%exec-query/bind db () "" query args ...))
--8<---------------cut here---------------end--------------->8---

That makes things slightly more readable IMO since you don’t end up with
two separate calls, one to prepare the statement and the other one to
bind its arguments.

Ludo’.
diff mbox series

Patch

diff --git a/guix/store/database.scm b/guix/store/database.scm
index de72b79860..7e3a2873ce 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -178,13 +178,14 @@  (define (last-insert-row-id db)
       ((#(id)) id)
       (_ #f))))
 
-(define path-id-sql
-  "SELECT id FROM ValidPaths WHERE path = :path")
-
 (define* (path-id db path)
   "If PATH exists in the 'ValidPaths' table, return its numerical
 identifier.  Otherwise, return #f."
-  (let ((stmt (sqlite-prepare db path-id-sql #:cache? #t)))
+  (let ((stmt (sqlite-prepare
+               db
+               "
+SELECT id FROM ValidPaths WHERE path = :path"
+               #:cache? #t)))
     (sqlite-bind-arguments stmt #:path path)
     (let ((result (sqlite-fold cons '() stmt)))
       (sqlite-finalize stmt)
@@ -192,14 +193,6 @@  (define* (path-id db path)
         ((#(id) . _) id)
         (_ #f)))))
 
-(define update-sql
-  "UPDATE ValidPaths SET hash = :hash, registrationTime = :time, deriver =
-:deriver, narSize = :size WHERE id = :id")
-
-(define insert-sql
-  "INSERT INTO ValidPaths (path, hash, registrationTime, deriver, narSize)
-VALUES (:path, :hash, :time, :deriver, :size)")
-
 (define-inlinable (assert-integer proc in-range? key number)
   (unless (integer? number)
     (throw 'wrong-type-arg proc
@@ -222,14 +215,28 @@  (define* (update-or-insert db #:key path deriver hash nar-size time)
 
   (let ((id (path-id db path)))
     (if id
-        (let ((stmt (sqlite-prepare db update-sql #:cache? #t)))
+        (let ((stmt (sqlite-prepare
+                     db
+                     "
+UPDATE ValidPaths
+SET hash = :hash,
+    registrationTime = :time,
+    deriver = :deriver,
+    narSize = :size
+WHERE id = :id"
+                     #:cache? #t)))
           (sqlite-bind-arguments stmt #:id id
                                  #:deriver deriver
                                  #:hash hash #:size nar-size #:time time)
           (sqlite-fold cons '() stmt)
           (sqlite-finalize stmt)
           (last-insert-row-id db))
-        (let ((stmt (sqlite-prepare db insert-sql #:cache? #t)))
+        (let ((stmt (sqlite-prepare
+                     db
+                     "
+INSERT INTO ValidPaths (path, hash, registrationTime, deriver, narSize)
+VALUES (:path, :hash, :time, :deriver, :size)"
+                     #:cache? #t)))
           (sqlite-bind-arguments stmt
                                  #:path path #:deriver deriver
                                  #:hash hash #:size nar-size #:time time)
@@ -237,13 +244,15 @@  (define* (update-or-insert db #:key path deriver hash nar-size time)
           (sqlite-finalize stmt)
           (last-insert-row-id db)))))
 
-(define add-reference-sql
-  "INSERT OR REPLACE INTO Refs (referrer, reference) VALUES (:referrer, :reference);")
-
 (define (add-references db referrer references)
   "REFERRER is the id of the referring store item, REFERENCES is a list
 ids of items referred to."
-  (let ((stmt (sqlite-prepare db add-reference-sql #:cache? #t)))
+  (let ((stmt (sqlite-prepare
+               db
+               "
+INSERT OR REPLACE INTO Refs (referrer, reference)
+VALUES (:referrer, :reference)"
+               #:cache? #t)))
     (for-each (lambda (reference)
                 (sqlite-reset stmt)
                 (sqlite-bind-arguments stmt #:referrer referrer