Consolidate Dataclass data update methods - use DIB for update only by XingY · Pull Request #7483 · LabKey/platform

XingY · 2026-03-10T03:06:33Z

Rationale

Related Pull Requests

Changes

Copilot

Pull request overview

This PR consolidates DataClass and Sample update behavior around DataIteratorBuilder (DIB)-based updates, removing LSID as an update/merge key and cleaning up deprecated DataClass storage by dropping the provisioned lsid column via a module upgrade.

Changes:

Remove LSID-as-key support for sample/data update & merge; enforce RowId or Name-based keys and ignore LSID when provided alongside valid keys.
Introduce shared updateRowsUsingPartitionedDIB() logic in AbstractQueryUpdateService and route Sample/DataClass updates through it.
Remove provisioned DataClass lsid column (domain kind + upgrade code + DB scripts) and update integration tests accordingly.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
experiment/src/org/labkey/experiment/samples/AbstractExpFolderImporter.java	Stops setting the removed `UseLsidForUpdate` option during TSV import.
experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java	Routes updates through partitioned DIB; removes LSID-key paths and legacy update methods.
experiment/src/org/labkey/experiment/api/ExpDataImpl.java	Selects DataClass provisioned properties by RowId (not LSID) to support dropped provisioned LSID column.
experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java	Enforces RowId/Name keys, blocks LSID-only keying, blocks RowId in merge (behind feature), and uses partitioned DIB updates.
experiment/src/org/labkey/experiment/api/DataClassDomainKind.java	Removes `lsid` from provisioned DataClass base properties, indexes, and FKs.
experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java	Adds deferred upgrade to drop provisioned DataClass `lsid` column (and related indices).
experiment/src/org/labkey/experiment/ExperimentModule.java	Bumps schema version to 26.005 and centralizes experimental flag constant usage.
experiment/src/org/labkey/experiment/ExpDataIterators.java	Updates derivation/update logic to use RowId/Name instead of LSID-for-update config; removes LSID update config usage.
experiment/src/client/test/integration/DataClassCrud.ispec.ts	Expands integration coverage for keying behavior (RowId/Name/LSID) and partitioned-duplicate detection.
experiment/resources/schemas/dbscripts/sqlserver/exp-26.004-26.005.sql	Executes Java upgrade to drop provisioned DataClass `lsid` column (SQL Server).
experiment/resources/schemas/dbscripts/postgresql/exp-26.004-26.005.sql	Executes Java upgrade to drop provisioned DataClass `lsid` column (PostgreSQL).
api/src/org/labkey/api/query/DefaultQueryUpdateService.java	Removes conversion-table hook and always uses `getDbTable()` for `convertTypes()` entrypoint.
api/src/org/labkey/api/query/AbstractQueryUpdateService.java	Adds `updateRowsUsingPartitionedDIB()` and small null-iterator safety in `_pump()`.
api/src/org/labkey/api/exp/query/ExpDataTable.java	Reorders enum constants (no functional behavior change expected).
api/src/org/labkey/api/exp/api/ExperimentService.java	Removes `UseLsidForUpdate` query option; adds shared experimental feature constant.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-10T03:15:16Z

experiment/src/client/test/integration/DataClassCrud.ispec.ts

+        // using update from file, verify update using rowId for data that doesn't exist on this datacalss should fail.
+        errorResp = await ExperimentCRUDUtils.importData(server, "RowId\tDescription\n" + row3RowId + "\tupdate\n", emptyDataClass, "UPDATE", topFolderOptions, editorUserOptions);
+        expect(errorResp.text).toContain('Data not found for [' + row3RowId + ']');


Typo in comment: "datacalss" should be "dataclass".

Copilot · 2026-03-10T03:15:17Z

experiment/src/client/test/integration/DataClassCrud.ispec.ts

+        // update date twice specifying the rowId across multiple partitions
+        await server.post('query', 'updateRows', {


Typo in comment: "update date" should be "update data".

Copilot · 2026-03-10T03:15:17Z

experiment/src/org/labkey/experiment/ExperimentModule.java

+        OptionalFeatureService.get().addExperimentalFeatureFlag(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE, "Allow RowId to be accepted when merging samples or dataclass data",
                "If the incoming data includes a RowId column we will allow the column but ignore it's values.", false);


Grammar in this feature description: "ignore it's values" should be "ignore its values".

Copilot · 2026-03-10T03:15:17Z

experiment/src/org/labkey/experiment/ExpDataIterators.java

                di = new SampleUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);
            else
-                di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);
+                di = new DataUpdateDerivationDataIterator(di, context, _container, _user, _currentDataType, _checkRequiredParents);//


Remove the trailing "//" at the end of this line; it looks like leftover debugging text and adds noise to the code.

Copilot · 2026-03-10T03:15:18Z

experiment/src/org/labkey/experiment/ExpDataIterators.java

            if (hasNext)
            {
-                String lsid = (String) get(_lsidCol);
+                String lsid = (String) get(_lsidCol); // why lsid?, insert or merge


Please remove the inline comment "// why lsid?, insert or merge". It reads like a note-to-self and doesn't clarify behavior for future readers.

Copilot · 2026-03-10T03:15:18Z

experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java

            for (Map<String, Object> dataRow : dataRows.values())
-                lsids.add((String) dataRow.get("lsid"));
+                lsids.add((String) dataRow.get("lsid")); // ?


Remove the "// ?" comment here. If there's an open question about whether LSIDs are always present in these rows, it should be resolved in code (e.g., null-check + clear error) rather than left as an inline question.

Copilot · 2026-03-10T03:15:18Z

experiment/src/client/test/integration/DataClassCrud.ispec.ts

+        // update date twice specifying the name across multiple partitions
        await server.post('query', 'updateRows', {


Typo in comment: "update date" should be "update data".

labkey-adam · 2026-03-10T03:19:51Z

@XingY there's an exp-26.004-26.005.sql script in 26.3 that should be merged to develop shortly.

XingY · 2026-03-10T16:56:49Z

@XingY there's an exp-26.004-26.005.sql script in 26.3 that should be merged to develop shortly.

OK. Thanks for the heads-up.

XingY added 3 commits March 9, 2026 19:23

Consolidate Dataclass data update methods - use DIB for update only

e661698

Enable upgrade script

09dd6b0

CRLF

b5e2ac2

XingY requested a review from Copilot March 10, 2026 03:07

Copilot started reviewing on behalf of XingY March 10, 2026 03:08 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

XingY added 6 commits March 10, 2026 11:16

Merge remote-tracking branch 'origin/develop' into fb_sourceDIB

dc19ef1

fix build

4d9ff01

merge from develop

a678378

fix merge

a68e8a8

fix merge

9d5aa75

fix update

4893660

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate Dataclass data update methods - use DIB for update only#7483

Consolidate Dataclass data update methods - use DIB for update only#7483
XingY wants to merge 9 commits intodevelopfrom
fb_sourceDIB

XingY commented Mar 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

Copilot AI Mar 10, 2026

Uh oh!

labkey-adam commented Mar 10, 2026

Uh oh!

XingY commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// update date twice specifying the rowId across multiple partitions
		await server.post('query', 'updateRows', {

		OptionalFeatureService.get().addExperimentalFeatureFlag(ExperimentService.EXPERIMENTAL_FEATURE_ALLOW_ROW_ID_MERGE, "Allow RowId to be accepted when merging samples or dataclass data",
		"If the incoming data includes a RowId column we will allow the column but ignore it's values.", false);

		// update date twice specifying the name across multiple partitions
		await server.post('query', 'updateRows', {

Conversation

XingY commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

labkey-adam commented Mar 10, 2026

Uh oh!

XingY commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XingY commented Mar 10, 2026 •

edited

Loading