java.jdbc

Clean up syntax for insert row / insert multiple row to improve options handling

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

Version 0.5.5 introduced a single options map, flagged by the :options argument. This is a compromise while I figure out the better way to change insert! to be a better behaved function, instead of being inherently variadic.

Activity

Hide
Sean Corfield added a comment -

This is a fair bit more tricky than create-table-ddl because there are several arities and argument combinations supported by a single function today (which, in hindsight, was clearly a bad idea!):

(insert! db table {:a row :of data}) ; the simplest case
(insert! db table {:first row :of data} {:second row :of data} {:etc etc} ...) ; insert an arbitrary number of rows
(insert! db table [:col1 :col2 :col3] [val1-1 val1-2 val1-3] [val2-1 val2-2] [val2-3] ...) ; insert an arbitrary number of rows by ordered columns

In that third case, the column names can be omitted – nil can be passed instead of that vector.

In addition to all of that, we want to be able to optionally pass an options map at the end.

For the multiple row cases, we could require the rows to be a sequence of maps (for case 2 above) or a sequence of vectors (for case 3):

(insert! db table [{:first row :of data} {:second row :of data} {:etc etc} ...]) ; insert an arbitrary number of rows
(insert! db table [:col1 :col2 :col3] [[val1-1 val1-2 val1-3] [val2-1 val2-2] [val2-3] ...]) ; insert an arbitrary number of rows by ordered columns

We can distinguish those cases easily enough:

(defn insert!
  ([db table rows] ...)
  ([db table cols values] ...))

We can even add the optional map of options:

(defn insert!
  ([db table rows] ...)
  ([db table cols-or-rows values-or-opts]
    (if (map? values-or-opts)
      ;; insert db table rows opts
      (insert-rows db table cols-or-rows values-or-opts)
      ;; insert db cols values opts
      (insert-cols db table cols-or-rows values-or-opts {})))
  ([db table cols values opts]
    (insert-cols db table cols values opts)))

This ignores the single row case, but we can add that in and we're still fine, although the logic is getting ugly:

(defn insert!
  ([db table row-or-rows] ; this case is ok
    (if (map? row-or-rows)
      ;; insert db table row opts
      (insert-one-row db table row-or-rows {})
      ;; insert db table rows opts
      (insert-rows db table row-or-rows {})))
  ([db table cols-or-rows values-or-opts]
    (if (map? values-or-opts)
      (if (map? cols-or-rows)
        ;; insert db table row opts
        (insert-one-row db table cols-or-rows values-or-opts)
        ;; insert db table rows opts
        (insert-rows db table cols-or-rows values-or-opts))
      ;; insert db cols values opts
      (insert-cols db table cols-or-rows values-or-opts {})))
  ([db table cols values opts]
    (insert-cols db table cols values opts)))

However, we cannot also maintain complete backward compatibility with the current calling sequence because of this case:

(insert! db table {:first row :of data} {:second row :of data})

We can disambiguate all other forms of the combined old and new syntax (we can tell sequence of rows from sequence of cols by (map? (first rows-or-cols))) and for the old columns syntax, we have repeated vectors optionally followed by a keyword - either deprecated options unrolled or the :options flag.

We could assume that the second map is options if a) it is empty or b) contains only keys from the set that insert! allows, namely :transaction? and :entities. That might conflict with inserting into a table with a column named entities where all the other columns were optional (are there databases where transaction? would be a valid column name?), so we would additionally need to check that the entities value was something function-like...

And we'd still need to support keywords in the argument list, even if they are deprecated, until 0.6.0 comes along.

Show
Sean Corfield added a comment - This is a fair bit more tricky than create-table-ddl because there are several arities and argument combinations supported by a single function today (which, in hindsight, was clearly a bad idea!):
(insert! db table {:a row :of data}) ; the simplest case
(insert! db table {:first row :of data} {:second row :of data} {:etc etc} ...) ; insert an arbitrary number of rows
(insert! db table [:col1 :col2 :col3] [val1-1 val1-2 val1-3] [val2-1 val2-2] [val2-3] ...) ; insert an arbitrary number of rows by ordered columns
In that third case, the column names can be omitted – nil can be passed instead of that vector. In addition to all of that, we want to be able to optionally pass an options map at the end. For the multiple row cases, we could require the rows to be a sequence of maps (for case 2 above) or a sequence of vectors (for case 3):
(insert! db table [{:first row :of data} {:second row :of data} {:etc etc} ...]) ; insert an arbitrary number of rows
(insert! db table [:col1 :col2 :col3] [[val1-1 val1-2 val1-3] [val2-1 val2-2] [val2-3] ...]) ; insert an arbitrary number of rows by ordered columns
We can distinguish those cases easily enough:
(defn insert!
  ([db table rows] ...)
  ([db table cols values] ...))
We can even add the optional map of options:
(defn insert!
  ([db table rows] ...)
  ([db table cols-or-rows values-or-opts]
    (if (map? values-or-opts)
      ;; insert db table rows opts
      (insert-rows db table cols-or-rows values-or-opts)
      ;; insert db cols values opts
      (insert-cols db table cols-or-rows values-or-opts {})))
  ([db table cols values opts]
    (insert-cols db table cols values opts)))
This ignores the single row case, but we can add that in and we're still fine, although the logic is getting ugly:
(defn insert!
  ([db table row-or-rows] ; this case is ok
    (if (map? row-or-rows)
      ;; insert db table row opts
      (insert-one-row db table row-or-rows {})
      ;; insert db table rows opts
      (insert-rows db table row-or-rows {})))
  ([db table cols-or-rows values-or-opts]
    (if (map? values-or-opts)
      (if (map? cols-or-rows)
        ;; insert db table row opts
        (insert-one-row db table cols-or-rows values-or-opts)
        ;; insert db table rows opts
        (insert-rows db table cols-or-rows values-or-opts))
      ;; insert db cols values opts
      (insert-cols db table cols-or-rows values-or-opts {})))
  ([db table cols values opts]
    (insert-cols db table cols values opts)))
However, we cannot also maintain complete backward compatibility with the current calling sequence because of this case:
(insert! db table {:first row :of data} {:second row :of data})
We can disambiguate all other forms of the combined old and new syntax (we can tell sequence of rows from sequence of cols by (map? (first rows-or-cols))) and for the old columns syntax, we have repeated vectors optionally followed by a keyword - either deprecated options unrolled or the :options flag. We could assume that the second map is options if a) it is empty or b) contains only keys from the set that insert! allows, namely :transaction? and :entities. That might conflict with inserting into a table with a column named entities where all the other columns were optional (are there databases where transaction? would be a valid column name?), so we would additionally need to check that the entities value was something function-like... And we'd still need to support keywords in the argument list, even if they are deprecated, until 0.6.0 comes along.
Hide
Sean Corfield added a comment -

We could also just deprecate insert! altogether and introduce insert-one! and insert-multi! but that breaks the asymmetry of the other methods.

Note that execute! already has a slightly strange calling sequence for applying a SQL statement multiple times:

(execute! db ["some SQL string" first set of data]) ; default of {:multi? false}
(execute! db ["some SQL string" [first set of data] [second set of data]] {:multi? true})

That's required because a SQL parameter can have an array value so we need to distinguish between those cases explicitly – and it wouldn't help with insert! since we can tell the difference between a single row and a series of rows. However, it does highlight that when inserting values by column, the proposed sequence of value vectors isn't entirely safe since you might be trying to insert such an array-valued SQL parameter

So perhaps a better path here is to deprecate the multi-row version of insert! and use it purely for single row insertions, and then add insert-multi! for the multiple row case – and it would require sequence of row values. This would still require some disambiguation logic for the arity-4 case but it would be simple, like the original [db table cols-or-rows values-or-opts] case above. This approach would mean that insert-multi! would not need to support any legacy (deprecated) cases and therefore could be "clean".

It wouldn't address the ambiguity of (insert! db table map-1 map-2) for legacy support but that horribleness would only be temporary until 0.6.0 came along.

Show
Sean Corfield added a comment - We could also just deprecate insert! altogether and introduce insert-one! and insert-multi! but that breaks the asymmetry of the other methods. Note that execute! already has a slightly strange calling sequence for applying a SQL statement multiple times:
(execute! db ["some SQL string" first set of data]) ; default of {:multi? false}
(execute! db ["some SQL string" [first set of data] [second set of data]] {:multi? true})
That's required because a SQL parameter can have an array value so we need to distinguish between those cases explicitly – and it wouldn't help with insert! since we can tell the difference between a single row and a series of rows. However, it does highlight that when inserting values by column, the proposed sequence of value vectors isn't entirely safe since you might be trying to insert such an array-valued SQL parameter So perhaps a better path here is to deprecate the multi-row version of insert! and use it purely for single row insertions, and then add insert-multi! for the multiple row case – and it would require sequence of row values. This would still require some disambiguation logic for the arity-4 case but it would be simple, like the original [db table cols-or-rows values-or-opts] case above. This approach would mean that insert-multi! would not need to support any legacy (deprecated) cases and therefore could be "clean". It wouldn't address the ambiguity of (insert! db table map-1 map-2) for legacy support but that horribleness would only be temporary until 0.6.0 came along.
Hide
Sean Corfield added a comment -

Implemented in 0.5.6. Clojure Guides updated. Release queued.

Show
Sean Corfield added a comment - Implemented in 0.5.6. Clojure Guides updated. Release queued.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: