Disable LabKey caching of ResultSet by default#7711
Conversation
|
I queued MS2 and Panorama suites as they may reverse ResultSets as part of rendering their nested grids. https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_ModuleSuites_Ms2Postgres/4022367 |
QueryHelper funnels every select through SelectBuilder.select(), whose default flipped to an uncached, streaming result set on this branch. Many QueryHelper callers (15 files across wnprc-modules, ehrModules, OConnorLabModules, and premiumModules) rely on the cached result set's full API — particularly getRowMap(), which a streaming result set does not support. In wnprc-modules' SimpleQuery the resulting UnsupportedOperationException is swallowed and surfaces as an empty result, causing WNPRC_EHRTest's consistent "Account acct103 not found in aliases table" setup failure on this branch. Request a cached result set explicitly to preserve the helper's longstanding semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
labkey-adam
left a comment
labkey-adam
left a comment
There was a problem hiding this comment.
Some comments to consider, but no further review required
| tableMap.put("__DATA", assayDataTable); | ||
|
|
||
| try (ResultSet rs = QueryService.get().select(schema, sqlf.getSQL(), tableMap, false, true)) | ||
| try (ResultSet rs = QueryService.get().getSelectBuilder(schema, sqlf.getSQL(), false, tableMap).select(true)) |
There was a problem hiding this comment.
Don't we want getSelectBuilder() to take a SQLFragment?
There was a problem hiding this comment.
@labkey-matthewb and I discussed this. Ultimately, we want it to take a LabKeySQLFragment or similar and keep SQLFragment for raw DB SQL.
| ArrayList<String> ids = new ArrayList<>(); | ||
| while (rs.next()) | ||
| ids.add(rs.getString(1)); | ||
| return ids; |
There was a problem hiding this comment.
Ideally, we'd convert this all to .buildSqlSelector().getArrayList(String.class); Maybe that's for another day.
There was a problem hiding this comment.
Yes, I don't want to figure out how to test this right now.
| default Results select() | ||
| { | ||
| return select(true); | ||
| return select(false); |
There was a problem hiding this comment.
Maybe too late for this, but it would be nice to replace the various boolean parameters with enums that elucidate the behaviors.
There was a problem hiding this comment.
Good idea for next time.
| return select.select(); | ||
| // select(true): legacy callers (especially EHR modules) rely on the cached result set's full API, | ||
| // e.g. getRowMap(), which a streaming result set does not support | ||
| return select.select(true); |
| dcMySampleParent = mySampleParent.getRenderer(); | ||
|
|
||
| assertTrue(results.next()); | ||
| ctx.setRow(results.getRowMap()); |
There was a problem hiding this comment.
Calling getRowMap() requires a cached Results, right? Should we document that method as such?
There was a problem hiding this comment.
JavaDoc added.
| } | ||
| if (!_container.equals(that._container)) return false; | ||
| return _schema.equals(that._schema); | ||
| } |
There was a problem hiding this comment.
Isn't the default equals() essentially this?
There was a problem hiding this comment.
Seems odd to remove hashCode() but not equals()
There was a problem hiding this comment.
Yes. Removed. I told IntelliJ to automatically apply some auto-refactors on save and it's surprisingly aggressive at converting some classes to records.
| return false; | ||
|
|
||
| return true; | ||
| if (obj instanceof SessionQuery(String sql, String metadata)) |
There was a problem hiding this comment.
Whoa, what is this strange syntax? Is that strictly a record thing?
There was a problem hiding this comment.
Again, default equals() won't do here?
There was a problem hiding this comment.
Same thing. Simplified.
3 participants
Rationale
We don't want to cache more than we need to. We just disabled Postgres JDBC caching for QueryService.getSelectBuilder() scenarios. Most scenarios shouldn't need a LabKey cache either.
Changes
CachedResultSetby defaultQueryService.select()andQueryService.selector()methods toQueryService.getSelectBuilder()Tasks 📍
Manual Testing