Missing field properties added #149
Conversation
WalkthroughThe pull request introduces extensive modifications to the Changes
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: 3
🧹 Nitpick comments (1)
src/main/environment/common_example.properties (1)
93-114: Standardize configuration format and add parameter documentation
- Use consistent notation (either dot or hyphen) for property names
- Add documentation for each parameter including:
- Valid ranges
- Default values
- Impact on system behavior
Example format:
# Duration in minutes for CZ operations # Range: 30-60, Default: 40 cz-duration=40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/environment/common_example.properties(4 hunks)
🔇 Additional comments (1)
src/main/environment/common_example.properties (1)
59-88: Improve scheduler configurations documentation and organization
- Consider adding detailed comments explaining the purpose and impact of each scheduler
- Group related schedulers together (e.g., all CTI-related schedulers)
- Verify the frequency of schedulers running every 1-5 minutes to ensure they don't overload the system
| ##commented the below lines as it is already defined above | ||
| # spring.datasource.url=jdbc:mysql://localhost:3306/db_iemr? | ||
| encDbUserName=zFlYsp9Z0s+lRvLM15A3g/Ba0w8VGs/1usuW7EsGF3k= | ||
| encDbPass=JGGAGn5wTlrbTLUHY+5BzfBa0w8VGs/1usuW7EsGF3k= | ||
| spring.datasource.driver-class-name=com.mysql.jdbc.Driver | ||
|
|
||
| secondary.datasource.url=<Enter DB_REPORTING URL here> | ||
| secondary.datasource.username=<Enter your DB_REPORTING username> | ||
| secondary.datasource.password=<Enter your DB_REPORTING password> | ||
| secondary.datasource.url=<Enter DB_REPORTING URL here> | ||
| secondary.datasource.driver-class-name=com.mysql.jdbc.Driver |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security improvements for database configurations
- Consider using environment variables or a secure vault for sensitive credentials
- Update the deprecated MySQL driver class
-secondary.datasource.driver-class-name=com.mysql.jdbc.Driver
+secondary.datasource.driver-class-name=com.mysql.cj.jdbc.Driver📝 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.
| ##commented the below lines as it is already defined above | |
| # spring.datasource.url=jdbc:mysql://localhost:3306/db_iemr? | |
| encDbUserName=zFlYsp9Z0s+lRvLM15A3g/Ba0w8VGs/1usuW7EsGF3k= | |
| encDbPass=JGGAGn5wTlrbTLUHY+5BzfBa0w8VGs/1usuW7EsGF3k= | |
| spring.datasource.driver-class-name=com.mysql.jdbc.Driver | |
| secondary.datasource.url=<Enter DB_REPORTING URL here> | |
| secondary.datasource.username=<Enter your DB_REPORTING username> | |
| secondary.datasource.password=<Enter your DB_REPORTING password> | |
| secondary.datasource.url=<Enter DB_REPORTING URL here> | |
| secondary.datasource.driver-class-name=com.mysql.jdbc.Driver | |
| ##commented the below lines as it is already defined above | |
| # spring.datasource.url=jdbc:mysql://localhost:3306/db_iemr? | |
| encDbUserName=zFlYsp9Z0s+lRvLM15A3g/Ba0w8VGs/1usuW7EsGF3k= | |
| encDbPass=JGGAGn5wTlrbTLUHY+5BzfBa0w8VGs/1usuW7EsGF3k= | |
| spring.datasource.driver-class-name=com.mysql.jdbc.Driver | |
| secondary.datasource.url=<Enter DB_REPORTING URL here> | |
| secondary.datasource.username=<Enter your DB_REPORTING username> | |
| secondary.datasource.password=<Enter your DB_REPORTING password> | |
| secondary.datasource.driver-class-name=com.mysql.cj.jdbc.Driver |
| #--------------------------NHM Agent Real Time Data---------------------------- | ||
| nhm.agent.real.time.data.url= http://175.101.1.83/apps/utility/alive_api.php | ||
| nhm.agent.real.time.data.cron.scheduler=0 */2 * ? * * | ||
| nhm.agent.real.time.data.cron.flag=true |
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded IP should be externalized following existing patterns
The CI environment already demonstrates the correct pattern using @env.NHM_AGENT_REAL_TIME_DATA_URL@. Other environments should follow this approach.
- Update in src/main/environment/common_dev.properties
- Update in src/main/environment/common_test.properties
- Update in src/main/environment/common_uat.properties
- Update in src/main/environment/common_example.properties
Note: The 2-minute polling interval appears to be an intentional design choice, controlled by a feature flag, and is consistently configured across all environments.
🔗 Analysis chain
Review NHM agent configuration for security and performance
- Replace hardcoded IP with configurable hostname
- Verify if 2-minute polling interval is necessary and won't overload the external service
-nhm.agent.real.time.data.url= http://175.101.1.83/apps/utility/alive_api.php
+nhm.agent.real.time.data.url=${NHM_AGENT_HOST}/apps/utility/alive_api.php🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for IP address usage
rg -l "175\.101\.1\.83"
# Look for environment variable patterns in properties files
fd -e properties -x rg -l "\$\{[A-Z_]+\}"
# Find cron scheduler references
rg "0 \*/2 \* \? \* \*"
# Search for NHM agent related configurations
rg "nhm\.agent"
Length of output: 2693
| @@ -110,3 +166,4 @@ eausadhaAuthorization=<Enter eausadha authorization key> | |||
| spring.main.allow-bean-definition-overriding=true | |||
| spring.main.allow-circular-references=true | |||
|
|
|||
| fileBasePath =<Enter your required basepath here>/Doc | |||
There was a problem hiding this comment.
💡 Codebase verification
Add security validation for file base path
The codebase properly handles directory creation and validation, but lacks security measures for the base path. Consider:
- Add validation to ensure the base path is within allowed boundaries
- Implement path sanitization to prevent directory traversal attacks
- Document the expected base path format and restrictions
🔗 Analysis chain
Validate and secure file base path configuration
- Ensure the path is within allowed secure directories
- Verify if the application validates/creates this directory at startup
- Consider adding documentation about required permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for file path validation in the codebase
rg -B 2 -A 2 "fileBasePath"
Length of output: 2490
Script:
#!/bin/bash
# Check for directory validation and complete usage context
ast-grep --pattern 'class CommonServiceImpl {
$$$
private String fileBasePath;
$$$
}'
# Also check for common directory validation patterns
rg -B 2 -A 2 "mkdirs|exists|isDirectory|createDirectories"
Length of output: 5620
|
@Asp-Codes a lot of these variables look common across environments. Can we keep it application.properties (and remove from test.properties - from where I guess you copied from)? |
Yes that is the ideal way but currently test dev and ci all these contain variables.Only in the common they are missing so I just added in it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/environment/common_example.properties (1)
92-113: Document configuration parametersNew configuration parameters lack documentation about their purpose and impact:
cz-duration=40everwellCalendarDuration=15lonicPageSize=10callRetryConfiguration=3avniRegistrationLimit=7Add comments explaining:
- The purpose of each parameter
- The impact of modifying these values
- The units of measurement (where applicable)
- The recommended ranges for these values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/environment/common_example.properties(5 hunks)
🔇 Additional comments (6)
src/main/environment/common_example.properties (6)
3-14: Security: Use secure database configuration practicesThe database configuration has several security concerns:
- Default credentials (root/1234) should not be used
- Using deprecated MySQL driver
- Sensitive credentials should be externalized
58-87: Review scheduler configurations and resource impactSeveral schedulers are configured to run frequently:
- CTI data check runs daily at 12:10 AM
- NHM dashboard runs daily at 12:01 AM
- Everwell data sync runs every 5 minutes when enabled
Please verify:
- The resource impact of running these schedulers
- Why certain schedulers are enabled by default
- If the cron expressions align with business requirements
Consider implementing a rate limiter or circuit breaker to prevent system overload from frequent scheduler executions.
114-117: Externalize hardcoded IP addressThe NHM agent URL contains a hardcoded IP address which should be externalized following the existing pattern using environment variables.
168-168: Secure the file base path configurationThe file base path configuration requires security validation and documentation.
134-136: Verify port changes for Everwell endpointsThe port for Everwell endpoints has been changed from 8080 to 8083. Please verify:
- If this change is coordinated with the Everwell service deployment
- If all related services are aware of this port change
27-31: Clarify identity service configurationThe comment "not sure abt this" suggests uncertainty about the identity configuration. Please:
- Verify if port 8094 is the correct port for identity services
- Document the purpose of each identity endpoint
- Remove the uncertain comment once verified
✅ Verification successful
Remove uncertainty comment and improve documentation
The identity configurations are valid and actively used in
IdentityBeneficiaryServiceImpl.java:
identity-api-url: Regular identity service endpointidentity-1097-api-url: 1097-specific identity service endpointPlease:
- Remove the "not sure abt this" comment as both configurations are necessary
- Add descriptive comments explaining the purpose of each endpoint
- Consider using placeholders like other environment files:
<Enter your socket address here>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these identity endpoints are referenced elsewhere rg "identity-api-url|identity-1097-api-url" --type propertiesLength of output: 98
Script:
#!/bin/bash # Search for identity configurations and URLs without file type restriction rg -l "identity-api-url|identity-1097-api-url|localhost:8094/identity-0.0.1|localhost:8094/1097identityapi" # Search for port 8094 usage rg "8094" # Find all properties files fd ".*\.properties$"Length of output: 1001
Script:
#!/bin/bash # Check identity configurations across all environments for f in src/main/environment/common_{ci,dev,test,uat}.properties; do echo "=== $f ===" rg "identity.*api.*url" "$f" done # Check how these properties are used in the service rg -A 5 "identity.*api.*url" src/main/java/com/iemr/common/service/beneficiary/IdentityBeneficiaryServiceImpl.java # Check if there's documentation about these endpoints fd "README|CONTRIBUTING|docs" -e mdLength of output: 8067
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/environment/common_example.properties (1)
95-114: Document configuration parametersNew configuration parameters have been added without documentation explaining their purpose and valid values:
- cz-duration
- TMprescriptionTemplate
- everwellCalendarDuration
- lonicPageSize
- callRetryConfiguration
- avniRegistrationLimit
Add comments explaining:
- Purpose of each parameter
- Valid value ranges
- Impact on system behavior
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/environment/common_example.properties(5 hunks)
🔇 Additional comments (4)
src/main/environment/common_example.properties (4)
115-118: Replace hardcoded IP with environment variableThe previous review already identified this issue. The hardcoded IP address should be externalized following the existing pattern using environment variables.
-nhm.agent.real.time.data.url= http://175.101.1.83/apps/utility/alive_api.php +nhm.agent.real.time.data.url= ${NHM_AGENT_HOST}/apps/utility/alive_api.php
169-169: Add security validation for file base pathThe previous review already identified this security concern. The file base path needs proper validation and security measures:
- Path validation within allowed boundaries
- Path sanitization
- Documentation of expected format and restrictions
61-88: Review scheduler configurationsSeveral scheduler configurations have been added with specific concerns:
- Multiple schedulers with overlapping purposes
- Some schedulers are enabled by default in example configuration
- Cron expressions might need validation
Run this script to analyze scheduler patterns across environments:
✅ Verification successful
Scheduler configurations are appropriate and well-structured
The review concerns have been analyzed and found to be unfounded:
- Each scheduler serves a distinct purpose without functional overlap
- The enabled schedulers (CTI data check and NHM dashboard) in the example configuration are appropriate for core functionality
- All cron expressions are valid and scheduled at non-conflicting intervals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check scheduler configurations across different environments fd -e properties -x rg "start-.*-scheduler|cron-scheduler-.*" # Validate cron expressions fd -e properties -x rg "cron.*=.*"Length of output: 3975
28-32: Remove redundant URL suffixesBased on the previous discussion between drtechie and Asp-irin, the
/identity-0.0.1suffix is not needed when running services locally.Please verify if these changes align with the local development setup:
-identity-api-url = http://localhost:8094/ +identity-api-url = http://localhost:8094 -identity-1097-api-url = http://localhost:8094/ +identity-1097-api-url = http://localhost:8094
| @@ -1,20 +1,20 @@ | |||
| # local env | |||
| server.port=8083 | |||
There was a problem hiding this comment.
Security: Avoid committing sensitive credentials
The database configuration exposes sensitive credentials:
- Plain text passwords
- Root user access
- Direct database URLs
Consider these security improvements:
- Use environment variables or secure vault for credentials
- Create dedicated database users with limited privileges
- Add placeholders instead of actual values in example properties
-spring.datasource.username=root
-spring.datasource.password=1234
+spring.datasource.username=${DB_USER}
+spring.datasource.password=${DB_PASSWORD}Also applies to: 4-6, 13-15
| everwell1097userAuthenticate = http://10.208.122.38:8083/user/userAuthenticate | ||
| everwelluserAuthenticate = https://beta-hub.everwell.org/token | ||
| everwellRegisterBenficiary = http://10.208.122.38:8080/commonapi-v1.0/beneficiary/create | ||
| everwellRegisterBenficiary = http://10.208.122.38:8083/beneficiary/create |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize API endpoint configuration
The API endpoints contain inconsistent port usage and hardcoded IPs:
- Some endpoints use HTTPS while others use HTTP
- Hardcoded IP (10.208.122.38) should be configurable
- Port 8083 is hardcoded
Standardize the configuration:
-everwell1097userAuthenticate = http://10.208.122.38:8083/user/userAuthenticate
+everwell1097userAuthenticate = ${EVERWELL_API_BASE_URL}/user/userAuthenticate
-everwellRegisterBenficiary = http://10.208.122.38:8083/beneficiary/create
+everwellRegisterBenficiary = ${EVERWELL_API_BASE_URL}/beneficiary/create📝 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.
| everwell1097userAuthenticate = http://10.208.122.38:8083/user/userAuthenticate | |
| everwelluserAuthenticate = https://beta-hub.everwell.org/token | |
| everwellRegisterBenficiary = http://10.208.122.38:8080/commonapi-v1.0/beneficiary/create | |
| everwellRegisterBenficiary = http://10.208.122.38:8083/beneficiary/create | |
| everwell1097userAuthenticate = ${EVERWELL_API_BASE_URL}/user/userAuthenticate | |
| everwelluserAuthenticate = https://beta-hub.everwell.org/token | |
| everwellRegisterBenficiary = ${EVERWELL_API_BASE_URL}/beneficiary/create |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/environment/common_example.properties (2)
95-114: Document configuration parametersPlease add comments explaining:
- The purpose and valid ranges for
cz-duration,everwellCalendarDuration- The impact of
callRetryConfigurationandavniRegistrationLimit- The significance of
lonicPageSize
135-137: Standardize API endpoint configurationConsider using environment variables for the base URL to make it configurable across environments:
-everwell1097userAuthenticate = http://localhost:8083/user/userAuthenticate -everwellRegisterBenficiary = http://localhost:8083/beneficiary/create +everwell1097userAuthenticate = ${COMMON_API_BASE_URL}/user/userAuthenticate +everwellRegisterBenficiary = ${COMMON_API_BASE_URL}/beneficiary/create
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/environment/common_example.properties(5 hunks)
🔇 Additional comments (4)
src/main/environment/common_example.properties (4)
2-15: Security: Avoid committing sensitive credentialsThe database configuration exposes sensitive credentials in plain text. This is a security risk that was previously identified.
28-32: LGTM! URL simplification implementedThe identity API URLs have been simplified as discussed in the PR comments.
115-118: Hardcoded IP should be externalizedThe hardcoded IP in the NHM agent URL should be externalized following existing patterns.
169-169: Add security validation for file base pathThe file base path configuration requires security validation to prevent directory traversal attacks.
| start-ctidatasync-scheduler=false | ||
| cron-scheduler-ctidatasync=0 0 2 */2 * ? | ||
|
|
||
| ##-------------------------------###cti data check with call detail report Scheduler------------------------------------------------------ | ||
|
|
||
| #Runs at everyday 12:10AM | ||
| start-ctidatacheck-scheduler=true | ||
| cron-scheduler-ctidatacheck=0 10 00 * * * | ||
|
|
||
| ##---------------------------------#### Registration schedular for Avni------------------------------------------------------------------------------ | ||
|
|
||
| start-avni-scheduler=false | ||
| cron-avni-registration=0 0/1 * * * ? * | ||
|
|
||
| ##------------------------------------------------#### Everwell Regsitration Scheduler--------------------------------------------------------------- | ||
|
|
||
| start-registration-scheduler=false | ||
| cron-scheduler-registration=0 34 19 * * ? * | ||
|
|
||
| ##----------------------------------------------------#everwell data sync----------------------------------------------------------- | ||
|
|
||
| start-everwelldatasync-scheduler=false | ||
| cron-scheduler-everwelldatasync=0 0/5 * * * ? * | ||
|
|
||
| ##-----------------------------------------------#NHM data dashboard schedular---------------------------------------------------------------- | ||
| # run at everyday 12:01AM | ||
| start-nhmdashboard-scheduler=true | ||
| cron-scheduler-nhmdashboard=0 1 * * * ? * |
There was a problem hiding this comment.
💡 Codebase verification
Scheduler configurations are valid but need optimization
The cron expressions are syntactically correct, and there are no direct timing conflicts. However:
- Consider increasing the interval for high-frequency schedulers:
cron-scheduler-everwelldatasync=0 0/5 * * * ? *(every 5 minutes) might be too aggressive- Multiple schedulers running every minute (SMS, Email, Avni) could impact system resources
- Ensure consistent scheduler states across different property files to avoid confusion
🔗 Analysis chain
Verify scheduler configurations
Please verify:
- The overlap between different data sync schedulers (CTI, Everwell, NHM)
- The cron expressions, especially:
cron-scheduler-ctidatasync=0 0 2 */2 * ?(runs every 2 days at 2 AM)cron-scheduler-everwelldatasync=0 0/5 * * * ? *(runs every 5 minutes)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other scheduler configurations that might conflict
rg "cron-scheduler-" --type properties
# Look for scheduler implementation classes
ast-grep --pattern 'class $_ implements $_Scheduler {
$$$
}'
Length of output: 138
Script:
#!/bin/bash
# Find and search properties files
fd -e properties -x cat {} | grep -i "scheduler\|cron"
# Search for scheduler implementations with broader patterns
ast-grep --pattern 'class $_ implements Scheduler'
ast-grep --pattern '@Scheduled'
Length of output: 4375
Script:
#!/bin/bash
# Search for scheduler implementation files
fd -e java -x cat {} | grep -A 5 -B 5 "@Scheduled"
# Look for specific scheduler classes
fd -e java -x grep -l "CTIDataSync\|EverwellDataSync\|NHMDashboard" {}
Length of output: 1184
|
|
||
| spring.datasource.url=jdbc:mysql://localhost:3306/db_iemr?autoReconnect=true&useSSL=false | ||
| # spring.datasource.url=jdbc:mysql://localhost:3306/db_iemr?autoReconnect=true&useSSL=false |
📋 Description
This PR adds missing field properties to the
common_example.propertiesfile and removes redundant code to improve code practices.Summary by CodeRabbit
Configuration Updates
Scheduler Management