Govt StateID and DistrictId changes #58
Conversation
WalkthroughThe pull request introduces enhancements to location-related data models and services, focusing on adding government-specific location identifiers (LGD IDs) for states and districts. The changes involve updating data classes ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTPRequestInterceptor
participant Validator
participant LocationService
participant Repository
Client->>HTTPRequestInterceptor: Send HTTP Request
HTTPRequestInterceptor->>Validator: Validate Request
Validator-->>HTTPRequestInterceptor: Validation Result
HTTPRequestInterceptor->>LocationService: Process Request
LocationService->>Repository: Fetch Location Data
Repository-->>LocationService: Return Data with LGD IDs
LocationService-->>Client: Return Location Details
Poem
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: 7
π§Ή Nitpick comments (3)
src/main/java/com/iemr/tm/repo/location/StateMasterRepo.java (1)
36-36: Improve query readability and type safetyThe current query returns Object[] which is not type-safe. Consider using column aliases and documenting the return structure.
Suggested improvements:
- @Query(" SELECT stateID, stateName,govtLGDStateID FROM States WHERE deleted != true ") + @Query("SELECT s.stateID as id, s.stateName as name, s.govtLGDStateID as lgdId " + + "FROM States s WHERE s.deleted != true")Also consider creating a DTO class to make the return type more type-safe:
public class StateDTO { private Integer id; private String name; private Integer lgdId; // Constructor matching query projection public StateDTO(Integer id, String name, Integer lgdId) { this.id = id; this.name = name; this.lgdId = lgdId; } // Getters }Then update the method signature:
- public ArrayList<Object[]> getStateMaster(); + public List<StateDTO> getStateMaster();src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java (1)
43-48: Consider using constructor injection instead of setter injection.Constructor injection is preferred for required dependencies as it:
- Ensures the dependency is available during object construction
- Promotes immutability
- Makes dependencies explicit
@Component public class HTTPRequestInterceptor implements HandlerInterceptor { - private Validator validator; + private final Validator validator; - @Autowired - public void setValidator(Validator validator) { - this.validator = validator; - } + @Autowired + public HTTPRequestInterceptor(Validator validator) { + this.validator = validator; + }src/main/java/com/iemr/tm/data/location/Districts.java (1)
143-148: Add parameter validation in the constructor.The constructor accepts Integer parameters but doesn't validate them. Consider adding validation to ensure the IDs are not null or negative.
public Districts(Integer districtID, String districtName, Integer govtLGDStateID, Integer govtLGDDistrictID) { + if (districtID == null || districtID < 0) { + throw new IllegalArgumentException("District ID cannot be null or negative"); + } + if (govtLGDStateID == null || govtLGDStateID < 0) { + throw new IllegalArgumentException("Government LGD State ID cannot be null or negative"); + } + if (govtLGDDistrictID == null || govtLGDDistrictID < 0) { + throw new IllegalArgumentException("Government LGD District ID cannot be null or negative"); + } this.districtID = districtID; this.districtName = districtName; this.govtLGDStateID = govtLGDStateID; this.govtLGDDistrictID = govtLGDDistrictID; }
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (6)
src/main/java/com/iemr/tm/data/location/Districts.java(2 hunks)src/main/java/com/iemr/tm/data/location/States.java(2 hunks)src/main/java/com/iemr/tm/repo/location/DistrictMasterRepo.java(1 hunks)src/main/java/com/iemr/tm/repo/location/StateMasterRepo.java(1 hunks)src/main/java/com/iemr/tm/service/location/LocationServiceImpl.java(2 hunks)src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java(2 hunks)
|
|
||
| @Column(name = "GovtStateID") | ||
| @Expose | ||
| private Integer govtLGDStateID; |
There was a problem hiding this comment.
Add getter/setter methods for govtLGDStateID field
The new field lacks accessor methods which are needed for proper encapsulation. Also consider aligning the column name with the field name for consistency.
Add the following methods:
+ public Integer getGovtLGDStateID() {
+ return this.govtLGDStateID;
+ }
+
+ public void setGovtLGDStateID(Integer govtLGDStateID) {
+ this.govtLGDStateID = govtLGDStateID;
+ }Consider renaming the column to match the field:
- @Column(name = "GovtStateID")
+ @Column(name = "GovtLGDStateID")π 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.
| @Column(name = "GovtStateID") | |
| @Expose | |
| private Integer govtLGDStateID; | |
| @Column(name = "GovtLGDStateID") | |
| @Expose | |
| private Integer govtLGDStateID; | |
| public Integer getGovtLGDStateID() { | |
| return this.govtLGDStateID; | |
| } | |
| public void setGovtLGDStateID(Integer govtLGDStateID) { | |
| this.govtLGDStateID = govtLGDStateID; | |
| } |
There was a problem hiding this comment.
@coderabbitai In DB its mentioned same
There was a problem hiding this comment.
@ravishanigarapu Understood, thank you for the clarification. Since the database column is named GovtStateID, it's appropriate to keep the annotation as @Column(name = "GovtStateID").
However, the field govtLGDStateID still lacks getter and setter methods. Consider adding them to ensure proper encapsulation and maintain consistency.
βοΈ Learnings added
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/data/location/States.java:46-49
Timestamp: 2024-12-16T11:43:56.194Z
Learning: In `src/main/java/com/iemr/tm/data/location/States.java`, the field `govtLGDStateID` corresponds to the database column `GovtStateID`, so the `@Column` annotation should be `@Column(name = "GovtStateID")`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| public States(int StateID, String StateName, Integer govtLGDStateID) { | ||
| this.stateID = StateID; | ||
| this.stateName = StateName; | ||
| this.govtLGDStateID = govtLGDStateID; | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Add input validation and deprecate old constructor
The new constructor should validate the govtLGDStateID parameter. Also, the old constructor should be marked as deprecated since it doesn't initialize all fields.
Apply these changes:
+ @Deprecated(since = "AMM-1166", forRemoval = false)
public States(int StateID, String StateName) {
this.stateID = StateID;
this.stateName = StateName;
}
public States(int StateID, String StateName, Integer govtLGDStateID) {
this.stateID = StateID;
this.stateName = StateName;
+ if (govtLGDStateID != null && govtLGDStateID <= 0) {
+ throw new IllegalArgumentException("govtLGDStateID must be positive when provided");
+ }
this.govtLGDStateID = govtLGDStateID;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@coderabbitai it is used for other methods its not deprecated
There was a problem hiding this comment.
@ravishanigarapu, understood. Thank you for the clarification that the old constructor is used in other methods and should not be deprecated.
οΌ γ»Ογ»οΌ?
βοΈ Learnings added
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/data/location/States.java:163-167
Timestamp: 2024-12-16T11:44:54.815Z
Learning: The constructor 'public States(int StateID, String StateName)' in 'src/main/java/com/iemr/tm/data/location/States.java' is used in other methods and should not be deprecated.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
π§ Learnings used
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/data/location/States.java:46-49
Timestamp: 2024-12-16T11:43:56.377Z
Learning: In `src/main/java/com/iemr/tm/data/location/States.java`, the field `govtLGDStateID` corresponds to the database column `GovtStateID`, so the `@Column` annotation should be `@Column(name = "GovtStateID")`.
| @@ -34,7 +34,7 @@ | |||
| @Repository | |||
|
|
|||
| public interface DistrictMasterRepo extends CrudRepository<Districts, Integer> { | |||
| @Query(" SELECT d.districtID, d.districtName FROM Districts d " + " WHERE d.stateID = :stateID AND d.deleted != true ") | |||
| @Query(" SELECT d.districtID, d.districtName,d.govtLGDStateID,d.govtLGDDistrictID FROM Districts d " + " WHERE d.stateID = :stateID AND d.deleted != true ") | |||
There was a problem hiding this comment.
π οΈ Refactor suggestion
Improve query readability, type safety, and input validation
Similar to StateMasterRepo, this query would benefit from better type safety and readability improvements. Also add parameter validation.
Suggested improvements:
+ @NotNull(message = "stateID cannot be null")
+ @Positive(message = "stateID must be positive")
@Param("stateID") Integer stateID
- @Query(" SELECT d.districtID, d.districtName,d.govtLGDStateID,d.govtLGDDistrictID FROM Districts d " + " WHERE d.stateID = :stateID AND d.deleted != true ")
+ @Query("SELECT d.districtID as id, " +
+ " d.districtName as name, " +
+ " d.govtLGDStateID as stateLgdId, " +
+ " d.govtLGDDistrictID as districtLgdId " +
+ "FROM Districts d " +
+ "WHERE d.stateID = :stateID " +
+ "AND d.deleted != true")Create a DTO for type-safe returns:
public class DistrictDTO {
private Integer id;
private String name;
private Integer stateLgdId;
private Integer districtLgdId;
// Constructor matching query projection
public DistrictDTO(Integer id, String name,
Integer stateLgdId, Integer districtLgdId) {
this.id = id;
this.name = name;
this.stateLgdId = stateLgdId;
this.districtLgdId = districtLgdId;
}
// Getters
}Update method signature:
- public ArrayList<Object[]> getDistrictMaster(@Param("stateID") Integer stateID);
+ public List<DistrictDTO> getDistrictMaster(@Param("stateID") Integer stateID);There was a problem hiding this comment.
@coderabbitai not required
There was a problem hiding this comment.
@ravishanigarapu, understood.
| 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.
Add null check for validator before use.
The validator is used without checking if it's null, which could lead to a NullPointerException.
- validator.checkKeyExists(authorization, remoteAddress);
+ if (validator == null) {
+ throw new IllegalStateException("Validator is not initialized");
+ }
+ validator.checkKeyExists(authorization, remoteAddress);π 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 remoteAddress = request.getHeader("X-FORWARDED-FOR"); | |
| if (remoteAddress == null || remoteAddress.trim().length() == 0) { | |
| remoteAddress = request.getRemoteAddr(); | |
| } | |
| validator.checkKeyExists(authorization, remoteAddress); | |
| String remoteAddress = request.getHeader("X-FORWARDED-FOR"); | |
| if (remoteAddress == null || remoteAddress.trim().length() == 0) { | |
| remoteAddress = request.getRemoteAddr(); | |
| } | |
| if (validator == null) { | |
| throw new IllegalStateException("Validator is not initialized"); | |
| } | |
| validator.checkKeyExists(authorization, remoteAddress); |
There was a problem hiding this comment.
@coderabbitai its mandatory thing
There was a problem hiding this comment.
@ravishanigarapu, understood. If the validator is mandatory and always initialized, then a null check is not necessary.
(οΌοΈΏοΌ)
βοΈ Learnings added
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java:93-97
Timestamp: 2024-12-16T11:46:15.486Z
Learning: In the `HTTPRequestInterceptor` class (`src/main/java/com/iemr/tm/utils/http/HTTPRequestInterceptor.java`), the `validator` is mandatory and always initialized, so null checks are unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @Column(name = "GovtStateID") | ||
| @Expose | ||
| private Integer govtLGDStateID; | ||
|
|
||
| @Column(name = "GovtDistrictID") | ||
| @Expose | ||
| private Integer govtLGDDistrictID; |
There was a problem hiding this comment.
Add getter/setter methods for new LGD ID fields.
The new government LGD ID fields are missing their accessor methods, which are needed for proper encapsulation and data access.
+ public Integer getGovtLGDStateID() {
+ return govtLGDStateID;
+ }
+
+ public void setGovtLGDStateID(Integer govtLGDStateID) {
+ this.govtLGDStateID = govtLGDStateID;
+ }
+
+ public Integer getGovtLGDDistrictID() {
+ return govtLGDDistrictID;
+ }
+
+ public void setGovtLGDDistrictID(Integer govtLGDDistrictID) {
+ this.govtLGDDistrictID = govtLGDDistrictID;
+ }π 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.
| @Column(name = "GovtStateID") | |
| @Expose | |
| private Integer govtLGDStateID; | |
| @Column(name = "GovtDistrictID") | |
| @Expose | |
| private Integer govtLGDDistrictID; | |
| @Column(name = "GovtStateID") | |
| @Expose | |
| private Integer govtLGDStateID; | |
| @Column(name = "GovtDistrictID") | |
| @Expose | |
| private Integer govtLGDDistrictID; | |
| public Integer getGovtLGDStateID() { | |
| return govtLGDStateID; | |
| } | |
| public void setGovtLGDStateID(Integer govtLGDStateID) { | |
| this.govtLGDStateID = govtLGDStateID; | |
| } | |
| public Integer getGovtLGDDistrictID() { | |
| return govtLGDDistrictID; | |
| } | |
| public void setGovtLGDDistrictID(Integer govtLGDDistrictID) { | |
| this.govtLGDDistrictID = govtLGDDistrictID; | |
| } |
There was a problem hiding this comment.
@coderabbitai using constaructor not required
There was a problem hiding this comment.
@ravishanigarapu Understood. If you're using the constructor and don't require getter and setter methods for these fields, that's acceptable. Thanks for clarifying.
| Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2], | ||
| (Integer) objArr[3]); |
There was a problem hiding this comment.
Add array bounds and null checking before accessing elements.
The code assumes the objArr array has at least 4 elements and that none are null. This could lead to ArrayIndexOutOfBoundsException or NullPointerException.
- Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2],
- (Integer) objArr[3]);
+ if (objArr.length < 4) {
+ throw new IllegalStateException("Insufficient data for district creation");
+ }
+ Integer districtId = (Integer) objArr[0];
+ String districtName = (String) objArr[1];
+ Integer govtLGDStateId = (Integer) objArr[2];
+ Integer govtLGDDistrictId = (Integer) objArr[3];
+ if (districtId == null || districtName == null || govtLGDStateId == null || govtLGDDistrictId == null) {
+ throw new IllegalStateException("Null values found in district data");
+ }
+ Districts districtMaster = new Districts(districtId, districtName, govtLGDStateId, govtLGDDistrictId);π 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.
| Districts districtMaster = new Districts((Integer) objArr[0], (String) objArr[1], (Integer) objArr[2], | |
| (Integer) objArr[3]); | |
| if (objArr.length < 4) { | |
| throw new IllegalStateException("Insufficient data for district creation"); | |
| } | |
| Integer districtId = (Integer) objArr[0]; | |
| String districtName = (String) objArr[1]; | |
| Integer govtLGDStateId = (Integer) objArr[2]; | |
| Integer govtLGDDistrictId = (Integer) objArr[3]; | |
| if (districtId == null || districtName == null || govtLGDStateId == null || govtLGDDistrictId == null) { | |
| throw new IllegalStateException("Null values found in district data"); | |
| } | |
| Districts districtMaster = new Districts(districtId, districtName, govtLGDStateId, govtLGDDistrictId); |
There was a problem hiding this comment.
@coderabbitai its mandatory to get those fields
There was a problem hiding this comment.
@ravishanigarapu Understood, thank you for the clarification.
(_/)
( ._. )
/
βοΈ Learnings added
Learnt from: ravishanigarapu
PR: PSMRI/TM-API#58
File: src/main/java/com/iemr/tm/service/location/LocationServiceImpl.java:180-181
Timestamp: 2024-12-16T11:47:37.331Z
Learning: In `LocationServiceImpl.java`, within the `getDistrictList` method, it is guaranteed that `districtMasterList` will always contain arrays with at least four non-null elements (`objArr[0]` to `objArr[3]`), and it is mandatory to get those fields. Therefore, array bounds and null checks are not necessary in this context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
π Description
JIRA ID: AMM-1166
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
β 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
New Features
Bug Fixes
Chores