weavejester / ragtime

Database-independent migration library
Eclipse Public License 1.0
608 stars 85 forks source link

No such var: next.jdbc.default-options/wrapped-connection? #152

Closed geraldodev closed 2 years ago

geraldodev commented 2 years ago

Hi,

There is no more wrapped-connection? var in next-jdbc which forbids ragtime compilation.

image

weavejester commented 2 years ago

Looks like that function was removed in 1.2.772, which explains why it previously passed the tests.

It appears as if that function was internal and shouldn't have been used. I'm also not sure why next.jdbc/get-connection wasn't used in all cases of get-db-metadata.

geraldodev commented 2 years ago

I'm trying ragtime, Previously I've used mybatis-migrations, but it appears that ragtime allows the execute sql workflow without trying to abstract sql commands. Same concept, very nice.

weavejester commented 2 years ago

Ah, I see why wrapped-connection? was used now. Unfortunately there doesn't seem to be an easy way of getting a Connection object from everything the next.jdbc functions accept. In execute-batch!, next.jdbc seems to do it manually as well:

   (if (or (instance? java.sql.Connection connectable)
           (and (satisfies? p/Connectable connectable)
                (instance? java.sql.Connection (:connectable connectable))))
     (with-open [ps (prepare connectable [sql] opts)]
       (execute-batch! ps param-groups opts))
     (with-open [con (get-connection connectable)]
       (execute-batch! con sql param-groups opts)))

So my guess is that we'll need to do a similar thing in Ragtime:

(defn- get-db-metadata [datasource]
  (cond
    (instance? java.sql.Connection datasource)
    (get-db-metadata* datasource)

    (instance? java.sql.Connection (:connectable datasource))
    (get-db-metadata* (:connectable datasource))

    :else
    (with-open [conn (jdbc/get-connection datasource)]
      (get-db-metadata* conn))))
seancorfield commented 2 years ago

The problem was that a) wrapped-connection? was wrong and b) it was added as specific to DefaultOptions (which it shouldn't have been -- and that whole ns is marked ^:no-doc so nothing in it should be depended on).

I'm open to adding next.jdbc.connection/wrapped-connection? as an official part of the API if it would make your life easier? Feel free to create a GH issue for it.

weavejester commented 2 years ago

Thanks for the feedback, @seancorfield. I'm unsure what the best solution would be, as I don't have a good understanding of how next.jdbc is meant to be used in this case.

The execute-batch! function accepts a Connection, a Connectable, or a map with a :connectable key that it expects to be a Connection. So if I want to write a function that operates on the same input, then I also need to write a similar set of logic to pull out the connection. Is this correct? Or have I missed an easier way of doing things?

seancorfield commented 2 years ago

It depends on what that function is doing -- I haven't looked at your source code -- but mostly you can just pass things through to next.jdbc and it will "do the right thing". Based on the snippet above, I'm assuming you're (only) doing this for fetching metadata, since there's nothing built into next.jdbc right now for that, and it needs an actual Connection?

If that is the case then, yes, you need to do what execute-batch! is doing because there's no generic way to deal with that in next.jdbc. I'm contemplating an on-connection macro that handles this but I haven't finished hammocking that idea yet.

seancorfield commented 2 years ago

I just took a look at the code and, yeah, if there was some built-in on-connection API, you'd be able to use that instead of sniffing the types yourself. I'll take a look at that this week...

weavejester commented 2 years ago

Based on the snippet above, I'm assuming you're (only) doing this for fetching metadata

Yep, that's exactly right.

seancorfield commented 2 years ago

com.seancorfield/next.jdbc {:mvn/version "1.2.780"} is available on Clojars with next.jdbc/on-connection (macro) to allow for this sort of code without needing to sniff the type. I also updated the docs about working with metadata with an example of on-connection.

rrudakov commented 2 years ago

Hello. Is this fix not released yet? I'm still facing this error with version 0.9.1

weavejester commented 2 years ago

Released 0.9.2.