Skip to content

Issue 52728: LKSM: Update from file with duplicate rows is only updating once.#6571

Merged
XingY merged 12 commits intodevelopfrom
fb_issue52728
Apr 22, 2025
Merged

Issue 52728: LKSM: Update from file with duplicate rows is only updating once.#6571
XingY merged 12 commits intodevelopfrom
fb_issue52728

Conversation

@XingY
Copy link
Copy Markdown
Contributor

@XingY XingY commented Apr 16, 2025

Rationale

Related Pull Requests

Changes

  • Added checkDuplicateUpdate to updateRow in various QUS so the same rows cannot be updated in the same updateRows call
  • Modified ExistingRecordDataIterator to check for previously seen keys so the same rows are not provided during update/merge
  • Add junit test coverage for updating/merging the same list rows using updateRows and DIB

@XingY XingY marked this pull request as ready for review April 17, 2025 22:38
Comment thread api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java Outdated
Comment thread api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java Outdated
@Override
public List<Map<String, Object>> updateRows(User user, Container container, List<Map<String, Object>> rows, List<Map<String, Object>> oldKeys, BatchValidationException errors, @Nullable Map<Enum, Object> configParameters, Map<String, Object> extraScriptContext)
throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException
throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException // TODO
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still TODO?

Comment thread api/src/org/labkey/api/query/AbstractQueryUpdateService.java
assertEquals("THREE", rows.get(3).get("s"));
assertNull(rows.get(3).get("i"));

// merge should fail if duplicate keys are provided
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue reference here?

Comment thread experiment/src/client/test/integration/DataClassCrud.ispec.ts Outdated
Comment thread api/src/org/labkey/api/dataiterator/ExistingRecordDataIterator.java
@XingY XingY requested a review from labkey-susanh April 22, 2025 16:21
Copy link
Copy Markdown
Contributor

@labkey-susanh labkey-susanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's still a lingering TODO in SpecimenUpdateService. Is that meant for this PR? I'll approve, but let me know if you want re-review for any change there (not that I know much about that service).

@XingY
Copy link
Copy Markdown
Contributor Author

XingY commented Apr 22, 2025

Looks like there's still a lingering TODO in SpecimenUpdateService. Is that meant for this PR? I'll approve, but let me know if you want re-review for any change there (not that I know much about that service).

I've removed that stale TODO. thanks for pointing out.

@XingY XingY merged commit b7ab3e6 into develop Apr 22, 2025
4 checks passed
@XingY XingY deleted the fb_issue52728 branch April 22, 2025 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants