Add sqlite support

[abuisman]

As promised a PR for making the gem work with multiple databases.

Consider this a proposal for the overall design. I believe it does work, but I haven't used it in any project yet.

I went for keeping all the postgres features working, while allowing the gem to use sqlite as well.

Let me know what you think.

Some design considerations:

loading query features in jsonb_accessor method

I include the query builder into the model in the jsonb_accessor method because I wanted to support multiple different databases inside of the same project. You could have a few models that are backed by postgres and some by sqlite. Including in jsonb_accessor makes more sense there. Also, it only includes the methods when you actually want the model to use jsonb_accessor.

conditional loading based on connection.adapter

I created two helper methods that check the model's connection to see which database adapter they use. For each module I have added a constant array that has a list of supported adapters so you can ask the module whether it supports the given adapter.

This is a bit much right now, I could've done it in the helper, but I see a future where QueryBuilder and AttributeQueryMethods can be replaced by database specific ones depending on which features we can implement for each database.

Sqlite in specs

I had a hard time figuring out how to use the db/config.yml to configure multiple databases and use the normal way to configure which connection a model uses with:

connects_to database: { writing: :primary, reading: :secondary }

So I've looked at the active_record source and found a way to establish the connection from within the model:

 Class.new(ActiveRecord::Base) do
      def self.name # active_record can't figure out the name itself because of the anonymous class
        "product"
      end

      root_dir = File.expand_path(__dir__)
      establish_connection adapter: "sqlite3", database: File.join(root_dir, "..", "db", "test.db")
      connection.create_table :products, force: true do |t|
         # ... 
      end

Specs

I've copy-pasted the postgres specs and removed all the specs for the attribute query methods.

[abuisman]

All activerecord 5 runs seem to fail because of missing json support for sqlite, but from what I've found, support for json columns was added in 5.

There is this gem that adds it for version 4: https://github.com/miriamtech/sqlite3_json_rails4

[haffla]

@abuisman this is looking great. Maybe you can add some kind of pre check for sqlite whether the Rails version supports json. If not you can raise a custom error and suggest the user to install the mentioned gem.

Edit: maybe not a good idea. If a user wants to use sqlite with an old Rails version they probably already have that gem installed. The error is not because of jsonb_accessor. So it isn't jsonb_accessors responsibility to establish compatibility with json.

Maybe write the tests like

if sqlite adapter supports json (alternatively compare Rails version)
  run tests
else
  expect to raise error
end

[abuisman]

@haffla Yes I agree with it not being jsonb_accessor's responsibility, good point. What I do find strange is that in that gem's readme it says that json support for sqlite was added from rails 5 onward, but that is the version that has failures here.

I'll try adding the gem and see what happens.

[abuisman]

Haha ok it won't work, the gem depends on active_record 4, etc.

So I agree with what you are saying, we should simply dismiss tests that run in situations that don't exist. Namely, json support for rails 5 with SQLite.

I'll see how I can elegantly set it up.

[haffla]

@abuisman you can use bundle exec appraisal rspec to run against all Rails versions on your local machine.

[abuisman]

@abuisman you can use bundle exec appraisal rspec to run against all Rails versions on your local machine.

Thanks! I did it another way an hour ago. But now I am in the forest 😄. I will fix it up this evening or tomorrow. Thanks for the tip.

[abuisman]

@haffla It is necessary because for some reason in ActiveRecord 5, dirty checking doesn't work when it is :json 🤷

I have now implemented the whole multiple database thing a bit differently: I have introduced the concept of an adapter.

I use the current connection's adapter name to make it work. In the future we could implement some query types for different databases differently. This opens up that possibility.

In the design I suggest here you'd have an apply method per adapter that would know what to insert into the model. If that becomes too freeform in the future, we could move some of the duplication to the BaseAdapter and make the database specific adapters more specialised. I don't know. Seems like something to think about when 1 or 2 extra ones are implemented with queries in them.

Let me know what you think!

[abuisman]

@haffla Hi again,

I have made some more final changes to my PR in which I have actually implemented full query support for SQLite which I am pretty excited about.

So what I've done is created the concept of adapters. I've moved the logic for the attribute query methods into the BaseAdapter's module, but I don't include it by default. The SqliteAdapter and PostgresqlAdapter's AttributeQueryMethods modules inherit from that one.

I've created define methods for each sort of query method individually so you can override each individually. For SQLite support I only needed to modify one of them.

I've moved the specs into shared examples which can now be run for each database type. I don't normally like shared examples very much, but in this case they work quite well.

What I'm not very happy about is the connection configuration part of it all. As seen here: https://github.com/madeintandem/jsonb_accessor/pull/152/files#diff-71ccfd4ab1c51ff90091821b553f23bd72b8b1cd42453122b99295f4947ea277R14-R40

It has been quite un-Rails like to get multiple databases working. This is probably due to ActiveRecord being loaded in a non-Rails project.

I could imagine adding MySQL support for example making this all even harder.

Maybe you have some ideas about this all?

[abuisman]

@haffla do you have time to have a look?

[abuisman]

Hmm that failure is strange. Locally I had everything passing. With the version matrix.

[haffla]

Hey @abuisman I am thrilled by your contribution. This looks very good. However I am currently in holidays until mid September. Therefore I cannot review this in depth. Will do when I am back.

What I'm not very happy about is the connection configuration part of it all.

If you look at spec_helper.rb you see that we are there connecting to the Postgres DB already. Is it maybe possible to create a second config file for sqlite and then depending on what adapter is being tested connect to the appropriate DB? Maybe this can be controlled via Rspec tags. Maybe even have different spec folders for the different adapters. Just ideas that I am throwing around without much thinking.

Anyway thank you!

[abuisman]

@haffla Hey that is nice to hear. I figured as much about you being away. No worries, there is no rush, I was just checking in.

With regards to the failures I know what is going on, I used a syntax for the SQLite queries that is only available from a certain version upwards. It seems like the docker image is using a lower one. I'll try to figure out how to fix it.

Furthermore I will look into your suggestions about the config.

Enjoy your holiday!