Make study crosstab report more robust#7342
Conversation
labkey-adam
left a comment
labkey-adam
left a comment
There was a problem hiding this comment.
Left some suggesitons... none required
| statSet.add(Stats.VAR); | ||
| else if ("Median".equals(stat)) | ||
| statSet.add(Stats.MEDIAN); | ||
| ResultSetUtil.close(results); |
There was a problem hiding this comment.
try-with-resources instead?
There was a problem hiding this comment.
try-with-resources is kinda clunky when it comes to potentially null values, so I went with this direction.
| else if ("Var".equals(stat)) | ||
| statSet.add(Stats.VAR); | ||
| else if ("Median".equals(stat)) | ||
| statSet.add(Stats.MEDIAN); |
There was a problem hiding this comment.
Old code, but seems so clunky. Especially when we have (unused) Stats.getStatFromString()... though we should probably switch ALL_STATS to a Map<String, StatDefinition> to improve this. Not required for this PR, of course.
| builder.addOption(col.getLabel(), col.getFieldKey().encode()) | ||
| .selected(null != selected && selected.equalsIgnoreCase(col.getFieldKey().encode())); | ||
| builder.addOption(new OptionBuilder(col.getLabel(), col.getFieldKey().encode()) | ||
| .selected(null != selected && selected.equalsIgnoreCase(col.getFieldKey().encode()))); |
There was a problem hiding this comment.
Was there a problem with the previous code? Looks equivalent but more verbose.
There was a problem hiding this comment.
Yes. addOption() returns the SelectBuilder so the old code called selected() on that, passing true or false. Those aren't the values of any options, so nothing was selected. This change calls selected() on the OptionBuilder
There was a problem hiding this comment.
Gotcha... subtle, I misinterpreted the parens.
labkey-tchad
left a comment
labkey-tchad
left a comment
There was a problem hiding this comment.
Manual testing didn't turn up anything
3 participants
Rationale
Recent scans on nightly.labkey.com caused a connection leak on requests to
study-reports-participantCrosstab.view. We can do better at validating parameters and handling theResultsobject, though I was unable to repro the leak locally.Example URLs from the scanner in this log file:
https://gist.githubusercontent.com/labkey-jony/541e4f1582918f63f05f46ca8f349f90/raw/3e5ad715159f407bf6e1a3b75469471f3018b8b0/threaddump.log
Changes
Tasks 📍