metabase / toucan Goto Github PK

A classy high-level Clojure library for defining application models and retrieving them from a DB

License: Eclipse Public License 1.0

Emacs Lisp 0.46% Clojure 99.33% Shell 0.21%

toucan hydration honeysql clojure orm metabase flexible sql

toucan's Issues

Better support for objects that don't have a primary key called :id

I think we could add a method to IModel called primary-key or something like that that could be used to get field(s) that should be used as the primary key (default implementation would return :id). It could accept either a keyword (for a single-column key) or a sequence of keywords for multiple-column keys.

Need tests for a model with byte array PK

should be possible to automatically convert between hex (in Clojure) and byte array in DB. Currently it seems like certain functions like update! don't support this.

Should atoms in toucan.hydrate flushed after defmodel?

When we change models, we may change hydration-keys, this change will not work unless we reload toucan.hydrate namespace. This looks inconvenient in development.

Since automagic-batched-hydration-key->model and automagic-batched-hydration-keys are delays, why not reload them after each time we call defmodel? maybe there's a better way for this?

Migrations?

Hi,

How do you recommend doing migrations with toucan?
Are we supposed to use a separate library for that?

@camsaul

Any Plans or Interest in Moving to HoneySQL 2?

Hi @camsaul!

I'm working on a project that uses Toucan and HoneySQL 2.x. I've run into a couple bugs where people try to feed a structured HoneySQL 2 query into Toucan. It's not a huge deal, but...

I was wondering: are there any plans (or even interest) in moving to HoneySQL 2?

Rollbacks in db/transaction

@camsaul I have been trying to use transactions with toucan, however I noticed with test my transaction are not being rolled back.

In my case i am running tests and wanting the data to not be committed and just be rolled back at the end, I am using this code.

   (db/transaction
    (jdbc/db-set-rollback-only! (db/connection))
   (db/insert! User :username "test_user" :password "test" :email "[email protected]")
    (f))

Digging into the src i noticed the rollback code was missing, is this intentional or any possibility of support rollback when things fail, I am not sure of the implications of just dropping in the rollback expression into the db/transact wrapper.

Working with Postgres Jsonb

I'm trying to integrate Toucan into a project that uses Postgres, but I can't insert into a Jsonb column. Here's the model:

(m/add-type! :json
  :in json/generate-string
  :out #(json/parse-string % keyword)) 

(m/defmodel Integration :integrations
  m/IModel
  (m/types [_]
    {:data :json}))

And here's the error:

user=> (db/insert! Integration {:registration "123" :data {"a" "b"}})
(db/insert! Integration {:registration "123" :data {"a" "b"}})
2019-04-09 19:04:13,218 [nRepl-session-f8c664bb-fa71-4a5c-b061-be9ef18ff331] DEBUG toucan.db - DB Call: INSERT INTO "integrations" ("registration", "data") VALUES (?, ()) 
Execution error (PSQLException) at org.postgresql.core.v3.QueryExecutorImpl/receiveErrorResponse (QueryExecutorImpl.java:2477).
ERROR: syntax error at or near ")"
  Position: 66

I've followed the docs to the letter, but it doesn't work. Thanks!

@camsaul

Select query does not work with sub-query

@camsaul

Example query:

(db/select [User :first-name :last-name]
            :id [:in {:select [:id] :from [User]}]
            {:order-by [:id]})

Error:

  act-msg: exception in actual: (db/select [User :first-name :last-name] :id [:in {:select [:id], :from [User]}] {:order-by [:id]})
    threw: class org.postgresql.util.PSQLException - ERROR: column "select" does not exist
  Position: 80
           org.postgresql.core.v3.QueryExecutorImpl$receiveErrorResponse (QueryExecutorImpl.java:2497)
           org.postgresql.core.v3.QueryExecutorImpl$processResults (QueryExecutorImpl.java:2233)
           org.postgresql.core.v3.QueryExecutorImpl$execute (QueryExecutorImpl.java:310)
           org.postgresql.jdbc.PgStatement$executeInternal (PgStatement.java:446)
           org.postgresql.jdbc.PgStatement$execute (PgStatement.java:370)
           org.postgresql.jdbc.PgPreparedStatement$executeWithFlags (PgPreparedStatement.java:149)
           org.postgresql.jdbc.PgPreparedStatement$executeQuery (PgPreparedStatement.java:108)
           clojure.java.jdbc$execute_query_with_params$invokeStatic (jdbc.clj:1079)
           on (jdbc.clj:1073)
           clojure.java.jdbc$db_query_with_resultset_STAR_$invokeStatic (jdbc.clj:1102)
           on (jdbc.clj:1082)
           clojure.java.jdbc$query$invokeStatic (jdbc.clj:1171)
           on (jdbc.clj:1133)
           toucan.db$query$invokeStatic (db.clj:308)
           toucan.db$query$doInvoke (db.clj:304)
           toucan.db$simple_select$invokeStatic (db.clj:414)
           on (db.clj:403)
           toucan.db$select$invokeStatic (db.clj:708)
           toucan.db$select$doInvoke (db.clj:702)

This issue was introduced in #87. A variant of this was reported in #88 and fixed in #89.

A workaround is to use an explicit {:where [:in :id {:select [:id] :from [User]}]} instead.

apply-type-fns should give better error message if type does not exist

Right now it just throws confusing NullPointerException.

No implementation of method: :post-insert of protocol: #'toucan.models/IModel found for class: nil

@camsaul

based on metabase/metabase#6295 and my own experience trying to get Metabase to connect to a pgpool-managed Postgresql cluster, it seems like there is a problem with how Toucan builds SQL queries, which leads to the above error when such queries are executed against pgpool.

Maybe Toucan is trying to leverage a feature of Postgres that pgpool doesn't support?

Or perhaps Toucan is working correctly but Metabase is not implementing it right?

Add ns qualified keyword options to IModel

@camsaul

Qualified keywords are pretty common in Clojure, and either inferring them from table names or providing them per-model would be nice to have.

I'm using next.jdbc right now, and it's handling a lot of that for me.

Update! with empty map should be no-op

Right now it just throws a potentially confusing error

Support invoking models via apply

e.g.

(apply User [:first-name "Cam", :last-name "Saul"])

Looks like we need to implement clojure.lang.IFn/applyTo

(db/select Model :f [:between v1 v2]) doesn't work

This should be translated to a HoneySQL clause like [:between f v1 v2] but instead it's throwing an ArityException. Look into this and fix

Model instance to map

hi @camsaul, I'm new to clojure, I'm trying to use toucan to build a rest api, but I can't find a way to respond to a request with a map that represents a record on my database, I have the following code:

(defn dbh [req]
  (let [user (User 3)]
    {:status 200
     :headers {"Content-Type" "application/json"}
     :body user  }))

but when I make a request i just get what seems to me (I don't know clojure) like a sequence of pairs

[:id 3][:name "wayne"][:email "[email protected]"][:password "ImFzZGFzZCI=.5lcSa6XsqitTjnXVD3BSGptcVbQ="]

could you please point me to the right direction to get a proper response?

Will not use Transaction when call db/insert with a binding to db-connection

@camsaul
We found when call db/transaction with a binding db-connection, it will never work, since the
connection function will always return db-connection. What I suggest is to adjust the priority to move transaction-connection to the top. I do some testing in my local env, it works.
(defn connection []
(or transaction-connection ;;fix the bug move transaction to the top, otherwise transaction will not work.
db-connection
@default-db-connection
(throw (Exception. "DB is not set up. Make sure to call set-default-db-connection! or bind db-connection.")))))))

Support (empty) on models

Something like

(empty User)

currently doesn't work.

Allow DB to be passed as an argument to models

Can the database be passed as an argument to the models, or does Toucan only support the database connection as a global var?

Add an `upsert!` function

Right now to do an "upsert" operation you have to do

(or (db/update! ...) ; update! returns truthy value iff something was updated
    (db/insert!))

However, that is subject to race conditions 🙀

It would be nice to simplify this common pattern and avoid the race conditions with an upsert! function or something like that. (suggestion credit: @thearthur)

Scanning for hydration keys does a keyword call on every var

The require-model-namespaces-and-find-hydration-keys function loops over every var in every namespace to check if it's a model. To do this it calls :toucan.models/model (via toucan.models/model?) on the var's value.

(:toucan.models/model val)

This is problematic since that value could be anything, and calling it like this with a keyword could have all kinds of side effects.

To make this concrete, say Datomic is present and loaded in the same process as Toucan, then it will call

(toucan.models/model? datomic.pull/normalized-pattern-cache)

Which raises an exception, making Toucan unusable.

@camsaul I'm not sure what to propose as the best solution here, in my project I've monkey-patched require-model-namespaces-and-find-hydration-fns to call (and (record? model) (models/model? model)). Would that be acceptable?

Have update-where! perform type conversions and call `pre-update` methods

We could add a new function called simple-update-where! that keeps the current functionality (doing neither).

Add ability to designate a function for DB setup & automatically run it

Slightly inconvenient that we have to call appropriate DB setup functions when using from the REPL, e.g. in Metabase:

(metabase.db/setup-db-if-needed!)

It would be more convenient if we could tag a certain function as the DB setup function:

(defn ^:toucan/setup-fn setup-db-if-needed! []
 ...)

and have Toucan automatically find and run this function using the namespace tools if the DB is not yet set up.

Fix the error message "DB is not setup" -> "DB is not set up"

Simple fix

Select queries do not work with custom types

Hi @camsaul! A regression issue was introduced with the latest library version upgrade, I guess. (Or probably I didn't notice this one before.) Select queries simply do not work with custom types, e.g. native PostgreSQL ENUM type. There is an exception that happens during the where+ clause construction.

class java.lang.ClassCastException - class clojure.lang.PersistentVector cannot be cast to class clojure.lang.Named (clojure.lang.PersistentVector and clojure.lang.Named are in unnamed module of loader 'app')
           toucan.test_models.pg_enum$kwd__GT_pg_enum$invokeStatic (pg_enum.clj:17)
           on (pg_enum.clj:15)
           toucan.models$do_type_fn$invokeStatic (models.clj:358)
           on (models.clj:354)
           toucan.db$where_PLUS_$invokeStatic (db.clj:463)
           on (db.clj:453)
           toucan.db$select_one$invokeStatic (db.clj:628)
...

So the fact is the current where+ fn implementation doesn't cater for the things like [:= <custom_type_value>].

Will provide you with a fix soon. Had to develop one during the weekend.

Cheers!

Add lazy/streaming versions of functions like select

When fetching huge numbers of rows this would certainly be a nice option to have. Whether we take advantage of DB cursors, or perhaps come up with some method to do a series of requests with paging, or some other method, this would prevent us from having to drop down to low-level clojure.java.jdbc queries for these sorts of things.

Params in query call possible ?

I have been looking at the code here

toucan/src/toucan/db.clj

Line 284 in e8fbd6d

(defn query

from what I can tell query does not take :params to pass in parameters ? or else I am not understanding whats going on.

Tried a few things like

(db/query honey-sql-query :search "text")

But this does not work, the doc string says it takes a honey sql form which i Take it is just the hash map form before its passed to sql/format which has a :params keyword to allow you to injected name params into your query.

@camsaul is this possible am i missing something, or would toucan need to be extended to allow for this ?

Transactions do not work with db-connection

The way I'm using Toucan is that I never call set-default-db-connection! and instead I always set *db-connection*. I'm using Integrant and explicitly passing the database connection fits that model better than having global state.

I just noticed that transactions do not work with *db-connection* – if you use toucan.db/transaction, the code inside the macro will not be run inside a transaction. This is because toucan.db/connection always returns *db-connection* if it's set:

toucan/src/toucan/db.clj

Lines 124 to 127 in c5567c8

    
           (or *db-connection* 
        
               *transaction-connection* 
        
               @default-db-connection 
        
               (throw (Exception. "DB is not set up. Make sure to call set-default-db-connection! or bind *db-connection*."))))

I'm not sure if this is a bug or an intentional feature, but I certainly expected transactions to work. As a workaround, I'm now binding toucan.db/*transaction-connection* (which is marked as private) instead of *db-connection* and that works fine.

Use multimethods for custom hydration functions

Instead of tagging them with metadata it would be better if we could make them multimethods that dispatch off of the model as well as the key they are hydrating. That way different methods could be used to hydrate the same key for instances of different models. The approach also seems a little cleaner than having hydrate look through all namespaces for functions with matching metadata

There are two things that might make this tricky:

The difference between single hydration and batched hydration functions would have to be addressed somehow (perhaps with two multimethods?)
This change would require reworking things a bit for people using the library

[Question] Ignore case on select

Hi,

Is there any way to run a query ignoring uppercase and lowercase? Or use an upper(filde) = upper(value)?

Thanks

Update! does not work with non-integer ids

@camsaul

When testing my application I get at java.lang.AssertionError: Assert failed: (integer? id) for toucan.db/update!. The :pre condition assumes the id is integer, however we use UUIDs.

I've created a PR: #19

Also, I can not run the tests locally. I get a org.postgresql.util.PSQLException: The server requested password-based authentication, but no password was provided. error. I think a local testing db is needed (a toucan_test database?). I'd recommend setting up a global fixture that creates the db if required.

Also, switch to CircleCI 2.0 https://circleci.com/docs/2.0/migrating-from-1-2/, then the same docker container used for the testing database could be run locally.

Fix docs to clarify that `update-where!` doesn't call pre-update (or post-update)

https://github.com/metabase/toucan/blob/master/docs/db-functions.md#update-where says update-where does not call pre-update while https://github.com/metabase/toucan/blob/master/docs/defining-models.md#pre-update says pre-update gets called, while post-update does not.

Toucan put a string as table name in sql using mysql

Hi @camsaul,
I'm playing with Toucan, connecting to a mysql db.
I configured Toucan to work with hikari datasource and mount:

(ns reviews.database
  (:require [hikari-cp.core :as h]
            [mount.core :refer [defstate]]
            [toucan.models :refer [set-root-namespace!]]
            [toucan.db :refer [set-default-db-connection!]]))

;move to configuration
(def datasource-options {:auto-commit        true
                         :read-only          false
                         :connection-timeout 30000
                         :validation-timeout 5000
                         :idle-timeout       600000
                         :max-lifetime       1800000
                         :minimum-idle       1
                         :maximum-pool-size  10
                         :pool-name          "db-pool"
                         :adapter            "mysql"
                         :username           "root"
                         :password           "root"
                         :database-name      "reviews-app"
                         :server-name        "localhost"
                         :port-number        3306
                         :register-mbeans    false})

(defn- create-datasource []
    (doto {:datasource (h/make-datasource datasource-options)}
          (set-default-db-connection!)))

(defstate database 
    :start (create-datasource)
    :stop (h/close-datasource (database :datasource)))

(set-root-namespace! 'reviews.models)

Having my model:

(ns reviews.models.review
    (:require [toucan.models :refer [defmodel]]))

(defmodel Review :reviews)

When I do:

user=> (t/debug-print-queries 
         (t/select 'Review))
;output
; {:select [:*],
;  :from [{:table :reviews, :name "Review", :toucan.models/model true}]}
; nil 
; SELECT *
; FROM "reviews"
; null

I get the following exception:

Execution error (MySQLSyntaxErrorException) at
jdk.internal.reflect.NativeConstructorAccessorImpl/newInstance0 (NativeConstructorAccessorImpl.java:-2).

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '"reviews"' at line 1
class com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException

The issue is that it writes the table name as "reviews" instead of review.
Have I misconfigured something or is this some sort of bug?

Hydrate many to many relation with intermediate table.

Hi @camsaul , I was wondering how should one do to hydrate a many to many relation with an intermediate table?

Setting :identifiers for JDBC connection is broken with toucan.db/query in 1.13.0

We're setting :identifiers for our JDBC connection to a function that converts underscores to dashes. However, Toucan 1.13.0 introduced the this change to toucan.db/query which overrides our identifiers function.

Now our underscores do not get transformed to dashes, which of course breaks many things, and there's no workaround. I think that the best thing would be to not set :identifiers here and just let clojure.java.jdbc handle it once the version with the fix has been released.

@camsaul

Allow extending protocols right from defmodel

The current way of specifying hydration keys is rather unwieldy, so I came up with this

(defmodel Video :videos
  models/IModel
  (hydration-keys [_]
    [:video :preview_video]))

I have a working patch (see this gist). Happy to submit it, just wanted to check first if this is something you'd be interested in.

Make sure `hydrate` works with fields that end in `-id`

Should insert-many! call post-insert on inserted objects?

Seems like this makes sense since it calls pre-insert. People can always use simple-insert-many! instead to avoid this behavior

apply-type-fns does not apply the function if the value is false

Hi @camsaul

I am attempting to run a type function, when the column value is false the function does not run, as per below.

(toucan.models/add-type! :demo
                         :in (fn [val]
                               (println "Demo type called")
                               (let [n-val (if val "True Transformed" "False Transformed")]
                                 n-val))
                         :out boolean)


(extend (class Venue)
  toucan.models/IModel
  (merge toucan.models/IModelDefaults {:default-fields (constantly #{:id :name :category})
                                :types          (constantly {:category :keyword
                                                             :demo :demo
                                                             }
                                                            )
                                :properties     (constantly {:timestamped? true})}))

(clojure.test/is (=
 #toucan.test_models.venue.VenueInstance{:demo "True Transformed"}
 (#'toucan.models/apply-type-fns #toucan.test_models.venue.VenueInstance{:demo "A"} :in)))
;=> true
(clojure.test/is (=
 #toucan.test_models.venue.VenueInstance{:demo "False Transformed"}
 (#'toucan.models/apply-type-fns #toucan.test_models.venue.VenueInstance{:demo nil} :in)))
;=> false
(clojure.test/is (=
 #toucan.test_models.venue.VenueInstance{:demo "True Transformed"}
 (#'toucan.models/apply-type-fns #toucan.test_models.venue.VenueInstance{:demo true} :in)))
;=> true
(clojure.test/is (=
 #toucan.test_models.venue.VenueInstance{:demo "False Transformed"}
 (#'toucan.models/apply-type-fns #toucan.test_models.venue.VenueInstance{:demo false} :in)))
;=> false
`

Hydration Functions are Too Eager

I've come across a situation with lazy model loading not updating simple hydration functions.

Define three models (user, permissions, company)
The company model definition defines a simple hydration for users (in the same company.clj file).
The user model definition defines a simple hydration fo permissions (in the same user.clj file).

In the main execution path, we can select a single company, then invoke hydrate for users. The users model will be lazily loaded and hydrated. Awesome.

During the loading of users, a new ^:hydration function is loaded for the permissions. Unfortunately, the simple hydration method has already realized the delay; and thus doesn't know of the new permissions hydration function.

The fix I've come across is to put hydrations into a seperate file and ensure it's required by what needs it. This however feels counter intuitive when models can be resolved when required.

Thoughts?

insert! returns differently between models

Hi, so I have two models here, lets say one is Model A and the other one is Model B. When I insert! Model A, the function returns its identity, but when I insert! Model B, the returned value is nil.

The difference between this model is that Model A is using generated id from mysql, where Model B is using id composed in my application:

(defmodel B :b
  IModel
  (pre-insert [payload]
              (assoc payload
                     :id (.toString (java.util.UUID/randomUUID))))

=> (insert! A {:name "John Doe" :email "[email protected]"}
#AInstance{:id 1 :name "John Doe" :email "[email protected]"}

=> (insert! B {:name "John Doe" :email "[email protected]"}
nil

Is this behavior expected?

Does Toucan support enumerated types?

@camsaul My postgres db has an enumerated type. I see this possible hack for making that work in java-jdbc, but that looks messy and possibly destroys the type safety of the enum in the first place (not clear). I can probably live with just strings here, but any advice on making enums work with Toucan?

	(or db-connection
	transaction-connection
	@default-db-connection
	(throw (Exception. "DB is not set up. Make sure to call set-default-db-connection! or bind db-connection."))))

metabase / toucan Goto Github PK

toucan's Issues

Recommend Projects

Recommend Topics

Recommend Org