GH Issue 1736: Keep calculated fields when renaming a sample type (#8186)

## Rationale
Renaming a sample type shortly after editing it could lose its
calculated fields ([GH Issue
1736](LabKey/internal-issues#1736)). During
the save, a background thread (e.g. search indexing) could reload the
query metadata cache with the pre-rename row, and the calculated-field
save then wrote that stale row back over the rename.

## Related Pull Requests
- <!-- list of links to related pull requests (replace this comment) -->

## Changes
- Saving query metadata with a new name always stores it under the new
name, even if the cached copy is stale
- The query metadata cache is cleared again after the transaction
commits, so a reload during the save doesn't leave stale data behind
- Server-side test that forces the concurrent cache reload during a
rename

Author: cnathe

query/src/org/labkey/query/MetadataTableJSON.java

@@ -152,9 +152,14 @@ public static void saveMetadata(String schemaName, String queryName, @Nullable S
         TableInfo rawTableInfo = getRawTableInfo(schema, queryName, updatedQueryName);
         TablesDocument doc = null;
         TableType xmlTable = null;
+        String savedQueryName = updatedQueryName != null ? updatedQueryName : queryName;
 
         if (queryDef != null)
         {
+            // GH Issue 1736: make sure to put the updated query name into the queryDef
+            if (updatedQueryName != null)
+                queryDef.setName(updatedQueryName);
+
             try
             {
                 doc = parseDocument(queryDef.getMetaData());
@@ -180,7 +185,7 @@ else if (updatedQueryName != null)
             queryDef = new QueryDef();
             queryDef.setSchema(schemaName);
             queryDef.setContainer(container.getId());
-            queryDef.setName(queryName);
+            queryDef.setName(savedQueryName);
         }
 
         if (doc == null)
@@ -192,7 +197,7 @@ else if (updatedQueryName != null)
         {
             TablesType tables = doc.addNewTables();
             xmlTable = tables.addNewTable();
-            xmlTable.setTableName(queryName);
+            xmlTable.setTableName(savedQueryName);
         }
 
         if (xmlTable.getColumns() == null)

query/src/org/labkey/query/QueryServiceImpl.java

@@ -53,6 +53,7 @@
 import org.labkey.api.data.ConvertHelper;
 import org.labkey.api.data.DbSchema;
 import org.labkey.api.data.DbSchemaType;
+import org.labkey.api.data.DbScope;
 import org.labkey.api.data.DisplayColumn;
 import org.labkey.api.data.Filter;
 import org.labkey.api.data.ForeignKey;
@@ -76,6 +77,7 @@
 import org.labkey.api.data.TableSelector;
 import org.labkey.api.data.VirtualTable;
 import org.labkey.api.data.dialect.SqlDialect;
+import org.labkey.api.exp.PropertyType;
 import org.labkey.api.exp.property.DomainKind;
 import org.labkey.api.exp.query.ExpTable;
 import org.labkey.api.gwt.client.AuditBehaviorType;
@@ -206,6 +208,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.TimeUnit;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -3878,5 +3881,66 @@ public void testRightAndIsnumeric() throws SQLException
                 assertEquals("isnumeric(NULL) on " + dialect, 0, results.getInt("n4"));
             }
         }
+
+        // GH Issue 1736
+        @Test
+        public void testRenameWithConcurrentCacheReload() throws Exception
+        {
+            DbScope scope = QueryManager.get().getDbSchema().getScope();
+            Container c = JunitUtil.getTestContainer();
+            User user = TestContext.get().getUser();
+            String oldName = "Users";
+            String newName = "junit" + GUID.makeHash();
+
+            QueryDef def = new QueryDef();
+            def.setSchema("core");
+            def.setName(oldName);
+            def.setContainer(c.getId());
+            def.setMetaData("<tables xmlns=\"http://labkey.org/data/xml\"><table tableName=\"" + oldName + "\" tableDbType=\"NOT_IN_DB\"><columns>" +
+                    "<column columnName=\"junitCalc\"><conceptURI>http://www.labkey.org/exp/xml#calculated</conceptURI><valueExpression>1</valueExpression></column>" +
+                    "</columns></table></tables>");
+            int queryDefId = QueryManager.get().insert(user, def).getQueryDefId();
+
+            GWTPropertyDescriptor calcField = new GWTPropertyDescriptor("junitCalc", PropertyType.INTEGER.getTypeUri());
+            calcField.setValueExpression("1");
+            // reloads the cache from committed data, as search indexing can while the rename is uncommitted
+            Runnable concurrentReload = () -> QueryManager.get().getQueryDef(c, "core", oldName, false);
+
+            try
+            {
+                try (DbScope.Transaction tx = scope.ensureTransaction())
+                {
+                    QueryManager.get().renameQuery(user, c, "core", oldName, newName);
+                    runOnOtherThread(concurrentReload);
+                    assertNull("Expected the cache to hold the pre-rename row", QueryManager.get().getQueryDef(c, "core", newName, false));
+
+                    QueryService.get().saveCalculatedFieldsMetadata("core", oldName, newName, List.of(calcField), true, user, c);
+                    runOnOtherThread(concurrentReload);
+                    tx.commit();
+                }
+
+                QueryDef saved = new TableSelector(QueryManager.get().getTableInfoQueryDef()).getObject(queryDefId, QueryDef.class);
+                assertEquals(newName, saved.getName());
+                assertTrue(saved.getMetaData(), saved.getMetaData().contains("tableName=\"" + newName + "\""));
+                assertTrue(saved.getMetaData(), saved.getMetaData().contains("junitCalc"));
+
+                assertNotNull("Cache should reload after commit", QueryManager.get().getQueryDef(c, "core", newName, false));
+                assertNull(QueryManager.get().getQueryDef(c, "core", oldName, false));
+            }
+            finally
+            {
+                QueryDef toDelete = new TableSelector(QueryManager.get().getTableInfoQueryDef()).getObject(queryDefId, QueryDef.class);
+                if (toDelete != null)
+                    QueryManager.get().delete(toDelete);
+            }
+        }
+
+        private void runOnOtherThread(Runnable r) throws InterruptedException
+        {
+            Thread t = new Thread(r);
+            t.start();
+            t.join(TimeUnit.SECONDS.toMillis(30));
+            assertFalse("Reader thread did not finish", t.isAlive());
+        }
     }
 }

query/src/org/labkey/query/persist/QueryDefCache.java

@@ -23,6 +23,7 @@
 import org.labkey.api.collections.CaseInsensitiveMapWrapper;
 import org.labkey.api.collections.IntHashMap;
 import org.labkey.api.data.Container;
+import org.labkey.api.data.DbScope;
 import org.labkey.api.data.SimpleFilter;
 import org.labkey.api.data.TableSelector;
 import org.labkey.query.QueryDefinitionImpl;
@@ -166,9 +167,20 @@ QueryDef getQueryDefById(Container container, int queryDefId)
         return QUERY_DEF_DB_CACHE.get(container)._queryDefIdMap.get(queryDefId);
     }
 
+    // GH Issue 1736: a thread outside the transaction can reload pre-commit rows after the immediate remove, so remove again after commit
     public static void uncache(Container c)
     {
-        QUERY_DEF_DB_CACHE.remove(c);
-        QueryDefinitionImpl.clearHasPkColumnCache();
+        QueryManager.get().getDbSchema().getScope().addCommitTask(new UncacheTask(c), DbScope.CommitTaskOption.IMMEDIATE, DbScope.CommitTaskOption.POSTCOMMIT, DbScope.CommitTaskOption.POSTROLLBACK);
+    }
+
+    // record equality lets the transaction dedupe repeated uncaches of the same container
+    private record UncacheTask(Container c) implements Runnable
+    {
+        @Override
+        public void run()
+        {
+            QUERY_DEF_DB_CACHE.remove(c);
+            QueryDefinitionImpl.clearHasPkColumnCache();
+        }
     }
 }