GitHub Issue #1569: Assay domain default values are lost on upgrade because of LSID key change

api/src/org/labkey/api/defaults/DefaultValueService.java

@@ -41,6 +41,7 @@
 public interface DefaultValueService
 {
     String DOMAIN_DEFAULT_VALUE_LSID_PREFIX = "DomainDefaultValue";
+    String USER_DEFAULT_VALUE_LSID_PREFIX = "UserDefaultValue";
 
     static DefaultValueService get()
     {

experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java

@@ -54,6 +54,7 @@
 import org.labkey.api.data.UpgradeCode;
 import org.labkey.api.data.dialect.BasePostgreSqlDialect;
 import org.labkey.api.data.dialect.PostgreSqlService;
+import org.labkey.api.exp.Lsid;
 import org.labkey.api.exp.OntologyManager;
 import org.labkey.api.exp.PropertyDescriptor;
 import org.labkey.api.exp.api.ExpSampleType;
@@ -62,6 +63,7 @@
 import org.labkey.api.exp.api.SampleTypeService;
 import org.labkey.api.exp.api.StorageProvisioner;
 import org.labkey.api.exp.property.Domain;
+import org.labkey.api.exp.property.DomainKind;
 import org.labkey.api.exp.property.DomainUtil;
 import org.labkey.api.exp.property.PropertyService;
 import org.labkey.api.files.FileContentService;
@@ -97,6 +99,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -816,4 +819,192 @@ public static void shortenAllStorageNames(ModuleContext context)
         if (!badColumnNames.isEmpty())
             LOG.error("Some storage column names are still too long!! {}", badColumnNames);
     }
+
+    // Legacy prefixes, frozen at their historical values so later renames of the service constants can't change what this upgrade matches.
+    private static final String LEGACY_CONTAINER_DEFAULTS_PREFIX = "DomainDefaultValue";
+    private static final String LEGACY_USER_DEFAULTS_PARENT_PREFIX = "UserDefaultValueParent";
+
+    /**
+     * GitHub Issue #1569: re-key folder-level default values by domain kind. The old key was the container plus the
+     * domain typeURI objectId, which repeats across kinds and is shared outright by an assay design's domains, so
+     * unrelated domains silently overwrote each other's defaults. Rows written before 25.7 are keyed by domain name
+     * instead; both forms are handled here. The owning domain is resolved from the properties the object actually
+     * holds, which is what disambiguates rows that several domains once shared.
+     */
+    @SuppressWarnings("unused")
+    public static void migrateDefaultValueLsids(ModuleContext context)
+    {
+        if (context.isNewInstall())
+            return;
+
+        try (Transaction tx = ExperimentService.get().ensureTransaction())
+        {
+            migrateContainerDefaults();
+            reparentUserDefaults();
+            tx.commit();
+        }
+
+        OntologyManager.clearCaches();
+    }
+
+    private static void migrateContainerDefaults()
+    {
+        SQLFragment sql = new SQLFragment()
+            .append("SELECT o.ObjectId, o.ObjectURI, MIN(pdm.DomainId) AS DomainId, COUNT(DISTINCT pdm.DomainId) AS DomainCount\n")
+            .append("FROM ").append(OntologyManager.getTinfoObject(), "o").append("\n")
+            .append("INNER JOIN ").append(OntologyManager.getTinfoObjectProperty(), "op").append(" ON op.ObjectId = o.ObjectId\n")
+            .append("INNER JOIN ").append(OntologyManager.getTinfoPropertyDomain(), "pdm").append(" ON pdm.PropertyId = op.PropertyId\n")
+            // the legacy prefix is followed by '.'; the qualified form written below inserts '-<kind>' before it
+            .append("WHERE o.ObjectURI LIKE ?\n").add("%:" + LEGACY_CONTAINER_DEFAULTS_PREFIX + ".Folder-%")
+            .append("GROUP BY o.ObjectId, o.ObjectURI");
+
+        List<ContainerDefaultRow> rows = new SqlSelector(ExperimentService.get().getSchema(), sql).getArrayList(ContainerDefaultRow.class);
+        int migrated = 0;
+
+        // Both legacy forms can map onto the same target. The objectId form was written by 25.7 or later, so it is the
+        // newer of the two and is moved first; the name form then loses the NOT EXISTS race below, as it should.
+        for (boolean objectIdForm : new boolean[] {true, false})
+        {
+            for (ContainerDefaultRow row : rows)
+            {
+                Domain domain = resolveDomain(row);
+                if (domain == null)
+                    continue;
+
+                Lsid domainLsid = new Lsid(domain.getTypeURI());
+                if (objectIdForm != new Lsid(row.objectURI()).getObjectId().equals(domainLsid.getObjectId()))
+                    continue;
+
+                String newUri = qualifiedLsid(row.objectURI(), domainLsid);
+                if (newUri == null || newUri.equals(row.objectURI()))
+                    continue;
+
+                if (renameObject(row.objectId(), newUri))
+                    migrated++;
+                else
+                    LOG.warn("Leaving default values at {}: {} already exists.", row.objectURI(), newUri);
+            }
+        }
+
+        LOG.info("Folder-level default values re-keyed by domain kind: {} of {} moved.", migrated, rows.size());
+    }
+
+    /**
+     * The per-user parent object groups a domain's scopes so they can be deleted together. A colliding parent collected
+     * children from several domains, so only one of them can keep it -- the rest get a parent of their own.
+     */
+    private static void reparentUserDefaults()
+    {
+        SQLFragment sql = new SQLFragment()
+            .append("SELECT child.ObjectId, child.ObjectURI, parent.ObjectId AS ParentId, parent.ObjectURI AS ParentURI, parent.Container,\n")
+            .append("  MIN(pdm.DomainId) AS DomainId, COUNT(DISTINCT pdm.DomainId) AS DomainCount\n")
+            .append("FROM ").append(OntologyManager.getTinfoObject(), "parent").append("\n")
+            .append("INNER JOIN ").append(OntologyManager.getTinfoObject(), "child").append(" ON child.OwnerObjectId = parent.ObjectId\n")
+            .append("INNER JOIN ").append(OntologyManager.getTinfoObjectProperty(), "op").append(" ON op.ObjectId = child.ObjectId\n")
+            .append("INNER JOIN ").append(OntologyManager.getTinfoPropertyDomain(), "pdm").append(" ON pdm.PropertyId = op.PropertyId\n")
+            .append("WHERE parent.ObjectURI LIKE ?\n").add("%:" + LEGACY_USER_DEFAULTS_PARENT_PREFIX + ".Folder-%")
+            .append("GROUP BY child.ObjectId, child.ObjectURI, parent.ObjectId, parent.ObjectURI, parent.Container");
+
+        List<UserDefaultRow> rows = new SqlSelector(ExperimentService.get().getSchema(), sql).getArrayList(UserDefaultRow.class);
+
+        // children of each legacy parent, bucketed by the qualified parent they now belong under
+        Map<Long, Map<String, List<UserDefaultRow>>> byLegacyParent = new LinkedHashMap<>();
+        for (UserDefaultRow row : rows)
+        {
+            Domain domain = resolveDomain(row);
+            if (domain == null)
+                continue;
+
+            String newParentUri = qualifiedLsid(row.parentURI(), new Lsid(domain.getTypeURI()));
+            if (newParentUri == null || newParentUri.equals(row.parentURI()))
+                continue;
+
+            byLegacyParent.computeIfAbsent(row.parentId(), k -> new LinkedHashMap<>())
+                .computeIfAbsent(newParentUri, k -> new ArrayList<>()).add(row);
+        }
+
+        int reparented = 0;
+
+        for (Map.Entry<Long, Map<String, List<UserDefaultRow>>> legacyParent : byLegacyParent.entrySet())
+        {
+            boolean legacyParentReused = false;
+
+            for (Map.Entry<String, List<UserDefaultRow>> group : legacyParent.getValue().entrySet())
+            {
+                // Renaming the legacy parent for the first group leaves nothing behind; its children still point at it
+                if (!legacyParentReused && renameObject(legacyParent.getKey(), group.getKey()))
+                {
+                    legacyParentReused = true;
+                    reparented += group.getValue().size();
+                    continue;
+                }
+
+                Container container = ContainerManager.getForId(group.getValue().get(0).container());
+                if (container == null)
+                    continue;
+
+                long newParentId = OntologyManager.ensureObject(container, group.getKey());
+                for (UserDefaultRow row : group.getValue())
+                {
+                    SQLFragment update = new SQLFragment("UPDATE ").append(OntologyManager.getTinfoObject())
+                        .append(" SET OwnerObjectId = ? WHERE ObjectId = ?").addAll(newParentId, row.objectId());
+                    new SqlExecutor(ExperimentService.get().getSchema()).execute(update);
+                    reparented++;
+                }
+            }
+        }
+
+        LOG.info("Per-user default values re-parented by domain kind: {} of {} moved.", reparented, rows.size());
+    }
+
+    /** The owning domain, or null when the object's properties don't identify exactly one resolvable user-created domain. */
+    private static Domain resolveDomain(DefaultValueRow row)
+    {
+        if (row.domainCount() != 1)
+        {
+            LOG.warn("Leaving default values at {}: properties span {} domains, so the owner is ambiguous.", row.objectURI(), row.domainCount());
+            return null;
+        }
+
+        Domain domain = PropertyService.get().getDomain(row.domainId());
+        if (domain == null)
+        {
+            LOG.warn("Leaving default values at {}: domain {} no longer exists.", row.objectURI(), row.domainId());
+            return null;
+        }
+
+        // Domains whose kind can't be resolved have always used the name-based key, so they are already correct
+        DomainKind<?> kind = domain.getDomainKind();
+        return kind != null && kind.isUserCreatedType() ? domain : null;
+    }
+
+    /** Rebuild a default-value LSID with the domain kind folded into the namespace prefix, preserving Folder-/User- scope. */
+    private static String qualifiedLsid(String objectURI, Lsid domainLsid)
+    {
+        Lsid existing = new Lsid(objectURI);
+        if (existing.getNamespaceSuffix() == null)
+            return null;
+        return new Lsid(existing.getNamespacePrefix() + "-" + domainLsid.getNamespacePrefix(), existing.getNamespaceSuffix(), domainLsid.getObjectId()).toString();
+    }
+
+    /** A row already at the target was written under the new scheme and is newer, so it wins. */
+    private static boolean renameObject(long objectId, String newUri)
+    {
+        SQLFragment update = new SQLFragment("UPDATE ").append(OntologyManager.getTinfoObject())
+            .append(" SET ObjectURI = ? WHERE ObjectId = ? AND NOT EXISTS (SELECT 1 FROM ")
+            .append(OntologyManager.getTinfoObject(), "existing").append(" WHERE existing.ObjectURI = ?)")
+            .addAll(newUri, objectId, newUri);
+        return new SqlExecutor(ExperimentService.get().getSchema()).execute(update) > 0;
+    }
+
+    private sealed interface DefaultValueRow permits ContainerDefaultRow, UserDefaultRow
+    {
+        String objectURI();
+        int domainId();
+        int domainCount();
+    }
+
+    private record ContainerDefaultRow(long objectId, String objectURI, int domainId, int domainCount) implements DefaultValueRow {}
+
+    private record UserDefaultRow(long objectId, String objectURI, long parentId, String parentURI, String container, int domainId, int domainCount) implements DefaultValueRow {}
 }

experiment/src/org/labkey/experiment/api/property/PropertyServiceImpl.java

@@ -48,6 +48,7 @@
 import org.labkey.api.data.Table;
 import org.labkey.api.data.TableInfo;
 import org.labkey.api.data.TableSelector;
+import org.labkey.api.defaults.DefaultValueService;
 import org.labkey.api.exceptions.OptimisticConflictException;
 import org.labkey.api.exp.ChangePropertyDescriptorException;
 import org.labkey.api.exp.DomainDescriptor;
@@ -730,11 +731,50 @@ public Map<String, Object> getUsageMetrics()
                 "propertyCountsByConcept", stripUriPrefixes(new SqlSelector(schema,
                         new SQLFragment("SELECT CASE WHEN ConceptURI IS NULL THEN 'null' ELSE ConceptURI END, COUNT(*) AS Count FROM exp.PropertyDescriptor GROUP BY ConceptURI")
                 ).getValueMap(String.class)),
-                        "conditionalFormattingFields", new SqlSelector(schema, new SQLFragment("SELECT COUNT (DISTINCT propertyid) from exp.conditionalformat")).getObject(Long.class),
+                "defaultValuePropertyCounts", Map.of(
+                        "folder", savedDefaultValuePropertyCounts(schema, "%:" + DefaultValueService.DOMAIN_DEFAULT_VALUE_LSID_PREFIX + "%.Folder-%"),
+                        "user", savedDefaultValuePropertyCounts(schema, "%:" + DefaultValueService.USER_DEFAULT_VALUE_LSID_PREFIX + ".Folder-%")
+                ),
+                "conditionalFormattingFields", new SqlSelector(schema, new SQLFragment("SELECT COUNT (DISTINCT propertyid) from exp.conditionalformat")).getObject(Long.class),
                 "storageColumnNameMismatches", storageColumnNameMismatches
         );
     }
 
+    /**
+     * Properties that a user has actually saved a default value for, counted by the property's default value type and
+     * by the namespace prefix of the domain that owns it.
+     */
+    private Map<String, Object> savedDefaultValuePropertyCounts(DbSchema schema, String lsidPattern)
+    {
+        Map<String, Set<Integer>> byDefaultValueType = new HashMap<>();
+        Map<String, Set<Integer>> byDomainKind = new HashMap<>();
+
+        // One row per property/domain pair, so a property shared by two domains counts toward both kinds
+        SQLFragment sql = new SQLFragment("""
+                SELECT DISTINCT OP.PropertyId, PD.DefaultValueType, DD.DomainURI
+                FROM exp.ObjectProperty OP
+                    INNER JOIN exp.PropertyDescriptor PD ON PD.PropertyId = OP.PropertyId
+                    INNER JOIN exp.PropertyDomain PDM ON PDM.PropertyId = OP.PropertyId
+                    INNER JOIN exp.DomainDescriptor DD ON DD.DomainId = PDM.DomainId
+                WHERE OP.ObjectId IN (SELECT ObjectId FROM exp.Object WHERE ObjectURI LIKE ?)""").add(lsidPattern);
+
+        new SqlSelector(schema, sql).forEachMap(row -> {
+            Integer propertyId = ((Number) row.get("PropertyId")).intValue();
+            Object defaultValueType = row.get("DefaultValueType");
+            Lsid domainLsid = new Lsid((String) row.get("DomainURI"));
+
+            byDefaultValueType.computeIfAbsent(defaultValueType == null ? "null" : defaultValueType.toString(), k -> new HashSet<>()).add(propertyId);
+            byDomainKind.computeIfAbsent(domainLsid.isValid() ? domainLsid.getNamespacePrefix() : "null", k -> new HashSet<>()).add(propertyId);
+        });
+
+        return Map.of("byDefaultValueType", countDistinct(byDefaultValueType), "byDomainKind", countDistinct(byDomainKind));
+    }
+
+    private static Map<String, Integer> countDistinct(Map<String, Set<Integer>> propertyIds)
+    {
+        return propertyIds.entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().size()));
+    }
+
     /**
      * @return a map where the URI keys have been stripped of everything before the hash or xsd: prefix.
      * http://www.labkey.org/exp/xml#attachment -> attachment

experiment/src/org/labkey/experiment/defaults/DefaultValueServiceImpl.java

@@ -17,6 +17,7 @@
 
 import org.apache.commons.beanutils.ConversionException;
 import org.apache.logging.log4j.LogManager;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.labkey.api.data.Container;
 import org.labkey.api.data.DbScope;
@@ -52,7 +53,6 @@
 
 public class DefaultValueServiceImpl implements DefaultValueService
 {
-    private static final String USER_DEFAULT_VALUE_LSID_PREFIX = "UserDefaultValue";
     private static final String USER_DEFAULT_VALUE_DOMAIN_PARENT = "UserDefaultValueParent";
 
     private final Lock _lock = new ReentrantLockWithName(DefaultValueServiceImpl.class, "_lock");
@@ -64,7 +64,7 @@ private String getContainerDefaultsLSID(Container container, Domain domain)
         if (kind != null && kind.isUserCreatedType())
         {
             Lsid domainLsid = new Lsid(domain.getTypeURI());
-            return (new Lsid(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, suffix, domainLsid.getObjectId())).toString();
+            return (new Lsid(qualifyByDomainKind(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, domainLsid), suffix, domainLsid.getObjectId())).toString();
         }
         else // for internal domains (such as audit domains), the domain name and objectId are often not the same.
             return (new Lsid(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, suffix, domain.getName())).toString();
@@ -78,18 +78,26 @@ private String getUserDefaultsParentLSID(Container container, User user, Domain
         if (kind != null && kind.isUserCreatedType())
         {
             Lsid domainLsid = new Lsid(domain.getTypeURI());
-            return (new Lsid(USER_DEFAULT_VALUE_DOMAIN_PARENT, suffix, domainLsid.getObjectId())).toString();
+            return (new Lsid(qualifyByDomainKind(USER_DEFAULT_VALUE_DOMAIN_PARENT, domainLsid), suffix, domainLsid.getObjectId())).toString();
         }
         else // for internal domains (such as audit domains), the domain name and objectId are often not the same.
             return (new Lsid(USER_DEFAULT_VALUE_DOMAIN_PARENT, suffix, domain.getName())).toString();
     }
 
+    // GitHub Issue #1569: Qualifying LSID by domain kind since multiple data types have distinct domain kinds
+    // Ex: assay designs have AssayDomain-Batch, AssayDomain-Run, AssayDomain-Result
+    private String qualifyByDomainKind(@NotNull String lsidPrefix, @NotNull Lsid domainLsid)
+    {
+        return lsidPrefix + "-" + domainLsid.getNamespacePrefix();
+    }
+
     private static final String WILD_CARD_PLACEHOLDER = "WILDCARD";
 
     private String getUserDefaultsWildcardLSID(Container container, Domain domain, boolean parentObject)
     {
-        String suffix = "Folder-" + container.getRowId() + ".User-" + WILD_CARD_PLACEHOLDER + (!parentObject ? "." + WILD_CARD_PLACEHOLDER : "");
-        String objectId = domain.getName();
+        String suffix = "Folder-" + container.getRowId() + ".User-" + WILD_CARD_PLACEHOLDER;
+        // getUserDefaultsLSID() appends the scope to the objectId, not to the namespace, so the wildcard has to match there
+        String objectId = domain.getName() + (!parentObject ? "." + WILD_CARD_PLACEHOLDER : "");
         String lsid = (new Lsid(USER_DEFAULT_VALUE_LSID_PREFIX, suffix, objectId)).toString();
         // this hack is to include '%' characters in an LSID-like string.  The '%' character can't be part of the
         // lsid components passed to the Lsid constructor, or it will be encoded as '%25'.