Extract SQLite into an adapter #204
Conversation
|
Thanks! I'll take a look in the next day or so. |
|
I noticed a few issues since I first pushed this up but I got everything fixed up now. No rush or anything on the review but I'm done making changes now! 😂 |
There was a problem hiding this comment.
This is great work, thank you so much for extracting all of this so nicely!
I made some comments, but they are almost all small things like naming, unnecessary variables, or breaking a method into multiple methods.
One thing I do think still remains to be extracted into the adapters: the BaseConfig#database_path_for method, which only exists because the SQLite :database value might be a URI.
The idea I have in mind is to remove the "pathiness" from the database configs:
- delete the
BaseConfig#database_path_formethod - remove the
:database_pathvalue from the TenantConfig hash - move
BaseConfig#coerce_pathinto the SQLite adapter - and change
SQLite#get_database_pathinto an instance variable@database_paththat's computed in the initializer by callingcoerce_path
but we can take care of that in a second PR if you prefer to keep the complexity down!
lib/active_record/tenanted/database_configurations/base_config.rb
Outdated
Show resolved
Hide resolved
| database_dir = File.dirname(database_path) | ||
| FileUtils.mkdir_p(database_dir) unless File.directory?(database_dir) |
There was a problem hiding this comment.
I think explicitly creating the directory is unnecessary in practice because the ready lock creates it already. I'm OK keeping it for clarity/completeness but I wanted to mention that it's probably duplicative.
There was a problem hiding this comment.
If we remove this a bunch of tests fail. Could be the test setup that's the problem? I left it as is for now though.
|
Thanks for the great review Mike! Made a lot of things much simpler and nicer. 🥰 Let me know if you want anything else changed. I opted not to address your comments above in this one. I'll create an issue and grab it in a follow up. Update here's the issue: #214 |
New pattern that makes it possible to support multiple database adapters
This extracts anything SQLite-specific into a generic database adapter class which forwards to a SQLite adapter class. This is a new pattern that will make it pretty easy to support different databases (I've got MySQL working in a different branch). Since this is a big change I wanted to validate the approach first and get this merged in for SQLite. But adding support for MYSQL is fairly trivial after this change.
This pattern is very similar to how Rails does it. In Rails there's the main ActiveRecord::Tasks::DatabaseTasks which defines the API and then forwards the implementation over to the specific adapter to do the work. For example here's the class for SQLite.
Originally I tried tackling this by using the adapters / methods that are provided by Rails but...it's just slightly different enough that it won't work as is. I think there's a world where we could upstream a bunch of these changes to into Rails and make it more compatible and/or make this gem more compatible for how Rails does it. Either way I thought it best to just do our own thing for now and cross that bridge another day. 😊
No breaking changes
There should be no actual changes from the current implementation for SQLite—except for one place when we drop the db (I'll add a comment at that line of code).
How I tested
I relied on the test suite for this one and extracted everything without breaking any tests. I also added tests for the adapter to ensure that it forwards everything over to the correct adapter methods. I didn't unit test the actual SQLite adapter since this is well covered by other tests but I would be happy to add those as well.