Abdm M1 V3 Changes #52
Conversation
WalkthroughThe pull request introduces multiple changes across several configuration and Java files, primarily focusing on enhancements to the Ayushman Bharat Health Account (ABHA) system. Key updates include the addition of new API endpoints in various properties files, the introduction of REST controllers for handling ABHA operations, and the implementation of services for managing authentication and enrollment processes. Additionally, several new data classes are created to encapsulate request and response data. The changes also involve modifications to existing classes, such as updating identifier types for MongoDB compatibility. Changes
Possibly related PRs
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: 29
π§Ή Outside diff range and nitpick comments (23)
src/main/java/com/wipro/fhir/data/v3/abhaCard/EnrollByAadhaar.java (2)
3-4: Remove unused importsThe
ArrayListandListimports are not being used in this class.-import java.util.ArrayList; -import java.util.List;
1-14: Consider implementing serialization for data transferSince this class is likely used for API requests/responses, consider implementing
Serializablefor proper serialization support.@Data -public class EnrollByAadhaar { +public class EnrollByAadhaar implements Serializable { + private static final long serialVersionUID = 1L;Don't forget to add the import:
import java.io.Serializable;src/main/java/com/wipro/fhir/data/v3/abhaCard/ConsentRequest.java (3)
5-6: Add class-level documentationPlease add Javadoc comments to describe the purpose of this class, its role in the ABHA consent management process, and any specific requirements or constraints.
@Data +/** + * Represents a consent request in the ABHA (Ayushman Bharat Health Account) system. + * This class encapsulates the consent details required during the enrollment process. + */ public class ConsentRequest {
8-9: Add field documentation and consider adding validationThe fields lack documentation explaining their purpose and any constraints. Additionally, consider adding validation to ensure data integrity.
+ /** + * The consent request code identifier. + * Must not be null or empty. + */ + @NotBlank(message = "Consent code is required") private String code; + /** + * Version of the consent request. + * Must follow semantic versioning format (e.g., "1.0.0"). + */ + @Pattern(regexp = "^\\d+\\.\\d+\\.\\d+$", message = "Version must follow semantic versioning format") private String version;Don't forget to add the necessary import:
import javax.validation.constraints.NotBlank; import javax.validation.constraints.Pattern;
6-10: Consider implementing builder pattern for object creationSince this class represents a request object that might grow with additional fields in the future, consider using the builder pattern for more flexible object creation.
Replace
@Datawith more specific Lombok annotations:-@Data +@Getter +@Builder +@NoArgsConstructor +@AllArgsConstructor public class ConsentRequest {Don't forget to add the necessary imports:
import lombok.Getter; import lombok.Builder; import lombok.NoArgsConstructor; import lombok.AllArgsConstructor;src/main/java/com/wipro/fhir/data/v3/abhaCard/OtpRequest.java (1)
3-3: Remove unused import.The
java.sql.Timestampimport is not being used in this class.-import java.sql.Timestamp;src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java (1)
70-82: Extract Header Preparation Logic to a Utility MethodThe code for preparing HTTP headers in both
requestOtpForAbhaLoginandverifyAbhaLoginmethods is duplicated. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this logic into a separate private method.Suggested refactor:
+ private MultiValueMap<String, String> prepareHeaders(String ndhmAuthToken) { + MultiValueMap<String, String> headers = new LinkedMultiValueMap<>(); + headers.add("Content-Type", MediaType.APPLICATION_JSON + ";charset=utf-8"); + headers.add("REQUEST-ID", UUID.randomUUID().toString()); + TimeZone tz = TimeZone.getTimeZone("UTC"); + DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); + df.setTimeZone(tz); + String nowAsISO = df.format(new Date()); + headers.add("TIMESTAMP", nowAsISO); + headers.add("Authorization", ndhmAuthToken); + return headers; + }Then replace the header preparation code in both methods:
- MultiValueMap<String, String> headers = new LinkedMultiValueMap<>(); - headers.add("Content-Type", MediaType.APPLICATION_JSON + ";charset=utf-8"); - headers.add("REQUEST-ID", UUID.randomUUID().toString()); - // ... Other header preparations ... - headers.add("Authorization", ndhmAuthToken); + MultiValueMap<String, String> headers = prepareHeaders(ndhmAuthToken);Also applies to: 145-155
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
85-97: Extract Common Header Preparation LogicThe HTTP header preparation code is repeated across multiple methods (
getOtpForEnrollment,enrollmentByAadhaar,verifyAuthByMobile,getAbhaCardPrinted). To enhance maintainability and adhere to the DRY principle, consider extracting this logic into a shared private method.Suggested refactor:
+ private MultiValueMap<String, String> prepareHeaders(String ndhmAuthToken) { + MultiValueMap<String, String> headers = new LinkedMultiValueMap<>(); + headers.add("Content-Type", MediaType.APPLICATION_JSON + ";charset=utf-8"); + headers.add("REQUEST-ID", UUID.randomUUID().toString()); + TimeZone tz = TimeZone.getTimeZone("UTC"); + DateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"); + df.setTimeZone(tz); + String nowAsISO = df.format(new Date()); + headers.add("TIMESTAMP", nowAsISO); + headers.add("Authorization", ndhmAuthToken); + return headers; + }Then replace the header preparation code in each method with a call to
prepareHeaders(ndhmAuthToken).Also applies to: 148-159, 240-251, 371-382
src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyService.java (1)
7-7: Clarify Exception Handling in Method SignatureThe method
getCertPublicKeydeclares that it throwsFHIRException. Ensure that implementing classes handle this exception appropriately and consider whether a more specific exception type would be beneficial.Consider defining a custom exception or documenting the circumstances under which
FHIRExceptionis thrown for clarity.src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyServiceImpl.java (1)
1-41: Add unit tests for the certificate serviceThe service lacks unit tests to verify the key retrieval, caching, and error handling logic.
Would you like me to help generate comprehensive unit tests for this service?
src/main/java/com/wipro/fhir/utils/Encryption.java (2)
26-27: Remove unused field.The
ndhmUserAuthenticatefield is injected but never used in the class.-@Value("${ndhmuserAuthenticate}") -private String ndhmUserAuthenticate;
31-48: Refactor to improve code maintainability.Consider extracting algorithm constants and adding proper exception handling with meaningful messages.
+private static final String RSA_ALGORITHM = "RSA"; +private static final String CIPHER_TRANSFORMATION = "RSA/ECB/OAEPWITHSHA-256ANDMGF1PADDING"; public String encrypt(String text, String publicKeyString) throws FHIRException { try { String encryptedTextBase64 = ""; byte[] publicKeyBytes = Base64.getDecoder().decode(publicKeyString); - KeyFactory keyFactory = KeyFactory.getInstance("RSA"); + KeyFactory keyFactory = KeyFactory.getInstance(RSA_ALGORITHM); X509EncodedKeySpec publicKeySpec = new X509EncodedKeySpec(publicKeyBytes); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); - Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWithSHA-1AndMGF1Padding"); + Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION); cipher.init(Cipher.ENCRYPT_MODE, publicKey); byte[] encryptedText = cipher.doFinal(text.getBytes()); encryptedTextBase64 = Base64.getEncoder().encodeToString(encryptedText); return encryptedTextBase64; + } catch (NoSuchAlgorithmException | InvalidKeySpecException | NoSuchPaddingException | + InvalidKeyException | IllegalBlockSizeException | BadPaddingException e) { + throw new FHIRException("Encryption failed: " + e.getMessage(), e); } }src/main/java/com/wipro/fhir/data/mongo/care_context/PatientCareContexts.java (2)
Line range hint
77-79: Remove commented code.Remove the commented out code block as it's no longer needed and can cause confusion.
-// @Expose -// @Field(value = "careContexts") -// private String careContextsList;
Line range hint
41-85: Consider adding validation constraints.The class could benefit from validation annotations to ensure data integrity, especially for required fields like identifier and healthId.
+import javax.validation.constraints.NotBlank; +import javax.validation.constraints.NotNull; @Field(value = "_id") private ObjectId _id; @Expose @Field(value = "identifier") +@NotBlank(message = "identifier is required") private String identifier; @Expose @Field(value = "HealthId") +@NotBlank(message = "HealthId is required") private String HealthId;src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java (2)
28-28: Remove redundant @crossorigin annotations.The
@CrossOriginannotation is already present at the class level, making method-level annotations redundant.-@CrossOrigin @Operation(summary = "verify OTP for Abha LOgin") @PostMapping(value = { "/verifyAbhaLogin" })Also applies to: 48-50
29-29: Fix typos in Operation summaries.The word "LOgin" is misspelled in the operation summaries.
-@Operation(summary = "Request OTP for Abha LOgin") +@Operation(summary = "Request OTP for ABHA Login") -@Operation(summary = "verify OTP for Abha LOgin") +@Operation(summary = "Verify OTP for ABHA Login")Also applies to: 49-49
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java (2)
55-71: Improve consistency in variable declaration and error messages.
- Unnecessary
resvariable declaration- Inconsistent log message (mentions "ABHA card" instead of enrollment)
public String abhaEnrollmentByAadhaar(@RequestBody String request) { logger.info("ABHA enrollment BY Aadhaar API request " + request); OutputResponse response = new OutputResponse(); - String res = null; try { if (request != null) { - res = createAbhaV3Service.enrollmentByAadhaar(request); - response.setResponse(res); + String result = createAbhaV3Service.enrollmentByAadhaar(request); + response.setResponse(result); } else throw new FHIRException("NDHM_FHIR Empty request object"); } catch (FHIRException e) { response.setError(5000, e.getMessage()); logger.error(e.toString()); } - logger.info("NDHM_FHIR generate OTP for ABHA card API response " + response.toString()); + logger.info("NDHM_FHIR ABHA enrollment by Aadhaar API response " + response.toString()); return response.toString(); }
96-110: Add request logging for printAbhaCard endpoint.The
printAbhaCardmethod is missing initial request logging, unlike other endpoints.public String printAbhaCard(@RequestBody String request) { + logger.info("Print ABHA card API request received"); OutputResponse response = new OutputResponse(); try { if (request != null) { String s = createAbhaV3Service.getAbhaCardPrinted(request); response.setResponse(s); } else throw new FHIRException("NDHM_FHIR Empty request object"); } catch (FHIRException e) { response.setError(5000, e.getMessage()); logger.error(e.toString()); } - logger.info("NDHM_FHIR generate OTP for ABHA card API respponse " + response.toString()); + logger.info("NDHM_FHIR print ABHA card API response " + response.toString()); return response.toString(); }src/main/environment/common_ci.properties (1)
87-93: Add documentation for new ABDM V3 APIs.Consider adding comments to describe the purpose and expected response format for each new API endpoint.
##ABDM V3 APIs +# Returns the public key certificate for authentication getAuthCertPublicKey = @env.ABDM_HEALTH_ID_BASE_URL@/api/v1/auth/cert +# Initiates the ABHA enrollment process by requesting OTP requestOtpForEnrollment = @env.ABDM_BASE_URL@/abha/api/v3/enrollment/request/otp +# Enrolls a user using their Aadhaar details abhaEnrollByAadhaar = @env.ABDM_BASE_URL@/abha/api/v3/enrollment/enrol/byAadhaar +# Generates the ABHA card in the specified format printAbhaCard = @env.ABDM_BASE_URL@/abha/api/v3/profile/account/abha-card +# Initiates the ABHA login process by requesting OTP abhaLoginRequestOtp = @env.ABDM_BASE_URL@/abha/api/v3/profile/login/request/otp +# Verifies the OTP for ABHA login verifyAbhaLogin = @env.ABDM_BASE_URL@/abha/api/v3/profile/login/verifysrc/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java (4)
Line range hint
338-350: Consider refactoring the care context data population logic.The code could be more maintainable by extracting the care context data population into a separate method.
-Object[] resData = null; -if (res.get(0) != null) { - resData = res.get(0); - cc.setAbdmFacilityId(resData[0] != null ? resData[0].toString() : null ); - cc.setCareContextLinkedDate(resData[1] != null ? resData[1].toString() : null); -} +private void populateCareContextData(CareContexts cc, List<Object[]> res) { + if (!res.isEmpty() && res.get(0) != null) { + Object[] resData = res.get(0); + cc.setAbdmFacilityId(resData[0] != null ? resData[0].toString() : null); + cc.setCareContextLinkedDate(resData[1] != null ? resData[1].toString() : null); + } +}
Line range hint
350-361: Enhance error handling with specific error messages.The error handling could be improved by providing more specific error messages when operations fail.
-PatientCareContexts resultSet = null; +PatientCareContexts resultSet = null; +String errorContext = ""; if (pDemo.getBeneficiaryID() != null) { pcc = patientCareContextsMongoRepo.findByIdentifier(pDemo.getBeneficiaryID().toString()); + errorContext = "beneficiaryID: " + pDemo.getBeneficiaryID(); if (pcc != null && pcc.getIdentifier() != null) { ccList = pcc.getCareContextsList(); ccList.add(cc); pcc.setCareContextsList(ccList); resultSet = patientCareContextsMongoRepo.save(pcc); + if (resultSet == null) { + logger.error("Failed to save care context for " + errorContext); + }
Line range hint
599-601: Consider moving configuration validation to startup.The abhaMode validation should be performed during application startup to fail fast if the configuration is invalid.
@PostConstruct private void validateConfiguration() { if (abhaMode == null || !(abhaMode.equalsIgnoreCase("abdm") || abhaMode.equalsIgnoreCase("sbx"))) { logger.warn("Invalid abhaMode configuration. Defaulting to 'sbx'"); abhaMode = "sbx"; } }
Line range hint
1-650: Improve documentation for deprecated methods.Several methods are marked as @deprecated but lack documentation about:
- Why they are deprecated
- What alternatives should be used
- When they will be removed
Consider adding this information to the JavaDoc comments.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
π Files selected for processing (26)
src/main/environment/common_ci.properties(1 hunks)src/main/environment/common_dev.properties(1 hunks)src/main/environment/common_example.properties(1 hunks)src/main/environment/common_test.properties(1 hunks)src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java(1 hunks)src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java(1 hunks)src/main/java/com/wipro/fhir/data/mongo/care_context/PatientCareContexts.java(2 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/BioRequest.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/ConsentRequest.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/EnrollByAadhaar.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/LoginAbhaRequest.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/LoginMethod.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/OtpRequest.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/RequestOTPEnrollment.java(1 hunks)src/main/java/com/wipro/fhir/data/v3/abhaCard/VerifyAbhaLogin.java(1 hunks)src/main/java/com/wipro/fhir/repo/common/AllergyIntoleranceDataModelRepo.java(2 hunks)src/main/java/com/wipro/fhir/repo/healthID/HealthIDRepo.java(1 hunks)src/main/java/com/wipro/fhir/repo/mongo/amrit_resource/PatientCareContextsMongoRepo.java(2 hunks)src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java(4 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyService.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CertificateKeyServiceImpl.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3Service.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3Service.java(1 hunks)src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java(1 hunks)src/main/java/com/wipro/fhir/utils/Encryption.java(1 hunks)
β Files skipped from review due to trivial changes (3)
- src/main/java/com/wipro/fhir/data/v3/abhaCard/LoginAbhaRequest.java
- src/main/java/com/wipro/fhir/data/v3/abhaCard/LoginMethod.java
- src/main/java/com/wipro/fhir/data/v3/abhaCard/RequestOTPEnrollment.java
π Additional comments (12)
src/main/java/com/wipro/fhir/data/v3/abhaCard/EnrollByAadhaar.java (2)
11-11: Consider type safety for authData map
The Map<String, Object> type for authData is very generic and could lead to runtime errors. Consider creating a specific type for authentication data to ensure type safety at compile time.
Let's check how authData is used across the codebase:
1-14: Add missing ConsentRequest import and documentation
- The
ConsentRequestclass is used but not imported - Consider adding class-level documentation to explain the purpose of this DTO
package com.wipro.fhir.data.v3.abhaCard;
import java.util.Map;
+import com.wipro.fhir.data.v3.abhaCard.ConsentRequest;
import lombok.Data;
+/**
+ * Data Transfer Object (DTO) for handling Aadhaar-based ABHA enrollment requests.
+ * This class encapsulates the authentication data and consent information required
+ * for enrolling a user using their Aadhaar details.
+ */
@Data
public class EnrollByAadhaar {Let's verify the correct package for ConsentRequest:
src/main/java/com/wipro/fhir/data/v3/abhaCard/ConsentRequest.java (1)
1-10: Verify usage in enrollment flow
The class seems to be part of the ABHA enrollment process. Let's verify its integration with related components.
β Verification successful
Based on the shell script results, I can now generate the final response as the usage is clear from the output.
ConsentRequest is properly integrated in the enrollment flow
The class is correctly used in the ABHA enrollment process:
- Used in
CreateAbhaV3ServiceImpl.javafor both Aadhaar and biometric enrollment flows - Properly integrated with
EnrollByAadhaarclass as a consent field - Consistently initialized with code "abha-enrollment" and version "1.4"
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ConsentRequest is used in the enrollment flow
# Find related enrollment classes and their usage of ConsentRequest
echo "Looking for enrollment-related classes..."
rg -l "EnrollByAadhaar|EnrollmentService" src/
echo "Checking ConsentRequest usage..."
rg -A 5 "ConsentRequest" src/
echo "Checking for similar consent-related classes..."
fd -e java -E test --exec grep -l "Consent" {} \; src/
Length of output: 3823
src/main/java/com/wipro/fhir/service/v3/abha/CreateAbhaV3ServiceImpl.java (1)
317-320: Update Consent Version to Latest
The consent version is set to "1.4". Ensure that this version is up-to-date with the current API specifications to avoid compatibility issues.
Please confirm whether version "1.4" is the latest consent version required by the API.
Also applies to: 351-354
src/main/java/com/wipro/fhir/repo/common/AllergyIntoleranceDataModelRepo.java (1)
34-34: Verify the impact of changing ID type from Long to BigInteger
The change in ID type could affect existing data and queries. Please ensure:
- Existing data can be properly mapped to BigInteger
- All service layer code handling IDs is updated
- Database schema changes (if any) are properly handled
β Verification successful
ID type change appears to be consistent with the entity model
The entity class AllergyIntoleranceDataModel already uses BigInteger for its ID field and other numeric identifiers. The constructor explicitly converts long values to BigInteger using BigInteger.valueOf((long) objArr[0]). Additionally, the entity class is currently not mapped to a database table (JPA annotations are commented out), suggesting this is a data transfer model.
- The ID field is already defined as
private BigInteger id - The constructor properly handles conversion from long to BigInteger
- No active database mapping is present (all JPA annotations are commented)
- The model is used primarily for data transformation as seen in
getAllergyListmethod
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any Long type usage in relation to AllergyIntoleranceDataModel
rg -l "AllergyIntoleranceDataModel.*Long|Long.*AllergyIntoleranceDataModel" --type java
# Search for potential ID handling in services
rg -l "AllergyIntoleranceDataModelRepo.*getId|AllergyIntoleranceDataModel.*getId" --type java
Length of output: 185
Script:
#!/bin/bash
# Search for the previous version of the file to see the ID type change
git log -p -- src/main/java/com/wipro/fhir/repo/common/AllergyIntoleranceDataModelRepo.java
# Search for any references to AllergyIntoleranceDataModel
rg "AllergyIntoleranceDataModel" --type java -A 2
# Search for the entity class to verify ID type
fd -e java AllergyIntoleranceDataModel
Length of output: 5046
Script:
#!/bin/bash
# Check the entity class for ID field type
cat src/main/java/com/wipro/fhir/data/resource_model/AllergyIntoleranceDataModel.java | grep -A 5 "class AllergyIntoleranceDataModel"
# Check for ID field and its annotations
ast-grep --pattern 'class AllergyIntoleranceDataModel {
$$$
private $_ id;
$$$
}'
Length of output: 371
Script:
#!/bin/bash
# Check the full content of the entity class to see ID field and its usage
cat src/main/java/com/wipro/fhir/data/resource_model/AllergyIntoleranceDataModel.java
# Look for any database migration scripts
fd -e sql
Length of output: 2933
src/main/java/com/wipro/fhir/repo/mongo/amrit_resource/PatientCareContextsMongoRepo.java (1)
33-33: LGTM! Type change improves type safety.
The change from String to ObjectId for the ID type parameter aligns with MongoDB best practices and provides better type safety.
Let's verify the impact on existing data:
β Verification successful
LGTM! The ID type change is safe and properly aligned.
The change from String to ObjectId for the ID type parameter in PatientCareContextsMongoRepo is safe because:
- The entity class
PatientCareContextsusesObjectIdfor its@Idfield - The repository's primary operations use the identifier field (String) for lookups, not the MongoDB _id
- No direct _id-based queries are used in the service layer
- The change aligns with MongoDB best practices for the _id field
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other repositories or services that might be affected by this ID type change
rg -l "PatientCareContexts.*id|id.*PatientCareContexts" --type java
Length of output: 69
Script:
#!/bin/bash
# Let's try a broader search to find files containing PatientCareContexts
rg -l "PatientCareContexts" --type java
# Also search for the entity class to understand the ID type change
fd PatientCareContexts.java
Length of output: 468
Script:
#!/bin/bash
# Let's check the entity class and its usage
cat src/main/java/com/wipro/fhir/data/mongo/care_context/PatientCareContexts.java
# Check the service implementation for ID usage
cat src/main/java/com/wipro/fhir/service/common/CommonServiceImpl.java
# Check the string object class that might be related
cat src/main/java/com/wipro/fhir/data/mongo/care_context/PatientCareContextsStringOBJ.java
Length of output: 27089
src/main/java/com/wipro/fhir/data/mongo/care_context/PatientCareContexts.java (1)
41-42: LGTM! Field type change aligns with MongoDB conventions.
The change from id to _id with ObjectId type follows MongoDB best practices and matches the repository changes.
src/main/environment/common_ci.properties (1)
85-93: Verify API version consistency.
The new endpoints use v3 while some existing endpoints use v0.5, v1, and v2. Verify if these different versions are intentional and if any endpoints should be migrated to v3.
β Verification successful
Let me generate additional verification scripts to understand the API endpoints better.
API version differences are intentional and correct
The version differences are intentional and reflect different API domains:
- v0.5: Gateway APIs for user sessions and care context
- v1: Core health ID registration, authentication, and facility services
- v2: Single endpoint for Aadhaar mobile OTP verification
- v3: New ABHA (Ayushman Bharat Health Account) specific endpoints
The v3 endpoints are part of a new ABHA-specific API group and don't overlap with the functionality of other versioned endpoints. Each version serves distinct purposes and there's no immediate need for migration.
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for different API versions in use
echo "API versions in use:"
rg -o 'v[0-9]+(\.[0-9]+)?' | sort | uniq -c
Length of output: 3854
Script:
#!/bin/bash
# Check the specific API endpoints and their versions in properties files
echo "API endpoints in properties files:"
rg "^[^#].*=(.*)/v[0-9]+(\.[0-9]+)?/" src/main/environment/common_ci.properties
echo -e "\nGrouping endpoints by version:"
rg "^[^#].*=(.*)/v[0-9]+(\.[0-9]+)?/" src/main/environment/common_ci.properties |
grep -o '/v[0-9]\+\.[0-9]\+\|/v[0-9]\+' |
sort |
uniq -c
Length of output: 2954
src/main/environment/common_example.properties (2)
87-94:
Review security implications of using sandbox URLs
The configuration uses sandbox URLs (healthidsbx and abhasbx) across all environments. This might pose security risks if accidentally used in production.
Let's verify the URL usage across the codebase:
Recommendations:
- Add clear environment indicators in property files
- Implement environment validation checks
- Document the process for switching between sandbox and production URLs
87-94: Verify API version compatibility and migration strategy
The codebase now includes a mix of ABDM API versions (v1, v2, and v3). This could lead to maintenance challenges and potential compatibility issues.
Let's analyze the API version usage across the codebase:
Consider:
- Documenting the migration strategy from v1/v2 to v3
- Creating a timeline for deprecating older versions
- Implementing version-specific service interfaces to manage the transition
β Verification successful
API version usage appears to be intentionally segregated
Based on the analysis of the API endpoints across the codebase, the mixing of API versions appears to be an intentional design choice where:
- V3 APIs (
/api/v3/) are exclusively used for new ABHA-specific operations (enrollment, login, card printing) - V1 APIs (
/api/v1/) handle legacy health ID operations and authentication - Only one V2 endpoint exists for a specific mobile OTP check functionality
This separation suggests a planned coexistence rather than a maintenance issue, with V3 APIs being introduced for new ABHA-specific features while maintaining backward compatibility for existing functionalities.
π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ABDM API version patterns in the codebase
echo "Analyzing ABDM API version usage..."
rg -i "abdm.*api/v[0-9]" --type java
# Search for ABDM service implementations
echo "Checking service implementations..."
ast-grep --pattern 'class $_ implements $_AbhaV3Service {
$$$
}'
Length of output: 13021
src/main/environment/common_dev.properties (1)
87-93: Reference previous comments
The same issues and recommendations from common_example.properties apply to this file.
src/main/environment/common_test.properties (1)
87-94: Reference previous comments
The same issues and recommendations from common_example.properties apply to this file.
| public class EnrollByAadhaar{ | ||
| public Map<String, Object> authData; | ||
| public ConsentRequest consent; | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Enhance encapsulation and add field validation
- The fields are declared as public, which breaks encapsulation. Consider making them private and relying on Lombok's @DaTa for access.
- Consider adding validation constraints for required fields using javax.validation annotations.
@Data
public class EnrollByAadhaar{
- public Map<String, Object> authData;
- public ConsentRequest consent;
+ @NotNull(message = "Authentication data is required")
+ private Map<String, Object> authData;
+
+ @NotNull(message = "Consent request is required")
+ private ConsentRequest consent;
}Don't forget to add the import:
import javax.validation.constraints.NotNull;There was a problem hiding this comment.
Those are not required fields and can be public
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai Those are not required fields and can be public
There was a problem hiding this comment.
@helenKaryamsetty, understood, thank you for the clarification. I'll note that the fields authData and consent in the EnrollByAadhaar class are not required and can remain public.
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/data/v3/abhaCard/EnrollByAadhaar.java:10-13
Timestamp: 2024-12-09T15:56:01.326Z
Learning: Fields `authData` and `consent` in the `EnrollByAadhaar` class (`src/main/java/com/wipro/fhir/data/v3/abhaCard/EnrollByAadhaar.java`) are not required and can remain public.
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.
| @Data | ||
| public class OtpRequest { | ||
|
|
||
| private String timestamp; | ||
| private String txnId; | ||
| private String otpValue; | ||
| private String mobile; | ||
|
|
There was a problem hiding this comment.
π οΈ Refactor suggestion
Add field validations and documentation.
Consider enhancing the class with:
- Field validations using javax.validation annotations
- JavaDoc documentation for the class and fields
- Proper type for timestamp field
Here's the suggested implementation:
+import javax.validation.constraints.NotBlank;
+import javax.validation.constraints.Pattern;
+import java.time.LocalDateTime;
import lombok.Data;
+/**
+ * Data Transfer Object for OTP request operations in ABHA card services.
+ */
@Data
public class OtpRequest {
- private String timestamp;
- private String txnId;
- private String otpValue;
- private String mobile;
+ private LocalDateTime timestamp;
+
+ @NotBlank(message = "Transaction ID is required")
+ private String txnId;
+
+ @NotBlank(message = "OTP value is required")
+ @Pattern(regexp = "^[0-9]{6}$", message = "OTP must be 6 digits")
+ private String otpValue;
+
+ @NotBlank(message = "Mobile number is required")
+ @Pattern(regexp = "^[0-9]{10}$", message = "Mobile number must be 10 digits")
+ private String mobile;π 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.
| @Data | |
| public class OtpRequest { | |
| private String timestamp; | |
| private String txnId; | |
| private String otpValue; | |
| private String mobile; | |
| import javax.validation.constraints.NotBlank; | |
| import javax.validation.constraints.Pattern; | |
| import java.time.LocalDateTime; | |
| import lombok.Data; | |
| /** | |
| * Data Transfer Object for OTP request operations in ABHA card services. | |
| */ | |
| @Data | |
| public class OtpRequest { | |
| private LocalDateTime timestamp; | |
| @NotBlank(message = "Transaction ID is required") | |
| private String txnId; | |
| @NotBlank(message = "OTP value is required") | |
| @Pattern(regexp = "^[0-9]{6}$", message = "OTP must be 6 digits") | |
| private String otpValue; | |
| @NotBlank(message = "Mobile number is required") | |
| @Pattern(regexp = "^[0-9]{10}$", message = "Mobile number must be 10 digits") | |
| private String mobile; |
There was a problem hiding this comment.
Pattern and not blank validations are all not required to check here they are validated in UI repos.
There was a problem hiding this comment.
@helenKaryamsetty, understood. Thank you for the clarification. I've noted this for future reviews.
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/data/v3/abhaCard/OtpRequest.java:7-14
Timestamp: 2024-12-09T15:37:18.039Z
Learning: Field validations such as `@NotBlank` and `@Pattern` annotations are not required in backend Java classes like `OtpRequest.java` in `com.wipro.fhir.data.v3.abhaCard`, because they are validated in the UI repositories.
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.
| LoginAbhaRequest loginAbhaRequest = InputMapper.gson().fromJson(request, LoginAbhaRequest.class); | ||
|
|
||
| publicKeyString = certificateKeyService.getCertPublicKey(); | ||
| if (loginAbhaRequest.getLoginId() != null) { | ||
| encryptedLoginId = encryption.encrypt(loginAbhaRequest.getLoginId(), publicKeyString); | ||
| reqOtpEnrollment.setLoginId(encryptedLoginId); | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle Potential Null Values for loginId
In both methods, there is a check for loginAbhaRequest.getLoginId() != null, but if loginId is null, the subsequent processing might fail or cause unintended behavior. Ensure that the loginId is mandatory and validate input accordingly.
Consider adding input validation and returning a meaningful error if loginId is missing.
Also applies to: 158-165
There was a problem hiding this comment.
@coderabbitai loginId is mandatory value. Even if it is null, process can be lead further no need any validation
There was a problem hiding this comment.
@helenKaryamsetty, understood. Thank you for the clarification. I have noted that even if loginId is null in the requestOtpForAbhaLogin and verifyAbhaLogin methods of LoginAbhaV3ServiceImpl.java, the process can proceed without input validation.
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:84-91
Timestamp: 2024-12-09T16:01:39.601Z
Learning: In the `requestOtpForAbhaLogin` and `verifyAbhaLogin` methods of `LoginAbhaV3ServiceImpl.java`, `loginId` is mandatory, but even if it is null, the process can proceed without input validation.
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.
| encryptedLoginId = encryption.encrypt(loginAbhaRequest.getLoginId(), publicKeyString); | ||
| reqOtpEnrollment.setLoginId(encryptedLoginId); | ||
| } |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Avoid Encrypting Null or Empty loginId
Before encrypting loginId, ensure it is neither null nor empty to prevent exceptions or unintended outcomes from the encryption method.
Add a check:
if (loginAbhaRequest.getLoginId() != null && !loginAbhaRequest.getLoginId().isEmpty()) {
encryptedLoginId = encryption.encrypt(loginAbhaRequest.getLoginId(), publicKeyString);
reqOtpEnrollment.setLoginId(encryptedLoginId);
} else {
throw new FHIRException("Login ID cannot be null or empty");
}Also applies to: 163-164
There was a problem hiding this comment.
@coderabbitai if it is null no need to throw error, Other things can be continued.
There was a problem hiding this comment.
@helenKaryamsetty, understood. Thank you for the clarification. I'll remember that if loginId is null, the process can continue without throwing an error.
π§ Learnings used
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/service/v3/abha/LoginAbhaV3ServiceImpl.java:84-91
Timestamp: 2024-12-09T16:01:39.809Z
Learning: In the `requestOtpForAbhaLogin` and `verifyAbhaLogin` methods of `LoginAbhaV3ServiceImpl.java`, `loginId` is mandatory, but even if it is null, the process can proceed without input validation.
| // Cipher used in ABHA v3 APIs | ||
| Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWithSHA-1AndMGF1Padding"); | ||
| cipher.init(Cipher.ENCRYPT_MODE, publicKey); |
There was a problem hiding this comment.
Security concern: Consider using a more modern hashing algorithm.
The current implementation uses SHA-1 which is cryptographically broken. Consider using SHA-256 or SHA-512 with OAEP padding.
-Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWithSHA-1AndMGF1Padding");
+Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWITHSHA-256ANDMGF1PADDING");π 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.
| // Cipher used in ABHA v3 APIs | |
| Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWithSHA-1AndMGF1Padding"); | |
| cipher.init(Cipher.ENCRYPT_MODE, publicKey); | |
| // Cipher used in ABHA v3 APIs | |
| Cipher cipher = Cipher.getInstance("RSA/ECB/OAEPWITHSHA-256ANDMGF1PADDING"); | |
| cipher.init(Cipher.ENCRYPT_MODE, publicKey); |
There was a problem hiding this comment.
According to the requirement we have to use SHA1 only.
There was a problem hiding this comment.
@helenKaryamsetty, understood. If the requirement specifies the use of SHA-1, it's acceptable to keep it as is.
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/utils/Encryption.java:39-41
Timestamp: 2024-12-09T15:41:47.253Z
Learning: In `Encryption.java`, when implementing encryption, if the requirement mandates using `RSA/ECB/OAEPWithSHA-1AndMGF1Padding` despite security concerns with SHA-1, it's acceptable to use it as required.
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.
| @CrossOrigin | ||
| @RestController | ||
| @RequestMapping(value = "/abhaLogin", headers = "Authorization") |
There was a problem hiding this comment.
Review security implications of unrestricted CORS.
The @CrossOrigin annotation without any parameters allows requests from any origin, which could pose security risks. Consider restricting the allowed origins based on your deployment environment.
-@CrossOrigin
+@CrossOrigin(origins = "${allowed.origins}", allowedHeaders = "Authorization")Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@coderabbitai all over the project we are maintaining only crossOrigin only to maintain the commonality using it
There was a problem hiding this comment.
@helenKaryamsetty Understood, thank you for the clarification. I will note that in this project, the @CrossOrigin annotation is intentionally used without parameters across all controllers to maintain consistency.
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java:18-20
Timestamp: 2024-12-09T16:09:31.378Z
Learning: In the Java Spring Boot project, all controllers, including `LoginAbhaV3Controller.java` at `src/main/java/com/wipro/fhir/controller/v3/abha/`, use the `@CrossOrigin` annotation without parameters to maintain consistency across the codebase.
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.
src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java
Show resolved
Hide resolved
| @Autowired | ||
| private GenerateHealthID_CardServiceImpl generateHealthID_CardServiceImpl; |
There was a problem hiding this comment.
π οΈ Refactor suggestion
Remove unused service injection.
The GenerateHealthID_CardServiceImpl is autowired but never used in this controller.
-@Autowired
-private GenerateHealthID_CardServiceImpl generateHealthID_CardServiceImpl;| ##ABDM V3 APIs | ||
| getAuthCertPublicKey = https://healthidsbx.abdm.gov.in/api/v1/auth/cert | ||
| requestOtpForEnrollment = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/request/otp | ||
| abhaEnrollByAadhaar = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/enrol/byAadhaar | ||
| printAbhaCard = https://abhasbx.abdm.gov.in/abha/api/v3/profile/account/abha-card | ||
| abhaLoginRequestOtp = https://abhasbx.abdm.gov.in/abha/api/v3/profile/login/request/otp | ||
| verifyAbhaLogin = https://abhasbx.abdm.gov.in/abha/api/v3/profile/login/verify | ||
|
|
There was a problem hiding this comment.
π οΈ Refactor suggestion
Consider extracting base URLs to environment variables
The new ABDM V3 API endpoints use hardcoded URLs with sandbox domains. For better environment management and security, consider:
- Extracting base URLs to environment variables
- Using different base URLs for different environments (dev, test, prod)
Apply this pattern for better configuration management:
##ABDM V3 APIs
-getAuthCertPublicKey = https://healthidsbx.abdm.gov.in/api/v1/auth/cert
-requestOtpForEnrollment = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/request/otp
-abhaEnrollByAadhaar = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/enrol/byAadhaar
-printAbhaCard = https://abhasbx.abdm.gov.in/abha/api/v3/profile/account/abha-card
-abhaLoginRequestOtp = https://abhasbx.abdm.gov.in/abha/api/v3/profile/login/request/otp
-verifyAbhaLogin = https://abhasbx.abdm.gov.in/abha/api/v3/profile/login/verify
+[email protected]_AUTH_BASE_URL@
+[email protected]_ABHA_BASE_URL@
+
+getAuthCertPublicKey=${abdm.v3.auth.base-url}/api/v1/auth/cert
+requestOtpForEnrollment=${abdm.v3.abha.base-url}/api/v3/enrollment/request/otp
+abhaEnrollByAadhaar=${abdm.v3.abha.base-url}/api/v3/enrollment/enrol/byAadhaar
+printAbhaCard=${abdm.v3.abha.base-url}/api/v3/profile/account/abha-card
+abhaLoginRequestOtp=${abdm.v3.abha.base-url}/api/v3/profile/login/request/otp
+verifyAbhaLogin=${abdm.v3.abha.base-url}/api/v3/profile/login/verifyπ 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.
| ##ABDM V3 APIs | |
| getAuthCertPublicKey = https://healthidsbx.abdm.gov.in/api/v1/auth/cert | |
| requestOtpForEnrollment = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/request/otp | |
| abhaEnrollByAadhaar = https://abhasbx.abdm.gov.in/abha/api/v3/enrollment/enrol/byAadhaar | |
| printAbhaCard = https://abhasbx.abdm.gov.in/abha/api/v3/profile/account/abha-card | |
| abhaLoginRequestOtp = https://abhasbx.abdm.gov.in/abha/api/v3/profile/login/request/otp | |
| verifyAbhaLogin = https://abhasbx.abdm.gov.in/abha/api/v3/profile/login/verify | |
| ##ABDM V3 APIs | |
| abdm.v3.auth.base-url[email protected]_AUTH_BASE_URL@ | |
| abdm.v3.abha.base-url[email protected]_ABHA_BASE_URL@ | |
| getAuthCertPublicKey=${abdm.v3.auth.base-url}/api/v1/auth/cert | |
| requestOtpForEnrollment=${abdm.v3.abha.base-url}/api/v3/enrollment/request/otp | |
| abhaEnrollByAadhaar=${abdm.v3.abha.base-url}/api/v3/enrollment/enrol/byAadhaar | |
| printAbhaCard=${abdm.v3.abha.base-url}/api/v3/profile/account/abha-card | |
| abhaLoginRequestOtp=${abdm.v3.abha.base-url}/api/v3/profile/login/request/otp | |
| verifyAbhaLogin=${abdm.v3.abha.base-url}/api/v3/profile/login/verify |
There was a problem hiding this comment.
@coderabbitai other urls are not maintained with variables so not maintaining these also with variables and using direct urls.
There was a problem hiding this comment.
@helenKaryamsetty Thank you for the clarification.
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/environment/common_example.properties:87-94
Timestamp: 2024-12-10T13:24:01.383Z
Learning: In the FHIR-API project, URLs are maintained directly in the properties files rather than using variables, so it's acceptable to use direct URLs instead of variables.
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.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Actionable comments posted: 4
π§Ή Outside diff range and nitpick comments (2)
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java (2)
19-21: Simplify CORS configuration.Multiple
@CrossOriginannotations are redundant. The class-level annotation is sufficient, and method-level annotations can be removed.@CrossOrigin @RestController @RequestMapping(value = "/abhaCreation", headers = "Authorization") public class CreateAbhaV3Controller { // ... - @CrossOrigin @Operation(summary = "Generate OTP for ABHA enrollment") @PostMapping(value = { "/requestOtpForAbhaEnrollment" }) // ... (remove @CrossOrigin from other methods as well)Also applies to: 29-29, 49-49, 70-70, 90-90
22-108: Consider implementing centralized error handling and logging.The controller could benefit from:
- A
@ControllerAdvicefor centralized exception handling- A custom logging aspect for consistent request/response logging
- An enum or constants class for error codes and messages
This would reduce code duplication and ensure consistent error handling and logging across all endpoints.
Would you like me to provide an example implementation of these improvements?
π§° Tools
πͺ GitHub Check: SonarCloud
[notice] 33-33: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
[notice] 53-53: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
[notice] 74-74: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
π Files selected for processing (1)
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java(1 hunks)
π§° Additional context used
π Learnings (1)
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/controller/v3/abha/LoginAbhaV3Controller.java:18-20
Timestamp: 2024-12-09T16:09:31.513Z
Learning: In the Java Spring Boot project, all controllers, including `LoginAbhaV3Controller.java` at `src/main/java/com/wipro/fhir/controller/v3/abha/`, use the `@CrossOrigin` annotation without parameters to maintain consistency across the codebase.
πͺ GitHub Check: SonarCloud
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
[notice] 33-33: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
[notice] 53-53: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
[notice] 74-74: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
π Additional comments (1)
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java (1)
12-12: Remove unused import.
The GenerateHealthID_CardServiceImpl import is not used in this controller.
-import com.wipro.fhir.service.ndhm.GenerateHealthID_CardServiceImpl;
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
Show resolved
Hide resolved
| @PostMapping(value = { "/abhaEnrollmentByAadhaar" }) | ||
| public String abhaEnrollmentByAadhaar(@RequestBody String request) { | ||
| logger.info("ABHA enrollment BY Aadhaar API request " + request); | ||
| OutputResponse response = new OutputResponse(); | ||
| String res = null; | ||
| try { | ||
| if (request != null) { | ||
| res = createAbhaV3Service.enrollmentByAadhaar(request); | ||
| response.setResponse(res); | ||
| } else | ||
| throw new FHIRException("NDHM_FHIR Empty request object"); | ||
| } catch (FHIRException e) { | ||
| response.setError(5000, e.getMessage()); | ||
| logger.error(e.toString()); | ||
| } | ||
| logger.info("NDHM_FHIR generate OTP for ABHA card API response " + response.toString()); | ||
| return response.toString(); | ||
| } |
There was a problem hiding this comment.
Fix logging and remove redundant variable in Aadhaar enrollment endpoint.
- Raw request logging poses security risks
- Response log message is incorrect (mentions OTP instead of enrollment)
- Variable 'res' is redundant
@PostMapping(value = { "/abhaEnrollmentByAadhaar" })
public String abhaEnrollmentByAadhaar(@RequestBody String request) {
- logger.info("ABHA enrollment BY Aadhaar API request " + request);
+ logger.info("ABHA enrollment by Aadhaar API request received");
OutputResponse response = new OutputResponse();
- String res = null;
try {
if (request != null) {
- res = createAbhaV3Service.enrollmentByAadhaar(request);
- response.setResponse(res);
+ response.setResponse(createAbhaV3Service.enrollmentByAadhaar(request));
} else
- throw new FHIRException("NDHM_FHIR Empty request object");
+ throw new FHIRException("ABHA-002: Empty request object");
} catch (FHIRException e) {
- response.setError(5000, e.getMessage());
+ response.setError(4002, "Aadhaar Enrollment Failed: " + e.getMessage());
logger.error(e.toString());
}
- logger.info("NDHM_FHIR generate OTP for ABHA card API response " + response.toString());
+ logger.info("Aadhaar enrollment completed with status: {}", response.hasError() ? "ERROR" : "SUCCESS");
return response.toString();
}π 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.
| @PostMapping(value = { "/abhaEnrollmentByAadhaar" }) | |
| public String abhaEnrollmentByAadhaar(@RequestBody String request) { | |
| logger.info("ABHA enrollment BY Aadhaar API request " + request); | |
| OutputResponse response = new OutputResponse(); | |
| String res = null; | |
| try { | |
| if (request != null) { | |
| res = createAbhaV3Service.enrollmentByAadhaar(request); | |
| response.setResponse(res); | |
| } else | |
| throw new FHIRException("NDHM_FHIR Empty request object"); | |
| } catch (FHIRException e) { | |
| response.setError(5000, e.getMessage()); | |
| logger.error(e.toString()); | |
| } | |
| logger.info("NDHM_FHIR generate OTP for ABHA card API response " + response.toString()); | |
| return response.toString(); | |
| } | |
| @PostMapping(value = { "/abhaEnrollmentByAadhaar" }) | |
| public String abhaEnrollmentByAadhaar(@RequestBody String request) { | |
| logger.info("ABHA enrollment by Aadhaar API request received"); | |
| OutputResponse response = new OutputResponse(); | |
| try { | |
| if (request != null) { | |
| response.setResponse(createAbhaV3Service.enrollmentByAadhaar(request)); | |
| } else | |
| throw new FHIRException("ABHA-002: Empty request object"); | |
| } catch (FHIRException e) { | |
| response.setError(4002, "Aadhaar Enrollment Failed: " + e.getMessage()); | |
| logger.error(e.toString()); | |
| } | |
| logger.info("Aadhaar enrollment completed with status: {}", response.hasError() ? "ERROR" : "SUCCESS"); | |
| return response.toString(); | |
| } |
π§° Tools
πͺ GitHub Check: SonarCloud
[notice] 53-53: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
There was a problem hiding this comment.
@coderabbitai kindly ignore all the logs related suggestions. Those are given according to our debugging usage
There was a problem hiding this comment.
@helenKaryamsetty, understood. I will ignore log-related suggestions in future reviews.
(γ»xγ»`)
βοΈ Learnings added
Learnt from: helenKaryamsetty
PR: PSMRI/FHIR-API#52
File: src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java:51-68
Timestamp: 2024-12-10T14:52:59.709Z
Learning: In the FHIR-API project, when reviewing code in Java controllers (e.g., `CreateAbhaV3Controller.java`), the team prefers to retain raw request logging for debugging purposes. Therefore, suggestions to remove or modify logging statements related to raw requests should be ignored.
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.
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
Show resolved
Hide resolved
src/main/java/com/wipro/fhir/controller/v3/abha/CreateAbhaV3Controller.java
Show resolved
Hide resolved
π Description
JIRA ID: AMM-984, AMM-1016, AMM-1017
β 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
Documentation
Chores