feat: add word assessment events #2210
Merged
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
AshishBagdane,
alexander-kuruvilla and
eymaal
and removed request for
a team
Closed
2 tasks
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2210 +/- ##
============================================
+ Coverage 16.06% 16.32% +0.26%
- Complexity 408 414 +6
============================================
Files 241 244 +3
Lines 6493 6621 +128
Branches 743 759 +16
============================================
+ Hits 1043 1081 +38
- Misses 5399 5489 +90
Partials 51 51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
WalkthroughThe changes add a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WordAssessmentEventsRestController
participant FileSystem
Client->>WordAssessmentEventsRestController: POST /upload (CSV file)
WordAssessmentEventsRestController->>WordAssessmentEventsRestController: Validate file (size, filename)
alt Invalid file
WordAssessmentEventsRestController-->>Client: JSON error response, HTTP 500
else Valid file
WordAssessmentEventsRestController->>FileSystem: Store file by language/Android ID/version
WordAssessmentEventsRestController-->>Client: JSON success response
end
sequenceDiagram
participant Scheduler
participant FileSystem
participant CsvHelper
participant WordAssessmentEventDao
participant StudentDao
participant Database
Scheduler->>FileSystem: Scan CSV files directory
FileSystem->>Scheduler: List of CSV files
loop For each CSV file
Scheduler->>CsvHelper: extractWordAssessmentEvents(file)
CsvHelper->>Scheduler: List of WordAssessmentEvent objects
loop For each event
Scheduler->>WordAssessmentEventDao: read(event.timestamp, androidId, packageName)
alt Event not exists
Scheduler->>StudentDao: read(androidId)
alt Student not exists
Scheduler->>Database: persist new Student
end
Scheduler->>Database: persist WordAssessmentEvent
end
end
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom-dependency-tree.txt(1 hunks)src/main/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestController.java(2 hunks)src/test/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestControllerTest.java(1 hunks)src/test/java/ai/elimu/rest/v2/analytics/WordLearningEventsRestControllerTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestController.java (2)
src/main/java/ai/elimu/rest/v2/analytics/WordLearningEventsRestController.java (1)
RestController(24-91)src/main/java/ai/elimu/util/AnalyticsHelper.java (1)
AnalyticsHelper(6-43)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (macos-latest, 21)
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: test_rest
🔇 Additional comments (1)
pom-dependency-tree.txt (1)
1-1: LGTM! Standard version bump.The version increment from
2.6.25-SNAPSHOTto2.6.27-SNAPSHOTis a routine maintenance change that aligns with the new word assessment events feature.
Comment on lines
+16
to
+58
| public class WordLearningEventsRestControllerTest { | ||
|
|
||
| @InjectMocks | ||
| private WordLearningEventsRestController wordLearningEventsRestController; | ||
|
|
||
| @Mock | ||
| private HttpServletResponse response; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| MockitoAnnotations.openMocks(this); | ||
| } | ||
|
|
||
| @Test | ||
| public void testHandleUploadCsvRequest_invalidFilename() { | ||
| MultipartFile multipartFile = new MockMultipartFile( | ||
| "file", | ||
| "invalid_filename.csv", | ||
| "text/csv", | ||
| "test content".getBytes() | ||
| ); | ||
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | ||
| JSONObject jsonObject = new JSONObject(jsonResponse); | ||
| assertEquals("error", jsonObject.getString("result")); | ||
| assertEquals("Unexpected filename", jsonObject.getString("errorMessage")); | ||
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| @Test | ||
| public void testHandleUploadCsvRequest_emptyFile() { | ||
| MultipartFile multipartFile = new MockMultipartFile( | ||
| "file", | ||
| "7161a85a0e4751cd_3001012_word-learning-events_2020-04-23.csv", | ||
| "text/csv", | ||
| "".getBytes() | ||
| ); | ||
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | ||
| JSONObject jsonObject = new JSONObject(jsonResponse); | ||
| assertEquals("error", jsonObject.getString("result")); | ||
| assertEquals("Empty file", jsonObject.getString("errorMessage")); | ||
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for successful file upload scenario.
The test class only covers error scenarios (invalid filename and empty file) but lacks coverage for the successful upload path. Consider adding a test that verifies successful CSV file processing.
+ @Test
+ public void testHandleUploadCsvRequest_success() {
+ MultipartFile multipartFile = new MockMultipartFile(
+ "file",
+ "7161a85a0e4751cd_3001012_word-learning-events_2020-04-23.csv",
+ "text/csv",
+ "sample,csv,content".getBytes()
+ );
+ String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response);
+ JSONObject jsonObject = new JSONObject(jsonResponse);
+ assertEquals("success", jsonObject.getString("result"));
+ assertEquals("The CSV file was uploaded", jsonObject.getString("successMessage"));
+ verify(response).setStatus(HttpServletResponse.SC_OK);
+ }📝 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
| public class WordLearningEventsRestControllerTest { | |
| @InjectMocks | |
| private WordLearningEventsRestController wordLearningEventsRestController; | |
| @Mock | |
| private HttpServletResponse response; | |
| @BeforeEach | |
| public void setUp() { | |
| MockitoAnnotations.openMocks(this); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_invalidFilename() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "invalid_filename.csv", | |
| "text/csv", | |
| "test content".getBytes() | |
| ); | |
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Unexpected filename", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_emptyFile() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "7161a85a0e4751cd_3001012_word-learning-events_2020-04-23.csv", | |
| "text/csv", | |
| "".getBytes() | |
| ); | |
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Empty file", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| } | |
| public class WordLearningEventsRestControllerTest { | |
| @InjectMocks | |
| private WordLearningEventsRestController wordLearningEventsRestController; | |
| @Mock | |
| private HttpServletResponse response; | |
| @BeforeEach | |
| public void setUp() { | |
| MockitoAnnotations.openMocks(this); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_invalidFilename() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "invalid_filename.csv", | |
| "text/csv", | |
| "test content".getBytes() | |
| ); | |
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Unexpected filename", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_emptyFile() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "7161a85a0e4751cd_3001012_word-learning-events_2020-04-23.csv", | |
| "text/csv", | |
| "".getBytes() | |
| ); | |
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Empty file", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_success() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "7161a85a0e4751cd_3001012_word-learning-events_2020-04-23.csv", | |
| "text/csv", | |
| "sample,csv,content".getBytes() | |
| ); | |
| String jsonResponse = wordLearningEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("success", jsonObject.getString("result")); | |
| assertEquals("The CSV file was uploaded", jsonObject.getString("successMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_OK); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/ai/elimu/rest/v2/analytics/WordLearningEventsRestControllerTest.java
around lines 16 to 58, the test class currently only covers error scenarios for
invalid filename and empty file uploads. Add a new test method that simulates a
valid CSV file upload with a correct filename and non-empty content, then assert
that the response indicates success and verify that the HTTP response status is
set appropriately for a successful operation.
Comment on lines
+16
to
+58
| public class WordAssessmentEventsRestControllerTest { | ||
|
|
||
| @InjectMocks | ||
| private WordAssessmentEventsRestController wordAssessmentEventsRestController; | ||
|
|
||
| @Mock | ||
| private HttpServletResponse response; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| MockitoAnnotations.openMocks(this); | ||
| } | ||
|
|
||
| @Test | ||
| public void testHandleUploadCsvRequest_invalidFilename() { | ||
| MultipartFile multipartFile = new MockMultipartFile( | ||
| "file", | ||
| "invalid_filename.csv", | ||
| "text/csv", | ||
| "test content".getBytes() | ||
| ); | ||
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | ||
| JSONObject jsonObject = new JSONObject(jsonResponse); | ||
| assertEquals("error", jsonObject.getString("result")); | ||
| assertEquals("Unexpected filename", jsonObject.getString("errorMessage")); | ||
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| @Test | ||
| public void testHandleUploadCsvRequest_emptyFile() { | ||
| MultipartFile multipartFile = new MockMultipartFile( | ||
| "file", | ||
| "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv", | ||
| "text/csv", | ||
| "".getBytes() | ||
| ); | ||
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | ||
| JSONObject jsonObject = new JSONObject(jsonResponse); | ||
| assertEquals("error", jsonObject.getString("result")); | ||
| assertEquals("Empty file", jsonObject.getString("errorMessage")); | ||
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for successful file upload scenario.
Similar to the WordLearningEventsRestControllerTest, this test class only covers error scenarios but lacks coverage for the successful upload path. Consider adding a test for the happy path to ensure complete test coverage.
+ @Test
+ public void testHandleUploadCsvRequest_success() {
+ MultipartFile multipartFile = new MockMultipartFile(
+ "file",
+ "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv",
+ "text/csv",
+ "sample,csv,content".getBytes()
+ );
+ String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response);
+ JSONObject jsonObject = new JSONObject(jsonResponse);
+ assertEquals("success", jsonObject.getString("result"));
+ assertEquals("The CSV file was uploaded", jsonObject.getString("successMessage"));
+ verify(response).setStatus(HttpServletResponse.SC_OK);
+ }📝 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
| public class WordAssessmentEventsRestControllerTest { | |
| @InjectMocks | |
| private WordAssessmentEventsRestController wordAssessmentEventsRestController; | |
| @Mock | |
| private HttpServletResponse response; | |
| @BeforeEach | |
| public void setUp() { | |
| MockitoAnnotations.openMocks(this); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_invalidFilename() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "invalid_filename.csv", | |
| "text/csv", | |
| "test content".getBytes() | |
| ); | |
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Unexpected filename", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_emptyFile() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv", | |
| "text/csv", | |
| "".getBytes() | |
| ); | |
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Empty file", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| } | |
| public class WordAssessmentEventsRestControllerTest { | |
| @InjectMocks | |
| private WordAssessmentEventsRestController wordAssessmentEventsRestController; | |
| @Mock | |
| private HttpServletResponse response; | |
| @BeforeEach | |
| public void setUp() { | |
| MockitoAnnotations.openMocks(this); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_invalidFilename() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "invalid_filename.csv", | |
| "text/csv", | |
| "test content".getBytes() | |
| ); | |
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Unexpected filename", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_emptyFile() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv", | |
| "text/csv", | |
| "".getBytes() | |
| ); | |
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("error", jsonObject.getString("result")); | |
| assertEquals("Empty file", jsonObject.getString("errorMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } | |
| @Test | |
| public void testHandleUploadCsvRequest_success() { | |
| MultipartFile multipartFile = new MockMultipartFile( | |
| "file", | |
| "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv", | |
| "text/csv", | |
| "sample,csv,content".getBytes() | |
| ); | |
| String jsonResponse = wordAssessmentEventsRestController.handleUploadCsvRequest(multipartFile, response); | |
| JSONObject jsonObject = new JSONObject(jsonResponse); | |
| assertEquals("success", jsonObject.getString("result")); | |
| assertEquals("The CSV file was uploaded", jsonObject.getString("successMessage")); | |
| verify(response).setStatus(HttpServletResponse.SC_OK); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/test/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestControllerTest.java
between lines 16 and 58, add a new test method to cover the successful file
upload scenario. Create a MultipartFile with a valid filename and non-empty
content that matches the expected pattern. Call handleUploadCsvRequest with this
file and the mocked response, then assert that the JSON response indicates
success and verify that the response status is set appropriately (e.g., HTTP 200
OK). This will ensure the happy path is tested alongside the existing error
cases.
Comment on lines
+47
to
+52
| // Expected format: "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv" | ||
| String originalFilename = multipartFile.getOriginalFilename(); | ||
| log.info("originalFilename: " + originalFilename); | ||
| if (originalFilename.length() != "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv".length()) { | ||
| throw new IllegalArgumentException("Unexpected filename"); | ||
| } |
Contributor
There was a problem hiding this comment.
Strengthen filename validation to prevent security vulnerabilities.
The current validation only checks filename length but doesn't validate the actual format pattern. This could allow malicious filenames that match the length but contain path traversal characters or other harmful content.
- if (originalFilename.length() != "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv".length()) {
- throw new IllegalArgumentException("Unexpected filename");
- }
+ // Validate filename is not null and matches expected pattern
+ if (originalFilename == null || originalFilename.length() != "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv".length()) {
+ throw new IllegalArgumentException("Unexpected filename");
+ }
+
+ // Validate filename pattern to prevent path traversal and ensure expected format
+ String expectedPattern = "^[a-f0-9]{16}_\\d{7}_word-assessment-events_\\d{4}-\\d{2}-\\d{2}\\.csv$";
+ if (!originalFilename.matches(expectedPattern)) {
+ throw new IllegalArgumentException("Filename does not match expected pattern");
+ }📝 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
| // Expected format: "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv" | |
| String originalFilename = multipartFile.getOriginalFilename(); | |
| log.info("originalFilename: " + originalFilename); | |
| if (originalFilename.length() != "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv".length()) { | |
| throw new IllegalArgumentException("Unexpected filename"); | |
| } | |
| // Expected format: "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv" | |
| String originalFilename = multipartFile.getOriginalFilename(); | |
| log.info("originalFilename: " + originalFilename); | |
| - if (originalFilename.length() != "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv".length()) { | |
| - throw new IllegalArgumentException("Unexpected filename"); | |
| - } | |
| + // Validate filename is not null and matches expected length | |
| + if (originalFilename == null || originalFilename.length() != "7161a85a0e4751cd_3001012_word-assessment-events_2020-04-23.csv".length()) { | |
| + throw new IllegalArgumentException("Unexpected filename"); | |
| + } | |
| + | |
| + // Validate filename pattern to prevent path traversal and ensure expected format | |
| + String expectedPattern = "^[a-f0-9]{16}_\\d{7}_word-assessment-events_\\d{4}-\\d{2}-\\d{2}\\.csv$"; | |
| + if (!originalFilename.matches(expectedPattern)) { | |
| + throw new IllegalArgumentException("Filename does not match expected pattern"); | |
| + } |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestController.java
around lines 47 to 52, the filename validation only checks the length, which is
insufficient and may allow malicious filenames. Replace the length check with a
strict pattern validation using a regular expression that matches the expected
filename format, ensuring it excludes path traversal characters and other
invalid content. This will strengthen security by validating the filename
structure explicitly.
Comment on lines
+54
to
+58
| String androidIdExtractedFromFilename = AnalyticsHelper.extractAndroidIdFromCsvFilename(originalFilename); | ||
| log.info("androidIdExtractedFromFilename: \"" + androidIdExtractedFromFilename + "\""); | ||
|
|
||
| Integer versionCodeExtractedFromFilename = AnalyticsHelper.extractVersionCodeFromCsvFilename(originalFilename); | ||
| log.info("versionCodeExtractedFromFilename: " + versionCodeExtractedFromFilename); |
Contributor
There was a problem hiding this comment.
Validate extracted metadata to prevent injection attacks.
The extracted Android ID and version code should be validated before using them in file paths to prevent directory traversal attacks.
String androidIdExtractedFromFilename = AnalyticsHelper.extractAndroidIdFromCsvFilename(originalFilename);
+ // Validate Android ID format (should be 16 hex characters)
+ if (!androidIdExtractedFromFilename.matches("^[a-f0-9]{16}$")) {
+ throw new IllegalArgumentException("Invalid Android ID format");
+ }
log.info("androidIdExtractedFromFilename: \"" + androidIdExtractedFromFilename + "\"");
Integer versionCodeExtractedFromFilename = AnalyticsHelper.extractVersionCodeFromCsvFilename(originalFilename);
+ // Validate version code is positive
+ if (versionCodeExtractedFromFilename <= 0) {
+ throw new IllegalArgumentException("Invalid version code");
+ }
log.info("versionCodeExtractedFromFilename: " + versionCodeExtractedFromFilename);🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestController.java
around lines 54 to 58, the extracted Android ID and version code from the
filename are used without validation, which risks directory traversal or
injection attacks. Add validation checks to ensure the Android ID contains only
allowed characters (e.g., alphanumeric) and the version code is a valid integer
within expected bounds before using them in file paths. Reject or sanitize any
invalid values to prevent malicious input from affecting file system operations.
Comment on lines
+64
to
+74
| File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai"); | ||
| File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language"))); | ||
| File analyticsDir = new File(languageDir, "analytics"); | ||
| File androidIdDir = new File(analyticsDir, "android-id-" + androidIdExtractedFromFilename); | ||
| File versionCodeDir = new File(androidIdDir, "version-code-" + versionCodeExtractedFromFilename); | ||
| File wordAssessmentEventsDir = new File(versionCodeDir, "word-assessment-events"); | ||
| wordAssessmentEventsDir.mkdirs(); | ||
| File csvFile = new File(wordAssessmentEventsDir, originalFilename); | ||
| log.info("Storing CSV file at " + csvFile); | ||
| FileUtils.writeByteArrayToFile(csvFile, bytes); | ||
| log.info("csvFile.exists(): " + csvFile.exists()); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve directory creation error handling and add path sanitization.
The mkdirs() method can fail silently, and the file path construction should include additional validation to prevent issues.
// Store the original CSV file on the filesystem
File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai");
File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language")));
File analyticsDir = new File(languageDir, "analytics");
File androidIdDir = new File(analyticsDir, "android-id-" + androidIdExtractedFromFilename);
File versionCodeDir = new File(androidIdDir, "version-code-" + versionCodeExtractedFromFilename);
File wordAssessmentEventsDir = new File(versionCodeDir, "word-assessment-events");
- wordAssessmentEventsDir.mkdirs();
+
+ // Ensure directory creation succeeds
+ if (!wordAssessmentEventsDir.exists() && !wordAssessmentEventsDir.mkdirs()) {
+ throw new RuntimeException("Failed to create directory: " + wordAssessmentEventsDir.getAbsolutePath());
+ }
+
File csvFile = new File(wordAssessmentEventsDir, originalFilename);
+
+ // Verify the final path is within expected directory (prevent path traversal)
+ if (!csvFile.getCanonicalPath().startsWith(wordAssessmentEventsDir.getCanonicalPath())) {
+ throw new SecurityException("Invalid file path detected");
+ }
+
log.info("Storing CSV file at " + csvFile);
FileUtils.writeByteArrayToFile(csvFile, bytes);
log.info("csvFile.exists(): " + csvFile.exists());📝 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
| File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai"); | |
| File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language"))); | |
| File analyticsDir = new File(languageDir, "analytics"); | |
| File androidIdDir = new File(analyticsDir, "android-id-" + androidIdExtractedFromFilename); | |
| File versionCodeDir = new File(androidIdDir, "version-code-" + versionCodeExtractedFromFilename); | |
| File wordAssessmentEventsDir = new File(versionCodeDir, "word-assessment-events"); | |
| wordAssessmentEventsDir.mkdirs(); | |
| File csvFile = new File(wordAssessmentEventsDir, originalFilename); | |
| log.info("Storing CSV file at " + csvFile); | |
| FileUtils.writeByteArrayToFile(csvFile, bytes); | |
| log.info("csvFile.exists(): " + csvFile.exists()); | |
| // Store the original CSV file on the filesystem | |
| File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai"); | |
| File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language"))); | |
| File analyticsDir = new File(languageDir, "analytics"); | |
| File androidIdDir = new File(analyticsDir, "android-id-" + androidIdExtractedFromFilename); | |
| File versionCodeDir = new File(androidIdDir, "version-code-" + versionCodeExtractedFromFilename); | |
| File wordAssessmentEventsDir = new File(versionCodeDir, "word-assessment-events"); | |
| // Ensure directory creation succeeds | |
| if (!wordAssessmentEventsDir.exists() && !wordAssessmentEventsDir.mkdirs()) { | |
| throw new RuntimeException("Failed to create directory: " + wordAssessmentEventsDir.getAbsolutePath()); | |
| } | |
| File csvFile = new File(wordAssessmentEventsDir, originalFilename); | |
| // Verify the final path is within expected directory (prevent path traversal) | |
| if (!csvFile.getCanonicalPath().startsWith(wordAssessmentEventsDir.getCanonicalPath())) { | |
| throw new SecurityException("Invalid file path detected"); | |
| } | |
| log.info("Storing CSV file at " + csvFile); | |
| FileUtils.writeByteArrayToFile(csvFile, bytes); | |
| log.info("csvFile.exists(): " + csvFile.exists()); |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestController.java
around lines 64 to 74, the call to mkdirs() for creating directories does not
check if the operation succeeded, which can cause silent failures. Modify the
code to verify the return value of mkdirs() and log or handle errors if
directory creation fails. Additionally, sanitize and validate the components
used in constructing file paths, such as language,
androidIdExtractedFromFilename, versionCodeExtractedFromFilename, and
originalFilename, to prevent invalid or malicious path inputs.
Comment on lines
+79
to
+84
| } catch (Exception ex) { | ||
| log.error(ex.getMessage()); | ||
|
|
||
| jsonResponseObject.put("result", "error"); | ||
| jsonResponseObject.put("errorMessage", ex.getMessage()); | ||
| response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling specificity.
The broad catch (Exception ex) clause may mask important error details and security exceptions should be handled differently from validation errors.
- } catch (Exception ex) {
- log.error(ex.getMessage());
+ } catch (IllegalArgumentException ex) {
+ log.warn("Validation error: " + ex.getMessage());
+
+ jsonResponseObject.put("result", "error");
+ jsonResponseObject.put("errorMessage", ex.getMessage());
+ response.setStatus(HttpStatus.BAD_REQUEST.value());
+ } catch (SecurityException ex) {
+ log.error("Security violation: " + ex.getMessage());
+
+ jsonResponseObject.put("result", "error");
+ jsonResponseObject.put("errorMessage", "Invalid request");
+ response.setStatus(HttpStatus.BAD_REQUEST.value());
+ } catch (Exception ex) {
+ log.error("Unexpected error: " + ex.getMessage(), ex);
jsonResponseObject.put("result", "error");
- jsonResponseObject.put("errorMessage", ex.getMessage());
+ jsonResponseObject.put("errorMessage", "Internal server error");
response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value());📝 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
| } catch (Exception ex) { | |
| log.error(ex.getMessage()); | |
| jsonResponseObject.put("result", "error"); | |
| jsonResponseObject.put("errorMessage", ex.getMessage()); | |
| response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); | |
| } catch (IllegalArgumentException ex) { | |
| log.warn("Validation error: " + ex.getMessage()); | |
| jsonResponseObject.put("result", "error"); | |
| jsonResponseObject.put("errorMessage", ex.getMessage()); | |
| response.setStatus(HttpStatus.BAD_REQUEST.value()); | |
| } catch (SecurityException ex) { | |
| log.error("Security violation: " + ex.getMessage()); | |
| jsonResponseObject.put("result", "error"); | |
| jsonResponseObject.put("errorMessage", "Invalid request"); | |
| response.setStatus(HttpStatus.BAD_REQUEST.value()); | |
| } catch (Exception ex) { | |
| log.error("Unexpected error: " + ex.getMessage(), ex); | |
| jsonResponseObject.put("result", "error"); | |
| jsonResponseObject.put("errorMessage", "Internal server error"); | |
| response.setStatus(HttpStatus.INTERNAL_SERVER_ERROR.value()); |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/rest/v2/analytics/WordAssessmentEventsRestController.java
between lines 79 and 84, replace the broad catch (Exception ex) with more
specific exception handling. Identify and catch distinct exceptions such as
validation exceptions and security-related exceptions separately, handling each
appropriately. This will improve error clarity and allow different responses or
logging for different error types instead of a generic catch-all.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (1)
23-44: LGTM! Test implementation follows established patterns.The test method correctly follows the existing pattern used by other event type tests in this class. It validates both the extraction count and specific properties of the first event.
Consider adding assertions for the second event as well to ensure comprehensive validation:
WordAssessmentEvent wordAssessmentEvent = wordAssessmentEvents.get(0); assertEquals(1742293958238L, wordAssessmentEvent.getTimestamp().getTimeInMillis()); assertEquals("5b7c682a12ecbe2e", wordAssessmentEvent.getAndroidId()); assertEquals("ai.elimu.kukariri.debug", wordAssessmentEvent.getPackageName()); assertEquals("ฉัน", wordAssessmentEvent.getWordText()); assertEquals(33, wordAssessmentEvent.getWordId()); assertEquals(1.0F, wordAssessmentEvent.getMasteryScore()); assertEquals(15000, wordAssessmentEvent.getTimeSpentMs()); + +WordAssessmentEvent secondEvent = wordAssessmentEvents.get(1); +// Add assertions for the second event propertiessrc/main/java/ai/elimu/web/analytics/students/StudentController.java (1)
89-106: LGTM! Word assessment event processing follows established patterns.The logic for fetching word assessment events and preparing weekly chart data is consistent with existing event types in the controller. The implementation correctly:
- Fetches events using the DAO
- Builds weekly count aggregation
- Adds data to the model for JSP consumption
Consider extracting the weekly aggregation logic into a common helper method to reduce code duplication across different event types:
private List<Integer> buildWeeklyEventCounts(List<? extends BaseEvent> events, Calendar calendar6MonthsAgo, Calendar calendarNow, SimpleDateFormat simpleDateFormat) { // Common aggregation logic }src/main/java/ai/elimu/dao/jpa/WordAssessmentEventDaoJpa.java (1)
34-42: Consider adding pagination for large datasets.The
readAllmethod could potentially return large result sets for users with many assessment events, which could impact performance and memory usage.Consider adding pagination support:
@Override -public List<WordAssessmentEvent> readAll(String androidId) throws DataAccessException { +public List<WordAssessmentEvent> readAll(String androidId, int offset, int limit) throws DataAccessException { return em.createQuery( "SELECT event " + "FROM WordAssessmentEvent event " + "WHERE event.androidId = :androidId " + "ORDER BY event.id") .setParameter("androidId", androidId) + .setFirstResult(offset) + .setMaxResults(limit) .getResultList(); }Alternatively, add a separate paginated method while keeping the current one for backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/webapp/WEB-INF/spring/applicationContext-jpa.xmlis excluded by!**/*.xmlsrc/test/resources/ai/elimu/util/csv/5b7c682a12ecbe2e_3002024_word-assessment-events_2025-06-01.csvis excluded by!**/*.csv,!**/*.csv
📒 Files selected for processing (10)
src/main/java/ai/elimu/dao/WordAssessmentEventDao.java(1 hunks)src/main/java/ai/elimu/dao/jpa/WordAssessmentEventDaoJpa.java(1 hunks)src/main/java/ai/elimu/entity/analytics/WordAssessmentEvent.java(1 hunks)src/main/java/ai/elimu/tasks/analytics/WordAssessmentEventImportScheduler.java(1 hunks)src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java(2 hunks)src/main/java/ai/elimu/web/analytics/students/StudentController.java(3 hunks)src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java(3 hunks)src/main/resources/META-INF/jpa-schema-export.sql(3 hunks)src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp(1 hunks)src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/ai/elimu/entity/analytics/WordAssessmentEvent.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/ai/elimu/tasks/analytics/WordAssessmentEventImportScheduler.java (1)
src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java (1)
Slf4j(28-245)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_ui
- GitHub Check: test_rest
- GitHub Check: test_rest
- GitHub Check: build (ubuntu-latest, 17)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: build (ubuntu-latest, 21)
🔇 Additional comments (10)
src/test/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelperTest.java (1)
16-18: LGTM! Imports are correctly added for the new test.The new imports for
WordAssessmentEventandAssessmentEventTypeare properly added to support the new test method.src/main/webapp/WEB-INF/jsp/analytics/students/id.jsp (2)
168-191: LGTM! Chart implementation follows established patterns.The Chart.js implementation for word assessment events is consistent with other charts in the file. The configuration uses appropriate styling and data binding.
192-218: LGTM! Table implementation correctly displays recent events.The table properly displays the 5 most recent word assessment events using reverse iteration logic. The column headers and data fields are appropriately mapped.
src/main/java/ai/elimu/dao/WordAssessmentEventDao.java (1)
1-15: LGTM! Clean and well-designed DAO interface.The interface follows established patterns in the codebase:
- Properly extends
GenericDao<WordAssessmentEvent>- Method signatures are clear and appropriate
- Correct exception handling with
DataAccessException- Good separation of concerns with focused functionality
The
readmethod for specific lookups andreadAllmethod for bulk retrieval provide the necessary data access operations for this entity.src/main/java/ai/elimu/web/analytics/students/StudentController.java (1)
8-8: LGTM! Dependencies and imports correctly added.The new import for
WordAssessmentEventand theWordAssessmentEventDaodependency are properly added to support word assessment event functionality.Also applies to: 14-14, 47-47
src/main/java/ai/elimu/dao/jpa/WordAssessmentEventDaoJpa.java (1)
15-31: Well-implemented composite key lookup with proper exception handling.The
readmethod correctly uses parameterized queries to prevent SQL injection and handles theNoResultExceptionappropriately by returning null when no matching record is found.src/main/resources/META-INF/jpa-schema-export.sql (2)
104-104: Schema cleanup properly implemented.The drop statement for the new table is correctly included in the cleanup section.
609-621: Well-designed table structure following existing patterns.The
WordAssessmentEventtable structure is consistent with other event tables in the schema, using appropriate data types and following established naming conventions.src/main/java/ai/elimu/tasks/analytics/WordAssessmentEventImportScheduler.java (2)
52-53: LGTM: Proper scheduling configuration.The cron expression and synchronization are well implemented. Running at 25 minutes past every hour provides good spacing, and the synchronized method prevents concurrent executions.
18-42: Excellent documentation.The javadoc with the detailed folder structure example is very helpful for understanding the expected directory layout and file naming conventions.
Comment on lines
+51
to
+75
| long timestampInMillis = Long.valueOf(csvRecord.get("timestamp")); | ||
| Calendar timestamp = Calendar.getInstance(TimeZone.getTimeZone("UTC")); | ||
| timestamp.setTimeInMillis(timestampInMillis); | ||
| wordAssessmentEvent.setTimestamp(timestamp); | ||
|
|
||
| String androidId = csvRecord.get("android_id"); | ||
| wordAssessmentEvent.setAndroidId(androidId); | ||
|
|
||
| String packageName = csvRecord.get("package_name"); | ||
| wordAssessmentEvent.setPackageName(packageName); | ||
|
|
||
| String wordText = csvRecord.get("word_text"); | ||
| wordAssessmentEvent.setWordText(wordText); | ||
|
|
||
| Long wordId = Long.valueOf(csvRecord.get("word_id")); | ||
| wordAssessmentEvent.setWordId(wordId); | ||
|
|
||
| Float masteryScore = Float.valueOf(csvRecord.get("mastery_score")); | ||
| wordAssessmentEvent.setMasteryScore(masteryScore); | ||
|
|
||
| Long timeSpentMs = Long.valueOf(csvRecord.get("time_spent_ms")); | ||
| wordAssessmentEvent.setTimeSpentMs(timeSpentMs); | ||
|
|
||
| String additionalData = csvRecord.get("additional_data"); | ||
| wordAssessmentEvent.setAdditionalData(additionalData); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for CSV field parsing.
The method directly parses CSV fields without validation, which could cause runtime exceptions for malformed data.
Add validation before parsing numeric fields:
- Long wordId = Long.valueOf(csvRecord.get("word_id"));
+ String wordIdStr = csvRecord.get("word_id");
+ if (wordIdStr == null || wordIdStr.trim().isEmpty()) {
+ log.warn("Skipping record with missing word_id");
+ continue;
+ }
+ Long wordId = Long.valueOf(wordIdStr);
wordAssessmentEvent.setWordId(wordId);
- Float masteryScore = Float.valueOf(csvRecord.get("mastery_score"));
+ String masteryScoreStr = csvRecord.get("mastery_score");
+ if (masteryScoreStr == null || masteryScoreStr.trim().isEmpty()) {
+ log.warn("Skipping record with missing mastery_score");
+ continue;
+ }
+ Float masteryScore = Float.valueOf(masteryScoreStr);
wordAssessmentEvent.setMasteryScore(masteryScore);
- Long timeSpentMs = Long.valueOf(csvRecord.get("time_spent_ms"));
+ String timeSpentMsStr = csvRecord.get("time_spent_ms");
+ if (timeSpentMsStr == null || timeSpentMsStr.trim().isEmpty()) {
+ log.warn("Skipping record with missing time_spent_ms");
+ continue;
+ }
+ Long timeSpentMs = Long.valueOf(timeSpentMsStr);
wordAssessmentEvent.setTimeSpentMs(timeSpentMs);🤖 Prompt for AI Agents
In src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java between
lines 51 and 75, the code parses numeric CSV fields directly without validation,
risking runtime exceptions on malformed data. Add input validation by checking
if each numeric field is non-null and matches the expected numeric format before
parsing. If validation fails, handle the error gracefully, such as by logging a
warning or skipping the record, to prevent exceptions during parsing.
Comment on lines
+31
to
+85
| public static List<WordAssessmentEvent> extractWordAssessmentEvents(File csvFile) { | ||
| log.info("extractWordAssessmentEvents"); | ||
|
|
||
| List<WordAssessmentEvent> wordAssessmentEvents = new ArrayList<>(); | ||
|
|
||
| // Iterate each row in the CSV file | ||
| Path csvFilePath = Paths.get(csvFile.toURI()); | ||
| log.info("csvFilePath: " + csvFilePath); | ||
| try { | ||
| Reader reader = Files.newBufferedReader(csvFilePath); | ||
| CSVFormat csvFormat = CSVFormat.DEFAULT.withFirstRecordAsHeader(); | ||
| log.info("header: " + Arrays.toString(csvFormat.getHeader())); | ||
| CSVParser csvParser = new CSVParser(reader, csvFormat); | ||
| for (CSVRecord csvRecord : csvParser) { | ||
| log.info("csvRecord: " + csvRecord); | ||
|
|
||
| // Convert from CSV to Java | ||
|
|
||
| WordAssessmentEvent wordAssessmentEvent = new WordAssessmentEvent(); | ||
|
|
||
| long timestampInMillis = Long.valueOf(csvRecord.get("timestamp")); | ||
| Calendar timestamp = Calendar.getInstance(TimeZone.getTimeZone("UTC")); | ||
| timestamp.setTimeInMillis(timestampInMillis); | ||
| wordAssessmentEvent.setTimestamp(timestamp); | ||
|
|
||
| String androidId = csvRecord.get("android_id"); | ||
| wordAssessmentEvent.setAndroidId(androidId); | ||
|
|
||
| String packageName = csvRecord.get("package_name"); | ||
| wordAssessmentEvent.setPackageName(packageName); | ||
|
|
||
| String wordText = csvRecord.get("word_text"); | ||
| wordAssessmentEvent.setWordText(wordText); | ||
|
|
||
| Long wordId = Long.valueOf(csvRecord.get("word_id")); | ||
| wordAssessmentEvent.setWordId(wordId); | ||
|
|
||
| Float masteryScore = Float.valueOf(csvRecord.get("mastery_score")); | ||
| wordAssessmentEvent.setMasteryScore(masteryScore); | ||
|
|
||
| Long timeSpentMs = Long.valueOf(csvRecord.get("time_spent_ms")); | ||
| wordAssessmentEvent.setTimeSpentMs(timeSpentMs); | ||
|
|
||
| String additionalData = csvRecord.get("additional_data"); | ||
| wordAssessmentEvent.setAdditionalData(additionalData); | ||
|
|
||
| wordAssessmentEvents.add(wordAssessmentEvent); | ||
| } | ||
| csvParser.close(); | ||
| } catch (IOException ex) { | ||
| log.error(ex.getMessage()); | ||
| } | ||
|
|
||
| return wordAssessmentEvents; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve resource management and add input validation.
The implementation follows the existing pattern but has several areas for improvement:
-
Resource Management: The CSV parser should be closed in a finally block or use try-with-resources to ensure proper cleanup even if exceptions occur.
-
Input Validation: No validation is performed on CSV field values before parsing them as Long/Float, which could cause NumberFormatException.
-
Code Duplication: This method duplicates significant logic from other extraction methods in the class.
Apply this refactor to improve resource management:
- try {
- Reader reader = Files.newBufferedReader(csvFilePath);
- CSVFormat csvFormat = CSVFormat.DEFAULT.withFirstRecordAsHeader();
- log.info("header: " + Arrays.toString(csvFormat.getHeader()));
- CSVParser csvParser = new CSVParser(reader, csvFormat);
+ try (Reader reader = Files.newBufferedReader(csvFilePath);
+ CSVParser csvParser = new CSVParser(reader, CSVFormat.DEFAULT.withFirstRecordAsHeader())) {
+ log.info("header: " + Arrays.toString(csvParser.getHeaderNames().toArray()));
for (CSVRecord csvRecord : csvParser) {
// ... existing logic ...
}
- csvParser.close();
} catch (IOException ex) {
log.error(ex.getMessage());
+ } catch (NumberFormatException ex) {
+ log.error("Invalid number format in CSV: " + ex.getMessage());
}Consider extracting common CSV parsing logic into a shared utility method to reduce code duplication across extraction methods.
📝 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
| public static List<WordAssessmentEvent> extractWordAssessmentEvents(File csvFile) { | |
| log.info("extractWordAssessmentEvents"); | |
| List<WordAssessmentEvent> wordAssessmentEvents = new ArrayList<>(); | |
| // Iterate each row in the CSV file | |
| Path csvFilePath = Paths.get(csvFile.toURI()); | |
| log.info("csvFilePath: " + csvFilePath); | |
| try { | |
| Reader reader = Files.newBufferedReader(csvFilePath); | |
| CSVFormat csvFormat = CSVFormat.DEFAULT.withFirstRecordAsHeader(); | |
| log.info("header: " + Arrays.toString(csvFormat.getHeader())); | |
| CSVParser csvParser = new CSVParser(reader, csvFormat); | |
| for (CSVRecord csvRecord : csvParser) { | |
| log.info("csvRecord: " + csvRecord); | |
| // Convert from CSV to Java | |
| WordAssessmentEvent wordAssessmentEvent = new WordAssessmentEvent(); | |
| long timestampInMillis = Long.valueOf(csvRecord.get("timestamp")); | |
| Calendar timestamp = Calendar.getInstance(TimeZone.getTimeZone("UTC")); | |
| timestamp.setTimeInMillis(timestampInMillis); | |
| wordAssessmentEvent.setTimestamp(timestamp); | |
| String androidId = csvRecord.get("android_id"); | |
| wordAssessmentEvent.setAndroidId(androidId); | |
| String packageName = csvRecord.get("package_name"); | |
| wordAssessmentEvent.setPackageName(packageName); | |
| String wordText = csvRecord.get("word_text"); | |
| wordAssessmentEvent.setWordText(wordText); | |
| Long wordId = Long.valueOf(csvRecord.get("word_id")); | |
| wordAssessmentEvent.setWordId(wordId); | |
| Float masteryScore = Float.valueOf(csvRecord.get("mastery_score")); | |
| wordAssessmentEvent.setMasteryScore(masteryScore); | |
| Long timeSpentMs = Long.valueOf(csvRecord.get("time_spent_ms")); | |
| wordAssessmentEvent.setTimeSpentMs(timeSpentMs); | |
| String additionalData = csvRecord.get("additional_data"); | |
| wordAssessmentEvent.setAdditionalData(additionalData); | |
| wordAssessmentEvents.add(wordAssessmentEvent); | |
| } | |
| csvParser.close(); | |
| } catch (IOException ex) { | |
| log.error(ex.getMessage()); | |
| } | |
| return wordAssessmentEvents; | |
| } | |
| public static List<WordAssessmentEvent> extractWordAssessmentEvents(File csvFile) { | |
| log.info("extractWordAssessmentEvents"); | |
| List<WordAssessmentEvent> wordAssessmentEvents = new ArrayList<>(); | |
| // Iterate each row in the CSV file | |
| Path csvFilePath = Paths.get(csvFile.toURI()); | |
| log.info("csvFilePath: " + csvFilePath); | |
| try (Reader reader = Files.newBufferedReader(csvFilePath); | |
| CSVParser csvParser = new CSVParser(reader, CSVFormat.DEFAULT.withFirstRecordAsHeader())) { | |
| log.info("header: " + Arrays.toString(csvParser.getHeaderNames().toArray())); | |
| for (CSVRecord csvRecord : csvParser) { | |
| log.info("csvRecord: " + csvRecord); | |
| // Convert from CSV to Java | |
| WordAssessmentEvent wordAssessmentEvent = new WordAssessmentEvent(); | |
| long timestampInMillis = Long.valueOf(csvRecord.get("timestamp")); | |
| Calendar timestamp = Calendar.getInstance(TimeZone.getTimeZone("UTC")); | |
| timestamp.setTimeInMillis(timestampInMillis); | |
| wordAssessmentEvent.setTimestamp(timestamp); | |
| String androidId = csvRecord.get("android_id"); | |
| wordAssessmentEvent.setAndroidId(androidId); | |
| String packageName = csvRecord.get("package_name"); | |
| wordAssessmentEvent.setPackageName(packageName); | |
| String wordText = csvRecord.get("word_text"); | |
| wordAssessmentEvent.setWordText(wordText); | |
| Long wordId = Long.valueOf(csvRecord.get("word_id")); | |
| wordAssessmentEvent.setWordId(wordId); | |
| Float masteryScore = Float.valueOf(csvRecord.get("mastery_score")); | |
| wordAssessmentEvent.setMasteryScore(masteryScore); | |
| Long timeSpentMs = Long.valueOf(csvRecord.get("time_spent_ms")); | |
| wordAssessmentEvent.setTimeSpentMs(timeSpentMs); | |
| String additionalData = csvRecord.get("additional_data"); | |
| wordAssessmentEvent.setAdditionalData(additionalData); | |
| wordAssessmentEvents.add(wordAssessmentEvent); | |
| } | |
| } catch (IOException ex) { | |
| log.error(ex.getMessage()); | |
| } catch (NumberFormatException ex) { | |
| log.error("Invalid number format in CSV: " + ex.getMessage()); | |
| } | |
| return wordAssessmentEvents; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/ai/elimu/util/csv/CsvAnalyticsExtractionHelper.java between
lines 31 and 85, improve resource management by replacing the current try-catch
with a try-with-resources statement to automatically close the CSVParser and
Reader, ensuring they are closed even if exceptions occur. Add input validation
before parsing CSV fields to Long or Float by checking if the field is non-null
and properly formatted to prevent NumberFormatException. To reduce code
duplication, refactor the common CSV parsing logic into a shared utility method
that can be reused by other extraction methods in the class.
Comment on lines
+83
to
+89
| try { | ||
| outputStream.write(bytes); | ||
| outputStream.flush(); | ||
| outputStream.close(); | ||
| } catch (IOException ex) { | ||
| log.error(ex.getMessage()); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and resource management.
The current error handling catches IOException but doesn't set an appropriate HTTP status code, which could confuse clients.
The streaming approach suggested above would also resolve the resource management issues by using try-with-resources for the CSVPrinter.
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java
around lines 83 to 89, improve error handling by setting an appropriate HTTP
status code in the catch block when IOException occurs to inform clients of the
failure. Also, refactor the resource management by using try-with-resources for
the CSVPrinter and output stream to ensure they are properly closed even if
exceptions happen, replacing manual close calls.
Comment on lines
+42
to
90
| List<WordAssessmentEvent> wordAssessmentEvents = wordAssessmentEventDao.readAll(student.getAndroidId()); | ||
| log.info("wordAssessmentEvents.size(): " + wordAssessmentEvents.size()); | ||
|
|
||
| // CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| // .setHeader( | ||
| // "id", | ||
| // "timestamp", | ||
| // "package_name", | ||
| // "letter_sound_letters", | ||
| // "letter_sound_sounds", | ||
| // "letter_sound_id", | ||
| // "mastery_score", | ||
| // "time_spent_ms", | ||
| // "additional_data" | ||
| // ) | ||
| // .build(); | ||
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | ||
| .setHeader( | ||
| "id", | ||
| "timestamp", | ||
| "package_name", | ||
| "word_text", | ||
| "word_id", | ||
| "mastery_score", | ||
| "time_spent_ms", | ||
| "additional_data" | ||
| ) | ||
| .build(); | ||
|
|
||
| // StringWriter stringWriter = new StringWriter(); | ||
| // CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
| StringWriter stringWriter = new StringWriter(); | ||
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | ||
|
|
||
| // for (WordAssessmentEvent wordAssessmentEvent : wordAssessmentEvents) { | ||
| // log.info("wordAssessmentEvent.getId(): " + wordAssessmentEvent.getId()); | ||
| for (WordAssessmentEvent wordAssessmentEvent : wordAssessmentEvents) { | ||
| log.info("wordAssessmentEvent.getId(): " + wordAssessmentEvent.getId()); | ||
|
|
||
| // csvPrinter.printRecord( | ||
| // wordAssessmentEvent.getId(), | ||
| // wordAssessmentEvent.getTimestamp().getTimeInMillis(), | ||
| // wordAssessmentEvent.getPackageName(), | ||
| // wordAssessmentEvent.getWordLetters(), | ||
| // wordAssessmentEvent.getWordSounds(), | ||
| // wordAssessmentEvent.getWordId(), | ||
| // wordAssessmentEvent.getMasteryScore(), | ||
| // wordAssessmentEvent.getTimeSpentMs(), | ||
| // wordAssessmentEvent.getAdditionalData() | ||
| // ); | ||
| // } | ||
| // csvPrinter.flush(); | ||
| // csvPrinter.close(); | ||
| csvPrinter.printRecord( | ||
| wordAssessmentEvent.getId(), | ||
| wordAssessmentEvent.getTimestamp().getTimeInMillis(), | ||
| wordAssessmentEvent.getPackageName(), | ||
| wordAssessmentEvent.getWordText(), | ||
| wordAssessmentEvent.getWordId(), | ||
| wordAssessmentEvent.getMasteryScore(), | ||
| wordAssessmentEvent.getTimeSpentMs(), | ||
| wordAssessmentEvent.getAdditionalData() | ||
| ); | ||
| } | ||
| csvPrinter.flush(); | ||
| csvPrinter.close(); | ||
|
|
||
| // String csvFileContent = stringWriter.toString(); | ||
| 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) { | ||
| // log.error(ex.getMessage()); | ||
| // } | ||
| response.setContentType("text/csv"); | ||
| byte[] bytes = csvFileContent.getBytes(); | ||
| response.setContentLength(bytes.length); | ||
| try { | ||
| outputStream.write(bytes); | ||
| outputStream.flush(); | ||
| outputStream.close(); | ||
| } catch (IOException ex) { | ||
| log.error(ex.getMessage()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address performance and memory concerns for large datasets.
The current implementation loads all events into memory before generating CSV, which could cause performance and memory issues for students with many assessment events.
Consider streaming the CSV output directly to avoid memory issues:
List<WordAssessmentEvent> wordAssessmentEvents = wordAssessmentEventDao.readAll(student.getAndroidId());
log.info("wordAssessmentEvents.size(): " + wordAssessmentEvents.size());
+ response.setContentType("text/csv");
+ response.setHeader("Content-Disposition", "attachment; filename=\"word-assessment-events.csv\"");
+
CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
.setHeader(
"id",
"timestamp",
"package_name",
"word_text",
"word_id",
"mastery_score",
"time_spent_ms",
"additional_data"
)
.build();
- StringWriter stringWriter = new StringWriter();
- CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat);
+ try (CSVPrinter csvPrinter = new CSVPrinter(response.getWriter(), csvFormat)) {
for (WordAssessmentEvent wordAssessmentEvent : wordAssessmentEvents) {
log.info("wordAssessmentEvent.getId(): " + wordAssessmentEvent.getId());
csvPrinter.printRecord(
wordAssessmentEvent.getId(),
wordAssessmentEvent.getTimestamp().getTimeInMillis(),
wordAssessmentEvent.getPackageName(),
wordAssessmentEvent.getWordText(),
wordAssessmentEvent.getWordId(),
wordAssessmentEvent.getMasteryScore(),
wordAssessmentEvent.getTimeSpentMs(),
wordAssessmentEvent.getAdditionalData()
);
}
- csvPrinter.flush();
- csvPrinter.close();
-
- 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) {
log.error(ex.getMessage());
+ response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}📝 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
| List<WordAssessmentEvent> wordAssessmentEvents = wordAssessmentEventDao.readAll(student.getAndroidId()); | |
| log.info("wordAssessmentEvents.size(): " + wordAssessmentEvents.size()); | |
| // CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | |
| // .setHeader( | |
| // "id", | |
| // "timestamp", | |
| // "package_name", | |
| // "letter_sound_letters", | |
| // "letter_sound_sounds", | |
| // "letter_sound_id", | |
| // "mastery_score", | |
| // "time_spent_ms", | |
| // "additional_data" | |
| // ) | |
| // .build(); | |
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | |
| .setHeader( | |
| "id", | |
| "timestamp", | |
| "package_name", | |
| "word_text", | |
| "word_id", | |
| "mastery_score", | |
| "time_spent_ms", | |
| "additional_data" | |
| ) | |
| .build(); | |
| // StringWriter stringWriter = new StringWriter(); | |
| // CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | |
| StringWriter stringWriter = new StringWriter(); | |
| CSVPrinter csvPrinter = new CSVPrinter(stringWriter, csvFormat); | |
| // for (WordAssessmentEvent wordAssessmentEvent : wordAssessmentEvents) { | |
| // log.info("wordAssessmentEvent.getId(): " + wordAssessmentEvent.getId()); | |
| for (WordAssessmentEvent wordAssessmentEvent : wordAssessmentEvents) { | |
| log.info("wordAssessmentEvent.getId(): " + wordAssessmentEvent.getId()); | |
| // csvPrinter.printRecord( | |
| // wordAssessmentEvent.getId(), | |
| // wordAssessmentEvent.getTimestamp().getTimeInMillis(), | |
| // wordAssessmentEvent.getPackageName(), | |
| // wordAssessmentEvent.getWordLetters(), | |
| // wordAssessmentEvent.getWordSounds(), | |
| // wordAssessmentEvent.getWordId(), | |
| // wordAssessmentEvent.getMasteryScore(), | |
| // wordAssessmentEvent.getTimeSpentMs(), | |
| // wordAssessmentEvent.getAdditionalData() | |
| // ); | |
| // } | |
| // csvPrinter.flush(); | |
| // csvPrinter.close(); | |
| csvPrinter.printRecord( | |
| wordAssessmentEvent.getId(), | |
| wordAssessmentEvent.getTimestamp().getTimeInMillis(), | |
| wordAssessmentEvent.getPackageName(), | |
| wordAssessmentEvent.getWordText(), | |
| wordAssessmentEvent.getWordId(), | |
| wordAssessmentEvent.getMasteryScore(), | |
| wordAssessmentEvent.getTimeSpentMs(), | |
| wordAssessmentEvent.getAdditionalData() | |
| ); | |
| } | |
| csvPrinter.flush(); | |
| csvPrinter.close(); | |
| // String csvFileContent = stringWriter.toString(); | |
| 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) { | |
| // log.error(ex.getMessage()); | |
| // } | |
| response.setContentType("text/csv"); | |
| byte[] bytes = csvFileContent.getBytes(); | |
| response.setContentLength(bytes.length); | |
| try { | |
| outputStream.write(bytes); | |
| outputStream.flush(); | |
| outputStream.close(); | |
| } catch (IOException ex) { | |
| log.error(ex.getMessage()); | |
| } | |
| } | |
| List<WordAssessmentEvent> wordAssessmentEvents = wordAssessmentEventDao.readAll(student.getAndroidId()); | |
| log.info("wordAssessmentEvents.size(): " + wordAssessmentEvents.size()); | |
| response.setContentType("text/csv"); | |
| response.setHeader("Content-Disposition", "attachment; filename=\"word-assessment-events.csv\""); | |
| CSVFormat csvFormat = CSVFormat.DEFAULT.builder() | |
| .setHeader( | |
| "id", | |
| "timestamp", | |
| "package_name", | |
| "word_text", | |
| "word_id", | |
| "mastery_score", | |
| "time_spent_ms", | |
| "additional_data" | |
| ) | |
| .build(); | |
| try (CSVPrinter csvPrinter = new CSVPrinter(response.getWriter(), csvFormat)) { | |
| for (WordAssessmentEvent wordAssessmentEvent : wordAssessmentEvents) { | |
| log.info("wordAssessmentEvent.getId(): " + wordAssessmentEvent.getId()); | |
| csvPrinter.printRecord( | |
| wordAssessmentEvent.getId(), | |
| wordAssessmentEvent.getTimestamp().getTimeInMillis(), | |
| wordAssessmentEvent.getPackageName(), | |
| wordAssessmentEvent.getWordText(), | |
| wordAssessmentEvent.getWordId(), | |
| wordAssessmentEvent.getMasteryScore(), | |
| wordAssessmentEvent.getTimeSpentMs(), | |
| wordAssessmentEvent.getAdditionalData() | |
| ); | |
| } | |
| } catch (IOException ex) { | |
| log.error(ex.getMessage()); | |
| response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/web/analytics/students/WordAssessmentEventsCsvExportController.java
between lines 42 and 90, the code loads all WordAssessmentEvent objects into
memory before writing the CSV, which can cause performance and memory issues for
large datasets. To fix this, refactor the code to stream the CSV output directly
to the response output stream as you iterate over the events, avoiding storing
all events or the entire CSV content in memory. Use the CSVPrinter with the
response's output stream wrapped in an OutputStreamWriter, and write each record
inside the loop, flushing as needed, then close the printer without buffering
the entire CSV in a StringWriter.
Comment on lines
+996
to
+999
| alter table WordAssessmentEvent | ||
| add constraint FKlxj22iqgrsvw76fld5vsrhb8c | ||
| foreign key (application_id) | ||
| references Application (id); |
Contributor
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consider adding database indexes for query performance.
While the foreign key constraint is properly defined, consider adding indexes on frequently queried fields to improve performance.
Consider adding these indexes after the table creation:
CREATE INDEX idx_wordassessmentevent_androidid ON WordAssessmentEvent (androidId);
CREATE INDEX idx_wordassessmentevent_timestamp ON WordAssessmentEvent (timestamp);
CREATE INDEX idx_wordassessmentevent_composite ON WordAssessmentEvent (timestamp, androidId, packageName);The composite index would particularly benefit the DAO's read method that queries on all three fields.
Verify if similar indexes exist on other event tables for consistency:
🏁 Script executed:
#!/bin/bash
# Check for existing indexes on similar event tables
rg -A 10 -B 2 "CREATE INDEX.*Event.*androidId" .
rg -A 10 -B 2 "CREATE INDEX.*Event.*timestamp" .Length of output: 101
Add indexes for performance on WordAssessmentEvent (and other event tables)
No existing indexes on androidId or timestamp were found for any *Event tables in src/main/resources/META-INF/jpa-schema-export.sql. To improve query performance and maintain consistency, please add the following immediately after the WordAssessmentEvent definition:
@@ src/main/resources/META-INF/jpa-schema-export.sql
alter table WordAssessmentEvent
add constraint FKlxj22iqgrsvw76fld5vsrhb8c
foreign key (application_id)
references Application (id);
+
+ CREATE INDEX idx_wordassessmentevent_androidid
+ ON WordAssessmentEvent (androidId);
+ CREATE INDEX idx_wordassessmentevent_timestamp
+ ON WordAssessmentEvent (timestamp);
+ CREATE INDEX idx_wordassessmentevent_composite
+ ON WordAssessmentEvent (timestamp, androidId, packageName);• Scan for other event tables (e.g., rg -n 'create table .*Event' src/main/resources/META-INF/jpa-schema-export.sql) and apply similar indexes on their frequently filtered columns.
• This will ensure the DAO’s read methods remain performant and consistent across all event tables.
📝 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
| alter table WordAssessmentEvent | |
| add constraint FKlxj22iqgrsvw76fld5vsrhb8c | |
| foreign key (application_id) | |
| references Application (id); | |
| alter table WordAssessmentEvent | |
| add constraint FKlxj22iqgrsvw76fld5vsrhb8c | |
| foreign key (application_id) | |
| references Application (id); | |
| CREATE INDEX idx_wordassessmentevent_androidid | |
| ON WordAssessmentEvent (androidId); | |
| CREATE INDEX idx_wordassessmentevent_timestamp | |
| ON WordAssessmentEvent (timestamp); | |
| CREATE INDEX idx_wordassessmentevent_composite | |
| ON WordAssessmentEvent (timestamp, androidId, packageName); |
🤖 Prompt for AI Agents
In src/main/resources/META-INF/jpa-schema-export.sql around lines 996 to 999,
after the foreign key constraint on WordAssessmentEvent, add indexes on
androidId, timestamp, and a composite index on (timestamp, androidId,
packageName) to improve query performance. Additionally, scan the file for other
event tables and apply similar indexes on their frequently queried columns to
maintain consistency and optimize DAO read operations.
Comment on lines
+57
to
+61
| File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai"); | ||
| File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language"))); | ||
| File analyticsDir = new File(languageDir, "analytics"); | ||
| log.info("analyticsDir: " + analyticsDir); | ||
| analyticsDir.mkdirs(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for directory operations.
The mkdirs() call should check its return value to ensure directories were created successfully.
- analyticsDir.mkdirs();
+ if (!analyticsDir.exists() && !analyticsDir.mkdirs()) {
+ log.error("Failed to create analytics directory: " + analyticsDir);
+ return;
+ }📝 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
| File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai"); | |
| File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language"))); | |
| File analyticsDir = new File(languageDir, "analytics"); | |
| log.info("analyticsDir: " + analyticsDir); | |
| analyticsDir.mkdirs(); | |
| File elimuAiDir = new File(System.getProperty("user.home"), ".elimu-ai"); | |
| File languageDir = new File(elimuAiDir, "lang-" + Language.valueOf(ConfigHelper.getProperty("content.language"))); | |
| File analyticsDir = new File(languageDir, "analytics"); | |
| log.info("analyticsDir: " + analyticsDir); | |
| - analyticsDir.mkdirs(); | |
| + if (!analyticsDir.exists() && !analyticsDir.mkdirs()) { | |
| + log.error("Failed to create analytics directory: " + analyticsDir); | |
| + return; | |
| + } |
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/tasks/analytics/WordAssessmentEventImportScheduler.java
around lines 57 to 61, the mkdirs() method is called without checking if the
directory creation succeeded. Modify the code to check the boolean return value
of mkdirs() and log an error or throw an exception if the directories could not
be created, ensuring proper error handling for directory operations.
Comment on lines
+75
to
+99
| List<WordAssessmentEvent> events = CsvAnalyticsExtractionHelper.extractWordAssessmentEvents(csvFile); | ||
| log.info("events.size(): " + events.size()); | ||
|
|
||
| // Store in database | ||
| for (WordAssessmentEvent event : events) { | ||
| // Check if the event has already been stored in the database | ||
| WordAssessmentEvent existingWordAssessmentEvent = wordAssessmentEventDao.read(event.getTimestamp(), event.getAndroidId(), event.getPackageName()); | ||
| if (existingWordAssessmentEvent != null) { | ||
| log.warn("The event has already been stored in the database. Skipping data import."); | ||
| continue; | ||
| } | ||
|
|
||
| // Generate Student ID | ||
| Student existingStudent = studentDao.read(event.getAndroidId()); | ||
| if (existingStudent == null) { | ||
| Student student = new Student(); | ||
| student.setAndroidId(event.getAndroidId()); | ||
| studentDao.create(student); | ||
| log.info("Stored Student in database with ID " + student.getId()); | ||
| } | ||
|
|
||
| // Store the event in the database | ||
| wordAssessmentEventDao.create(event); | ||
| log.info("Stored event in database with ID " + event.getId()); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider performance optimization and file tracking.
The current implementation processes all CSV files every hour, which could lead to:
- Performance issues with large datasets due to duplicate checking for every event
- Unnecessary reprocessing of the same files
Consider implementing:
- File tracking mechanism (e.g., storing processed file names/checksums)
- Batch processing for database operations
- Transaction management for data consistency
+ // Track processed files to avoid reprocessing
+ // Consider adding a ProcessedFile entity to track completed imports
+
+ // Batch process events for better performance
+ List<WordAssessmentEvent> batchEvents = new ArrayList<>();
+ for (WordAssessmentEvent event : events) {
+ // ... duplicate check logic ...
+ batchEvents.add(event);
+
+ if (batchEvents.size() >= 100) { // Batch size
+ wordAssessmentEventDao.createBatch(batchEvents);
+ batchEvents.clear();
+ }
+ }
+ if (!batchEvents.isEmpty()) {
+ wordAssessmentEventDao.createBatch(batchEvents);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/tasks/analytics/WordAssessmentEventImportScheduler.java
around lines 75 to 99, the current code processes all CSV files every hour
without tracking which files have been processed, causing performance issues and
redundant processing. To fix this, implement a file tracking mechanism by
storing processed file names or checksums to skip already processed files.
Additionally, refactor the database operations to batch insert events instead of
inserting one by one, and wrap the batch processing in a transaction to ensure
data consistency and improve performance.
Comment on lines
+62
to
+106
| for (File analyticsDirFile : analyticsDir.listFiles()) { | ||
| if (analyticsDirFile.getName().startsWith("android-id-")) { | ||
| File androidIdDir = new File(analyticsDir, analyticsDirFile.getName()); | ||
| for (File androidIdDirFile : androidIdDir.listFiles()) { | ||
| if (androidIdDirFile.getName().startsWith("version-code-")) { | ||
| File versionCodeDir = new File(androidIdDir, androidIdDirFile.getName()); | ||
| for (File versionCodeDirFile : versionCodeDir.listFiles()) { | ||
| if (versionCodeDirFile.getName().equals("word-assessment-events")) { | ||
| File wordAssessmentEventsDir = new File(versionCodeDir, versionCodeDirFile.getName()); | ||
| for (File csvFile : wordAssessmentEventsDir.listFiles()) { | ||
| log.info("csvFile: " + csvFile); | ||
|
|
||
| // Convert from CSV to Java | ||
| List<WordAssessmentEvent> events = CsvAnalyticsExtractionHelper.extractWordAssessmentEvents(csvFile); | ||
| log.info("events.size(): " + events.size()); | ||
|
|
||
| // Store in database | ||
| for (WordAssessmentEvent event : events) { | ||
| // Check if the event has already been stored in the database | ||
| WordAssessmentEvent existingWordAssessmentEvent = wordAssessmentEventDao.read(event.getTimestamp(), event.getAndroidId(), event.getPackageName()); | ||
| if (existingWordAssessmentEvent != null) { | ||
| log.warn("The event has already been stored in the database. Skipping data import."); | ||
| continue; | ||
| } | ||
|
|
||
| // Generate Student ID | ||
| Student existingStudent = studentDao.read(event.getAndroidId()); | ||
| if (existingStudent == null) { | ||
| Student student = new Student(); | ||
| student.setAndroidId(event.getAndroidId()); | ||
| studentDao.create(student); | ||
| log.info("Stored Student in database with ID " + student.getId()); | ||
| } | ||
|
|
||
| // Store the event in the database | ||
| wordAssessmentEventDao.create(event); | ||
| log.info("Stored event in database with ID " + event.getId()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Add null checks and error handling for directory traversal.
The nested directory traversal has several critical issues:
- Null pointer safety:
listFiles()can return null if the directory doesn't exist or can't be read - Error handling: No try-catch blocks around the main processing logic
- File reprocessing: All CSV files are processed every time, with no tracking of already processed files
+ try {
for (File analyticsDirFile : analyticsDir.listFiles()) {
+ if (analyticsDir.listFiles() == null) {
+ log.warn("Cannot read analytics directory: " + analyticsDir);
+ return;
+ }
+
if (analyticsDirFile.getName().startsWith("android-id-")) {
File androidIdDir = new File(analyticsDir, analyticsDirFile.getName());
+ if (androidIdDir.listFiles() == null) {
+ log.warn("Cannot read android ID directory: " + androidIdDir);
+ continue;
+ }
for (File androidIdDirFile : androidIdDir.listFiles()) {
if (androidIdDirFile.getName().startsWith("version-code-")) {
File versionCodeDir = new File(androidIdDir, androidIdDirFile.getName());
+ if (versionCodeDir.listFiles() == null) {
+ log.warn("Cannot read version code directory: " + versionCodeDir);
+ continue;
+ }
for (File versionCodeDirFile : versionCodeDir.listFiles()) {
if (versionCodeDirFile.getName().equals("word-assessment-events")) {
File wordAssessmentEventsDir = new File(versionCodeDir, versionCodeDirFile.getName());
+ if (wordAssessmentEventsDir.listFiles() == null) {
+ log.warn("Cannot read word assessment events directory: " + wordAssessmentEventsDir);
+ continue;
+ }
for (File csvFile : wordAssessmentEventsDir.listFiles()) {
log.info("csvFile: " + csvFile);
// Convert from CSV to Java
List<WordAssessmentEvent> events = CsvAnalyticsExtractionHelper.extractWordAssessmentEvents(csvFile);
log.info("events.size(): " + events.size());
// Store in database
for (WordAssessmentEvent event : events) {
// Check if the event has already been stored in the database
WordAssessmentEvent existingWordAssessmentEvent = wordAssessmentEventDao.read(event.getTimestamp(), event.getAndroidId(), event.getPackageName());
if (existingWordAssessmentEvent != null) {
log.warn("The event has already been stored in the database. Skipping data import.");
continue;
}
// Generate Student ID
Student existingStudent = studentDao.read(event.getAndroidId());
if (existingStudent == null) {
Student student = new Student();
student.setAndroidId(event.getAndroidId());
studentDao.create(student);
log.info("Stored Student in database with ID " + student.getId());
}
// Store the event in the database
wordAssessmentEventDao.create(event);
log.info("Stored event in database with ID " + event.getId());
}
}
}
}
}
}
}
}
+ } catch (Exception e) {
+ log.error("Error during CSV import process", e);
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/ai/elimu/tasks/analytics/WordAssessmentEventImportScheduler.java
between lines 62 and 106, add null checks after every listFiles() call to ensure
the directory exists and is readable before iterating. Wrap the main processing
logic inside try-catch blocks to handle and log any exceptions gracefully.
Implement a mechanism to track processed CSV files, such as maintaining a record
of processed filenames or timestamps, to avoid reprocessing the same files on
subsequent runs.
This was referenced Jun 9, 2025
This was referenced Jun 20, 2025
This was referenced Jul 1, 2025
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.