Feature/9272/automatic secret refresh from azure #9286
Conversation
WalkthroughThis pull request centralizes environment-backed configuration by introducing multiple property components (RemoteWebClientProperties, GoogleProperties, OpenAiProperties, AzureProperties, SecurityProperties) and replaces scattered Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Prop as Property Bean
participant Env as Environment
participant Caller as Controller/Service
App->>Prop: constructor injection
activate Prop
Prop->>Prop: `@PostConstruct` validateProperties()
Prop->>Env: getProperty(key)
alt value present
Env-->>Prop: value
Prop->>Prop: log info (validated)
else missing
Env-->>Prop: null
Prop->>Prop: log error
Prop-->>Prop: throw IllegalStateException
end
deactivate Prop
Caller->>Prop: call getter()
activate Prop
Prop->>Env: getProperty(key)
alt value present
Env-->>Prop: value
Prop-->>Caller: return value
else missing
Env-->>Prop: null
Prop-->>Caller: throw IllegalStateException
end
deactivate Prop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Areas to focus on during review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (11)
core/src/main/resources/application.properties (1)
2-2: Verify that the 15-minute refresh interval aligns with operational requirements.The default refresh interval for Azure Key Vault secrets is 30 minutes, and can be configured via
spring.cloud.azure.keyvault.secret.property-sources[].refresh-interval. This change reduces it to 15 minutes, which will increase the frequency of API calls to Azure Key Vault.Consider whether this interval:
- Is appropriate for your application's secret rotation cadence and cost implications
- Should remain hardcoded or be externalized to an environment variable (consistent with other configuration in this file like
${KEY_VAULT_ENDPOINT},${DATASOURCE_URL}, etc.)If the 15-minute interval is specifically chosen for security or compliance requirements, a brief comment in the configuration would help future maintainers understand the rationale.
If you want this to be environment-specific, consider externalizing it:
-spring.cloud.azure.keyvault.secret.property-sources[0].refresh-interval=15m +spring.cloud.azure.keyvault.secret.property-sources[0].refresh-interval=${AZURE_KEYVAULT_REFRESH_INTERVAL:15m}This approach would allow different environments to override the default if needed.
service/src/test/java/greencity/service/DataBaseBackupServiceImplTest.java (1)
34-44: Consider consistency in property injection approach.While the PR refactors Azure properties to use
AzureProperties, these datasource properties still use@Valueannotations. For consistency, consider whether these should also be moved to a centralized properties class.core/src/main/java/greencity/controller/EcoNewsController.java (1)
226-226: Consider using a constant for the "enabled" string and add null safety.The string literal "enabled" is a magic value that could be defined as a constant. Additionally, consider null safety if
getRelevance()might return null.Apply this improvement:
-boolean isRelevanceEnabled = openAiProperties.getRelevance().equals("enabled"); +boolean isRelevanceEnabled = "enabled".equalsIgnoreCase( + openAiProperties.getRelevance());Alternatively, define a constant:
private static final String RELEVANCE_ENABLED = "enabled"; // Then use: boolean isRelevanceEnabled = RELEVANCE_ENABLED.equalsIgnoreCase( openAiProperties.getRelevance());The
equalsIgnoreCasewith the constant on the left prevents NPE ifgetRelevance()returns null.service-api/src/test/java/greencity/client/RestClientTest.java (1)
27-102: Good adaptation toRemoteWebClientProperties; consider narrowing LENIENT strictnessThe constructor wiring and stubbing of
RemoteWebClientPropertieslook correct and keep existing URL and system email expectations aligned with the new configuration bean.One minor suggestion: class-level
@MockitoSettings(strictness = Strictness.LENIENT)can hide accidental unused stubs over time. If feasible, you might prefer the default strictness and mark only specific stubs as lenient (e.g., vialenient().when(...)) in tests that truly need it. This keeps the suite a bit safer while still supporting shared setup.service-api/src/main/java/greencity/properties/AzureProperties.java (1)
1-48: Azure properties component is consistent and clear (only tiny Javadoc nit)This class cleanly follows the shared pattern for property beans: fail-fast validation in
@PostConstruct, specificErrorMessageuse, and no caching so callers always read the latest Environment values. That aligns well with the PR’s configuration refactor.Only an optional polish: the Javadoc line
* Сlass for retrieving...appears to use a non-Latin “C”; you may want to correct it toClassfor readability.service-api/src/main/java/greencity/properties/OpenAiProperties.java (1)
1-41: OpenAI properties wiring looks good; fix small log message typoThe configuration access and validation logic here are consistent with the other property beans and give clear failure modes when required values are missing.
There is a minor typo in the success log message:
log.info("All OpneAi properties validated successfully.");Consider correcting
"OpneAi"to"OpenAi"(or"OpenAI") to keep logs clean and searchable.service-api/src/main/java/greencity/properties/SecurityProperties.java (1)
1-42: Security properties handling is appropriate; minor naming polishThe fail-fast validation of JWT secret and access token expiration is a good fit for security-sensitive configuration, and the pattern is consistent with the other property beans.
Minor nit: the local variable in
getJwtAccessTokenExpiration()is namedaccesssTokenExpiration(threescharacters). Renaming it toaccessTokenExpirationwould tidy up the code, though it doesn’t affect behavior.service-api/src/main/java/greencity/client/RestClient.java (2)
16-17: Centralizing RestClient configuration viaRemoteWebClientPropertieslooks solidInjecting
RemoteWebClientPropertiesand wiring it through the constructor is a clean way to centralize remote endpoint and system email config, and it aligns well with the rest of the PR. This should make future changes to server addresses or system email much easier and safer.As a small polish-only follow‑up, consider tightening the Javadoc wording (fixing typos like
Environmet/creatand naming itRemoteWebClientPropertiesrather thanRemoteWebClient) to keep documentation clear for future maintainers.Also applies to: 51-54, 66-78, 79-89
91-122: Property-driven URLs and system-email based JWT creation look correct and consistentAll usages of the new properties are wired consistently:
- UBS calls use
getGreencityUbsServerAddress()(e.g., notifications endpoint) while all other calls usegetGreencityUserServerAddress(), which matches the intent of the methods.- Every previously hard-coded/user-address field access has been replaced by
remoteWebClientPropertiesacross user lookup, management, notifications, and scheduled email methods.setHeader()now falls back to a JWT generated withremoteWebClientProperties.getSystemEmailAddress(), tying token creation into the new config source as intended.This is a good, low-risk refactor that improves configurability and prepares the client for dynamic property updates. If you want to reduce repetition later, you could introduce small helpers like
userServiceBaseUrl()/ubsServiceBaseUrl()to wrap the repeatedgetGreencityUserServerAddress()/getGreencityUbsServerAddress()calls, but that’s purely a readability/DRY improvement and not required for correctness.Also applies to: 130-383, 390-409, 424-430
service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
256-270: New configuration error messages are clear; consider minor style alignmentThe added
*_NOT_SETconstants nicely standardize error handling for missing configuration (server addresses, timeouts, Azure connection, JWT/OpenAI/Google keys, etc.) and will make startup failures and logs much more understandable.For consistency with existing messages, you might optionally:
- Capitalize the first word (e.g.,
"Greencity server address not set."), and- Ensure punctuation/casing matches the surrounding messages.
This is purely cosmetic; the current texts are functionally fine.
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
36-97: Getter implementations are straightforward; consider small safety and maintainability tweaksThe individual getters correctly:
- Read each property from
Environmentusing the expected key.- Guard against missing/empty values (
StringUtils.hasTextfor strings, null checks for integers).- Log and throw
IllegalStateExceptionwith the correspondingErrorMessageconstant when required values aren’t provided.A couple of optional refinements you might consider:
- Extract the property keys into
private static final Stringconstants (e.g.,GREENCITY_SERVER_ADDRESS_KEY = "address"). This reduces string duplication and the risk of typos if these keys are reused elsewhere.- For
getConnectionTimeout()andgetResponseTimeout(), you may want to validate that the values are positive (and perhaps within a reasonable upper bound) to catch misconfigurations earlier.- The variable name
systemEmailAddreshas a minor typo; renaming it tosystemEmailAddresswill improve readability.Functionally this class looks correct and it cleanly encapsulates the new configuration surface for remote web clients.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
core/src/main/java/greencity/controller/EcoNewsController.java(3 hunks)core/src/main/java/greencity/controller/HabitAssignController.java(3 hunks)core/src/main/java/greencity/webcontroller/ManagementController.java(3 hunks)core/src/main/java/greencity/webcontroller/ManagementEventController.java(5 hunks)core/src/main/resources/application.properties(1 hunks)core/src/test/java/greencity/controller/EcoNewsControllerTest.java(3 hunks)core/src/test/java/greencity/controller/HabitAssignControllerTest.java(2 hunks)core/src/test/java/greencity/webcontroller/ManagementControllerTest.java(3 hunks)core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java(2 hunks)service-api/pom.xml(1 hunks)service-api/src/main/java/greencity/client/RestClient.java(20 hunks)service-api/src/main/java/greencity/client/config/UserRemoteClientConfig.java(2 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/properties/AzureProperties.java(1 hunks)service-api/src/main/java/greencity/properties/GoogleProperties.java(1 hunks)service-api/src/main/java/greencity/properties/OpenAiProperties.java(1 hunks)service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java(1 hunks)service-api/src/main/java/greencity/properties/SecurityProperties.java(1 hunks)service-api/src/main/java/greencity/security/jwt/JwtTool.java(5 hunks)service-api/src/test/java/greencity/client/RestClientTest.java(4 hunks)service-api/src/test/java/greencity/client/UserRemoteClientConfigTest.java(3 hunks)service-api/src/test/java/greencity/properties/AzurePropertiesTest.java(1 hunks)service-api/src/test/java/greencity/properties/GooglePropertiesTest.java(1 hunks)service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java(1 hunks)service-api/src/test/java/greencity/properties/RemoteWebClientPropertiesTest.java(1 hunks)service-api/src/test/java/greencity/properties/SecurityPropertiesTest.java(1 hunks)service-api/src/test/java/greencity/security/jwt/JwtToolTest.java(2 hunks)service/src/main/java/greencity/config/GoogleApiConfiguration.java(2 hunks)service/src/main/java/greencity/service/CommentServiceImpl.java(0 hunks)service/src/main/java/greencity/service/DatabaseBackupServiceImpl.java(3 hunks)service/src/main/java/greencity/service/NotificationServiceImpl.java(3 hunks)service/src/main/java/greencity/service/OpenAIServiceImpl.java(4 hunks)service/src/test/java/greencity/service/DataBaseBackupServiceImplTest.java(2 hunks)service/src/test/java/greencity/service/NotificationServiceImplTest.java(2 hunks)service/src/test/java/greencity/service/OpenAIServiceImplTest.java(4 hunks)
💤 Files with no reviewable changes (1)
- service/src/main/java/greencity/service/CommentServiceImpl.java
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-06-27T09:17:13.475Z
Learnt from: RostyslavZadyraichuk
Repo: ita-social-projects/GreenCity PR: 8681
File: service/src/main/java/greencity/service/OpenAIServiceImpl.java:162-184
Timestamp: 2025-06-27T09:17:13.475Z
Learning: In the GreenCity project's OpenAIServiceImpl, the user RostyslavZadyraichuk prefers a fail-fast approach for response parsing using try-catch blocks to handle NullPointerException and ClassCastException, converting them to domain-specific OpenAIResponseException. This approach avoids redundant null checks and follows the principle that exceptions should be exceptional, maintaining cleaner code.
Applied to files:
core/src/main/java/greencity/controller/EcoNewsController.javacore/src/test/java/greencity/controller/EcoNewsControllerTest.javaservice/src/test/java/greencity/service/OpenAIServiceImplTest.javaservice/src/main/java/greencity/service/OpenAIServiceImpl.java
📚 Learning: 2025-04-30T21:07:48.800Z
Learnt from: ChernenkoVitaliy
Repo: ita-social-projects/GreenCity PR: 8372
File: core/src/main/java/greencity/controller/ExportSettingsController.java:0-0
Timestamp: 2025-04-30T21:07:48.800Z
Learning: The GreenCity application has a custom Pageable configuration that sets default values (page 0, size 20) when not explicitly specified, making PageableDefault annotation unnecessary.
Applied to files:
core/src/main/java/greencity/controller/EcoNewsController.javacore/src/main/java/greencity/webcontroller/ManagementEventController.javacore/src/main/java/greencity/controller/HabitAssignController.javaservice-api/src/main/java/greencity/client/RestClient.java
📚 Learning: 2025-08-22T15:53:22.505Z
Learnt from: RostyslavZadyraichuk
Repo: ita-social-projects/GreenCity PR: 8931
File: dao/src/main/java/greencity/repository/EcoNewsRelevanceRepo.java:12-22
Timestamp: 2025-08-22T15:53:22.505Z
Learning: When limiting JPQL query results to a fixed number (like "last 10 items"), use Pageable parameter with PageRequest.of(0, N) instead of JPQL LIMIT clause, which is not supported and causes runtime exceptions.
Applied to files:
core/src/main/java/greencity/controller/EcoNewsController.javacore/src/main/java/greencity/controller/HabitAssignController.java
📚 Learning: 2025-03-13T10:15:35.344Z
Learnt from: ChernenkoVitaliy
Repo: ita-social-projects/GreenCity PR: 8234
File: service/src/main/java/greencity/service/ExportSettingsServiceImpl.java:48-53
Timestamp: 2025-03-13T10:15:35.344Z
Learning: The ExportSettingsServiceImpl.getEnvironmentVariables method is designed to return all environment variables, and filtering is not required as the feature is secured through Spring Security and secretKey validation.
Applied to files:
service-api/src/main/java/greencity/properties/SecurityProperties.java
📚 Learning: 2025-06-27T08:41:16.994Z
Learnt from: RostyslavZadyraichuk
Repo: ita-social-projects/GreenCity PR: 8681
File: service/src/main/java/greencity/scheduler/EcoNewsGenerationJob.java:19-26
Timestamp: 2025-06-27T08:41:16.994Z
Learning: In the GreenCity project, the user RostyslavZadyraichuk prefers to handle retry logic and error handling at the service layer (OpenAIService) rather than adding additional error handling at the job level (EcoNewsGenerationJob). The OpenAIService already implements comprehensive retry logic with MAX_REQUEST_ATTEMPTS.
Applied to files:
core/src/test/java/greencity/controller/EcoNewsControllerTest.java
📚 Learning: 2025-03-14T16:22:58.043Z
Learnt from: maks741
Repo: ita-social-projects/GreenCity PR: 8244
File: service/src/main/java/greencity/service/UserNotificationServiceImpl.java:78-123
Timestamp: 2025-03-14T16:22:58.043Z
Learning: In UserNotificationServiceImpl, the use of .limit(2L * page.getPageSize()) when merging notifications from GreenCity and UBS is intentional. This ensures that all notifications from both sources can be accommodated without losing any data during the merging process.
Applied to files:
service-api/src/main/java/greencity/client/RestClient.java
📚 Learning: 2025-01-13T20:48:48.731Z
Learnt from: KostashchukIryna
Repo: ita-social-projects/GreenCity PR: 8022
File: core/src/main/resources/templates/core/header.html:23-25
Timestamp: 2025-01-13T20:48:48.731Z
Learning: In the GreenCity application, the logout endpoint "/management/logout" is configured to use GET method in SecurityConfig, which is the intended behavior for this application.
Applied to files:
core/src/main/java/greencity/webcontroller/ManagementController.java
🧬 Code graph analysis (10)
core/src/test/java/greencity/webcontroller/ManagementControllerTest.java (1)
core/src/test/java/greencity/TestConst.java (1)
TestConst(3-16)
service-api/src/main/java/greencity/properties/SecurityProperties.java (4)
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
Component(18-98)service-api/src/main/java/greencity/properties/AzureProperties.java (1)
Component(18-48)service-api/src/main/java/greencity/properties/GoogleProperties.java (1)
Component(11-41)service-api/src/main/java/greencity/properties/OpenAiProperties.java (1)
Component(11-41)
service-api/src/test/java/greencity/properties/AzurePropertiesTest.java (4)
service-api/src/test/java/greencity/properties/GooglePropertiesTest.java (1)
ExtendWith(17-106)service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java (1)
ExtendWith(17-97)service-api/src/test/java/greencity/properties/RemoteWebClientPropertiesTest.java (1)
ExtendWith(17-216)service-api/src/test/java/greencity/properties/SecurityPropertiesTest.java (1)
ExtendWith(17-102)
service-api/src/main/java/greencity/properties/OpenAiProperties.java (4)
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
Component(18-98)service-api/src/main/java/greencity/properties/AzureProperties.java (1)
Component(18-48)service-api/src/main/java/greencity/properties/GoogleProperties.java (1)
Component(11-41)service-api/src/main/java/greencity/properties/SecurityProperties.java (1)
Component(11-42)
service-api/src/main/java/greencity/properties/AzureProperties.java (4)
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
Component(18-98)service-api/src/main/java/greencity/properties/GoogleProperties.java (1)
Component(11-41)service-api/src/main/java/greencity/properties/OpenAiProperties.java (1)
Component(11-41)service-api/src/main/java/greencity/properties/SecurityProperties.java (1)
Component(11-42)
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (2)
service/src/main/java/greencity/service/NotificationServiceImpl.java (1)
Slf4j(48-414)service-api/src/main/java/greencity/client/RestClient.java (1)
Component(48-456)
service-api/src/main/java/greencity/properties/GoogleProperties.java (4)
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
Component(18-98)service-api/src/main/java/greencity/properties/AzureProperties.java (1)
Component(18-48)service-api/src/main/java/greencity/properties/OpenAiProperties.java (1)
Component(11-41)service-api/src/main/java/greencity/properties/SecurityProperties.java (1)
Component(11-42)
service-api/src/test/java/greencity/properties/GooglePropertiesTest.java (4)
service-api/src/test/java/greencity/properties/AzurePropertiesTest.java (1)
ExtendWith(17-105)service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java (1)
ExtendWith(17-97)service-api/src/test/java/greencity/properties/RemoteWebClientPropertiesTest.java (1)
ExtendWith(17-216)service-api/src/test/java/greencity/properties/SecurityPropertiesTest.java (1)
ExtendWith(17-102)
service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java (4)
service-api/src/test/java/greencity/properties/AzurePropertiesTest.java (1)
ExtendWith(17-105)service-api/src/test/java/greencity/properties/GooglePropertiesTest.java (1)
ExtendWith(17-106)service-api/src/test/java/greencity/properties/RemoteWebClientPropertiesTest.java (1)
ExtendWith(17-216)service-api/src/test/java/greencity/properties/SecurityPropertiesTest.java (1)
ExtendWith(17-102)
service-api/src/test/java/greencity/properties/RemoteWebClientPropertiesTest.java (4)
service-api/src/test/java/greencity/properties/AzurePropertiesTest.java (1)
ExtendWith(17-105)service-api/src/test/java/greencity/properties/GooglePropertiesTest.java (1)
ExtendWith(17-106)service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java (1)
ExtendWith(17-97)service-api/src/test/java/greencity/properties/SecurityPropertiesTest.java (1)
ExtendWith(17-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (26)
service/src/main/java/greencity/config/GoogleApiConfiguration.java (2)
20-21: Clean delegation to property accessor.The method correctly delegates API key retrieval to
googleProperties.getGoogleApiKey(), maintaining the same functionality while benefiting from centralized property management. This allows theGooglePropertiesclass to handle validation, potential caching, or secret refresh logic transparently.
4-12: Verification confirmed—no issues found. All changes are sound.The investigation confirms that
GooglePropertiesis properly configured as a Spring@Componentand includes robust validation ingetGoogleApiKey(). The method validates against null/empty values and throwsIllegalStateExceptionwith a meaningful error message if the property is missing. Additionally,@PostConstructensures validation runs immediately upon bean initialization, catching configuration issues early.The constructor injection pattern in
GoogleApiConfigurationis correctly implemented and aligns perfectly with the centralized property management approach that enables future Azure Key Vault integration. The refactoring maintains clean, testable code while improving the architecture.core/src/main/java/greencity/controller/EcoNewsController.java (1)
70-70: LGTM!Good refactoring from
@Valueinjection to constructor-injected property class. This improves testability and centralizes configuration management.core/src/test/java/greencity/controller/EcoNewsControllerTest.java (1)
105-105: LGTM!Reducing visibility from
publicto package-private for thesetUpmethod is appropriate. The@BeforeEachannotation doesn't require public visibility.core/src/test/java/greencity/webcontroller/ManagementControllerTest.java (1)
30-31: LGTM!Excellent implementation of the property mock. The
RemoteWebClientPropertiesmock is properly configured in the test, providing the expected server address value. This is a good pattern for other test files to follow.Also applies to: 62-63
service-api/src/test/java/greencity/client/UserRemoteClientConfigTest.java (1)
33-34: LGTM!Excellent and comprehensive mock setup. All required properties from
RemoteWebClientPropertiesare properly configured:
- Server address
- Connection timeout
- Response timeout
- System email address
This is a great reference for how the property mocks should be configured in test files.
Also applies to: 57-61
service-api/src/test/java/greencity/security/jwt/JwtToolTest.java (1)
50-54: LGTM! Clean refactor to use SecurityProperties mock.The migration from ReflectionTestUtils to mocking SecurityProperties is a cleaner approach that better reflects production dependency injection. The test setup now explicitly documents the required property values through the mock configuration.
service/src/test/java/greencity/service/NotificationServiceImplTest.java (1)
82-83: LGTM! Mock added to support refactored service.The RemoteWebClientProperties mock correctly supports the NotificationServiceImpl's refactored dependency injection pattern. The test setup remains clean and focused.
core/src/main/java/greencity/controller/HabitAssignController.java (2)
66-66: LGTM! Clean dependency injection pattern.The RemoteWebClientProperties field follows proper constructor injection via Lombok's @requiredargsconstructor. This centralizes configuration management as intended by the PR.
745-751: Code validation is properly implemented.Verification confirms that
RemoteWebClientPropertiesincludes a@PostConstruct validateProperties()method that explicitly validates theclient.addressproperty at application startup. ThegetClientAddress()method checks that the property is non-empty and throwsIllegalStateExceptionwith a descriptive error if missing. This prevents runtime failures when the endpoint constructs the redirect URL. The implementation is also covered by comprehensive unit tests confirming both success and failure scenarios.service/src/main/java/greencity/service/NotificationServiceImpl.java (2)
61-61: LGTM! Proper dependency injection.The RemoteWebClientProperties field is correctly injected via constructor, following the established pattern in this PR.
407-413: LGTM! URL construction updated correctly.The createBaseLink method now uses
remoteWebClientProperties.getClientAddress()consistently for both notification types. The conditional logic for EVENT_COMMENT vs. default notifications remains intact.core/src/main/java/greencity/webcontroller/ManagementController.java (1)
47-59: LGTM! Redirect logic correctly updated.The login method now uses
remoteWebClientProperties.getGreencityUserServerAddress()to construct the redirect URL, properly centralizing the configuration.service/src/main/java/greencity/service/DatabaseBackupServiceImpl.java (2)
42-42: LGTM! Proper dependency injection.The AzureProperties field follows the correct constructor injection pattern with final modifier.
162-179: No action required—Azure properties are already validated at startup.Verification confirms that AzureProperties correctly implements startup validation through a
@PostConstructmethod that validates bothazure.connection.stringandazure.container.nameproperties. Both getters throwIllegalStateExceptionwith descriptive error messages if properties are missing, ensuring fail-fast behavior. Comprehensive unit tests verify this validation logic works as expected. The code inDatabaseBackupServiceImplis safe to use these properties.service-api/src/test/java/greencity/properties/RemoteWebClientPropertiesTest.java (1)
1-216: LGTM! Comprehensive test coverage.The test class provides thorough coverage of RemoteWebClientProperties:
- All seven getters are tested for both success and failure paths
- Validation logic is tested for success and failure scenarios
- LogCaptor properly verifies error and info logs
- Test structure is consistent with other property test classes in the PR
The tests follow good practices with clear arrange-act-assert patterns and meaningful assertions.
service/src/test/java/greencity/service/OpenAIServiceImplTest.java (1)
69-76: LGTM! Consistent refactor to use OpenAiProperties mock.The test setup correctly migrates from ReflectionTestUtils to mocking OpenAiProperties for the API key, aligning with the broader PR pattern. The @MockitoSettings(strictness = Strictness.LENIENT) annotation is appropriate for this test suite's complexity. Other properties remain set via ReflectionTestUtils, which is acceptable as they may not be externalized in the current scope.
service-api/src/main/java/greencity/properties/GoogleProperties.java (1)
1-41: Consistent Google properties validation componentThis class fits neatly into the shared properties pattern: constructor-injected
Environment,@PostConstructvalidation, and clear error signaling via specificErrorMessageconstants. The behavior is straightforward and matches the intended refactor away from scattered@Valueusage.service-api/src/main/java/greencity/security/jwt/JwtTool.java (1)
9-9: LGTM! Clean migration to centralized properties.The refactoring successfully replaces scattered
@Valueannotations with dependency injection ofSecurityProperties, improving maintainability and testability. All usages correctly delegate to the properties bean.Also applies to: 39-39, 45-49, 85-85, 91-91, 102-102, 170-170
service/src/main/java/greencity/service/OpenAIServiceImpl.java (1)
10-10: LGTM! Consistent property migration.The migration from direct
@Valueinjection toOpenAiPropertiesaligns with the centralized configuration pattern. All API key references properly delegate to the properties bean.Also applies to: 50-50, 52-62, 237-242, 258-270
core/src/main/java/greencity/webcontroller/ManagementEventController.java (1)
17-18: LGTM! Controller properly refactored to use centralized properties.The migration from
@Valueannotations toRemoteWebClientPropertiesandGooglePropertiesimproves configuration management. All usages are correctly updated throughout the controller methods.Also applies to: 91-92, 129-129, 193-195, 245-248
service-api/src/main/java/greencity/client/config/UserRemoteClientConfig.java (1)
15-15: LGTM! WebClient configuration properly centralized.All configuration values (base URL, timeouts, system email) are now sourced from
RemoteWebClientProperties, eliminating scattered@Valueannotations and improving maintainability.Also applies to: 38-38, 42-55, 57-70
service-api/src/test/java/greencity/properties/GooglePropertiesTest.java (1)
1-106: LGTM! Comprehensive test coverage.The test class provides thorough coverage of
GooglePropertiesbehavior, including success cases, missing property exceptions, and logging validation. The structure is consistent with other property test classes in the PR.service-api/src/test/java/greencity/properties/SecurityPropertiesTest.java (1)
1-102: LGTM! Well-structured security property tests.The test class provides comprehensive coverage of
SecurityProperties, including token key and expiration validation. The structure is consistent with other property test classes, and all scenarios are properly tested.service-api/src/test/java/greencity/properties/AzurePropertiesTest.java (1)
1-105: LGTM! Thorough Azure property testing.The test class provides complete coverage of
AzurePropertiesbehavior, including connection string and container name validation. The testing approach is consistent with other property classes in this PR.service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
24-34: Fail-fast property validation is helpful; ensure all profiles define required keys
validateProperties()eagerly resolves all required properties at startup and fails fast with clear error messages if any are missing. That’s a good operational choice for critical configuration like base URLs, client address, timeouts, and system email, and it aligns with the new error constants.Because this runs in
@PostConstruct, any missing property will prevent the application context from starting. Please double-check that all active Spring profiles (including test and local dev) define these keys, otherwise some contexts may start failing unexpectedly.
core/src/main/java/greencity/webcontroller/ManagementController.java
Outdated
Show resolved
Hide resolved
core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java
Show resolved
Hide resolved
service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java
Show resolved
Hide resolved
service/src/test/java/greencity/service/DataBaseBackupServiceImplTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
service-api/src/main/java/greencity/properties/OpenAiProperties.java (1)
33-40: Consider using Boolean type for the relevance flag.The property name
greencity.relevance.enabledfollows the convention for boolean flags, but the method returns aString. This creates ambiguity about valid values and requires string comparison in consuming code.Consider refactoring to use a boolean type for type safety and clarity:
-public String getRelevance() { - String relevance = environment.getProperty("greencity.relevance.enabled"); - if (!StringUtils.hasText(relevance)) { - log.error(ErrorMessage.RELEVANCE_NOT_SET); - throw new IllegalStateException(ErrorMessage.RELEVANCE_NOT_SET); - } - return relevance; +public Boolean getRelevance() { + Boolean relevance = environment.getProperty("greencity.relevance.enabled", Boolean.class); + if (relevance == null) { + log.error(ErrorMessage.RELEVANCE_NOT_SET); + throw new IllegalStateException(ErrorMessage.RELEVANCE_NOT_SET); + } + return relevance; }This aligns with Spring Boot conventions where
.enabledproperties are typically boolean, and makes the intent clearer to maintainers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/main/java/greencity/webcontroller/ManagementController.java(3 hunks)core/src/test/java/greencity/controller/EcoNewsControllerTest.java(4 hunks)core/src/test/java/greencity/controller/HabitAssignControllerTest.java(3 hunks)core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java(2 hunks)service-api/pom.xml(1 hunks)service-api/src/main/java/greencity/client/RestClient.java(20 hunks)service-api/src/main/java/greencity/properties/OpenAiProperties.java(1 hunks)service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java(1 hunks)service/src/test/java/greencity/service/DataBaseBackupServiceImplTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- service-api/pom.xml
- service-api/src/test/java/greencity/properties/OpenAiPropertiesTest.java
- service-api/src/main/java/greencity/client/RestClient.java
- service/src/test/java/greencity/service/DataBaseBackupServiceImplTest.java
- core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java
- core/src/test/java/greencity/controller/HabitAssignControllerTest.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-27T09:17:13.475Z
Learnt from: RostyslavZadyraichuk
Repo: ita-social-projects/GreenCity PR: 8681
File: service/src/main/java/greencity/service/OpenAIServiceImpl.java:162-184
Timestamp: 2025-06-27T09:17:13.475Z
Learning: In the GreenCity project's OpenAIServiceImpl, the user RostyslavZadyraichuk prefers a fail-fast approach for response parsing using try-catch blocks to handle NullPointerException and ClassCastException, converting them to domain-specific OpenAIResponseException. This approach avoids redundant null checks and follows the principle that exceptions should be exceptional, maintaining cleaner code.
Applied to files:
core/src/test/java/greencity/controller/EcoNewsControllerTest.java
📚 Learning: 2025-06-27T08:41:16.994Z
Learnt from: RostyslavZadyraichuk
Repo: ita-social-projects/GreenCity PR: 8681
File: service/src/main/java/greencity/scheduler/EcoNewsGenerationJob.java:19-26
Timestamp: 2025-06-27T08:41:16.994Z
Learning: In the GreenCity project, the user RostyslavZadyraichuk prefers to handle retry logic and error handling at the service layer (OpenAIService) rather than adding additional error handling at the job level (EcoNewsGenerationJob). The OpenAIService already implements comprehensive retry logic with MAX_REQUEST_ATTEMPTS.
Applied to files:
core/src/test/java/greencity/controller/EcoNewsControllerTest.java
📚 Learning: 2025-01-13T20:48:48.731Z
Learnt from: KostashchukIryna
Repo: ita-social-projects/GreenCity PR: 8022
File: core/src/main/resources/templates/core/header.html:23-25
Timestamp: 2025-01-13T20:48:48.731Z
Learning: In the GreenCity application, the logout endpoint "/management/logout" is configured to use GET method in SecurityConfig, which is the intended behavior for this application.
Applied to files:
core/src/main/java/greencity/webcontroller/ManagementController.java
🧬 Code graph analysis (1)
service-api/src/main/java/greencity/properties/OpenAiProperties.java (4)
service-api/src/main/java/greencity/properties/RemoteWebClientProperties.java (1)
Component(18-98)service-api/src/main/java/greencity/properties/AzureProperties.java (1)
Component(18-48)service-api/src/main/java/greencity/properties/GoogleProperties.java (1)
Component(11-41)service-api/src/main/java/greencity/properties/SecurityProperties.java (1)
Component(11-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
core/src/main/java/greencity/webcontroller/ManagementController.java (2)
15-15: Good implementation with final modifier.The field is properly declared as
private final, ensuring immutability and making the constructor injection contract explicit. This addresses previous feedback and aligns with best practices for dependency injection.
50-54: Code change is verified and approved.The
RemoteWebClientProperties.getGreencityUserServerAddress()method properly validates the configuration property on application startup via@PostConstruct, throwing anIllegalStateExceptionif the property is missing or empty. TheUriComponentsBuilderapproach correctly handles URL construction with proper path appending, making the refactoring to centralized configuration a solid improvement.service-api/src/main/java/greencity/properties/OpenAiProperties.java (3)
11-15: LGTM! Consistent pattern with other property classes.The class structure follows the established pattern used in RemoteWebClientProperties, AzureProperties, GoogleProperties, and SecurityProperties. Using
@Componentwith constructor injection via@RequiredArgsConstructoris clean and idiomatic.
17-22: LGTM! Fail-fast validation at startup.The
@PostConstructvalidation ensures properties are validated when the bean is initialized, preventing runtime failures later. This matches the pattern used in other property classes.
24-31: LGTM! Proper validation and error handling.The method correctly validates the property existence using
StringUtils.hasText()and provides clear error messages. The pattern is consistent with other property accessor methods in the codebase.core/src/test/java/greencity/controller/EcoNewsControllerTest.java (2)
22-22: LGTM! Mock field properly added.The import and mock field for
OpenAiPropertiesare correctly added to support the controller's new dependency.Also applies to: 89-89
120-121: Mock configuration addresses the past review comment.The
openAiProperties.getRelevance()mock is now properly configured in thesetUp()method, which resolves the concern raised in the previous review. All tests using relevance checks will now have the expected behavior.Note: The mock returns
"enabled"as a String, which is consistent with the currentOpenAiProperties.getRelevance()API. If that method is refactored to return a Boolean (as suggested in the OpenAiProperties review), this test setup will need to be updated accordingly.
|
SashaPog
deleted the
feature/9272/automatic-secret-refresh-from-azure
branch
Issue
9272
Summary by CodeRabbit