server: Fix fine-grained incremental cache invalidation (fix #3759) (#6027)

This issue was very tricky to track down, but fortunately easy to fix.
The interaction here is subtle enough that it’s difficult to put into
English what would go wrong in what circumstances, but the new unit test
captures precisely that interaction to ensure it remains fixed.

CHANGELOG.md

@@ -67,6 +67,10 @@ This release contains the [PDV refactor (#4111)](https://github.com/hasura/graph
 
 (Add entries here in the order of: server, console, cli, docs, others)
 
+- server: Fix fine-grained incremental cache invalidation (fix #3759)
+
+  This issue could cause enum table values to sometimes not be properly reloaded without restarting `graphql-engine`. Now a `reload_metadata` API call (or clicking “Reload enum values” in the console) should consistently force a reload of all enum table values.
+
 - server: add `--websocket-compression` command-line flag for enabling websocket compression (fix #3292)
 - server: some mutations that cannot be performed will no longer be in the schema (for instance, `delete_by_pk` mutations won't be shown to users that do not have select permissions on all primary keys) (#4111)
 - server: miscellaneous description changes (#4111)

server/src-lib/Hasura/Incremental/Internal/Cache.hs

@@ -70,10 +70,12 @@ instance (MonadUnique m) => ArrowCache m (Rule m) where
         (k $! (s <> s')) (b, s') (listenAccesses r')
 
       cached accesses a b (Rule r) = Rule \s a' k -> if
-        | unchanged accesses a a' -> k s b (cached accesses a b (Rule r))
+        | unchanged accesses a a' -> (k $! (s <> accesses)) b (cached accesses a b (Rule r))
         | otherwise -> r s a' \s' (b', accesses') r' -> k s' b' (cached accesses' a' b' r')
 
-  newDependency = arrM \a -> newUniqueS <&> \u -> Dependency (DependencyRoot u) a
+  newDependency = Rule \s a k -> do
+    key <- DependencyRoot <$> newUniqueS
+    k s (Dependency key a) (arr (Dependency key))
   {-# INLINABLE newDependency #-}
 
   dependOn = Rule \s (Dependency key v) k -> (k $! recordAccess key AccessedAll s) v dependOn

server/src-test/Hasura/IncrementalSpec.hs

@@ -31,6 +31,29 @@ spec = do
       (_, state3) <- runStateT (Inc.rebuild result2 (True, True)) 0
       state3 `shouldBe` 2
 
+    it "tracks dependencies within nested uses of cache across multiple executions" do
+      let rule :: (MonadWriter String m, MonadUnique m)
+               => Inc.Rule m (Inc.InvalidationKey, Inc.InvalidationKey) ()
+          rule = proc (key1, key2) -> do
+            dep1 <- Inc.newDependency -< key2
+            (key1, dep1) >- Inc.cache (proc (_, dep2) ->
+              dep2 >- Inc.cache (proc dep3 -> do
+                Inc.dependOn -< dep3
+                arrM tell -< "executed"))
+            returnA -< ()
+
+      let key1 = Inc.initialInvalidationKey
+          key2 = Inc.invalidate key1
+
+      (result1, log1) <- runWriterT $ Inc.build rule (key1, key1)
+      log1 `shouldBe` "executed"
+
+      (result2, log2) <- runWriterT $ Inc.rebuild result1 (key2, key1)
+      log2 `shouldBe` ""
+
+      (_, log3) <- runWriterT $ Inc.rebuild result2 (key2, key2)
+      log3 `shouldBe` "executed"
+
   describe "keyed" $ do
     it "preserves incrementalization when entries don’t change" $ do
       let rule :: (MonadWriter (S.HashSet (String, Integer)) m, MonadUnique m)

server/tests-py/queries/v1/set_table_is_enum/set_and_delayed_reload/introspect_enum_values.yaml

@@ -0,0 +1,44 @@
+- description: Insert a new enum value without reloading
+  url: /v1/query
+  status: 200
+  query:
+    type: run_sql
+    args:
+      sql: |
+        INSERT INTO crust VALUES ('thick');
+
+- description: Check that the previous enum values are cached
+  url: /v1/graphql
+  status: 200
+  response:
+    data:
+      crust:
+        enumValues:
+        - name: thin
+  query: &crust_query
+    query: |
+      {
+        crust: __type(name: "crust_enum") {
+          enumValues {
+            name
+          }
+        }
+      }
+
+- description: Reload the metadata
+  url: /v1/query
+  status: 200
+  query:
+    type: reload_metadata
+    args: {}
+
+- description: Check that the enum values are updated
+  url: /v1/graphql
+  status: 200
+  response:
+    data:
+      crust:
+        enumValues:
+        - name: thick
+        - name: thin
+  query: *crust_query

server/tests-py/queries/v1/set_table_is_enum/set_and_delayed_reload/setup.yaml

@@ -0,0 +1,24 @@
+type: bulk
+args:
+- type: run_sql
+  args:
+    sql: |
+      CREATE TABLE crust (name text NOT NULL PRIMARY KEY);
+      INSERT INTO crust VALUES ('thin');
+- type: add_existing_table_or_view
+  args: crust
+- type: set_table_is_enum
+  args:
+    table: crust
+    is_enum: true
+- type: run_sql
+  args:
+    sql: |
+      CREATE TABLE pizza
+        ( id serial NOT NULL PRIMARY KEY
+        , name text NOT NULL
+        , crust text NOT NULL REFERENCES crust );
+
+      INSERT INTO pizza (name, crust) VALUES ('margarita', 'thin');
+- type: add_existing_table_or_view
+  args: pizza

server/tests-py/queries/v1/set_table_is_enum/set_and_delayed_reload/teardown.yaml

@@ -0,0 +1,8 @@
+type: bulk
+args:
+- type: run_sql
+  args:
+    sql: |
+      DROP TABLE pizza;
+      DROP TABLE crust;
+    cascade: true

server/tests-py/test_v1_queries.py

@@ -729,6 +729,16 @@ class TestSetTableIsEnum:
     def test_relationship_with_inconsistent_enum_table(self, hge_ctx):
         check_query_f(hge_ctx, self.dir() + '/relationship_with_inconsistent_enum_table.yaml')
 
+# regression test for issue #3759
+@usefixtures('per_method_tests_db_state')
+class TestSetTableIsEnumSetAndDelayedReload:
+    @classmethod
+    def dir(cls):
+        return 'queries/v1/set_table_is_enum/set_and_delayed_reload'
+
+    def test_introspect_enum_values(self, hge_ctx):
+        check_query_f(hge_ctx, self.dir() + '/introspect_enum_values.yaml')
+
 @usefixtures('per_method_tests_db_state')
 class TestSetTableCustomFields: