Use UpdateService instead of raw DbSchema layer for assay_template edits#292
Use UpdateService instead of raw DbSchema layer for assay_template edits#292bbimber wants to merge 5 commits into
Conversation
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
Ok this looks good. I added a few comments. A couple of them I think need to be fixed before merging this in.
|
@bbimber there are a couple test failures I think related to my suggestions above. Let me know if you want me to push any of those changes. |
|
hey @labkey-martyp: this is a very old codepath that my group doesnt really use anymore, so it took a minute to remember how it's all supposed to work. My last commits tightened up behavior for cross-container rows. The intended usage involves creating/editing/saving rows from the same container, including workbooks. My changes some times query across a larger container space it identify the existence of a template, but errors if it's from the wrong container. |
labkey-martyp
force-pushed
the
25.7_fb_scope_assay_template_lookups2
branch
from
cfe725e to
d8e37ff
Compare
This module sets sourceCompatibility=17, but List.getFirst() was added in Java 21 (SequencedCollection), so it failed to resolve. Replaced both calls with List.get(0).
…ted validations
The switch to QueryUpdateService passed the update key as Map.of("rowId", templateId), but the primary key column is "rowid" and Map.of produces a case-sensitive map, so DefaultQueryUpdateService.getKeys() never found it and threw InvalidKeyException: Value for key field 'rowid' was null or not supplied!. This broke ViralLoadAssayTest. Corrected the key to "rowid" in both AssayHelper.saveTemplate and DefaultAssayParser.saveTemplate.
The broadened catch (Exception) in AssayHelper.saveTemplate was logging expected BatchValidationExceptions from validateTemplate()/insertRows() via _log.error, so normal validation failures (e.g. "Must provide at least 2 negative controls") landed in labkey-errors.log and tripped the Selenium harness's checkErrors(), failing ELISPOT_AssayTest. Let BatchValidationException propagate ahead of the generic catch in both methods so these surface as client validation errors, not server errors.
labkey-martyp
left a comment
labkey-martyp
left a comment
There was a problem hiding this comment.
Ok tests passing now. Ready to merge in.
2 participants
@labkey-martyp, this is another take on: #290. I think by using UpdateService instead of DbSchema directly we solve the container/permission issues. I added some additional validation over update permissions to fail faster as well.