rake: Fix migration tasks (#71)

Previously, the migration task relied upon the query IDs generated by
ReadySet to determine what caches it needed to create or drop. This was
error prone because when caches are created via the gem, they are
created via the ID generated from the query string generated by Rails
when it invoked the queries. When the caches are created via the
*migration file*, they are created via the query string that was
obtained from calling `SHOW CACHES`, which has a number of rewrites,
including star expansion and fully-qualified table and column names.
Creating a cache from this query string would create a cache with a
different ID than creating the same cache from the Rails-generated query
string.

In addition, the migration task was always attempting to create caches
via the query ID from the migration file, which relies upon there being
an entry in SHOW PROXIED QUERIES for that query ID. This may not always
be the case, since it's possible a user may run the cache migrations
before routing queries for those caches.

This commit fixes these two issues by removing the dependence on query
IDs entirely and instead compares the query strings in the migration
file to the query strings in SHOW CACHES to determine which caches
should be created and dropped.

Finally, this commit also rewrites the rake task tests to run directly
against
the Postgres and ReadySet instances running in CI to ensure that the
logic is
sound.

lib/readyset/caches.rb

@@ -6,13 +6,12 @@ class << self
       attr_reader :caches
     end
 
-    def self.cache(id:, always: false)
+    def self.cache(always: false)
       @caches ||= Set.new
 
       query = yield
 
       @caches << Query::CachedQuery.new(
-        id: id,
         text: query.strip,
         always: always,
       )

lib/tasks/readyset.rake

@@ -11,6 +11,7 @@ namespace :readyset do
       template = File.read(File.join(File.dirname(__FILE__), '../templates/caches.rb.tt'))
 
       queries = Readyset::Query::CachedQuery.all
+
       f = File.new(Readyset.configuration.migration_path, 'w')
       f.write(ERB.new(template, trim_mode: '-').result(binding))
       f.close
@@ -29,41 +30,37 @@ namespace :readyset do
       # definition of the `Readyset::Caches` subclass is garbage collected too
       container = Object.new
       container.instance_eval(File.read(file))
-      caches = container.singleton_class::ReadysetCaches.caches
-
-      caches_on_readyset = Readyset::Query::CachedQuery.all.index_by(&:id)
-      caches_on_readyset_ids = caches_on_readyset.keys.to_set
-
-      caches_in_migration_file = caches.index_by(&:id)
-      caches_in_migration_file_ids = caches_in_migration_file.keys.to_set
+      caches_in_migration_file = container.singleton_class::ReadysetCaches.caches.index_by(&:text)
+      caches_on_readyset = Readyset::Query::CachedQuery.all.index_by(&:text)
 
-      to_drop_ids = caches_on_readyset_ids - caches_in_migration_file_ids
-      to_create_ids = caches_in_migration_file_ids - caches_on_readyset_ids
+      to_drop = caches_on_readyset.keys - caches_in_migration_file.keys
+      to_create = caches_in_migration_file.keys - caches_on_readyset.keys
 
-      if to_drop_ids.size.positive? || to_create_ids.size.positive?
+      if to_drop.size.positive? || to_create.size.positive?
         dropping = 'Dropping'.red
         creating = 'creating'.green
-        print "#{dropping} #{to_drop_ids.size} caches and #{creating} #{to_create_ids.size} " \
-          'caches. Continue? (y/n) '
+        print "#{dropping} #{to_drop.size} caches and #{creating} #{to_create.size} caches. " \
+          'Continue? (y/n) '
         $stdout.flush
         y_or_n = STDIN.gets.strip
 
         if y_or_n == 'y'
-          if to_drop_ids.size.positive?
-            bar = ProgressBar.create(title: 'Dropping caches', total: to_drop_ids.size)
+          if to_drop.size.positive?
+            bar = ProgressBar.create(title: 'Dropping caches', total: to_drop.size)
 
-            to_drop_ids.each do |id|
+            to_drop.each do |text|
               bar.increment
-              Readyset.drop_cache!(name_or_id: id)
+              Readyset.drop_cache!(caches_on_readyset[text].name)
             end
           end
 
-          if to_create_ids.size.positive?
-            bar = ProgressBar.create(title: 'Creating caches', total: to_create_ids.size)
+          if to_create.size.positive?
+            bar = ProgressBar.create(title: 'Creating caches', total: to_create.size)
 
-            to_create_ids.each do |id|
+            to_create.each do |text|
               bar.increment
-              Readyset.create_cache!(id: id)
+              cache = caches_in_migration_file[text]
+              Readyset.create_cache!(sql: text, always: cache.always)
             end
           end
         end

lib/templates/caches.rb.tt

@@ -1,6 +1,6 @@
 class ReadysetCaches < Readyset::Caches
 <% queries.each do |query| -%>
-  cache id: <%= query.id.dump %>, always: <%= query.always %> do
+  cache always: <%= query.always %> do
     <<~SQL
       <%= query.text.gsub("\n", "\n      ") %>
     SQL

spec/caches_spec.rb

@@ -5,9 +5,9 @@
     end
 
     it 'adds a cache with the given attributes to the @caches ivar' do
-      query = build(:cached_query, always: true, count: nil, name: nil)
+      query = build(:cached_query, always: true, count: nil, id: nil, name: nil)
 
-      Readyset::Caches.cache(always: true, id: query.id) { query.text }
+      Readyset::Caches.cache(always: true) { query.text }
 
       caches = Readyset::Caches.instance_variable_get(:@caches)
       expect(caches.size).to eq(1)
@@ -16,9 +16,9 @@
 
     context 'when no always parameter is passed' do
       it 'defaults the always parameter to false' do
-        query = build(:cached_query, count: nil, name: nil)
+        query = build(:cached_query, count: nil, id: nil, name: nil)
 
-        Readyset::Caches.cache(id: query.id) { query.text }
+        Readyset::Caches.cache { query.text }
 
         always = Readyset::Caches.instance_variable_get(:@caches).first.always
         expect(always).to eq(false)
@@ -28,8 +28,8 @@
 
   describe '.caches' do
     it 'returns the caches stored in the @caches ivar' do
-      query = build(:cached_query, count: nil, name: nil)
-      Readyset::Caches.cache(always: query.always, id: query.id) { query.text }
+      query = build(:cached_query, count: nil, id: nil, name: nil)
+      Readyset::Caches.cache(always: query.always) { query.text }
 
       result = Readyset::Caches.caches
 

spec/rake_spec.rb

@@ -14,8 +14,8 @@
       describe 'dump' do
         it 'dumps the current set of caches to a migration file' do
           # Setup
-          allow(Readyset::Query::CachedQuery).to receive(:all).
-            and_return([build(:cached_query), build(:cached_query_2)])
+          cache1 = build_and_create_cache(:cached_query, count: nil, id: nil, name: nil)
+          cache2 = build_and_create_cache(:cached_query_2, count: nil, id: nil, name: nil)
 
           # Execute
           Rake::Task['readyset:caches:dump'].execute
@@ -27,8 +27,8 @@
 
           caches = subclasses.first.caches
           expect(caches.size).to eq(2)
-          expect(caches).to include(build(:cached_query, count: nil, name: nil))
-          expect(caches).to include(build(:cached_query_2, count: nil, name: nil))
+          expect(caches).to include(cache1)
+          expect(caches).to include(cache2)
         end
       end
 
@@ -42,27 +42,33 @@
         context "when the migration file contains caches that don't exist on ReadySet" do
           it "creates the caches in the migration file that don't exist on ReadySet" do
             # Setup
-            cache_to_create = build(:cached_query_2)
-            generate_migration_file([build(:cached_query), cache_to_create])
+            existing_cache = build_and_create_cache(:cached_query)
+            cache_to_create = build_and_create_cache(:cached_query_2)
+
+            Rake::Task['readyset:caches:dump'].execute
+            cache_to_create.drop!
 
-            allow(Readyset::Query::CachedQuery).to receive(:all).and_return([build(:cached_query)])
-            allow(Readyset).to receive(:create_cache!).with(id: cache_to_create.id)
             allow(STDIN).to receive(:gets).and_return("y\n")
 
             # Execute
             Rake::Task['readyset:caches:migrate'].execute
 
             # Verify
-            expect(Readyset).to have_received(:create_cache!).with(id: cache_to_create.id)
+            caches = Readyset::Query::CachedQuery.all
+            expect(caches.size).to eq(2)
+            cache_texts = caches.map(&:text)
+            expect(cache_texts).to include(existing_cache.text)
+            expect(cache_texts).to include(cache_to_create.text)
           end
 
           it 'prints the expected output' do
             # Setup
-            cache_to_create = build(:cached_query_2)
-            generate_migration_file([build(:cached_query), cache_to_create])
+            build_and_create_cache(:cached_query)
+            cache_to_create = build_and_create_cache(:cached_query_2)
+
+            Rake::Task['readyset:caches:dump'].execute
+            cache_to_create.drop!
 
-            allow(Readyset::Query::CachedQuery).to receive(:all).and_return([build(:cached_query)])
-            allow(Readyset).to receive(:create_cache!).with(id: cache_to_create.id)
             allow(STDIN).to receive(:gets).and_return("y\n")
 
             # Execute + Verify
@@ -75,30 +81,27 @@
 
         context "when ReadySet has caches that don't exist in the migration file" do
           it 'drops the caches that exist on ReadySet that are not in the migration file' do
-            # Setup
-            generate_migration_file([build(:cached_query)])
+            existing_cache = build_and_create_cache(:cached_query)
+            Rake::Task['readyset:caches:dump'].execute
+            build_and_create_cache(:cached_query_2)
 
-            cache_to_drop = build(:cached_query_2)
-            allow(Readyset::Query::CachedQuery).to receive(:all).
-              and_return([build(:cached_query), cache_to_drop])
-            allow(Readyset).to receive(:drop_cache!).with(name_or_id: cache_to_drop.id)
             allow(STDIN).to receive(:gets).and_return("y\n")
 
             # Execute
             Rake::Task['readyset:caches:migrate'].execute
 
             # Verify
-            expect(Readyset).to have_received(:drop_cache!).with(name_or_id: cache_to_drop.id)
+            caches = Readyset::Query::CachedQuery.all
+            cache_texts = caches.map(&:text)
+            expect(cache_texts).to eq([existing_cache.text])
           end
 
           it 'prints the expected output' do
             # Setup
-            generate_migration_file([build(:cached_query)])
+            build_and_create_cache(:cached_query)
+            Rake::Task['readyset:caches:dump'].execute
+            build_and_create_cache(:cached_query_2)
 
-            cache_to_drop = build(:cached_query_2)
-            allow(Readyset::Query::CachedQuery).to receive(:all).
-              and_return([build(:cached_query), cache_to_drop])
-            allow(Readyset).to receive(:drop_cache!).with(name_or_id: cache_to_drop.id)
             allow(STDIN).to receive(:gets).and_return("y\n")
 
             # Execute + Verify
@@ -108,12 +111,6 @@
               to_stdout
           end
         end
-
-        def generate_migration_file(caches)
-          allow(Readyset::Query::CachedQuery).to receive(:all).and_return(caches)
-          Rake::Task['readyset:caches:dump'].execute
-          allow(Readyset::Query::CachedQuery).to receive(:all).and_call_original
-        end
       end
     end
   end