#162 add study id for runs #163
Conversation
Updated schema file for adding study id
Updated Data fetcher code to incorporate study id filter
Updated Search fields with study id
Updated Analysis with study id field
Updated Run repository to include study id related fields
| if (analysisId instanceof String) { | ||
| return Mono.just(new Analysis((String) analysisId)); | ||
| final Object studyId = values.get("studyId"); | ||
| if (analysisId instanceof String && studyId instanceof String) { |
There was a problem hiding this comment.
Do we need to handle the cases with just analysisId or just studyId? I think they are both optional
There was a problem hiding this comment.
both are optional but for the Analysis object creation we need both the arguments..
|
|
||
| // short circuit here since can't find runs for invalid analysisId | ||
| if (isNullOrEmpty(analysisId) || !analysisId.equals(filerAnalysisId)) { | ||
| if ((isNullOrEmpty(analysisId) || !analysisId.equals(filerAnalysisId)) | ||
| && (isNullOrEmpty(studyId) || !analysisId.equals(filterStudyId))) { |
There was a problem hiding this comment.
| && (isNullOrEmpty(studyId) || !analysisId.equals(filterStudyId))) { | |
| && (isNullOrEmpty(studyId) || !studyId.equals(filterStudyId))) { |
There was a problem hiding this comment.
I have updated the code. Can you please review.
| .put( | ||
| STUDY_ID, | ||
| value -> { | ||
| val q = new MultiMatchQueryBuilder(value, STUDY_SEARCH_FIELDS.toArray(String[]::new)); | ||
| q.minimumShouldMatch("100%"); | ||
| return q; | ||
| }) |
There was a problem hiding this comment.
Should this be TermQueryBuilder instead of a MultiMatchQueryBuilder ? I think a run can have multiple analysis_ids but only a single Study ID ever.
There was a problem hiding this comment.
It is a TermQueryBuilder also I have made the changes accordingly.
Updated Data fetchers code based on the review comments
Updated Run repository code based on the review comments
| return Mono.just(new Analysis((String) analysisId, (String) studyId)); | ||
| final Object studyId = values.get("studyId"); | ||
| if (analysisId instanceof String || studyId instanceof String) { | ||
| return Mono.just(new Analysis((String) analysisId, (String) studyId); |
There was a problem hiding this comment.
Just confirming before approval, will this handle properly null values for analysisId and studyId?
There was a problem hiding this comment.
Good point, I have updated the Analysis.java constructor logic to check the null values.
Updated null check for analysisId and studyId
| public Analysis(String analysisId, String studyId) { | ||
| if (null!=analysisId) { | ||
| this.analysisId = analysisId; | ||
| } else { | ||
| this.analysisId = ""; | ||
| } | ||
| if (null!=studyId) { | ||
| this.studyId = studyId; | ||
| } else { | ||
| this.studyId = ""; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can this be accomplished with @AllArgsConstructor and some annotations for default values? Its also possible that for the filters having null works better than empty string, might change the matching behavior.
Updated the code to incorporate the study id in runs filter and in runs method along with current analysis id filter