Redis Changes and download master changes#54
Conversation
WalkthroughThe changes involve multiple modifications across various classes in the application, focusing on enhancing session management, improving configuration property handling, and refining authorization processes. Key updates include the introduction of new classes for managing session data and validation, modifications to existing repository methods for timestamp precision, and adjustments to error handling mechanisms. Overall, the changes aim to streamline functionality and improve the robustness of session and configuration management. Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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}
There was a problem hiding this comment.
Actionable comments posted: 19
Outside diff range and nitpick comments (5)
src/main/java/com/iemr/mmu/utils/sessionObject/SessionObject.java (2)
18-18: Remove the commented@Requiredannotation.The
@Requiredannotation is not needed here as the@Autowiredannotation already marks the dependency as required.- // @Required
1-73: Summary of the review:The
SessionObjectclass is properly structured and annotated as a Spring component. It manages session objects using Redis operations through the injectedRedisStoragedependency.However, there are a few areas that need improvement:
The config values
extendExpirationTimeandsessionExpiryTimeare initialized in the constructor but fetched again fromConfigPropertiesfor each method invocation. This is inefficient and should be refactored to use the already initialized fields.The purpose and logic behind updating a concurrent session object based on the
userNamefield in theupdateConcurrentSessionObjectmethod are not clear from the given context. More clarification is needed to understand how this fits into the overall system design and requirements.Please address the refactoring suggestions and provide more context on the concurrent session object update logic.
src/main/java/com/iemr/mmu/utils/validator/Validator.java (2)
22-22: Remove deprecated@RequiredannotationThe
@Requiredannotation is deprecated in recent versions of Spring and is currently commented out. It's safe to remove it to clean up the code.Apply this diff to remove the commented-out annotation:
- // @Required
42-42: Declare logger asstatic finalFor best practices, the logger should be declared as
static finalto ensure that only one instance exists and to reflect that it does not change.Apply this diff:
- private Logger logger = LoggerFactory.getLogger(Validator.class); + private static final Logger logger = LoggerFactory.getLogger(Validator.class);src/main/java/com/iemr/mmu/utils/config/ConfigProperties.java (1)
96-96: Typo in log messages: 'retrival' should be 'retrieval'There's a typo in the log messages where "retrival" should be "retrieval".
Apply this diff to fix the typo:
-logger.error(propertyName + " retrival failed.", e); +logger.error(propertyName + " retrieval failed.", e);Repeat this correction for all occurrences.
Also applies to: 106-106, 116-116, 126-126, 136-136
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- src/main/java/com/iemr/mmu/repo/syncActivity_syncLayer/SyncDownloadMasterRepo.java (1 hunks)
- src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/config/ConfigProperties.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/http/HttpInterceptor.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/redis/RedisSessionException.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/sessionObject/SessionObject.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/validator/Validator.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/iemr/mmu/service/dataSyncActivity/DownloadDataFromServerImpl.java
Additional comments not posted (8)
src/main/java/com/iemr/mmu/utils/redis/RedisSessionException.java (1)
1-13: LGTM!The custom exception class
RedisSessionExceptionis implemented correctly:
- It extends the
IEMRExceptionclass, which is likely a base exception class in the project.- It provides two constructors:
- One that takes a message and a cause, and passes them to the super constructor.
- Another that takes only a message and passes it to the super constructor.
- This is a common and recommended pattern for defining custom exception classes in Java.
The code follows Java naming conventions and there are no apparent issues with the logic or syntax.
src/main/java/com/iemr/mmu/repo/syncActivity_syncLayer/SyncDownloadMasterRepo.java (1)
42-42: Verify impact of changinglastDownloadDateprecision.The change from
current_date()tocurrent_timestamp()for thelastDownloadDatefield seems to be an intentional improvement in precision to capture more granular download timestamps.However, please verify the following:
Ensure that this change does not break any downstream processes, reporting, or assumptions that might be relying on the
lastDownloadDatefield being a date-only value.Review the database schema to confirm that the
lastDownloadDatecolumn is defined with a data type that can accommodate a full timestamp. If it's currently a date-only column, a schema update might be necessary.Scan the codebase for any references to the
lastDownloadDatefield to ensure compatibility with the new timestamp format.Run the following script to search for potential impact areas:
src/main/java/com/iemr/mmu/utils/sessionObject/SessionObject.java (2)
24-28: LGTM!The constructor correctly initializes the
extendExpirationTimeandsessionExpiryTimefields fromConfigProperties.
50-52: Clarify the purpose and logic of updating a concurrent session object.The
updateConcurrentSessionObjectmethod is called to update a concurrent session object based on theuserNamefield in the session data. However, the purpose and logic behind this operation is not clear from the given context.
- What is the significance of updating a concurrent session object?
- How does updating a concurrent session object based on
userNamefit into the overall system design and requirements?Please provide more context and clarification on this aspect of the code.
src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (4)
43-59: LGTM!The
setObjectmethod correctly sets the value for a given key and updates the session expiration time if the key does not exist. The logic is sound and the implementation is accurate.
61-80: LGTM!The
getObjectmethod correctly retrieves the value for a given key and extends the expiration time if specified. The logic is sound and the implementation is accurate. The method also correctly throws aRedisSessionExceptionif the session data does not exist.
82-89: LGTM!The
deleteObjectmethod correctly deletes the session object for a given key using the Redis connection. The logic is sound and the implementation is accurate.
91-113: LGTM!The
updateObjectmethod correctly updates the value for a given key and extends the expiration time if specified. The logic is sound and the implementation is accurate. The method also correctly throws aRedisSessionExceptionif the session data does not exist.
| public String getSessionObject(String key) throws RedisSessionException { | ||
| Boolean extendExpirationTime = ConfigProperties.getExtendExpiryTime(); | ||
| Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime(); | ||
|
|
||
| return objectStore.getObject(key, Boolean.valueOf(extendExpirationTime), sessionExpiryTime); | ||
| } |
There was a problem hiding this comment.
Use the already initialized fields.
The extendExpirationTime and sessionExpiryTime fields are already initialized in the constructor from ConfigProperties. Instead of fetching these values again for each method invocation, directly use the initialized fields.
- Boolean extendExpirationTime = ConfigProperties.getExtendExpiryTime();
- Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime();
+ Boolean extendExpirationTime = this.extendExpirationTime;
+ Integer sessionExpiryTime = this.sessionExpiryTime;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.
| public String getSessionObject(String key) throws RedisSessionException { | |
| Boolean extendExpirationTime = ConfigProperties.getExtendExpiryTime(); | |
| Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime(); | |
| return objectStore.getObject(key, Boolean.valueOf(extendExpirationTime), sessionExpiryTime); | |
| } | |
| public String getSessionObject(String key) throws RedisSessionException { | |
| Boolean extendExpirationTime = this.extendExpirationTime; | |
| Integer sessionExpiryTime = this.sessionExpiryTime; | |
| return objectStore.getObject(key, Boolean.valueOf(extendExpirationTime), sessionExpiryTime); | |
| } |
| public String updateSessionObject(String key, String value) throws RedisSessionException { | ||
| Boolean extendExpirationTime = ConfigProperties.getExtendExpiryTime(); | ||
| Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime(); | ||
|
|
||
| updateConcurrentSessionObject(key, value, extendExpirationTime, sessionExpiryTime); | ||
| return objectStore.updateObject(key, value, Boolean.valueOf(extendExpirationTime), sessionExpiryTime); | ||
| } |
There was a problem hiding this comment.
Use the already initialized fields.
The extendExpirationTime and sessionExpiryTime fields are already initialized in the constructor from ConfigProperties. Instead of fetching these values again for each method invocation, directly use the initialized fields.
- Boolean extendExpirationTime = ConfigProperties.getExtendExpiryTime();
- Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime();
+ Boolean extendExpirationTime = this.extendExpirationTime;
+ Integer sessionExpiryTime = this.sessionExpiryTime;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.
| public String updateSessionObject(String key, String value) throws RedisSessionException { | |
| Boolean extendExpirationTime = ConfigProperties.getExtendExpiryTime(); | |
| Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime(); | |
| updateConcurrentSessionObject(key, value, extendExpirationTime, sessionExpiryTime); | |
| return objectStore.updateObject(key, value, Boolean.valueOf(extendExpirationTime), sessionExpiryTime); | |
| } | |
| public String updateSessionObject(String key, String value) throws RedisSessionException { | |
| Boolean extendExpirationTime = this.extendExpirationTime; | |
| Integer sessionExpiryTime = this.sessionExpiryTime; | |
| updateConcurrentSessionObject(key, value, extendExpirationTime, sessionExpiryTime); | |
| return objectStore.updateObject(key, value, Boolean.valueOf(extendExpirationTime), sessionExpiryTime); | |
| } |
| public String setSessionObject(String key, String value) throws RedisSessionException { | ||
| Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime(); | ||
| return objectStore.setObject(key, value, sessionExpiryTime); | ||
| } |
There was a problem hiding this comment.
Use the already initialized field.
The sessionExpiryTime field is already initialized in the constructor from ConfigProperties. Instead of fetching this value again for each method invocation, directly use the initialized field.
- Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime();
+ Integer sessionExpiryTime = this.sessionExpiryTime;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.
| public String setSessionObject(String key, String value) throws RedisSessionException { | |
| Integer sessionExpiryTime = ConfigProperties.getSessionExpiryTime(); | |
| return objectStore.setObject(key, value, sessionExpiryTime); | |
| } | |
| public String setSessionObject(String key, String value) throws RedisSessionException { | |
| Integer sessionExpiryTime = this.sessionExpiryTime; | |
| return objectStore.setObject(key, value, sessionExpiryTime); | |
| } |
| throw new Exception(); | ||
| } | ||
| } | ||
| } catch (Exception e) | ||
| { | ||
| throw new IEMRException("Session is expired. Please login again."); |
There was a problem hiding this comment.
Avoid throwing and catching generic Exception
Throwing and catching the generic Exception can obscure the actual cause of errors and complicate debugging. Instead, throw a specific exception or handle the condition appropriately.
Apply this diff to throw a specific exception:
- throw new Exception();
+ throw new IEMRException("IP address mismatch. Session may be compromised.");
}
}
- } catch (Exception e)
+ } catch (IEMRException e)
{
- throw new IEMRException("Session is expired. Please login again.");
+ throw e;
}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.
| throw new Exception(); | |
| } | |
| } | |
| } catch (Exception e) | |
| { | |
| throw new IEMRException("Session is expired. Please login again."); | |
| throw new IEMRException("IP address mismatch. Session may be compromised."); | |
| } | |
| } | |
| } catch (IEMRException e) | |
| { | |
| throw e; |
| private static Boolean enableIPValidation = false; | ||
|
|
||
| @Autowired(required = true) | ||
| // @Required | ||
| public void setSessionObject(SessionObject sessionObject) | ||
| { | ||
| this.session = sessionObject; | ||
| } | ||
|
|
||
| // private static void setSessionObject() { | ||
| // if (session == null) { | ||
| // session = new SessionObject(); | ||
| // } | ||
| // } | ||
|
|
||
| public Validator() | ||
| { | ||
| if (!enableIPValidation) | ||
| { | ||
| enableIPValidation = ConfigProperties.getBoolean("enableIPValidation"); | ||
| } |
There was a problem hiding this comment.
Avoid modifying static variables in the constructor
The enableIPValidation variable is declared as static, but it is being modified within the instance constructor. Modifying static variables in the constructor can lead to unexpected behavior in multi-threaded environments and is generally discouraged.
Consider initializing enableIPValidation in a static initializer block or at the point of declaration.
Apply this diff to initialize enableIPValidation at the point of declaration:
- private static Boolean enableIPValidation = false;
+ private static final Boolean enableIPValidation = ConfigProperties.getBoolean("enableIPValidation");Then remove the initialization from the constructor:
- public Validator()
- {
- if (!enableIPValidation)
- {
- enableIPValidation = ConfigProperties.getBoolean("enableIPValidation");
- }
- }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.
| private static Boolean enableIPValidation = false; | |
| @Autowired(required = true) | |
| // @Required | |
| public void setSessionObject(SessionObject sessionObject) | |
| { | |
| this.session = sessionObject; | |
| } | |
| // private static void setSessionObject() { | |
| // if (session == null) { | |
| // session = new SessionObject(); | |
| // } | |
| // } | |
| public Validator() | |
| { | |
| if (!enableIPValidation) | |
| { | |
| enableIPValidation = ConfigProperties.getBoolean("enableIPValidation"); | |
| } | |
| private static final Boolean enableIPValidation = ConfigProperties.getBoolean("enableIPValidation"); | |
| @Autowired(required = true) | |
| // @Required | |
| public void setSessionObject(SessionObject sessionObject) | |
| { | |
| this.session = sessionObject; | |
| } | |
| // private static void setSessionObject() { | |
| // if (session == null) { | |
| // session = new SessionObject(); | |
| // } | |
| // } |
| @Component | ||
| public class HttpInterceptor implements HandlerInterceptor { | ||
| Logger logger = LoggerFactory.getLogger(this.getClass().getName()); | ||
| private Validator validator; |
There was a problem hiding this comment.
Prefer Constructor Injection Over Setter Injection for Mandatory Dependencies
The Validator and SessionObject dependencies are injected via setter methods. It's a best practice to use constructor injection for mandatory dependencies to ensure immutability and make the class's requirements explicit.
Refactor the class to use constructor injection:
public class HttpInterceptor implements HandlerInterceptor {
- private Validator validator;
- private SessionObject sessionObject;
- @Autowired
- public void setValidator(Validator validator) {
- this.validator = validator;
- }
- @Autowired
- public void setSessionObject(SessionObject sessionObject) {
- this.sessionObject = sessionObject;
- }
+ private final Validator validator;
+ private final SessionObject sessionObject;
+ @Autowired
+ public HttpInterceptor(Validator validator, SessionObject sessionObject) {
+ this.validator = validator;
+ this.sessionObject = sessionObject;
+ }Also applies to: 53-53, 49-51, 56-58
| Logger logger = LoggerFactory.getLogger(this.getClass().getName()); | ||
| private Validator validator; | ||
|
|
||
| Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); |
There was a problem hiding this comment.
Consider Using Fully Qualified Class Name in Logger Initialization
The logger is initialized using getClass().getSimpleName(), which returns only the simple class name. Using getClass().getName() provides the fully qualified class name, offering more context in logs, especially in larger projects where class names might overlap.
Apply this change to enhance logging clarity:
- Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName());
+ Logger logger = LoggerFactory.getLogger(this.getClass().getName());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.
| Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); | |
| Logger logger = LoggerFactory.getLogger(this.getClass().getName()); |
| String authorization = request.getHeader("Authorization"); | ||
| logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization | ||
| + " || method :: " + request.getMethod()); |
There was a problem hiding this comment.
Ensure Safe Handling of Null or Missing Authorization Header
The authorization header is retrieved and used in logging without null checks. This could lead to a NullPointerException or logging "null" as the authorization value, which might not be desirable.
Add a null check for the authorization header:
String authorization = request.getHeader("Authorization");
+ if (authorization == null || authorization.trim().isEmpty()) {
+ logger.warn("Authorization header is missing or empty for request URI: " + request.getRequestURI());
+ // Handle missing authorization appropriately, e.g., set status to false or return an error response
+ }
logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization :: " + authorization
+ " || method :: " + request.getMethod());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.
| String authorization = request.getHeader("Authorization"); | |
| logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization | |
| + " || method :: " + request.getMethod()); | |
| String authorization = request.getHeader("Authorization"); | |
| if (authorization == null || authorization.trim().isEmpty()) { | |
| logger.warn("Authorization header is missing or empty for request URI: " + request.getRequestURI()); | |
| // Handle missing authorization appropriately, e.g., set status to false or return an error response | |
| } | |
| logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization | |
| + " || method :: " + request.getMethod()); |
| logger.debug("In postHandle we are Intercepting the Request"); | ||
| String authorization = request.getHeader("Authorization"); | ||
| logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization); | ||
| if (authorization != null) { | ||
| // redisStorage.updateConcurrentSessionObject(redisStorage.getSessionObject(authorization)); | ||
| // redisStorage.updateSessionObject(authorization); | ||
| sessionObject.updateSessionObject(authorization, sessionObject.getSessionObject(authorization)); |
There was a problem hiding this comment.
Ensure Session Object Updates Handle Null Values Safely
The code updates the session object without checking if sessionObject.getSessionObject(authorization) returns null. If it does, this could lead to a NullPointerException.
Add a null check before updating the session:
if (authorization != null) {
+ SessionData sessionData = sessionObject.getSessionObject(authorization);
+ if (sessionData != null) {
sessionObject.updateSessionObject(authorization, sessionData);
+ } else {
+ logger.warn("Session data not found for authorization: " + authorization);
+ // Handle the missing session data as needed
+ }
}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.
| logger.debug("In postHandle we are Intercepting the Request"); | |
| String authorization = request.getHeader("Authorization"); | |
| logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization); | |
| if (authorization != null) { | |
| // redisStorage.updateConcurrentSessionObject(redisStorage.getSessionObject(authorization)); | |
| // redisStorage.updateSessionObject(authorization); | |
| sessionObject.updateSessionObject(authorization, sessionObject.getSessionObject(authorization)); | |
| logger.debug("In postHandle we are Intercepting the Request"); | |
| String authorization = request.getHeader("Authorization"); | |
| logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization); | |
| if (authorization != null) { | |
| SessionData sessionData = sessionObject.getSessionObject(authorization); | |
| if (sessionData != null) { | |
| sessionObject.updateSessionObject(authorization, sessionData); | |
| } else { | |
| logger.warn("Session data not found for authorization: " + authorization); | |
| // Handle the missing session data as needed | |
| } | |
| } |
| OutputResponse output = new OutputResponse(); | ||
| output.setError(e); | ||
| response.setContentType(MediaType.APPLICATION_JSON); | ||
|
|
||
| // response.setContentLength(e.getLocalizedMessage().length()); | ||
| response.setContentLength(output.toString().length()); | ||
| response.setHeader("Access-Control-Allow-Origin", "*"); | ||
| response.getOutputStream().print(output.toString()); | ||
|
|
There was a problem hiding this comment.
Handle Exceptions More Specifically and Securely
Catching the generic Exception class can obscure the root cause of errors and potentially expose sensitive information if exceptions are sent to clients. It's better to catch specific exceptions and avoid returning stack traces or internal messages in API responses.
Modify the exception handling to catch specific exceptions and provide a user-friendly error message:
- } catch (Exception e) {
+ } catch (SpecificExceptionType e) {
OutputResponse output = new OutputResponse();
- output.setError(e);
+ output.setError(new ErrorResponse("An unexpected error occurred. Please try again later."));
response.setContentType(MediaType.APPLICATION_JSON);
response.setContentLength(output.toString().length());
response.setHeader("Access-Control-Allow-Origin", "*");
response.getOutputStream().print(output.toString());
status = false;
}Replace SpecificExceptionType with the actual exceptions you expect (e.g., IOException, ValidationException). Ensure that detailed error information is logged internally but not exposed to the client.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (1)
src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (1)
43-58: Consider if the behavior of not overwriting existing non-empty values is desired for all use cases.The
setObjectmethod provides a way to set a value in Redis with an expiration time, but only if the key doesn't already have a non-empty value. This behavior might be intended to avoid overwriting existing non-empty values.However, it's unclear if this is the desired behavior for all use cases. Consider if there are scenarios where you might want to overwrite the value regardless of its current state.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/main/java/com/iemr/mmu/utils/config/ConfigProperties.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/http/HttpInterceptor.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/sessionobject/SessionObject.java (1 hunks)
- src/main/java/com/iemr/mmu/utils/validator/Validator.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/iemr/mmu/utils/config/ConfigProperties.java
- src/main/java/com/iemr/mmu/utils/validator/Validator.java
Additional comments not posted (12)
src/main/java/com/iemr/mmu/utils/sessionobject/SessionObject.java (5)
19-22: LGTM!The method is a simple setter for the
RedisStorageobject, and the implementation is correct.
24-28: LGTM!The constructor correctly initializes the
extendExpirationTimeandsessionExpiryTimevariables with values from theConfigPropertiesclass.
34-38: LGTM!The method correctly retrieves a session object from Redis using the
getObjectmethod of theRedisStorageobject, passing thesessionExpiryTimeas an argument.
40-43: LGTM!The method correctly sets a session object in Redis using the
setObjectmethod of theRedisStorageobject, passing thesessionExpiryTimeas an argument.
45-51: LGTM!The method correctly updates a session object in Redis using the
updateObjectmethod of theRedisStorageobject, passing theextendExpirationTimeandsessionExpiryTimeas arguments. It also calls theupdateConcurrentSessionObjectmethod to update a concurrent session object.src/main/java/com/iemr/mmu/utils/redis/RedisStorage.java (1)
60-75: LGTM!The updated
getObjectmethod looks good:
- It retrieves the value for a given key from Redis and extends its expiration time.
- It throws a specific
RedisSessionExceptionif the value is not found or is empty, providing clear error handling.- The exception type change from a generic
ExceptiontoRedisSessionExceptionis an improvement.src/main/java/com/iemr/mmu/utils/http/HttpInterceptor.java (6)
33-35: LGTM!The new imports are valid and necessary for the class.
61-64: LGTM!The updated logger statements using parameterized logging are a good practice.
70-88: LGTM!The new
switchcase block correctly handles the APIs that should be allowed without authorization.
106-109: LGTM!The response handling in the exception block is appropriate.
120-122: LGTM!The updated logger statements using parameterized logging are a good practice.
134-134: LGTM!The updated logger statement in the
afterCompletionmethod is acceptable.
| private void updateConcurrentSessionObject(String key, String value, Boolean extendExpirationTime, | ||
| Integer sessionExpiryTime) { | ||
| try { | ||
| JsonObject jsnOBJ = new JsonObject(); | ||
| JsonParser jsnParser = new JsonParser(); | ||
| JsonElement jsnElmnt = jsnParser.parse(value); | ||
| jsnOBJ = jsnElmnt.getAsJsonObject(); | ||
| if (jsnOBJ.has("userName") && jsnOBJ.get("userName") != null) { | ||
| objectStore.updateObject(jsnOBJ.get("userName").getAsString().trim().toLowerCase(), key, | ||
| extendExpirationTime, sessionExpiryTime); | ||
| } | ||
| } catch (Exception e) { | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle exceptions properly instead of swallowing them.
The method is catching and ignoring any exceptions that may occur. This is not a good practice, as it can make it difficult to diagnose and fix issues in the code. If an exception occurs, it will be silently ignored, and the concurrent session object may not be updated correctly.
Consider logging the exception or throwing a custom exception to handle it properly. For example:
- } catch (Exception e) {
- }
+ } catch (Exception e) {
+ log.error("Error updating concurrent session object", e);
+ throw new RedisSessionException("Error updating concurrent session object", e);
+ }Committable suggestion was skipped due to low confidence.
| public Long deleteObject(String key) throws RedisSessionException { | ||
| RedisConnection redCon = connection.getConnection(); | ||
| Long userRespFromRedis = Long.valueOf(0L); | ||
|
|
||
| userRespFromRedis = redCon.del(key.getBytes()); | ||
|
|
||
| return userRespFromRedis; | ||
| } |
There was a problem hiding this comment.
Remove or add the appropriate conditions for the declared RedisSessionException.
The deleteObject method provides the functionality to delete a Redis key and return the number of keys deleted, which looks good.
However, the method signature declares that it throws a RedisSessionException, but there is no explicit throw statement in the method body. This could confuse callers of the method.
Consider either removing the throws RedisSessionException from the method signature if it's not needed, or adding the appropriate conditions to throw the exception in the method body.
| public String updateObject(String key, String value, Boolean extendExpirationTime, int expirationTime) | ||
| throws RedisSessionException { | ||
| RedisConnection redCon = connection.getConnection(); | ||
|
|
||
|
|
||
| if ((sessionData != null) && sessionData.length > 0) { | ||
| logger.info("updating session time in redis for " + key); | ||
| redisCon.stringCommands().set(key.getBytes(), sessionData, Expiration.seconds(sessionExpiryTimeInSec), | ||
| SetOption.UPSERT); | ||
| byte[] sessionData = redCon.get(key.getBytes()); | ||
| String userRespFromRedis = null; | ||
| if (sessionData != null) { | ||
| userRespFromRedis = new String(redCon.get(key.getBytes())); | ||
| } | ||
|
|
||
| if ((userRespFromRedis != null) && (userRespFromRedis.trim().length() != 0)) { | ||
|
|
||
| redCon.set(key.getBytes(), value.getBytes(), Expiration.seconds(expirationTime), SetOption.UPSERT); | ||
| } else { | ||
| throw new Exception("Unable to fetch session object from Redis server"); | ||
| throw new RedisSessionException("Unable to fetch session object from Redis server"); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove or implement the functionality for the unused extendExpirationTime parameter.
The updateObject method provides the functionality to update the value and expiration time for a key in Redis, but only if the key already exists and has a non-empty value. It throws a RedisSessionException if the key doesn't exist or has an empty value, providing clear error handling. This looks good.
However, the extendExpirationTime parameter is unused in the method body. This could indicate an incomplete implementation or a redundant parameter.
Consider either removing the extendExpirationTime parameter if it's not needed, or implementing the intended functionality for it in the method body.
| String remoteAddress = request.getHeader("X-FORWARDED-FOR"); | ||
| if (remoteAddress == null || remoteAddress.trim().length() == 0) { | ||
| remoteAddress = request.getRemoteAddr(); | ||
| } | ||
| validator.checkKeyExists(authorization, remoteAddress); |
There was a problem hiding this comment.
Validate and sanitize the remote address.
As mentioned in the previous review, using the X-FORWARDED-FOR header directly can be insecure, as clients can spoof this header. Relying on this value without validation might lead to security vulnerabilities.
Consider implementing additional validation to ensure the remoteAddress is trustworthy before passing it to the validator.checkKeyExists method.
| OutputResponse output = new OutputResponse(); | ||
| output.setError(e); |
There was a problem hiding this comment.
Handle exceptions more securely.
Catching the generic Exception and setting it directly in the OutputResponse may expose sensitive information to clients.
Consider catching specific exceptions and providing a user-friendly error message instead:
} catch (SpecificExceptionType e) {
OutputResponse output = new OutputResponse();
output.setError(new ErrorResponse("An unexpected error occurred. Please try again later."));
// ...
}Replace SpecificExceptionType with the actual exceptions you expect (e.g., IOException, ValidationException). Ensure that detailed error information is logged internally but not exposed to the client.
| if (authorization != null) { | ||
| // redisStorage.updateConcurrentSessionObject(redisStorage.getSessionObject(authorization)); | ||
| // redisStorage.updateSessionObject(authorization); | ||
| sessionObject.updateSessionObject(authorization, sessionObject.getSessionObject(authorization)); |
There was a problem hiding this comment.
Ensure session object updates handle null values safely.
The code updates the session object without checking if sessionObject.getSessionObject(authorization) returns null. If it does, this could lead to a NullPointerException.
Add a null check before updating the session:
SessionData sessionData = sessionObject.getSessionObject(authorization);
if (sessionData != null) {
sessionObject.updateSessionObject(authorization, sessionData);
} else {
logger.warn("Session data not found for authorization: " + authorization);
// Handle the missing session data as needed
}| } | ||
| } catch (Exception e) { | ||
| logger.error("postHandle failed with error " + e.getMessage()); | ||
| logger.debug("postHandle failed with error " + e.getMessage()); |
There was a problem hiding this comment.
Log exceptions with appropriate severity and details.
Logging exceptions at the debug level may cause important exceptions to be overlooked, and valuable debugging information could be missed.
Update the logging to use the error level and include the stack trace:
logger.error("postHandle failed with error", e);This change ensures that significant errors are properly recorded and can be investigated if necessary.
|
2 participants
📋 Description
JIRA ID: AMM-985
download masters are not working
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
Release Notes
New Features
SessionObjectclass for improved session management using Redis.Validatorclass to enhance session validation and IP address checks during user logins.ConfigPropertiesclass for centralized management of application configuration settings.Improvements
HttpInterceptorclass for better session management.Refactor
RedisStorageclass for clarity and functionality.