From 3049de2f8ee70a75656dc038cad91bff597406b8 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 7 Aug 2026 16:54:55 -0700 Subject: [PATCH 1/4] getDataClassByLsid --- .../experiment/api/DataClassDomainKind.java | 19 +++++++++++++++++-- .../experiment/api/ExperimentServiceImpl.java | 5 +++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java index 409c15c5366..752c35dad4e 100644 --- a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java +++ b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java @@ -24,6 +24,7 @@ import org.labkey.api.compliance.ComplianceService; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; +import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbSchemaType; import org.labkey.api.data.DbScope; @@ -193,7 +194,14 @@ public DefaultValueType getDefaultDefaultType(Domain domain) private ExpDataClassImpl getDataClass(Domain domain) { - return ExperimentServiceImpl.get().getDataClass(domain.getTypeURI()); + return getDataClassByLsid(domain.getContainer(), domain.getTypeURI()); + } + + // Prefer the cached container-scoped lookup; fall back to the uncached global query for a DataClass outside that scope. + private static ExpDataClassImpl getDataClassByLsid(@Nullable Container c, String lsid) + { + ExpDataClassImpl dataClass = c == null ? null : ExperimentServiceImpl.get().getDataClassByLsid(c, lsid, true); + return dataClass != null ? dataClass : ExperimentServiceImpl.get().getDataClass(lsid); } @Override @@ -470,7 +478,14 @@ public UpdateableTableInfo.ObjectUriType getObjectUriColumn() @Override public DataClassDomainKindProperties getDomainKindProperties(GWTDomain domain, Container container, User user) { - ExpDataClass dc = domain != null ? ExperimentService.get().getDataClass(domain.getDomainURI()) : null; + ExpDataClass dc = null; + if (domain != null) + { + // domain.getContainer() is the definition container, but is unset on client-supplied domains, so fall back to the request container + Container domainContainer = ContainerManager.getForId(domain.getContainer()); + dc = getDataClassByLsid(domainContainer == null ? container : domainContainer, domain.getDomainURI()); + } + return new DataClassDomainKindProperties(dc); } diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 3f1c735caab..ab3148d912b 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -1843,6 +1843,11 @@ public ExpDataClassImpl getDataClass(@NotNull Container c, long rowId, boolean i return getDataClass(c, includeProjectAndShared, (dataClass -> dataClass.getRowId() == rowId)); } + public @Nullable ExpDataClassImpl getDataClassByLsid(@NotNull Container c, @NotNull String lsid, boolean includeProjectAndShared) + { + return getDataClass(c, includeProjectAndShared, (dataClass -> lsid.equals(dataClass.getLSID()))); + } + private ExpDataClassImpl getDataClass(@NotNull Container c, boolean includeProjectAndShared, Predicate predicate) { List containerIds = createContainerList(c, includeProjectAndShared); From ae45ec1cb327c312789c2d000e53316b61e0400f Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 10 Aug 2026 09:15:17 -0700 Subject: [PATCH 2/4] Cache data classes by LSID (just like sample types) --- .../experiment/api/DataClassDomainKind.java | 19 ++--------- .../experiment/api/ExperimentServiceImpl.java | 33 +++++++++++++------ 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java index 752c35dad4e..409c15c5366 100644 --- a/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java +++ b/experiment/src/org/labkey/experiment/api/DataClassDomainKind.java @@ -24,7 +24,6 @@ import org.labkey.api.compliance.ComplianceService; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; -import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbSchemaType; import org.labkey.api.data.DbScope; @@ -194,14 +193,7 @@ public DefaultValueType getDefaultDefaultType(Domain domain) private ExpDataClassImpl getDataClass(Domain domain) { - return getDataClassByLsid(domain.getContainer(), domain.getTypeURI()); - } - - // Prefer the cached container-scoped lookup; fall back to the uncached global query for a DataClass outside that scope. - private static ExpDataClassImpl getDataClassByLsid(@Nullable Container c, String lsid) - { - ExpDataClassImpl dataClass = c == null ? null : ExperimentServiceImpl.get().getDataClassByLsid(c, lsid, true); - return dataClass != null ? dataClass : ExperimentServiceImpl.get().getDataClass(lsid); + return ExperimentServiceImpl.get().getDataClass(domain.getTypeURI()); } @Override @@ -478,14 +470,7 @@ public UpdateableTableInfo.ObjectUriType getObjectUriColumn() @Override public DataClassDomainKindProperties getDomainKindProperties(GWTDomain domain, Container container, User user) { - ExpDataClass dc = null; - if (domain != null) - { - // domain.getContainer() is the definition container, but is unset on client-supplied domains, so fall back to the request container - Container domainContainer = ContainerManager.getForId(domain.getContainer()); - dc = getDataClassByLsid(domainContainer == null ? container : domainContainer, domain.getDomainURI()); - } - + ExpDataClass dc = domain != null ? ExperimentService.get().getDataClass(domain.getDomainURI()) : null; return new DataClassDomainKindProperties(dc); } diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index ab3148d912b..35caf3474a8 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -336,6 +336,10 @@ public class ExperimentServiceImpl implements ExperimentService, ObjectReference private final Cache EXPERIMENT_RUN_CACHE = DatabaseCache.get(getExpSchema().getScope(), getTinfoExperimentRun().getCacheSize(), "Experiment Run by LSID", new ExperimentRunCacheLoader()); + /** DataClass LSID -> Container */ + private final Cache dataClassLsidCache = CacheManager.getStringKeyCache(CacheManager.UNLIMITED, CacheManager.DAY, "DataClass to container"); + + /** ContainerId -> DataClasses */ private final Cache> dataClassCache = CacheManager.getBlockingStringKeyCache(CacheManager.UNLIMITED, CacheManager.DAY, "Data classes", (containerId, _) -> { Container c = ContainerManager.getForId(containerId); @@ -1843,11 +1847,6 @@ public ExpDataClassImpl getDataClass(@NotNull Container c, long rowId, boolean i return getDataClass(c, includeProjectAndShared, (dataClass -> dataClass.getRowId() == rowId)); } - public @Nullable ExpDataClassImpl getDataClassByLsid(@NotNull Container c, @NotNull String lsid, boolean includeProjectAndShared) - { - return getDataClass(c, includeProjectAndShared, (dataClass -> lsid.equals(dataClass.getLSID()))); - } - private ExpDataClassImpl getDataClass(@NotNull Container c, boolean includeProjectAndShared, Predicate predicate) { List containerIds = createContainerList(c, includeProjectAndShared); @@ -1879,12 +1878,26 @@ public ExpDataClassImpl getDataClass(long rowId) @Override public @Nullable ExpDataClassImpl getDataClass(@NotNull String lsid) { - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("lsid"), lsid); - DataClass dataClass = new TableSelector(getTinfoDataClass(), filter, null).getObject(DataClass.class); - if (dataClass == null) - return null; + String containerId = dataClassLsidCache.get(lsid); + Container c = null; + if (containerId != null) + c = ContainerManager.getForId(containerId); - return new ExpDataClassImpl(dataClass); + ExpDataClassImpl dataClass = null; + if (null != c) + dataClass = getDataClass(c, false, dc -> lsid.equals(dc.getLSID())); + if (null == dataClass) + { + Filter filter = new SimpleFilter(ExpDataClassTable.Column.LSID.fieldKey(), lsid); + DataClass dc = new TableSelector(getTinfoDataClass(), filter, null).getObject(DataClass.class); + if (dc != null) + dataClass = new ExpDataClassImpl(dc); + } + + if (null != dataClass && !dataClass.getContainer().getId().equals(containerId)) + dataClassLsidCache.put(lsid, dataClass.getContainer().getId()); + + return dataClass; } @Override From 9c9d812d81cbf865ac543e31a3a745e6542431ac Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 10 Aug 2026 10:11:02 -0700 Subject: [PATCH 3/4] Review feedback --- .../experiment/api/ExperimentServiceImpl.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 35caf3474a8..50e98716caa 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -1894,8 +1894,12 @@ public ExpDataClassImpl getDataClass(long rowId) dataClass = new ExpDataClassImpl(dc); } - if (null != dataClass && !dataClass.getContainer().getId().equals(containerId)) - dataClassLsidCache.put(lsid, dataClass.getContainer().getId()); + if (null != dataClass) + { + Container dcContainer = dataClass.getContainer(); + if (dcContainer != null && dcContainer.getId().equals(containerId)) + dataClassLsidCache.put(lsid, dataClass.getContainer().getId()); + } return dataClass; } @@ -8151,7 +8155,10 @@ public ValidationException updateDataClass(@NotNull Container c, @NotNull User u GWTDomain update, @Nullable String auditUserComment) { - ExpDataClassImpl dataClass = (ExpDataClassImpl) dc; + // Re-read so the mutations below don't write through to a DataClass bean shared via dataClassCache + ExpDataClassImpl dataClass = getDataClass(dc.getRowId()); + if (dataClass == null) + return new ValidationException("Data class not found: " + dc.getName()); Map oldProps = dataClass.getAuditRecordMap(); Map newProps = properties != null ? properties.getAuditRecordMap() : dataClass.getAuditRecordMap() /* no update */; From 418210e357e6948103f30838c163436459fa3ed2 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Mon, 10 Aug 2026 10:11:15 -0700 Subject: [PATCH 4/4] Tests --- .../api/ExpDataClassDataTestCase.jsp | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp index 523d823869f..aea05c07c71 100644 --- a/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp +++ b/experiment/src/org/labkey/experiment/api/ExpDataClassDataTestCase.jsp @@ -49,6 +49,7 @@ <%@ page import="org.labkey.api.exp.api.DataClassDomainKindProperties" %> <%@ page import="org.labkey.api.exp.api.ExpData" %> <%@ page import="org.labkey.api.exp.api.ExpDataClass" %> +<%@ page import="org.labkey.api.exp.api.ExperimentJSONConverter" %> <%@ page import="org.labkey.api.exp.api.ExperimentService" %> <%@ page import="org.labkey.api.exp.list.ListDefinition" %> <%@ page import="org.labkey.api.exp.list.ListItem" %> @@ -94,6 +95,7 @@ <%@ page import="java.util.ArrayList" %> <%@ page import="java.util.Collection" %> <%@ page import="java.util.Collections" %> +<%@ page import="java.util.HashMap" %> <%@ page import="java.util.HashSet" %> <%@ page import="static java.util.Collections.emptyList" %> <%@ page import="static org.junit.Assert.*" %> @@ -1175,6 +1177,79 @@ public void testUpdateAuditForLongField() throws Exception } } +@Test +public void testDataClassLsidCache() throws Exception +{ + final Container sub = ContainerManager.createContainer(c, "subLsidCache", _user); + ExpDataClassImpl projectDataClass = ExperimentServiceImpl.get().createDataClass(c, _user, "lsidCacheProject", null, List.of(new GWTPropertyDescriptor("prop", "string")), emptyList(), null, null); + ExpDataClassImpl subDataClass = ExperimentServiceImpl.get().createDataClass(sub, _user, "lsidCacheSub", null, List.of(new GWTPropertyDescriptor("prop", "string")), emptyList(), null, null); + String projectLsid = projectDataClass.getLSID(); + String subLsid = subDataClass.getLSID(); + + // first lookup populates the mapping, second is served from the container's data class cache + for (int i = 0; i < 2; i++) + { + ExpDataClass fromProject = ExperimentService.get().getDataClass(projectLsid); + assertNotNull("Lookup " + i + " by LSID should resolve the data class", fromProject); + assertEquals("lsidCacheProject", fromProject.getName()); + assertEquals(projectDataClass.getRowId(), fromProject.getRowId()); + assertEquals(c.getId(), fromProject.getContainer().getId()); + + ExpDataClass fromSub = ExperimentService.get().getDataClass(subLsid); + assertNotNull("Lookup " + i + " by LSID should resolve the subfolder data class", fromSub); + assertEquals("lsidCacheSub", fromSub.getName()); + assertEquals(sub.getId(), fromSub.getContainer().getId()); + } + + projectDataClass.delete(_user); + assertNull("Deleted data class should not resolve from a stale LSID mapping", ExperimentService.get().getDataClass(projectLsid)); + assertNotNull("Sibling data class should be unaffected", ExperimentService.get().getDataClass(subLsid)); +} + +@Test +public void testFailedDataClassUpdateDoesNotCorruptCache() throws Exception +{ + ExperimentServiceImpl.get().createDataClass(c, _user, "failedUpdateParent", null, List.of(new GWTPropertyDescriptor("prop", "string")), emptyList(), null, null); + + DataClassDomainKindProperties createOptions = new DataClassDomainKindProperties(); + createOptions.setDescription("original description"); + ExpDataClassImpl child = ExperimentServiceImpl.get().createDataClass(c, _user, "failedUpdateChild", createOptions, List.of(new GWTPropertyDescriptor("prop", "string")), emptyList(), null, null); + + // hasMissingRequiredParent() short-circuits on an empty data class, so the rejection needs a parentless row + List> rows = new ArrayList<>(); + Map row = new CaseInsensitiveHashMap<>(); + row.put("name", "failedUpdateChild-1"); + rows.add(row); + helper.insertRows(c, rows, child.getName()); + + // warm the mapping so the second lookup comes back from the container's data class cache + assertNotNull(ExperimentService.get().getDataClass(child.getLSID())); + final ExpDataClass toUpdate = ExperimentService.get().getDataClass(child.getLSID()); + assertNotNull(toUpdate); + + Map parentAlias = new HashMap<>(); + parentAlias.put("inputType", ExperimentJSONConverter.DATA_INPUTS_ALIAS_PREFIX + "failedUpdateParent"); + parentAlias.put("required", true); + + final DataClassDomainKindProperties options = new DataClassDomainKindProperties(); + options.setRowId(child.getRowId()); + options.setName("failedUpdateChildRenamed"); + options.setDescription("attempted description"); + options.setImportAliases(Map.of("parentAlias", parentAlias)); + + final GWTDomain original = DomainUtil.getDomainDescriptor(_user, child.getDomain()); + final GWTDomain update = DomainUtil.getDomainDescriptor(_user, child.getDomain()); + + ApiUsageException e = assertThrows(ApiUsageException.class, + () -> ExperimentService.get().updateDataClass(c, _user, toUpdate, options, original, update, null)); + assertThat(e.getMessage(), containsString("cannot be required as a parent type")); + + ExpDataClass reloaded = ExperimentService.get().getDataClass(c, "failedUpdateChild"); + assertNotNull("Original name should still resolve after a rejected update", reloaded); + assertEquals("Description should not reflect the rejected update", "original description", reloaded.getDescription()); + assertNull("Attempted name should not resolve after a rejected update", ExperimentService.get().getDataClass(c, "failedUpdateChildRenamed")); +} + private @NotNull TableInfo getDataClassTable(String dataClassName) { UserSchema schema = QueryService.get().getUserSchema(_user, c, ExpSchema.SCHEMA_EXP_DATA);