fix: resolve 5 bugs in scheduling, specialist, JWT, and config layers#69
Open
PragyaTripathi990 wants to merge 2 commits into
Open
fix: resolve 5 bugs in scheduling, specialist, JWT, and config layers#69PragyaTripathi990 wants to merge 2 commits into
PragyaTripathi990 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR applies Java Time-based date/slot handling and transactional semantics in SchedulingServiceImpl, relaxes JwtUtil token invalidation errors, fixes a ConfigProperties key, and adds defensive row validation and logging in SpecialistServiceImpl. ChangesScheduling Service Timezone & Transactional Enhancement
Service Fixes & Configuration Corrections
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
- SchedulingServiceImpl: replace IntStream.forEach lambda that silently swallowed TMException with a plain for-loop so checked exceptions propagate correctly; add @transactional so partial DB writes on error are rolled back atomically - SchedulingServiceImpl: replace deprecated Timestamp.getHours()/ getMinutes() with toLocalDateTime().toLocalTime() to avoid timezone-sensitive slot index miscalculations; replace deprecated new Date(year,month,date) constructors with LocalDate + ZoneId - SpecialistServiceImpl.getAllSpecialist: add action.length < 7 bounds guard before accessing action[6] to prevent ArrayIndexOutOfBounds when the query returns fewer columns than expected - JwtUtil.invalidateToken: remove RuntimeException rethrow in catch block; an already-expired/invalid token is a no-op, not an error — prevents HTTP 500 on logout with expired tokens - ConfigProperties.getExtendExpiryTime: fix wrong property key from "iemr.session.expiry.time" (numeric) to "iemr.extend.expiry.time" (boolean) so session extension actually reads the correct config flag
PragyaTripathi990
force-pushed
the
fix/bugfixes-scheduler
branch
from
29dc94b to
7d4cad6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/iemr/tm/service/schedule/SchedulingServiceImpl.java (1)
158-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDay-span calculation is DST-sensitive and can skip/duplicate dates.
(endDate.getTime() - startDate.getTime()) / 86400000is not stable across DST transitions, so loop bounds can be wrong. UseLocalDate+ChronoUnit.DAYSand iterate date objects directly.Suggested fix
+import java.time.temporal.ChronoUnit; ... - Integer durationDays = (int) ((endDate.getTime() - startDate.getTime()) / 86400000); + LocalDate startLocalDate = LocalDate.ofInstant(startDate.toInstant(), ZoneId.systemDefault()); + LocalDate endLocalDate = LocalDate.ofInstant(endDate.toInstant(), ZoneId.systemDefault()); + long durationDays = ChronoUnit.DAYS.between(startLocalDate, endLocalDate); ... - for (int e = 0; e <= durationDays; e++) { - Date i = new Date(startDate.getTime()); - i.setDate(i.getDate() + e); - Integer day = i.getDay(); + for (long e = 0; e <= durationDays; e++) { + LocalDate d = startLocalDate.plusDays(e); + Date i = Date.from(d.atStartOfDay(ZoneId.systemDefault()).toInstant()); + Integer day = i.getDay();Also applies to: 182-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/schedule/SchedulingServiceImpl.java` around lines 158 - 159, The current day-span calculation using Integer durationDays = (int) ((endDate.getTime() - startDate.getTime()) / 86400000) is DST-sensitive and can produce wrong loop bounds; update SchedulingServiceImpl to convert the Date objects (startDate, endDate) to java.time.LocalDate and compute the span with java.time.temporal.ChronoUnit.DAYS (or iterate from startLocalDate to endLocalDate using LocalDate.plusDays()) when building the loop that currently uses durationDays; replace both the occurrence around the durationDays variable and the similar logic at the later block (lines referenced 182-185) to operate on LocalDate instances rather than millis arithmetic so iteration is stable across DST transitions.
🧹 Nitpick comments (2)
src/main/java/com/iemr/tm/utils/JwtUtil.java (1)
65-67: 💤 Low valueLGTM - logout gracefully handles invalid tokens.
The silent success on exception is correct behavior for logout endpoints. When a token is already expired or malformed, attempting to invalidate it should not fail.
Optional improvement: consider adding debug-level logging to aid troubleshooting:
🔍 Optional logging enhancement
} catch (Exception e) { // Token is already expired or invalid — nothing to denylist, treat as success + logger.debug("Token invalidation skipped (already expired/invalid): {}", e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/utils/JwtUtil.java` around lines 65 - 67, The catch block in JwtUtil that swallows exceptions during token invalidation should remain silent for correctness but add a debug-level log to aid troubleshooting; inside the catch (Exception e) in JwtUtil (the logout/token invalidation method), call the class logger at debug/trace level including a short message and e.getMessage()/exception to record expired/malformed tokens without changing behavior or returning an error.src/main/java/com/iemr/tm/service/specialist/SpecialistServiceImpl.java (1)
55-66: ⚡ Quick winConsider adding similar bounds checking to
getspecialistUserfor consistency.The
getspecialistUsermethod accesses array indices 0-10 without bounds validation, whilegetAllSpecialistnow has defensive checks. If the repository can return malformed data in one method, it may also occur in the other.🛡️ Proposed fix to add bounds checking
if (obj.size() > 0) { for (Object[] action : obj) { + if (action.length < 11) { + logger.warn("Skipping malformed specialist row with {} columns (expected 11) for specializationID: {}, parkingPlaceID: {}", + action.length, specializationID, parkingplaceID); + continue; + } specialistList.add(new Specialist((String) action[0], ((Number) action[1]).longValue(), (String) action[2], (String) action[3], (String) action[4], ((Number) action[5]).longValue(), (String) action[6], (String) action[7], (String) action[8], (String) action[9], (String) action[10]));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/iemr/tm/service/specialist/SpecialistServiceImpl.java` around lines 55 - 66, In SpecialistServiceImpl, update the getspecialistUser method to mirror the defensive checks used in the getspecialistSP handling: ensure the returned List<Object[]> (from specializationRepo.getspecialistUser) is not null/empty and for each Object[] verify its length is at least 11 before indexing; when a row is malformed skip it (or log a warning) and only construct new Specialist instances after safely casting numeric fields using ((Number) action[index]).longValue() and string fields with (String) action[index].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/iemr/tm/service/schedule/SchedulingServiceImpl.java`:
- Around line 97-103: In SchedulingServiceImpl, validate the computed slot
window before calling slotdetail.substring: after computing startLocal,
endLocal, startslot and endslot, check that startslot < endslot, both are >= 0,
and endslot <= slotdetail.length(); if any of these checks fail throw a
TMException with a clear message (e.g., "Invalid slot window: startslot X
endslot Y for slotdetail length Z") instead of allowing
StringIndexOutOfBoundsException; update the code paths that use
startslot/endslot (the substring call referencing slotdetail) to rely on this
validation.
In `@src/main/java/com/iemr/tm/service/specialist/SpecialistServiceImpl.java`:
- Line 76: Replace the silent skip of malformed CSV/array rows in
SpecialistServiceImpl by logging the skipped row for troubleshooting: when you
encounter the check if (action.length < 7) in the method that processes "action"
entries, emit a warning (e.g., logger.warn) that includes the row contents
(Arrays.toString(action) or similar) and any context (e.g., patientId or loop
index if available) before continuing, so malformed data is recorded for later
diagnosis.
---
Outside diff comments:
In `@src/main/java/com/iemr/tm/service/schedule/SchedulingServiceImpl.java`:
- Around line 158-159: The current day-span calculation using Integer
durationDays = (int) ((endDate.getTime() - startDate.getTime()) / 86400000) is
DST-sensitive and can produce wrong loop bounds; update SchedulingServiceImpl to
convert the Date objects (startDate, endDate) to java.time.LocalDate and compute
the span with java.time.temporal.ChronoUnit.DAYS (or iterate from startLocalDate
to endLocalDate using LocalDate.plusDays()) when building the loop that
currently uses durationDays; replace both the occurrence around the durationDays
variable and the similar logic at the later block (lines referenced 182-185) to
operate on LocalDate instances rather than millis arithmetic so iteration is
stable across DST transitions.
---
Nitpick comments:
In `@src/main/java/com/iemr/tm/service/specialist/SpecialistServiceImpl.java`:
- Around line 55-66: In SpecialistServiceImpl, update the getspecialistUser
method to mirror the defensive checks used in the getspecialistSP handling:
ensure the returned List<Object[]> (from specializationRepo.getspecialistUser)
is not null/empty and for each Object[] verify its length is at least 11 before
indexing; when a row is malformed skip it (or log a warning) and only construct
new Specialist instances after safely casting numeric fields using ((Number)
action[index]).longValue() and string fields with (String) action[index].
In `@src/main/java/com/iemr/tm/utils/JwtUtil.java`:
- Around line 65-67: The catch block in JwtUtil that swallows exceptions during
token invalidation should remain silent for correctness but add a debug-level
log to aid troubleshooting; inside the catch (Exception e) in JwtUtil (the
logout/token invalidation method), call the class logger at debug/trace level
including a short message and e.getMessage()/exception to record
expired/malformed tokens without changing behavior or returning an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9fa07d03-e758-4aba-bf8d-c7f868b0eada
📒 Files selected for processing (4)
src/main/java/com/iemr/tm/service/schedule/SchedulingServiceImpl.javasrc/main/java/com/iemr/tm/service/specialist/SpecialistServiceImpl.javasrc/main/java/com/iemr/tm/utils/JwtUtil.javasrc/main/java/com/iemr/tm/utils/config/ConfigProperties.java
…layers - Replace millisecond day-span arithmetic with ChronoUnit.DAYS.between() in SchedulingServiceImpl.markAvailability to avoid DST-sensitive off-by-one errors - Replace deprecated Date.setDate() loop with LocalDate.plusDays() iteration - Add slot window bounds validation before slotdetail.substring() to prevent StringIndexOutOfBoundsException on invalid time inputs - Add SLF4J logger to SpecialistServiceImpl and log a warning when malformed specialist rows (fewer than 7 columns) are skipped in getAllSpecialist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
TMExceptioninSchedulingServiceImpl— replacedIntStream.forEachlambda with a plainforloop so exceptions propagate; added@Transactional(rollbackFor = TMException.class)for atomicityDateAPIs — replacedTimestamp.getHours()/getMinutes()withtoLocalDateTime().toLocalTime()and deprecatednew Date(year, month, date)constructors withLocalDate.ofInstant(..., ZoneId.systemDefault())ArrayIndexOutOfBoundsExceptionrisk inSpecialistServiceImpl.getAllSpecialist— added bounds guard before accessingaction[6]JwtUtil.invalidateToken— removed erroneousRuntimeExceptionrethrow in catch blockConfigProperties.getExtendExpiryTime— changed from"iemr.session.expiry.time"(integer) to"iemr.extend.expiry.time"(boolean) so session extension config is read correctlyTest plan
markAvailabilitywith a date range: verifyTMExceptionon a conflict rolls back all insertsgetAllSpecialistreturns results withoutArrayIndexOutOfBoundsExceptioniemr.extend.expiry.time=truecausesgetExtendExpiryTime()to returntrueSummary by CodeRabbit
Bug Fixes
Refactor