Database insert function taking Phel map

[jasalt]

It would be nice to have a simple function that inserts a map representing column names with their row values into DB. I'm not fully sure how it should be secured against injection attacks and such but prepared statements seem to offer some protection. Quick example how it might work:

(defn pdo-insert-map
    [conn table m]
    (let [m-keys (keys m)
         column-names (str/join ", " (map name m-keys))
         placeholders (str/join ", " m-keys)
         stmt-sql (str "INSERT INTO " table " (" column-names ") "
                       "VALUES (" placeholders ")")
         stmt (pdo/prepare pdo-conn stmt-sql)]

		 (dofor [[k v] :pairs m]
				(statement/bind-value stmt k v))

		 (php/-> (stmt :stmt) (execute))))

(pdo-insert-map pdo-conn "my_table"
                {:uuid   "testuuid11"
                :create_date "2024-01-01 11:11:11"          
                :order_id   234
                :product_id 345
                :qty        5
                })

It does not allow re-using same statement over multiple calls and does not set types explicitly (if needed?) but maybe it could be split up into parts that would simplify the insertion process while helping with security.

[jasalt]

Return value of (php/-> (stmt :stmt) (execute)) is true even if any rows are not affected so my example seems flawed but returning value of (php/-> (stmt :stmt) (rowCount)) instead seems to be more useful or maybe (pdo/last-insert-id conn) even better in case of function doing singular inserts.

That would be more inline with other tools also, wpdb->insert() as example that I'm replacing with PDO in some code as I find wpdb functions lacking in other ways (e.g. wpdb::get_results() not casting to PHP types while reading, does not throw exceptions on failure as expected but returning error info as HTML, leans on global variable use etc.).

[jasalt]

Clojure JDBC library function https://cljdoc.org/d/com.github.seancorfield/next.jdbc/1.3.981/api/next.jdbc.sql#insert! would be a good role model I think. "Given a connectable object, a table name, and a data hash map, inserts the data as a single row in the database and attempts to return a map of generated keys.".

[smeghead]

@jasalt
Thank you!

Currently, phel-pdo is positioned as a simple PDO wrapper library.

PDO does not take on the role of building SQL, which is essentially what it does.

The ability to pass a table name and map and insert is in the ORM category, so it may be better suited to be implemented as an ORM library in a separate library.

What do you think?

[jasalt]

@smeghead right, I see there some SQL generation / driver wrapping separation made between next.jdbc and honeysql (by same author) but those projects have long history already. With that and possible PDP vs JDBC differences etc, I don't disagree that this library should possibly have more narrow focus compared to next.jdbc to get started with.

[jasalt]

The ability to pass a table name and map and insert is in the ORM category, so it may be better suited to be implemented as an ORM library in a separate library.

With this earlier pdo-insert-map function, as it uses a prepared statement (mainly for security) it needs to handle the PDO object, and cannot simply produce a SQL string to be passed for execution. Also, I see that functionality like this in next.jdbc is separated from main jdbc.clj ns into sql.clj.

Maybe a separate minimal "ORM library" would include some functions such as in sql.clj that would wrap phel-pdo library, named "phel-pdo-sql", hehe.

[jasalt]

Also noted some differences between JDBC and PDO when mimicking the next.jdbc insert! in that PDO does not provide similar Java getGeneratedKeys method I understood is used in next.jdbc src to return a map of generated keys from insert statement execution. In PDO I only find lastInsertId and it would require extra query using that to have similar return value as with the next.jdbc's function. So similar return behavior would not make sense as default at least, going with returning lastInsertId seems best.

[smeghead]

@jasalt
I agree that if you are basing the PDO, it is appropriate that the return value of the function to insert! should be the lastInsertId.

The table parameter in the pdo-insert-map above will be concatenated directly into the SQL statement as a string.
I think it needs validation or escaping to prevent injection.

[jasalt]

The table parameter in the pdo-insert-map above will be concatenated directly into the SQL statement as a string.
I think it needs validation or escaping to prevent injection.

@smeghead yes, I indeed skipped that after not finding a similar feature as with parameter escaping with statements.

Looking at next-jdbc, there's check for ; character https://github.com/seancorfield/next-jdbc/blob/56bd2356ac542ff6c3667aec24cb0207dafe4e40/src/next/jdbc/sql/builder.clj#L23, maybe similar would be feasible.

Otherwise legal table names would need to be queried before actual query is made which seems to get complicated as there doesn't seem to be a standard query for getting table info. PDO::ATTR_DRIVER_NAME could give hint for which SQL query syntax to use for which DB but then many require a database name in the table info SQL query which PDO connection object cannot provide.