Cleaned up Configuration and refactored how @database_url is set (#…

…59)

Added `fetch_database_url` in `Readyset::Configuration` to fetch
`@database_url` from `config/database.yml` or `ENV`, with improved error
handling and Rails environment context.

---
Regarding `git`, I rebased the branch to isolate the config commits
compared to `main`, cherry-picked commits after `6e79649`
(add-route-and-exclude), and merged with main. I think it's stuck at 49
commits due to github squashing merged PRs + how I committed the first
version of the config interface?

---------

Co-authored-by: Ethan Donowitz <[email protected]>

lib/readyset.rb

@@ -9,29 +9,40 @@
 require 'readyset/railtie' if defined?(Rails::Railtie)
 require 'readyset/relation_extension'
 
+# The Readyset module provides functionality to integrate ReadySet caching
+# with Ruby on Rails applications.
+# It offers methods to configure and manage ReadySet caches,
+# as well as to route database queries through ReadySet.
 module Readyset
+  # Sets the configuration for Readyset.
+  # @!attribute [w] configuration
   attr_writer :configuration
 
+  # Retrieves the Readyset configuration, initializing it if it hasn't been set yet.
+  # @return [Readyset::Configuration] the current configuration for Readyset.
   def self.configuration
     @configuration ||= Configuration.new
   end
 
+  class << self
+    alias_method :config, :configuration
+  end
+
+  # Configures Readyset by providing a block with configuration details.
+  # @yieldparam [Readyset::Configuration] configuration the current configuration instance.
+  # @yieldreturn [void]
   def self.configure
-    yield(configuration)
+    yield configuration
   end
 
-  # Creates a new cache on ReadySet using the given ReadySet query ID or SQL query. Exactly one of
-  # the `id` or `sql` keyword arguments must be provided.
-  #
-  # This method is a no-op if a cache for the given ID/query already exists.
-  #
-  # @param [String] id the ReadySet query ID of the query from which a cache should be created
-  # @param [String] sql the SQL string from which a cache should be created
-  # @param [String] name the name for the cache being created
-  # @param [Boolean] always whether the cache should always be used. if this is true, queries to
-  # these caches will never fall back to the database
+  # Creates a new cache on ReadySet using the given ReadySet query ID or SQL query.
+  # @param id [String] the ReadySet query ID of the query from which a cache should be created.
+  # @param sql [String] the SQL string from which a cache should be created.
+  # @param name [String] the name for the cache being created.
+  # @param always [Boolean] whether the cache should always be used;
+  # queries to these caches will never fall back to the database if this is true.
   # @return [void]
-  # @raise [ArgumentError] raised if exactly one of the `id` or `sql` arguments was not provided
+  # @raise [ArgumentError] raised if exactly one of the `id` or `sql` arguments was not provided.
   def self.create_cache!(id: nil, sql: nil, name: nil, always: false)
     if (sql.nil? && id.nil?) || (!sql.nil? && !id.nil?)
       raise ArgumentError, 'Exactly one of the `id` and `sql` parameters must be provided'
@@ -53,20 +64,11 @@ def self.create_cache!(id: nil, sql: nil, name: nil, always: false)
     nil
   end
 
-  def self.current_config
-    configuration.inspect
-  end
-
-  # Creates a new cache on ReadySet using the given SQL query or ReadySet query ID. Exactly one of
-  # the `name_or_id` or `sql` keyword arguments must be provided.
-  #
-  # This method is a no-op if a cache for the given ID/query already doesn't exist.
-  #
-  # @param [String] name_or_id the name or the ReadySet query ID of the cache that should be dropped
-  # @param [String] sql a SQL string for a query whose associated cache should be dropped
+  # Drops an existing cache on ReadySet using the given SQL query or ReadySet query ID.
+  # @param name_or_id [String] the name or ReadySet query ID of the cache that should be dropped.
+  # @param sql [String] a SQL string for a query whose associated cache should be dropped.
   # @return [void]
-  # @raise [ArgumentError] raised if exactly one of the `name_or_id` or `sql` arguments was not
-  # provided
+  # @raise [ArgumentError] if exactly one of the `name_or_id` or `sql` arguments was not provided.
   def self.drop_cache!(name_or_id: nil, sql: nil)
     if (sql.nil? && name_or_id.nil?) || (!sql.nil? && !name_or_id.nil?)
       raise ArgumentError, 'Exactly one of the `name_or_id` and `sql` parameters must be provided'
@@ -82,44 +84,46 @@ def self.drop_cache!(name_or_id: nil, sql: nil)
   end
 
   # Executes a raw SQL query against ReadySet. The query is sanitized prior to being executed.
-  #
-  # @param [Array<Object>] *sql_array the SQL array to be executed against ReadySet
-  # @return [PG::Result]
+  # @note This method is not part of the public API.
+  # @param sql_array [Array<Object>] the SQL array to be executed against ReadySet.
+  # @return [PG::Result] the result of executing the SQL query.
   def self.raw_query(*sql_array) # :nodoc:
     ActiveRecord::Base.connected_to(role: reading_role, shard: shard, prevent_writes: false) do
       ActiveRecord::Base.connection.execute(ActiveRecord::Base.sanitize_sql_array(sql_array))
     end
   end
 
-  # Routes to ReadySet any queries that occur in the given block. If `prevent_writes` is true, an
-  # attempt to execute a write within the given block will raise an error. Keep in mind that if
-  # `prevent_writes` is false, any writes that occur within the given block will be proxied through
-  # ReadySet to the database.
-  #
-  # @param [Boolean] prevent_writes prevent writes from being executed on the connection to ReadySet
-  # @yield a block whose queries should be routed to ReadySet
-  # @return the value of the last line of the block
+  # Routes to ReadySet any queries that occur in the given block.
+  # @param prevent_writes [Boolean] if true, prevents writes from being executed on
+  # the connection to ReadySet.
+  # @yield a block whose queries should be routed to ReadySet.
+  # @return the value of the last line of the block.
   def self.route(prevent_writes: true, &block)
     if prevent_writes
-      ActiveRecord::Base.
-        connected_to(role: reading_role, shard: shard, prevent_writes: true, &block)
+      ActiveRecord::Base.connected_to(role: reading_role, shard: shard, prevent_writes: true,
+        &block)
     else
-      ActiveRecord::Base.
-        connected_to(role: writing_role, shard: shard, prevent_writes: false, &block)
+      ActiveRecord::Base.connected_to(role: writing_role, shard: shard, prevent_writes: false,
+        &block)
     end
   end
 
   private
 
+  # Delegates the shard method to the configuration.
   class << self
     private(*delegate(:shard, to: :configuration))
   end
 
+  # Returns the reading role for ActiveRecord connections.
+  # @return [Symbol] the reading role.
   def self.reading_role
     ActiveRecord.reading_role
   end
   private_class_method :reading_role
 
+  # Returns the writing role for ActiveRecord connections.
+  # @return [Symbol] the writing role.
   def self.writing_role
     ActiveRecord.writing_role
   end

lib/readyset/configuration.rb

@@ -3,37 +3,10 @@
 
 module Readyset
   class Configuration
-    attr_accessor :database_url, :shard
+    attr_accessor :shard
 
     def initialize
-      @database_url = ENV['READYSET_URL'] || default_connection_url
       @shard = :readyset
     end
-
-    def self.current_config
-      configuration.inspect
-    end
-
-    def default_connection_url
-      # Check if Rails is defined and if database configuration is available
-      'postgres://user:password@localhost:5432/readyset'
-    end
-
-    # @return [Readyset::Configuration] Readyset's current configuration
-    def self.configuration
-      @configuration ||= Configuration.new
-    end
-
-    # Set Readyset's configuration
-    # @param config [Readyset::Configuration]
-    class << self
-      attr_writer :configuration
-    end
-
-    # Modify Readyset's current configuration
-    # @yieldparam [Readyset::Configuration] config current Readyset config
-    def self.configure
-      yield configuration
-    end
   end
 end

spec/configuration_spec.rb

@@ -1,32 +1,13 @@
-# configuration_spec.rb
-# spec/readyset-rails/configuration_spec.rb
+# spec/configuration_spec.rb
 
 require 'spec_helper'
 require 'readyset/configuration'
 
 RSpec.describe Readyset::Configuration do
-  let(:config) { described_class.new }
-
-  context 'when no database_url is specified' do
-    it 'defaults to dummy url' do
-      default_url = 'postgres://user:password@localhost:5432/readyset'
-      expect(config.database_url).to eq default_url
-    end
-  end
-
-  context 'when database_url ENV var is specified' do
-    it 'is used instead of dummy url' do
-      ENV['READYSET_URL'] = 'postgres://test:password@localhost:5433/readyset'
-      expect(config.database_url).to eq 'postgres://test:password@localhost:5433/readyset'
-    end
-  end
-
-  context 'when database_url is specified' do
-    it 'is used instead of dummy url' do
-      readyset_url = 'postgres://user_test:password@localhost:5433/readyset'
-      config.database_url = readyset_url
-
-      expect(config.database_url).to eq readyset_url
+  describe '#initialize' do
+    it 'initializes shard with the symbol :readyset' do
+      config = Readyset::Configuration.new
+      expect(config.shard).to eq(:readyset)
     end
   end
 end

spec/ready_set_spec.rb

@@ -7,6 +7,16 @@
     expect(Readyset::VERSION).not_to be nil
   end
 
+  describe '.configuration' do
+    it 'returns the current configuration object' do
+      expect(Readyset.configuration).to be_an_instance_of(Readyset::Configuration)
+    end
+
+    it 'is aliased as .config' do
+      expect(Readyset.config).to eq(Readyset.configuration)
+    end
+  end
+
   describe '.create_cache!' do
     let(:query) { build(:proxied_query) }
 

spec/spec_helper.rb

@@ -14,6 +14,7 @@
 
 # This is a bit of a hack. Combustion doesn't appear to support migrating multiple databases, so we
 # just copy the primary database file to serve as the database for our fake ReadySet instance
+
 primary_db_file = Rails.configuration.database_configuration['test']['primary']['database']
 readyset_db_file = Rails.configuration.database_configuration['test']['readyset']['database']
 FileUtils.cp("spec/internal/#{primary_db_file}", "spec/internal/#{readyset_db_file}")