Lost scheme/driver reflection since v0.12.0?

[luislavena]

Hello!

While working on drift project, I had an internal prototype that work with different DB drivers by inspecting #uri of the given connection.

You can see the commits I had from previous version of crystal-db here, specifically .klass_for

However, since 0.12.0 refactor, is not possible to determine the driver without having to load the different shards and compare the connection:

db.using_connection do |conn|
  p! conn.is_a?(SQLite3::Connection) # => true
end

This will force Drift to depend on all the other DB drivers, causing unneeded code to be parsed, compiled, etc.

I tried to look at connection_options and others, but appears that the scheme from URI is no longer kept.

Any suggestion on how to approach this? I received some interest in allowing Drift work with adapters other than SQLite3.

Thank you. ❤️ ❤️ ❤️

[bcardiff]

Is checking out a connection possible in your use case?

You want drift to support multiple drivers and the adapters for those to live in the same repo, right? It sounds like peer-dependencies/plugin design. Otherwise you end with drift-sqlite, drift-pg, etc.

I think that a way to allow these peer-dependencies is to use @top_level.has_constant? (see https://crystal-lang.org/reference/1.13/syntax_and_semantics/macros/index.html). With that you can include the adapters and generate a case (or ifs, in the previous link there are some pitfalls for case statements). For this to work you might need for drivers to be required before drift. Or maybe there is a workaround using macro finished hook, I'm less certain about this last bit.

Am I understanding the problem correctly?

[luislavena]

Is checking out a connection possible in your use case?

I can check the connection, but it no longer has uri for me to validate the schema, which forces me to use the macro approach you mention.

As you mention using macros forces to some awkward code (as you mention macros in the middle of case is not doable):

require "sqlite3"

def adapter_for(conn : DB::Connection)
  {% if @top_level.has_constant?("SQLite3") %}
    return "sqlite3" if conn.is_a?(SQLite3::Connection)
  {% end %}
  {% if @top_level.has_constant?("MySql") %}
    return "mysql" if conn.is_a?(MySql::Connection)
  {% end %}
  {% if @top_level.has_constant?("PG") %}
    return "postgresql" if conn.is_a?(PG::Connection)
  {% end %}
  raise ArgumentError.new(%{no adapter for connection "#{conn.class}"})
end

DB.open("sqlite3:app.db") do |db|
  db.using_connection do |conn|
    p! adapter_for(conn)
  end
end

$ crystal run 2.cr
adapter_for(conn) # => "sqlite3"

You want drift to support multiple drivers and the adapters for those to live in the same repo, right? It sounds like peer-dependencies/plugin design. Otherwise you end with drift-sqlite, drift-pg, etc.

Definitely I would like to avoid create an maintain different shards for 20-30 lines of code, as the project part of Drift that is adapter-specific is the SQL queries 😅

I was trying to avoid as much as possible macros to keep things simple, so I was trying to understand why URI was gone from the Driver/Database and no longer accessible at the connection level.

I'm honestly ok if this is the only way, but wanted to check first as I might be missing something (98% of the time what happens to me) 😁

Thank you again for your response and time! ❤️ ❤️ ❤️

[bcardiff]

The tension was that in order to allow arbitrary connection factories (like via tls tunneling) it was better to get rid of the assumption that the conection can be configured entirely from a uri.

That being said I think is reasonable to expect some driver_name property available to make this code easier. You will still be having something like peer dependencies because you expect drift to work with some shards if present. But as long as you only use crystal-db api there is no need to even conditionally require specific drivers for the adapters.

[bcardiff]

the non-macro way would be to just case on the class name.

Would that be enough?

db.using_connection do |conn|
  p! conn.class.name
end

[luislavena]

the non-macro way would be to just case on the class name.

🤦 x 💯

I was so focused on the trees that entirely missed the forest! Completely forgot I can rely on .class.name to obtain the name of the connection.

Definitely I can use that and avoid macros entirely.

Thank you again for your response and patience @bcardiff ❤️ ❤️ ❤️

[bcardiff]

Always a pleasure Luis ☺️.

I also missed the class.name first 😅. Relying on a magic string value and on a class name that would unlikely change is the same. Maybe check prefix for super extra stability (?)

When you think is right we can advertise drift and probably other database schema migrators in crystal-db for discovery.

[ysbaddaden]

There's still one advantage of accessing the scheme is to not tie the library to a specific adapter class name (it could change) when we want the SQL flavor.