feat: post error message in discord during csv export #2281
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jo-elimu
requested review from
SnehaHS65,
Souvik-Cyclic and
vrudas
and removed request for
a team
3 tasks
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2281 +/- ##
============================================
- Coverage 16.49% 16.47% -0.02%
Complexity 450 450
============================================
Files 254 254
Lines 7470 7477 +7
Branches 849 849
============================================
Hits 1232 1232
- Misses 6173 6180 +7
Partials 65 65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
WalkthroughThe code updates several analytics CSV export controllers to wrap their logic in try-catch blocks that handle all exceptions, not just IOExceptions. Upon any exception, an error message is logged and a notification is sent to a Discord analytics channel using DiscordHelper. No changes were made to method signatures or exported entities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant DiscordHelper
User->>Controller: HTTP request for CSV export
Controller->>Controller: Process CSV export logic
alt Success
Controller->>User: Return CSV file
else Exception occurs
Controller->>DiscordHelper: postToChannel(Channel.ANALYTICS, error message)
Controller->>User: Log error, return error response
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java (1)
34-38: Fix method signature inconsistency and improve error handling.The method signature declares
throws IOExceptionbut the catch block now handles all exceptions without re-throwing them, breaking the method contract. Additionally, when errors occur, clients receive no HTTP error response.Apply this diff to fix the issues:
public void handleRequest( @PathVariable Long studentId, HttpServletResponse response, OutputStream outputStream - ) throws IOException { + ) { // ... existing code ... } catch (Exception ex) { - log.error(ex.getMessage()); + log.error("Error during CSV export of word assessment events", ex); + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of word assessment events: `" + ex.getClass() + ": " + ex.getMessage() + "`"); }Also applies to: 89-92
♻️ Duplicate comments (3)
src/main/java/ai/elimu/web/analytics/students/NumberLearningEventsCsvExportController.java (1)
34-38: Apply the same fixes as WordAssessmentEventsCsvExportController.This controller has identical issues: method signature inconsistency and missing client error response.
Reference the fix suggested in
WordAssessmentEventsCsvExportController.javalines 34-38,89-92.Also applies to: 89-92
src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java (1)
34-38: Apply consistent error handling fixes.This controller has the same method signature and error response issues as the other controllers in this PR.
Apply the same fixes suggested for
WordAssessmentEventsCsvExportController.javalines 34-38,89-92.Also applies to: 88-91
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java (1)
35-39: Apply consistent error handling improvements.This controller has the same method signature and error response issues as the other controllers.
Apply the same fixes suggested for
WordAssessmentEventsCsvExportController.javalines 34-38,89-92.Also applies to: 91-94
🧹 Nitpick comments (2)
src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java (1)
89-92: Consider more specific exception handling.Catching all
Exceptiontypes is very broad and might mask programming errors. Consider catching specific exceptions likeIOException,SQLException, or creating custom exceptions for different failure scenarios.Example approach:
- } catch (Exception ex) { + } catch (IOException | SQLException ex) { log.error("Error during CSV export of word assessment events", ex); response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of word assessment events: `" + ex.getClass() + ": " + ex.getMessage() + "`"); + } catch (Exception ex) { + log.error("Unexpected error during CSV export", ex); + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + DiscordHelper.postToChannel(Channel.ANALYTICS, "Unexpected error during CSV export: `" + ex.getClass() + ": " + ex.getMessage() + "`");src/main/java/ai/elimu/web/analytics/students/LetterSoundLearningEventsCsvExportController.java (1)
56-57: Consider removing or documenting the commented code.The commented-out code for
letter_sound_lettersandletter_sound_soundsfields suggests these were intentionally excluded from the CSV export. Consider either:
- Removing the commented code if these fields are permanently excluded
- Adding a comment explaining why these fields are excluded
- Converting to a TODO comment if they should be included in the future
- // "letter_sound_letters", - // "letter_sound_sounds", + // TODO: Exclude letter_sound_letters and letter_sound_sounds for privacy reasonsAlso applies to: 71-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/web/analytics/students/LetterSoundAssessmentEventsCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/LetterSoundLearningEventsCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/NumberLearningEventsCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/VideoLearningEventsCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: test_rest
🔇 Additional comments (9)
pom-dependency-tree.txt (1)
1-1: Version bump looks appropriate.The version update from
2.6.76-SNAPSHOTto2.6.78-SNAPSHOTaligns with the enhancement being made to CSV export error handling.src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java (1)
7-8: LGTM: Clean import additions for Discord integration.The imports for
DiscordHelperandChannelare properly added to support the error notification functionality.src/main/java/ai/elimu/web/analytics/students/NumberLearningEventsCsvExportController.java (1)
7-8: LGTM: Consistent import pattern.The DiscordHelper imports are correctly added, maintaining consistency with other controllers.
src/main/java/ai/elimu/web/analytics/students/WordLearningEventsCsvExportController.java (1)
7-8: LGTM: Consistent import additions.The DiscordHelper imports follow the same pattern as other controllers in this PR.
src/main/java/ai/elimu/web/analytics/students/StoryBookLearningEventsCsvExportController.java (2)
8-9: LGTM: Consistent import structure.The DiscordHelper imports maintain the same pattern established in other controllers.
48-50: Good: Android ID redaction is protected by error handling.The Android ID redaction logic is correctly placed within the try block, ensuring that any exceptions during this process are caught and reported.
src/main/java/ai/elimu/web/analytics/students/VideoLearningEventsCsvExportController.java (1)
8-9: LGTM: Clean import additions for Discord integration.The imports for
DiscordHelperandDiscordHelper.Channelare properly added to support the new error notification feature.src/main/java/ai/elimu/web/analytics/students/LetterSoundAssessmentEventsCsvExportController.java (1)
7-8: LGTM: Proper imports for Discord functionality.The Discord helper imports are correctly added.
src/main/java/ai/elimu/web/analytics/students/LetterSoundLearningEventsCsvExportController.java (1)
7-8: LGTM: Discord imports properly added.The necessary imports for Discord error reporting are correctly included.
Comment on lines
+43
to
95
| try { | ||
| Student student = studentDao.read(studentId); | ||
| log.info("student.getAndroidId(): " + student.getAndroidId()); | ||
|
|
||
| List<VideoLearningEvent> videoLearningEvents = videoLearningEventDao.readAll(student.getAndroidId()); | ||
| log.info("videoLearningEvents.size(): " + videoLearningEvents.size()); | ||
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | ||
| videoLearningEvent.setAndroidId(AnalyticsHelper.redactAndroidId(videoLearningEvent.getAndroidId())); | ||
| } | ||
| List<VideoLearningEvent> videoLearningEvents = videoLearningEventDao.readAll(student.getAndroidId()); | ||
| log.info("videoLearningEvents.size(): " + videoLearningEvents.size()); | ||
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | ||
| videoLearningEvent.setAndroidId(AnalyticsHelper.redactAndroidId(videoLearningEvent.getAndroidId())); | ||
| } | ||
|
|
||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "learning_event_type", | ||
| "additional_data", | ||
| "research_experiment", | ||
| "experiment_group", | ||
| "video_title", | ||
| "video_id" | ||
| ).build(); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | ||
| log.info("videoLearningEvent.getId(): " + videoLearningEvent.getId()); | ||
| csvPrinter.printRecord( | ||
| videoLearningEvent.getId(), | ||
| videoLearningEvent.getTimestamp().getTimeInMillis() / 1_000, | ||
| videoLearningEvent.getPackageName(), | ||
| videoLearningEvent.getLearningEventType(), | ||
| videoLearningEvent.getAdditionalData(), | ||
| videoLearningEvent.getResearchExperiment().ordinal(), | ||
| videoLearningEvent.getExperimentGroup().ordinal(), | ||
| videoLearningEvent.getVideoTitle(), | ||
| videoLearningEvent.getVideoId() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "learning_event_type", | ||
| "additional_data", | ||
| "research_experiment", | ||
| "experiment_group", | ||
| "video_title", | ||
| "video_id" | ||
| ).build(); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | ||
| log.info("videoLearningEvent.getId(): " + videoLearningEvent.getId()); | ||
| csvPrinter.printRecord( | ||
| videoLearningEvent.getId(), | ||
| videoLearningEvent.getTimestamp().getTimeInMillis() / 1_000, | ||
| videoLearningEvent.getPackageName(), | ||
| videoLearningEvent.getLearningEventType(), | ||
| videoLearningEvent.getAdditionalData(), | ||
| videoLearningEvent.getResearchExperiment().ordinal(), | ||
| videoLearningEvent.getExperimentGroup().ordinal(), | ||
| videoLearningEvent.getVideoTitle(), | ||
| videoLearningEvent.getVideoId() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
|
|
||
| String csvFileContent = stringWriter.toString(); | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
| try { | ||
| String csvFileContent = stringWriter.toString(); | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
| outputStream.write(bytes); | ||
| outputStream.flush(); | ||
| outputStream.close(); | ||
| } catch (IOException ex) { | ||
| } catch (Exception ex) { | ||
| log.error(ex.getMessage()); | ||
| DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of video learning events: `" + ex.getClass() + ": " + ex.getMessage() + "`"); | ||
| } |
Contributor
There was a problem hiding this comment.
Review exception handling and HTTP response management.
While the Discord error notification is a good addition, there are several concerns with the current implementation:
-
Overly broad exception handling: Catching
Exceptionmay mask programming errors that should be addressed rather than silently logged. -
HTTP response state: When an exception occurs, no HTTP response content is written, potentially leaving clients with an incomplete or confusing response.
-
Potential information disclosure: The Discord message exposes exception class names and messages, which could contain sensitive information.
Consider this improved approach:
- } catch (Exception ex) {
- log.error(ex.getMessage());
- DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of video learning events: `" + ex.getClass() + ": " + ex.getMessage() + "`");
+ } catch (Exception ex) {
+ log.error("Error during CSV export of video learning events for studentId: " + studentId, ex);
+
+ // Send error response to client
+ response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ response.setContentType("text/plain");
+ try {
+ outputStream.write("Internal server error occurred during CSV export".getBytes());
+ outputStream.flush();
+ } catch (IOException ioEx) {
+ log.error("Failed to write error response", ioEx);
+ }
+
+ // Sanitize error message for Discord
+ String sanitizedMessage = "Error during CSV export of video learning events for student " + studentId + ": " + ex.getClass().getSimpleName();
+ DiscordHelper.postToChannel(Channel.ANALYTICS, sanitizedMessage);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| Student student = studentDao.read(studentId); | |
| log.info("student.getAndroidId(): " + student.getAndroidId()); | |
| List<VideoLearningEvent> videoLearningEvents = videoLearningEventDao.readAll(student.getAndroidId()); | |
| log.info("videoLearningEvents.size(): " + videoLearningEvents.size()); | |
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | |
| videoLearningEvent.setAndroidId(AnalyticsHelper.redactAndroidId(videoLearningEvent.getAndroidId())); | |
| } | |
| List<VideoLearningEvent> videoLearningEvents = videoLearningEventDao.readAll(student.getAndroidId()); | |
| log.info("videoLearningEvents.size(): " + videoLearningEvents.size()); | |
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | |
| videoLearningEvent.setAndroidId(AnalyticsHelper.redactAndroidId(videoLearningEvent.getAndroidId())); | |
| } | |
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | |
| .setHeader( | |
| "id", | |
| "timestamp", | |
| "package_name", | |
| "learning_event_type", | |
| "additional_data", | |
| "research_experiment", | |
| "experiment_group", | |
| "video_title", | |
| "video_id" | |
| ).build(); | |
| StringWriter stringWriter = new StringWriter(); | |
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | |
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | |
| log.info("videoLearningEvent.getId(): " + videoLearningEvent.getId()); | |
| csvPrinter.printRecord( | |
| videoLearningEvent.getId(), | |
| videoLearningEvent.getTimestamp().getTimeInMillis() / 1_000, | |
| videoLearningEvent.getPackageName(), | |
| videoLearningEvent.getLearningEventType(), | |
| videoLearningEvent.getAdditionalData(), | |
| videoLearningEvent.getResearchExperiment().ordinal(), | |
| videoLearningEvent.getExperimentGroup().ordinal(), | |
| videoLearningEvent.getVideoTitle(), | |
| videoLearningEvent.getVideoId() | |
| ); | |
| } | |
| csvPrinter.flush(); | |
| csvPrinter.close(); | |
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | |
| .setHeader( | |
| "id", | |
| "timestamp", | |
| "package_name", | |
| "learning_event_type", | |
| "additional_data", | |
| "research_experiment", | |
| "experiment_group", | |
| "video_title", | |
| "video_id" | |
| ).build(); | |
| StringWriter stringWriter = new StringWriter(); | |
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | |
| for (VideoLearningEvent videoLearningEvent : videoLearningEvents) { | |
| log.info("videoLearningEvent.getId(): " + videoLearningEvent.getId()); | |
| csvPrinter.printRecord( | |
| videoLearningEvent.getId(), | |
| videoLearningEvent.getTimestamp().getTimeInMillis() / 1_000, | |
| videoLearningEvent.getPackageName(), | |
| videoLearningEvent.getLearningEventType(), | |
| videoLearningEvent.getAdditionalData(), | |
| videoLearningEvent.getResearchExperiment().ordinal(), | |
| videoLearningEvent.getExperimentGroup().ordinal(), | |
| videoLearningEvent.getVideoTitle(), | |
| videoLearningEvent.getVideoId() | |
| ); | |
| } | |
| csvPrinter.flush(); | |
| csvPrinter.close(); | |
| String csvFileContent = stringWriter.toString(); | |
| response.setContentType("text/csv"); | |
| byte[] bytes = csvFileContent.getBytes(); | |
| response.setContentLength(bytes.length); | |
| try { | |
| String csvFileContent = stringWriter.toString(); | |
| response.setContentType("text/csv"); | |
| byte[] bytes = csvFileContent.getBytes(); | |
| response.setContentLength(bytes.length); | |
| outputStream.write(bytes); | |
| outputStream.flush(); | |
| outputStream.close(); | |
| } catch (IOException ex) { | |
| } catch (Exception ex) { | |
| log.error(ex.getMessage()); | |
| DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of video learning events: `" + ex.getClass() + ": " + ex.getMessage() + "`"); | |
| } | |
| try { | |
| // … existing try block unchanged … | |
| outputStream.write(bytes); | |
| outputStream.flush(); | |
| outputStream.close(); | |
| } catch (Exception ex) { | |
| // Improved logging with full stacktrace and context | |
| log.error("Error during CSV export of video learning events for studentId: " + studentId, ex); | |
| // Send a proper HTTP 500 response to the client | |
| response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| response.setContentType("text/plain"); | |
| try { | |
| outputStream.write("Internal server error occurred during CSV export".getBytes()); | |
| outputStream.flush(); | |
| } catch (IOException ioEx) { | |
| log.error("Failed to write error response", ioEx); | |
| } | |
| // Sanitize and send a concise error notification to Discord | |
| String sanitizedMessage = | |
| "Error during CSV export of video learning events for student " | |
| + studentId | |
| + ": " | |
| + ex.getClass().getSimpleName(); | |
| DiscordHelper.postToChannel(Channel.ANALYTICS, sanitizedMessage); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/VideoLearningEventsCsvExportController.java
between lines 43 and 95, the current exception handling is too broad by catching
Exception, which can hide programming errors. Refine the catch block to handle
more specific exceptions where possible. Additionally, ensure that when an
exception occurs, a proper HTTP error response with an appropriate status code
and a generic error message is sent to the client to avoid leaving the response
incomplete. Finally, modify the Discord error notification to avoid exposing
sensitive exception details by logging only a generic error message or sanitized
information instead of full exception class names and messages.
Comment on lines
+41
to
94
| try { | ||
| Student student = studentDao.read(studentId); | ||
| log.info("student.getAndroidId(): " + student.getAndroidId()); | ||
|
|
||
| List<LetterSoundAssessmentEvent> letterSoundAssessmentEvents = letterSoundAssessmentEventDao.readAll(student.getAndroidId()); | ||
| log.info("letterSoundAssessmentEvents.size(): " + letterSoundAssessmentEvents.size()); | ||
| List<LetterSoundAssessmentEvent> letterSoundAssessmentEvents = letterSoundAssessmentEventDao.readAll(student.getAndroidId()); | ||
| log.info("letterSoundAssessmentEvents.size(): " + letterSoundAssessmentEvents.size()); | ||
|
|
||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "mastery_score", | ||
| "time_spent_ms", | ||
| "additional_data", | ||
| "research_experiment", | ||
| "experiment_group", | ||
| "letter_sound_letters", | ||
| "letter_sound_sounds", | ||
| "letter_sound_id" | ||
| ).build(); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| for (LetterSoundAssessmentEvent letterSoundAssessmentEvent : letterSoundAssessmentEvents) { | ||
| log.info("letterSoundAssessmentEvent.getId(): " + letterSoundAssessmentEvent.getId()); | ||
| csvPrinter.printRecord( | ||
| letterSoundAssessmentEvent.getId(), | ||
| letterSoundAssessmentEvent.getTimestamp().getTimeInMillis() / 1_000, | ||
| letterSoundAssessmentEvent.getPackageName(), | ||
| letterSoundAssessmentEvent.getMasteryScore(), | ||
| letterSoundAssessmentEvent.getTimeSpentMs(), | ||
| letterSoundAssessmentEvent.getAdditionalData(), | ||
| letterSoundAssessmentEvent.getResearchExperiment().ordinal(), | ||
| letterSoundAssessmentEvent.getExperimentGroup().ordinal(), | ||
| letterSoundAssessmentEvent.getLetterSoundLetters(), | ||
| letterSoundAssessmentEvent.getLetterSoundSounds(), | ||
| letterSoundAssessmentEvent.getLetterSoundId() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "mastery_score", | ||
| "time_spent_ms", | ||
| "additional_data", | ||
| "research_experiment", | ||
| "experiment_group", | ||
| "letter_sound_letters", | ||
| "letter_sound_sounds", | ||
| "letter_sound_id" | ||
| ).build(); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| for (LetterSoundAssessmentEvent letterSoundAssessmentEvent : letterSoundAssessmentEvents) { | ||
| log.info("letterSoundAssessmentEvent.getId(): " + letterSoundAssessmentEvent.getId()); | ||
| csvPrinter.printRecord( | ||
| letterSoundAssessmentEvent.getId(), | ||
| letterSoundAssessmentEvent.getTimestamp().getTimeInMillis() / 1_000, | ||
| letterSoundAssessmentEvent.getPackageName(), | ||
| letterSoundAssessmentEvent.getMasteryScore(), | ||
| letterSoundAssessmentEvent.getTimeSpentMs(), | ||
| letterSoundAssessmentEvent.getAdditionalData(), | ||
| letterSoundAssessmentEvent.getResearchExperiment().ordinal(), | ||
| letterSoundAssessmentEvent.getExperimentGroup().ordinal(), | ||
| letterSoundAssessmentEvent.getLetterSoundLetters(), | ||
| letterSoundAssessmentEvent.getLetterSoundSounds(), | ||
| letterSoundAssessmentEvent.getLetterSoundId() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
|
|
||
| String csvFileContent = stringWriter.toString(); | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
| try { | ||
| String csvFileContent = stringWriter.toString(); | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
| outputStream.write(bytes); | ||
| outputStream.flush(); | ||
| outputStream.close(); | ||
| } catch (IOException ex) { | ||
| } catch (Exception ex) { | ||
| log.error(ex.getMessage()); | ||
| DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of letter-sound assessment events: `" + ex.getClass() + ": " + ex.getMessage() + "`"); | ||
| } |
Contributor
There was a problem hiding this comment.
Same exception handling concerns as VideoLearningEventsCsvExportController.
This implementation has identical issues to the VideoLearningEventsCsvExportController:
- Overly broad exception catching that may hide programming errors
- No HTTP error response sent to clients when exceptions occur
- Potential information leakage in Discord error messages
Apply the same improvements suggested for VideoLearningEventsCsvExportController:
- } catch (Exception ex) {
- log.error(ex.getMessage());
- DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of letter-sound assessment events: `" + ex.getClass() + ": " + ex.getMessage() + "`");
+ } catch (Exception ex) {
+ log.error("Error during CSV export of letter-sound assessment events for studentId: " + studentId, ex);
+
+ response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ response.setContentType("text/plain");
+ try {
+ outputStream.write("Internal server error occurred during CSV export".getBytes());
+ outputStream.flush();
+ } catch (IOException ioEx) {
+ log.error("Failed to write error response", ioEx);
+ }
+
+ String sanitizedMessage = "Error during CSV export of letter-sound assessment events for student " + studentId + ": " + ex.getClass().getSimpleName();
+ DiscordHelper.postToChannel(Channel.ANALYTICS, sanitizedMessage);🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/LetterSoundAssessmentEventsCsvExportController.java
between lines 41 and 94, the exception handling is too broad, catching all
exceptions without distinguishing error types, does not send an appropriate HTTP
error response to the client, and posts detailed exception information to
Discord which may leak sensitive data. Refactor the catch block to catch more
specific exceptions where possible, send a proper HTTP error status and message
in the response to inform the client of the failure, and sanitize or limit the
error details posted to Discord to avoid exposing sensitive internal
information.
Comment on lines
+41
to
90
| try { | ||
| Student student = studentDao.read(studentId); | ||
| log.info("student.getAndroidId(): " + student.getAndroidId()); | ||
|
|
||
| List<LetterSoundLearningEvent> letterSoundLearningEvents = letterSoundLearningEventDao.readAll(student.getAndroidId()); | ||
| log.info("letterSoundLearningEvents.size(): " + letterSoundLearningEvents.size()); | ||
| List<LetterSoundLearningEvent> letterSoundLearningEvents = letterSoundLearningEventDao.readAll(student.getAndroidId()); | ||
| log.info("letterSoundLearningEvents.size(): " + letterSoundLearningEvents.size()); | ||
|
|
||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "additional_data", | ||
| "research_experiment", | ||
| "experiment_group", | ||
| // "letter_sound_letters", | ||
| // "letter_sound_sounds", | ||
| "letter_sound_id" | ||
| ).build(); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| for (LetterSoundLearningEvent letterSoundLearningEvent : letterSoundLearningEvents) { | ||
| log.info("letterSoundLearningEvent.getId(): " + letterSoundLearningEvent.getId()); | ||
| csvPrinter.printRecord( | ||
| letterSoundLearningEvent.getId(), | ||
| letterSoundLearningEvent.getTimestamp().getTimeInMillis() / 1_000, | ||
| letterSoundLearningEvent.getPackageName(), | ||
| letterSoundLearningEvent.getAdditionalData(), | ||
| letterSoundLearningEvent.getResearchExperiment().ordinal(), | ||
| letterSoundLearningEvent.getExperimentGroup().ordinal(), | ||
| // letterSoundLearningEvent.getLetterSoundLetters(), | ||
| // letterSoundLearningEvent.getLetterSoundSounds(), | ||
| letterSoundLearningEvent.getLetterSoundId() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "additional_data", | ||
| "research_experiment", | ||
| "experiment_group", | ||
| // "letter_sound_letters", | ||
| // "letter_sound_sounds", | ||
| "letter_sound_id" | ||
| ).build(); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| for (LetterSoundLearningEvent letterSoundLearningEvent : letterSoundLearningEvents) { | ||
| log.info("letterSoundLearningEvent.getId(): " + letterSoundLearningEvent.getId()); | ||
| csvPrinter.printRecord( | ||
| letterSoundLearningEvent.getId(), | ||
| letterSoundLearningEvent.getTimestamp().getTimeInMillis() / 1_000, | ||
| letterSoundLearningEvent.getPackageName(), | ||
| letterSoundLearningEvent.getAdditionalData(), | ||
| letterSoundLearningEvent.getResearchExperiment().ordinal(), | ||
| letterSoundLearningEvent.getExperimentGroup().ordinal(), | ||
| // letterSoundLearningEvent.getLetterSoundLetters(), | ||
| // letterSoundLearningEvent.getLetterSoundSounds(), | ||
| letterSoundLearningEvent.getLetterSoundId() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
|
|
||
| String csvFileContent = stringWriter.toString(); | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
|
|
||
| String csvFileContent = stringWriter.toString(); | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
| try { | ||
| outputStream.write(bytes); | ||
| outputStream.flush(); | ||
| outputStream.close(); | ||
| } catch (IOException ex) { | ||
| } catch (Exception ex) { | ||
| log.error(ex.getMessage()); | ||
| DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of letter-sound learning events: `" + ex.getClass() + ": " + ex.getMessage() + "`"); | ||
| } |
Contributor
There was a problem hiding this comment.
Identical exception handling issues as other controllers.
This controller has the same problematic exception handling pattern as the other two CSV export controllers:
- Catching all exceptions may mask important programming errors
- No proper HTTP error response for clients
- Potential information disclosure in Discord messages
For consistency across all CSV export controllers, apply the same error handling improvements:
- } catch (Exception ex) {
- log.error(ex.getMessage());
- DiscordHelper.postToChannel(Channel.ANALYTICS, "Error during CSV export of letter-sound learning events: `" + ex.getClass() + ": " + ex.getMessage() + "`");
+ } catch (Exception ex) {
+ log.error("Error during CSV export of letter-sound learning events for studentId: " + studentId, ex);
+
+ response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ response.setContentType("text/plain");
+ try {
+ outputStream.write("Internal server error occurred during CSV export".getBytes());
+ outputStream.flush();
+ } catch (IOException ioEx) {
+ log.error("Failed to write error response", ioEx);
+ }
+
+ String sanitizedMessage = "Error during CSV export of letter-sound learning events for student " + studentId + ": " + ex.getClass().getSimpleName();
+ DiscordHelper.postToChannel(Channel.ANALYTICS, sanitizedMessage);🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/LetterSoundLearningEventsCsvExportController.java
around lines 41 to 90, the current exception handling catches all exceptions
broadly, logs only the message, sends potentially sensitive error details to
Discord, and does not set an appropriate HTTP error response. To fix this,
narrow the exception handling to catch only expected exceptions, log full stack
traces for debugging, avoid sending detailed error information to Discord to
prevent information disclosure, and set a proper HTTP error status code and
message in the response to inform clients of the failure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
Purpose
Technical Details
Testing Instructions
Screenshots
Format Checks
Note
Files in PRs are automatically checked for format violations with
mvn spotless:check.If this PR contains files with format violations, run
mvn spotless:applyto fix them.