readyset: Fix quoting in create/drop cache methods (#66)

This commit fixes the identifier quoting in our Readyset.create_cache!
and Readyset.drop_cache! methods. Specifically, the query name passed to
`DROP CACHE` should always be quoted (since it's an identifier) and if
we are passing an ID to `CREATE CACHE`, we need to quote that as well.
The main idea here is that whenever we pass the a query ID or query name
to `CREATE CACHE` or `DROP CACHE`, it needs to be quoted, and whenever
we pass a query string to `CREATE CACHE` it should not be quoted.

This PR includes a revert of #64 and some additional logic to properly
handle the quoting.

Fixes #63

lib/readyset.rb

@@ -36,30 +36,48 @@ def self.configure
   end
 
   # Creates a new cache on ReadySet using the given ReadySet query ID or SQL query.
-  # @param id_or_sql [String] the query ID or SQL string from which a cache should be created.
+  # @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]
-  def self.create_cache!(id_or_sql, name: nil, always: false)
+  # @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'
+    end
+
+    connection = Readyset.route { ActiveRecord::Base.connection }
+    from =
+      if sql
+        sql
+      else
+        connection.quote_column_name(id)
+      end
+
     if always && name
-      raw_query('CREATE CACHE ALWAYS %s FROM %s', name, id_or_sql)
+      quoted_name = connection.quote_column_name(name)
+      raw_query("CREATE CACHE ALWAYS #{quoted_name} FROM #{from}")
     elsif always
-      raw_query('CREATE CACHE ALWAYS FROM %s', id_or_sql)
+      raw_query("CREATE CACHE ALWAYS FROM #{from}")
     elsif name
-      raw_query('CREATE CACHE %s FROM %s', name, id_or_sql)
+      quoted_name = connection.quote_column_name(name)
+      raw_query("CREATE CACHE #{quoted_name} FROM #{from}")
     else
-      raw_query('CREATE CACHE FROM %s', id_or_sql)
+      raw_query("CREATE CACHE FROM #{from}")
     end
 
     nil
   end
 
-  # Drops an existing cache on ReadySet.
-  # @param query_identifier [String] the name of the cache that should be dropped
+  # Drops an existing cache on ReadySet using the given query name.
+  # @param name [String] the name of the cache that should be dropped.
   # @return [void]
-  def self.drop_cache!(query_identifier)
-    raw_query('DROP CACHE %s', query_identifier)
+  def self.drop_cache!(name)
+    connection = Readyset.route { ActiveRecord::Base.connection }
+    quoted_name = connection.quote_column_name(name)
+    raw_query("DROP CACHE #{quoted_name}")
 
     nil
   end

spec/ready_set_spec.rb

@@ -18,60 +18,62 @@
   end
 
   describe '.create_cache!' do
-    let(:query) { build(:proxied_query) }
+    context 'when given neither a SQL string nor an ID' do
+      subject { Readyset.create_cache! }
 
-    subject { Readyset.create_cache!(query.id) }
-
-    it_behaves_like 'a wrapper around a ReadySet SQL extension', 'CREATE CACHE FROM %s' do
-      let(:args) { [query.id] }
+      it 'raises an ArgumentError' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
     end
 
-    context 'when only the "always" parameter is passed' do
-      subject { Readyset.create_cache!(query.id, always: true) }
+    context 'when given both a SQL string and an ID' do
+      subject { Readyset.create_cache!(sql: 'SELECT * FROM t WHERE x = 1', id: 'fake_query_id') }
 
-      it_behaves_like 'a wrapper around a ReadySet SQL extension', 'CREATE CACHE ALWAYS FROM %s' do
-        let(:args) { [query.id] }
+      it 'raises an ArgumentError' do
+        expect { subject }.to raise_error(ArgumentError)
       end
     end
 
-    context 'when only the "name" parameter is passed' do
-      subject { Readyset.create_cache!(query.id, name: name) }
+    context 'when given a SQL string but not an ID' do
+      subject { Readyset.create_cache!(sql: 'SELECT * FROM t WHERE x = 1') }
 
-      let(:name) { 'test cache' }
+      it_behaves_like 'a wrapper around a ReadySet SQL extension',
+        'CREATE CACHE FROM SELECT * FROM t WHERE x = 1'
+    end
 
-      it_behaves_like 'a wrapper around a ReadySet SQL extension', 'CREATE CACHE %s FROM %s' do
-        let(:args) { [name, query.id] }
-      end
+    context 'when given an ID but not a SQL string' do
+      subject { Readyset.create_cache!(id: 'fake_query_id') }
+
+      it_behaves_like 'a wrapper around a ReadySet SQL extension',
+        'CREATE CACHE FROM "fake_query_id"'
     end
 
-    context 'when both the "always" and "name" parameters are passed' do
-      subject { Readyset.create_cache!(query.id, always: true, name: name) }
+    context 'when only the "always" parameter is passed' do
+      subject { Readyset.create_cache!(id: 'fake_query_id', always: true) }
 
-      let(:name) { 'test cache' }
+      it_behaves_like 'a wrapper around a ReadySet SQL extension',
+        'CREATE CACHE ALWAYS FROM "fake_query_id"'
+    end
+
+    context 'when only the "name" parameter is passed' do
+      subject { Readyset.create_cache!(id: 'fake_query_id', name: 'test_cache') }
 
       it_behaves_like 'a wrapper around a ReadySet SQL extension',
-          'CREATE CACHE ALWAYS %s FROM %s' do
-        let(:args) { [name, query.id] }
-      end
+        'CREATE CACHE "test_cache" FROM "fake_query_id"'
     end
 
-    context 'when neither the "always" nor the "name" parameters are passed' do
-      subject { Readyset.create_cache!(query.id) }
+    context 'when both the "always" and "name" parameters are passed' do
+      subject { Readyset.create_cache!(id: 'fake_query_id', always: true, name: 'test_cache') }
 
-      it_behaves_like 'a wrapper around a ReadySet SQL extension', 'CREATE CACHE FROM %s' do
-        let(:args) { [query.id] }
-      end
+      it_behaves_like 'a wrapper around a ReadySet SQL extension',
+        'CREATE CACHE ALWAYS "test_cache" FROM "fake_query_id"'
     end
   end
 
   describe '.drop_cache!' do
-    let(:query) { build(:proxied_query) }
+    subject { Readyset.drop_cache!('query_name') }
 
-    subject { Readyset.drop_cache!(query.id) }
-
-    it_behaves_like 'a wrapper around a ReadySet SQL extension', 'DROP CACHE %s' do
-      let(:args) { [query.id] }
-    end
+    it_behaves_like 'a wrapper around a ReadySet SQL extension', 'DROP CACHE "query_name"'
   end
 
   describe '.raw_query' do