Custom sample filtering performance and security improvement #11274
Conversation
dippindots
force-pushed
the
demo-fix-prepared-statements
branch
4 times, most recently
from
8e58dbb to
5ff57e9
Compare
| @@ -65,6 +66,13 @@ private StudyViewFilterHelper(@NonNull StudyViewFilter studyViewFilter, | |||
| this.categorizedGenericAssayDataCountFilter = extractGenericAssayDataCountFilters(studyViewFilter, genericAssayProfilesMap); | |||
| this.customDataSamples = customDataSamples; | |||
| this.involvedCancerStudies = involvedCancerStudies; | |||
| if (studyViewFilter != null && studyViewFilter.getSampleIdentifiers() != null) { | |||
There was a problem hiding this comment.
@dippindots is there a reason why we don't just calculate this on the fly? it's a derivation of getSampleIdentifiers. I guess it's an optimization to save it? I'm just coming from the Mobx world where we would make this a computed.
There was a problem hiding this comment.
Yea, so Java just recently implemented this concept of Property you are talking about. With Records.
But I agree this shouldn't be in the constructor and we could just add this to the filteredSampleIdentifiers method to dynamically calculate this
There was a problem hiding this comment.
But if it's called multiple times then, absent some kind of mobx-like computed thing, we should just leave it. I think that's the existing pattern with "build", right? You do the calculation up front?
There was a problem hiding this comment.
I added this to the filteredSampleIdentifiers method to dynamically calculate it
src/main/java/org/cbioportal/web/parameter/CustomSampleIdentifier.java
Outdated
Show resolved
Hide resolved
| public String getUniqueSampleId() { | ||
| // Assuming studyId and sampleId are available in SampleIdentifier | ||
| // Concatenate with "_" in between if both values are not null | ||
| if (getStudyId() != null && getSampleId() != null) { |
There was a problem hiding this comment.
Can this even happen? a sample Id with one of these null?
There was a problem hiding this comment.
I don't think we have this case, but we do see a study view filter missing studyIds, do you think I should remove this check?
dippindots
force-pushed
the
demo-fix-prepared-statements
branch
from
5ff57e9 to
a9b44d6
Compare
|
Fix #11195
concathas poor performance during my test, we should try to not useconcatin a largeforeachclausearrayin the query, and it has better performance, try replace large foreach witharrayIn this pull request:
arrayto improve performance