Skip to content

Commit aa0ba5c

Browse files
tim48-robottim48-robot
committed
fix(api): address Copilot review feedback and add test coverage
- Use ObjectMapper for proper JSON escaping (prevents injection) - Reuse ErrorResponse class for consistency with GlobalExceptionHandler - Check response.isCommitted() before writing error response - Catch IOException specifically instead of broad Exception - Fix log message typo (clinicalDataBinCountFilter -> clinicalDataCountFilter) - Add comprehensive unit tests for error handling scenarios
1 parent 6c767bf commit aa0ba5c

File tree

2 files changed

+193
-38
lines changed

2 files changed

+193
-38
lines changed

‎src/main/java/org/cbioportal/legacy/web/util/InvolvedCancerStudyExtractorInterceptor.java‎

Lines changed: 45 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.fasterxml.jackson.databind.ObjectMapper;
3636
import jakarta.servlet.http.HttpServletRequest;
3737
import jakarta.servlet.http.HttpServletResponse;
38+
import java.io.IOException;
3839
import java.util.ArrayList;
3940
import java.util.Arrays;
4041
import java.util.Collection;
@@ -43,6 +44,7 @@
4344
import java.util.List;
4445
import java.util.Set;
4546
import java.util.stream.Collectors;
47+
import org.cbioportal.application.rest.error.ErrorResponse;
4648
import org.cbioportal.legacy.model.AlterationFilter;
4749
import org.cbioportal.legacy.model.MolecularProfile;
4850
import org.cbioportal.legacy.model.MolecularProfileCaseIdentifier;
@@ -165,24 +167,29 @@ public class InvolvedCancerStudyExtractorInterceptor implements HandlerIntercept
165167
* @param message the error message to include in the response body
166168
*/
167169
private void sendBadRequestResponse(HttpServletResponse response, String message) {
168-
try {
169-
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
170-
response.setContentType("application/json");
171-
response.setCharacterEncoding("UTF-8");
170+
if (response.isCommitted()) {
171+
LOG.warn("Cannot send error response - response already committed");
172+
return;
173+
}
172174

175+
try {
173176
// Truncate message before Jackson source location info if present
174-
String truncatedMessage = message;
177+
String cleanMessage = message;
175178
int sourceIndex = message.indexOf(" at [Source:");
176179
if (sourceIndex != -1) {
177-
truncatedMessage = message.substring(0, sourceIndex);
180+
cleanMessage = message.substring(0, sourceIndex);
178181
}
179182

180-
String jsonError =
181-
String.format("{\"message\":\"%s\"}", truncatedMessage.replace("\"", "\\\""));
182-
response.getWriter().write(jsonError);
183+
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
184+
response.setContentType("application/json");
185+
response.setCharacterEncoding("UTF-8");
186+
187+
// Use ObjectMapper for proper JSON escaping (matches GlobalExceptionHandler pattern)
188+
ErrorResponse errorResponse = new ErrorResponse("Invalid request body: " + cleanMessage);
189+
response.getWriter().write(objectMapper.writeValueAsString(errorResponse));
183190
response.getWriter().flush();
184-
} catch (Exception ex) {
185-
LOG.error("Failed to send error response: {}", ex.getMessage());
191+
} catch (IOException ex) {
192+
LOG.error("Failed to write error response: {}", ex.getMessage());
186193
}
187194
}
188195

@@ -297,7 +304,7 @@ private boolean extractAttributesFromPatientFilter(
297304
}
298305
} catch (Exception e) {
299306
LOG.error("exception thrown during extraction of patientFilter: {}", e.getMessage());
300-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
307+
sendBadRequestResponse(response, e.getMessage());
301308
return false;
302309
}
303310
return true;
@@ -333,7 +340,7 @@ private boolean extractAttributesFromSampleFilter(
333340
}
334341
} catch (Exception e) {
335342
LOG.error("exception thrown during extraction of sampleFilter: {}", e.getMessage());
336-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
343+
sendBadRequestResponse(response, e.getMessage());
337344
return false;
338345
}
339346
return true;
@@ -369,7 +376,7 @@ private boolean extractAttributesFromMolecularProfileFilter(
369376
}
370377
} catch (Exception e) {
371378
LOG.error("exception thrown during extraction of molecularProfileFilter: {}", e.getMessage());
372-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
379+
sendBadRequestResponse(response, e.getMessage());
373380
return false;
374381
}
375382
return true;
@@ -407,7 +414,7 @@ private boolean extractAttributesFromClinicalAttributeCountFilter(
407414
} catch (Exception e) {
408415
LOG.error(
409416
"exception thrown during extraction of clinicalAttributeCountFilter: {}", e.getMessage());
410-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
417+
sendBadRequestResponse(response, e.getMessage());
411418
return false;
412419
}
413420
return true;
@@ -448,7 +455,7 @@ private boolean extractAttributesFromNamespaceAttributeCountFilter(
448455
LOG.error(
449456
"exception thrown during extraction of namespaceAttributeCountFilter: {}",
450457
e.getMessage());
451-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
458+
sendBadRequestResponse(response, e.getMessage());
452459
return false;
453460
}
454461
return true;
@@ -482,7 +489,7 @@ private boolean extractAttributesFromClinicalDataMultiStudyFilter(
482489
} catch (Exception e) {
483490
LOG.error(
484491
"exception thrown during extraction of clinicalDataMultiStudyFilter: {}", e.getMessage());
485-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
492+
sendBadRequestResponse(response, e.getMessage());
486493
return false;
487494
}
488495
return true;
@@ -522,7 +529,7 @@ private boolean extractAttributesFromGenePanelDataMultipleStudyFilter(
522529
LOG.error(
523530
"exception thrown during extraction of genePanelSampleMolecularIdentifiers: {}",
524531
e.getMessage());
525-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
532+
sendBadRequestResponse(response, e.getMessage());
526533
return false;
527534
}
528535
return true;
@@ -563,7 +570,7 @@ private boolean extractAttributesFromMolecularDataMultipleStudyFilter(
563570
LOG.error(
564571
"exception thrown during extraction of molecularDataMultipleStudyFilter: {}",
565572
e.getMessage());
566-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
573+
sendBadRequestResponse(response, e.getMessage());
567574
return false;
568575
}
569576
return true;
@@ -606,7 +613,7 @@ private boolean extractAttributesFromGenericAssayDataMultipleStudyFilter(
606613
LOG.error(
607614
"exception thrown during extraction of genericAssayDataMultipleStudyFilter: {}",
608615
e.getMessage());
609-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
616+
sendBadRequestResponse(response, e.getMessage());
610617
return false;
611618
}
612619
return true;
@@ -643,7 +650,7 @@ private boolean extractAttributesFromMutationMultipleStudyFilter(
643650
} catch (Exception e) {
644651
LOG.error(
645652
"exception thrown during extraction of mutationMultipleStudyFilter: {}", e.getMessage());
646-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
653+
sendBadRequestResponse(response, e.getMessage());
647654
return false;
648655
}
649656
return true;
@@ -678,7 +685,7 @@ private boolean extractAttributesFromSampleIdentifiers(
678685
}
679686
} catch (Exception e) {
680687
LOG.error("exception thrown during extraction of sampleIdentifiers: {}", e.getMessage());
681-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
688+
sendBadRequestResponse(response, e.getMessage());
682689
return false;
683690
}
684691
return true;
@@ -701,7 +708,7 @@ private boolean extractAttributesFromClinicalDataBinCountFilter(
701708
} catch (Exception e) {
702709
LOG.error(
703710
"exception thrown during extraction of clinicalDataBinCountFilter: {}", e.getMessage());
704-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
711+
sendBadRequestResponse(response, e.getMessage());
705712
return false;
706713
}
707714
return true;
@@ -724,7 +731,7 @@ private boolean extractAttributesFromGenomicDataBinCountFilter(
724731
} catch (Exception e) {
725732
LOG.error(
726733
"exception thrown during extraction of genomicDataBinCountFilter: {}", e.getMessage());
727-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
734+
sendBadRequestResponse(response, e.getMessage());
728735
return false;
729736
}
730737
return true;
@@ -746,7 +753,7 @@ private boolean extractAttributesFromGenomicDataCountFilter(
746753
}
747754
} catch (Exception e) {
748755
LOG.error("exception thrown during extraction of genomicDataCountFilter: {}", e.getMessage());
749-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
756+
sendBadRequestResponse(response, e.getMessage());
750757
return false;
751758
}
752759
return true;
@@ -773,7 +780,7 @@ private boolean extractAttributesFromGenericAssayDataBinCountFilter(
773780
LOG.error(
774781
"exception thrown during extraction of genericAssayDataBinCountFilter: {}",
775782
e.getMessage());
776-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
783+
sendBadRequestResponse(response, e.getMessage());
777784
return false;
778785
}
779786
return true;
@@ -797,7 +804,7 @@ private boolean extractAttributesFromGenericAssayDataCountFilter(
797804
} catch (Exception e) {
798805
LOG.error(
799806
"exception thrown during extraction of genericAssayDataCountFilter: {}", e.getMessage());
800-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
807+
sendBadRequestResponse(response, e.getMessage());
801808
return false;
802809
}
803810
return true;
@@ -808,7 +815,7 @@ private boolean extractAttributesFromClinicalDataCountFilter(
808815
try {
809816
ClinicalDataCountFilter clinicalDataCountFilter =
810817
objectMapper.readValue(request.getInputStream(), ClinicalDataCountFilter.class);
811-
LOG.debug("extracted clinicalDataBinCountFilter: {}", clinicalDataCountFilter);
818+
LOG.debug("extracted clinicalDataCountFilter: {}", clinicalDataCountFilter);
812819
LOG.debug("setting interceptedClinicalDataCountFilter to {}", clinicalDataCountFilter);
813820
request.setAttribute("interceptedClinicalDataCountFilter", clinicalDataCountFilter);
814821
if (cacheMapUtil.hasCacheEnabled()) {
@@ -819,8 +826,8 @@ private boolean extractAttributesFromClinicalDataCountFilter(
819826
}
820827
} catch (Exception e) {
821828
LOG.error(
822-
"exception thrown during extraction of clinicalDataBinCountFilter: {}", e.getMessage());
823-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
829+
"exception thrown during extraction of clinicalDataCountFilter: {}", e.getMessage());
830+
sendBadRequestResponse(response, e.getMessage());
824831
return false;
825832
}
826833
return true;
@@ -843,7 +850,7 @@ private boolean extractAttributesFromNamespaceDataCountFilter(
843850
} catch (Exception e) {
844851
LOG.error(
845852
"exception thrown during extraction of namespaceDataCountFilter: {}", e.getMessage());
846-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
853+
sendBadRequestResponse(response, e.getMessage());
847854
return false;
848855
}
849856
return true;
@@ -868,7 +875,7 @@ private boolean extractAttributesFromGroupFilter(
868875
}
869876
} catch (Exception e) {
870877
LOG.error("exception thrown during extraction of groupFilter: {}", e.getMessage());
871-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
878+
sendBadRequestResponse(response, e.getMessage());
872879
return false;
873880
}
874881
return true;
@@ -900,7 +907,7 @@ private boolean extractAttributesFromStudyViewFilter(
900907
}
901908
} catch (Exception e) {
902909
LOG.error("exception thrown during extraction of studyViewFilter: {}", e.getMessage());
903-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
910+
sendBadRequestResponse(response, e.getMessage());
904911
return false;
905912
}
906913
return true;
@@ -930,7 +937,7 @@ private boolean extractAttributesFromMolecularProfileCasesGroups(
930937
LOG.error(
931938
"exception thrown during extraction of molecularProfileCasesGroupFilters: {}",
932939
e.getMessage());
933-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
940+
sendBadRequestResponse(response, e.getMessage());
934941
return false;
935942
}
936943
return true;
@@ -970,7 +977,7 @@ private boolean extractAttributesFromMolecularProfileCasesGroupsAndAlterationTyp
970977
LOG.error(
971978
"exception thrown during extraction of molecularProfileCasesGroupFilters: {}",
972979
e.getMessage());
973-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
980+
sendBadRequestResponse(response, e.getMessage());
974981
return false;
975982
}
976983
return true;
@@ -998,7 +1005,7 @@ private boolean extractAttributesFromStructuralVariantFilter(
9981005
} catch (Exception e) {
9991006
LOG.error(
10001007
"exception thrown during extraction of structuralVariantFilter: {}", e.getMessage());
1001-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
1008+
sendBadRequestResponse(response, e.getMessage());
10021009
return false;
10031010
}
10041011
return true;
@@ -1170,7 +1177,7 @@ private boolean extractCancerStudyIdsFromSurvivalRequest(
11701177
}
11711178
} catch (Exception e) {
11721179
LOG.error("exception thrown during extraction of survivalRequest: {}", e.getMessage());
1173-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
1180+
sendBadRequestResponse(response, e.getMessage());
11741181
return false;
11751182
}
11761183
return true;
@@ -1198,7 +1205,7 @@ private boolean extractCancerStudyIdsFromClinicalEventAttributeRequest(
11981205
LOG.error(
11991206
"exception thrown during extraction of clinicalEventAttributeRequest: {}",
12001207
e.getMessage());
1201-
sendBadRequestResponse(response, "Invalid request body: " + e.getMessage());
1208+
sendBadRequestResponse(response, e.getMessage());
12021209
return false;
12031210
}
12041211
return true;

0 commit comments

Comments
 (0)