Fix MCEDT/HCV keystore race condition causing upload failures#2228
Fix MCEDT/HCV keystore race condition causing upload failures#2228yingbull merged 6 commits intohotfix/01232026from
Conversation
…re instance-based The static `clientKeystore` variable in EdtClientBuilder was shared between MCEDT and HCV services, causing a race condition where HCV validation would overwrite the MCEDT keystore path. This resulted in MCEDT upload failures after HCV validation operations.
Enhance JavaDoc documentation for setClientKeystoreFilename and setExternalClientKeystoreFilename methods to comply with CLAUDE.md standards. Added detailed descriptions, @param tags with specific data types, and @SInCE tags based on git history. - EdtClientBuilder.setClientKeystoreFilename: Added context about instance-based keystore paths preventing race conditions - DelegateFactory/OnlineHCValidator/EDTBaseTest: Converted C-style comments to proper JavaDoc with comprehensive parameter documentation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Reviewer's GuideConverts the EDT client keystore configuration from a static, shared setting to an instance-level setting on EdtClientBuilder, and updates MCEDT and HCV callers (plus tests) to pass and configure the builder explicitly so each service uses its own keystore and avoids race conditions. Sequence diagram for independent MCEDT and HCV keystore configurationsequenceDiagram
actor User
participant MCEDTService as MCEDTService
participant HCVService as HCVService
participant DelegateFactory as DelegateFactory
participant OnlineHCValidator as OnlineHCValidator
participant EdtClientBuilder_MCEDT as EdtClientBuilder_MCEDT
participant EdtClientBuilder_HCV as EdtClientBuilder_HCV
User->>MCEDTService: triggerUpload()
MCEDTService->>DelegateFactory: newDelegate(serviceId)
DelegateFactory->>EdtClientBuilder_MCEDT: new EdtClientBuilder(config)
DelegateFactory->>DelegateFactory: setExternalClientKeystoreFilename(EdtClientBuilder_MCEDT, mcedtKeystorePath)
DelegateFactory->>EdtClientBuilder_MCEDT: setClientKeystoreFilename(mcedtKeystoreUrl)
DelegateFactory->>EdtClientBuilder_MCEDT: build(EDTDelegate)
User->>HCVService: validateHealthCard()
HCVService->>OnlineHCValidator: new OnlineHCValidator()
OnlineHCValidator->>EdtClientBuilder_HCV: new EdtClientBuilder(config)
OnlineHCValidator->>OnlineHCValidator: setExternalClientKeystoreFilename(EdtClientBuilder_HCV, hcvKeystorePath)
OnlineHCValidator->>EdtClientBuilder_HCV: setClientKeystoreFilename(hcvKeystoreUrl)
OnlineHCValidator->>EdtClientBuilder_HCV: build(HCValidation)
Note over EdtClientBuilder_MCEDT,EdtClientBuilder_HCV: Each EdtClientBuilder instance now maintains its own clientKeystore value, eliminating cross-service race conditions.
Updated class diagram for EdtClientBuilder and callersclassDiagram
class EdtClientBuilder {
- static AtomicBoolean isInitialized
- static String DEFAULT_CLIENT_KEYSTORE
- String clientKeystore
- EdtClientBuilderConfig config
+ EdtClientBuilder(EdtClientBuilderConfig config)
+ void setClientKeystoreFilename(String filename)
+ <T> T build(Class serviceClass)
+ static void configureSsl(HTTPConduit conduit)
}
class DelegateFactory {
- static Properties props
- static UserPropertyDAO userPropertyDAO
+ static EDTDelegate newDelegate(String serviceId)
+ static void setUserPropertyDAO(UserPropertyDAO userPropertyDAO)
- static void setExternalClientKeystoreFilename(EdtClientBuilder builder, String clientKeystorePropertiesPath)
}
class OnlineHCValidator {
- static Logger logger
- EdtClientBuilder builder
- HCValidation validation
+ OnlineHCValidator()
- void setBuilder(EdtClientBuilder builder)
- static void setExternalClientKeystoreFilename(EdtClientBuilder builder, String clientKeystorePropertiesPath)
}
class EDTDelegate
class HCValidation
class EdtClientBuilderConfig
class HTTPConduit
class UserPropertyDAO
DelegateFactory --> EdtClientBuilder : creates
DelegateFactory --> EDTDelegate : builds
OnlineHCValidator --> EdtClientBuilder : creates
OnlineHCValidator --> HCValidation : builds
EdtClientBuilder --> EdtClientBuilderConfig : uses
EdtClientBuilder ..> HTTPConduit : configureSsl()
DelegateFactory ..> UserPropertyDAO : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe changes refactor keystore configuration in EdtClientBuilder from static to instance-scoped members. The clientKeystore field and setClientKeystoreFilename method are converted from class-level to instance-level, with corresponding updates to call sites across DelegateFactory, OnlineHCValidator, and EDTBaseTest to pass builder instances when configuring keystores. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The three
setExternalClientKeystoreFilenamehelpers inDelegateFactory,OnlineHCValidator, andEDTBaseTestnow share almost identical logic; consider extracting a common utility or adding aEdtClientBuilder.setClientKeystoreFromPath(String)method to avoid duplication and keep the behavior consistent in one place. - The
setExternalClientKeystoreFilenamemethods are stillstaticbut now mutate the passed-inEdtClientBuilderinstance; it may be clearer and less error-prone to make these instance-level helpers (or move the logic ontoEdtClientBuilder) to better reflect the instance-scoped keystore configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The three `setExternalClientKeystoreFilename` helpers in `DelegateFactory`, `OnlineHCValidator`, and `EDTBaseTest` now share almost identical logic; consider extracting a common utility or adding a `EdtClientBuilder.setClientKeystoreFromPath(String)` method to avoid duplication and keep the behavior consistent in one place.
- The `setExternalClientKeystoreFilename` methods are still `static` but now mutate the passed-in `EdtClientBuilder` instance; it may be clearer and less error-prone to make these instance-level helpers (or move the logic onto `EdtClientBuilder`) to better reflect the instance-scoped keystore configuration.
## Individual Comments
### Comment 1
<location> `src/main/java/ca/openosp/openo/integration/mchcv/OnlineHCValidator.java:139-142` </location>
<code_context>
+ * @since 2025-08-26
*/
- private static void setExternalClientKeystoreFilename(String clientKeystorePropertiesPath) {
+ private static void setExternalClientKeystoreFilename(EdtClientBuilder builder, String clientKeystorePropertiesPath) {
if (clientKeystorePropertiesPath == null) {
return;
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider treating empty/blank paths like null to avoid accidentally using the current directory as a keystore path.
Because only `null` is treated as "no override", an empty or whitespace value will still call `Files.exists` with `""`, which resolves to the current directory and almost always exists. That directory would then be used as the keystore location, likely failing in a confusing way. Please treat empty/blank values like `null` and return before calling `Files.exists`.
</issue_to_address>
### Comment 2
<location> `src/main/java/ca/openosp/openo/integration/mcedt/DelegateFactory.java:118-121` </location>
<code_context>
+ * @since 2025-08-26
*/
- private static void setExternalClientKeystoreFilename(String clientKeystorePropertiesPath) {
+ private static void setExternalClientKeystoreFilename(EdtClientBuilder builder, String clientKeystorePropertiesPath) {
if (clientKeystorePropertiesPath == null) {
return;
</code_context>
<issue_to_address>
**issue:** Align handling of empty/blank keystore paths with the documented "null or does not exist" behavior.
If this property is set to an empty or whitespace-only string, `Files.exists` will likely succeed on `.` and we’ll end up using a directory URL as the keystore path, contrary to the Javadoc’s "null or does not exist" contract. Adding a `trim().isEmpty()` check alongside the null check would align behavior with the docs and prevent this misconfiguration.
</issue_to_address>
### Comment 3
<location> `src/test/java/ca/openosp/openo/integration/mcedt/EDTBaseTest.java:340` </location>
<code_context>
config.setMtomEnabled(true);
EdtClientBuilder builder = new EdtClientBuilder(config);
- setExternalClientKeystoreFilename(props.getProperty("mcedt.service.clientKeystore.properties"));
+ setExternalClientKeystoreFilename(builder, props.getProperty("mcedt.service.clientKeystore.properties"));
EDTDelegate edtDelegate = builder.build(EDTDelegate.class);
if (logger.isInfoEnabled()) {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that proves keystore configuration is isolated per EdtClientBuilder instance (regression test for the race condition).
Right now the test only updates the helper signature and doesn’t validate the new per‑builder behavior. Please add a regression test (here or in a test class near `EdtClientBuilder`) that, for example:
- Instantiates two `EdtClientBuilder`s with different keystore property paths.
- Calls `setExternalClientKeystoreFilename` on each builder separately.
- Verifies (via the built delegates or exposed configuration) that changing the keystore on one builder does not affect the other.
This ensures the test would fail if `clientKeystore` were ever made static again.
Suggested implementation:
```java
EDTDelegate edtDelegate = builder.build(EDTDelegate.class);
if (logger.isInfoEnabled()) {
logger.info("Created new EDT delegate " + edtDelegate);
return edtDelegate;
}
@Test
public void clientKeystoreConfigurationIsIsolatedPerBuilderInstance() throws Exception {
// Arrange: create two independent EDT configurations and builders
EDTConfiguration config1 = new EDTConfiguration();
config1.setMtomEnabled(true);
EDTConfiguration config2 = new EDTConfiguration();
config2.setMtomEnabled(true);
EdtClientBuilder builder1 = new EdtClientBuilder(config1);
EdtClientBuilder builder2 = new EdtClientBuilder(config2);
// Act: configure different external client keystore files on each builder
setExternalClientKeystoreFilename(builder1, "test-keystore-1.properties");
setExternalClientKeystoreFilename(builder2, "test-keystore-2.properties");
// Assert: client keystore configuration is not shared between builders
java.lang.reflect.Field clientKeystoreField = EdtClientBuilder.class.getDeclaredField("clientKeystore");
clientKeystoreField.setAccessible(true);
Object clientKeystore1 = clientKeystoreField.get(builder1);
Object clientKeystore2 = clientKeystoreField.get(builder2);
org.junit.Assert.assertNotNull("First builder should have client keystore configured", clientKeystore1);
org.junit.Assert.assertNotNull("Second builder should have client keystore configured", clientKeystore2);
org.junit.Assert.assertNotSame("Client keystore must be isolated per builder instance", clientKeystore1, clientKeystore2);
}
/**
* Set an external client keystore properties file for the EDT client builder in test context.
* This method configures a custom keystore properties file path for MCEDT service
```
Assumptions and potential adjustments:
1. This test assumes `EDTConfiguration` has a no-arg constructor and a `setMtomEnabled(boolean)` method, and that `EdtClientBuilder` has a constructor accepting `EDTConfiguration` — both are implied by the existing code snippet. If additional configuration is required to build a valid `EdtClientBuilder` in your environment (e.g., `setServiceId` or other mandatory fields), set those on `config1` and `config2` in the test as well.
2. The test uses `@Test` and fully qualified `org.junit.Assert` to avoid modifying imports. Ensure the class already uses JUnit (which it appears to) and that the appropriate JUnit dependency is present.
3. The test relies on the existence of a non-static field named `clientKeystore` in `EdtClientBuilder`. If the field name differs, update `"clientKeystore"` accordingly. The test will fail if that field is made `static`, providing the regression protection you requested.
</issue_to_address>
### Comment 4
<location> `src/test/java/ca/openosp/openo/integration/mcedt/EDTBaseTest.java:359-362` </location>
<code_context>
+ * @since 2025-08-26
*/
- protected static void setExternalClientKeystoreFilename(String clientKeystorePropertiesPath) {
+ protected static void setExternalClientKeystoreFilename(EdtClientBuilder builder, String clientKeystorePropertiesPath) {
if (clientKeystorePropertiesPath == null) {
return;
</code_context>
<issue_to_address>
**suggestion (testing):** Extend tests to cover null and non-existent keystore path handling in the test helper.
The helper’s Javadoc promises that a null or non-existent keystore path falls back to the default keystore. Currently it returns early on null and only proceeds when `Files.exists(...)` is true, but there are no tests covering these cases. Please add tests that:
- Call the helper with `clientKeystorePropertiesPath == null` and verify it does not fail and still uses the default keystore (or leaves the configuration unchanged).
- Call it with a path to a non-existent file and verify the default keystore is still used.
This will tie the behavior to the documented contract and guard against regressions.
Suggested implementation:
```java
* This method configures a custom keystore properties file path for MCEDT service
* client certificate authentication during testing. If the provided path is null or the
* file does not exist, the default keystore at src/main/resources/clientKeystore.properties
* will be used.
*
* @param builder EdtClientBuilder the EDT client builder instance to configure
* @param clientKeystorePropertiesPath String the absolute path to the client keystore properties file, or null to use default
* @since 2025-08-26
*/
protected static void setExternalClientKeystoreFilename(EdtClientBuilder builder, String clientKeystorePropertiesPath) {
if (clientKeystorePropertiesPath == null) {
return;
}
if (Files.exists(signaturePropFile)) {
File file = new File(clientKeystorePropertiesPath);
try {
builder.setClientKeystoreFilename(file.toURI().toURL().toString());
} catch (MalformedURLException e) {
logger.error("Malformed URL: " + clientKeystorePropertiesPath, e);
}
}
}
/**
* Tests for {@link #setExternalClientKeystoreFilename(EdtClientBuilder, String)} to verify
* that null and non-existent keystore paths leave the builder configuration unchanged and
* thus fall back to the default keystore, as documented in the method contract.
*/
@Test
void setExternalClientKeystoreFilename_nullPath_usesDefaultKeystore() {
// given
EdtClientBuilder builder = Mockito.mock(EdtClientBuilder.class);
// when
setExternalClientKeystoreFilename(builder, null);
// then
Mockito.verify(builder, Mockito.never()).setClientKeystoreFilename(Mockito.anyString());
}
@Test
void setExternalClientKeystoreFilename_nonExistentPath_usesDefaultKeystore() {
// given
EdtClientBuilder builder = Mockito.mock(EdtClientBuilder.class);
// use a path that does not exist
Path nonExistentPath = Paths.get("target", "does-not-exist", "clientKeystore.properties");
assertFalse(Files.exists(nonExistentPath), "Test precondition failed: non-existent path unexpectedly exists");
// when
setExternalClientKeystoreFilename(builder, nonExistentPath.toAbsolutePath().toString());
// then
Mockito.verify(builder, Mockito.never()).setClientKeystoreFilename(Mockito.anyString());
}
```
To make the above test methods compile and integrate cleanly, you will also need to:
1. **Ensure the tests are inside the EDTBaseTest class body**
- The replacement block above assumes that:
- The `setExternalClientKeystoreFilename` method is inside `EDTBaseTest`.
- The two new `@Test` methods are also members of the same class.
- If your actual method or class structure differs (e.g. the method is in a nested helper class), move the test methods so that they remain in a JUnit test class that can access `setExternalClientKeystoreFilename`.
2. **Add the necessary imports at the top of `EDTBaseTest.java` (or adjust if they already exist):**
```java
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertFalse;
import org.mockito.Mockito;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
```
- If your project uses JUnit 4 instead of JUnit 5:
- Replace `org.junit.jupiter.api.Test` with `org.junit.Test`.
- Replace `import static org.junit.jupiter.api.Assertions.assertFalse;` with `import static org.junit.Assert.assertFalse;`.
- If Mockito is already statically imported (e.g. `import static org.mockito.Mockito.*;`), you can simplify the calls accordingly.
3. **Keep consistency with your existing test style**
- If `EDTBaseTest` uses a different mocking framework or manually constructed `EdtClientBuilder` test doubles instead of Mockito, replace the `Mockito.mock(...)` and verification calls with the equivalent in your codebase.
- If you prefer to verify the builder state via a getter instead of interaction-based testing, you can replace:
```java
Mockito.verify(builder, Mockito.never()).setClientKeystoreFilename(Mockito.anyString());
```
with assertions on the builder’s internal state (e.g. checking that the client keystore filename is still the default or `null`).
4. **If `signaturePropFile` is not actually derived from the passed path**
- The non-existent path test assumes the `Files.exists(...)` check in `setExternalClientKeystoreFilename` ultimately uses the passed in `clientKeystorePropertiesPath` (directly or via `signaturePropFile`).
- If `signaturePropFile` is currently a constant or derived differently, you may need to:
- Refactor the method so that `signaturePropFile` is constructed from `clientKeystorePropertiesPath` (e.g. `Paths.get(clientKeystorePropertiesPath)`), or
- Adapt the test to use whatever path the method actually checks for existence.
- Align the implementation so that the behavior tested (null and non-existent paths falling back to the default keystore) matches the method’s Javadoc exactly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical race condition in EDT client keystore configuration that was causing MCEDT document upload failures after HCV health card validation. The issue stemmed from clientKeystore being a static variable shared across all EdtClientBuilder instances, allowing one service (HCV) to overwrite the keystore configuration of another service (MCEDT).
Changes:
- Changed
clientKeystorefrom static to instance variable inEdtClientBuilder, allowing each service to maintain its own independent keystore configuration - Updated
setClientKeystoreFilename()from static to instance method to support per-instance configuration - Modified all call sites (DelegateFactory, OnlineHCValidator, EDTBaseTest) to pass the builder instance to the keystore configuration method
- Enhanced documentation from C-style comments to proper JavaDoc with detailed parameter descriptions and context
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/main/java/ca/openosp/openo/integration/ebs/client/ng/EdtClientBuilder.java |
Converted clientKeystore from static to instance variable and updated setClientKeystoreFilename() to instance method with improved JavaDoc |
src/main/java/ca/openosp/openo/integration/mcedt/DelegateFactory.java |
Updated to pass builder instance to setExternalClientKeystoreFilename() and enhanced method documentation |
src/main/java/ca/openosp/openo/integration/mchcv/OnlineHCValidator.java |
Updated to pass HCV builder instance to setExternalClientKeystoreFilename() and enhanced method documentation |
src/test/java/ca/openosp/openo/integration/mcedt/EDTBaseTest.java |
Updated test helper method to accept builder parameter and enhanced documentation |
Code Review❌ CLAUDE.md Compliance Issue: Incorrect @SInCE DatesThe
Affected locations: Required fix: Change all instances of ✅ Code Quality: No Bugs FoundThe implementation correctly fixes the race condition by converting Generated with Claude Code |
|
@claude Will you commit the suggestion you gave about the date? Also, please follow all the Sourcery suggestions in separate commits. |
|
Claude finished @D3V41's task in 2m 56s —— View job Tasks Completed ✅
SummaryI've successfully implemented all requested changes in 4 separate commits: Commit 1: Documentation Fix (
Commit 2: Bug Fix (
Commit 3: Regression Test (
Commit 4: Test Coverage (
All changes have been pushed to the | |
Per CLAUDE.md documentation standards, @SInCE tags must use accurate dates from git history. Updated all keystore configuration method JavaDoc to reflect the actual commit date of these changes. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
…ration Added validation to check for empty or whitespace-only strings in addition to null checks. This prevents Files.exists() from resolving to the current directory when an empty string is provided, which would cause authentication failures with confusing error messages. Addresses Sourcery code review comments on DelegateFactory.java and OnlineHCValidator.java. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
Added test to verify that EdtClientBuilder instances maintain independent keystore configurations. This prevents regression to the previous race condition where a static clientKeystore variable was shared across all instances, causing MCEDT uploads to fail after HCV validation. The test uses reflection to verify that two builder instances can be configured with different keystore paths without interfering with each other. If clientKeystore were ever made static again, this test would fail. Addresses Sourcery code review suggestion for regression test coverage. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
Added tests to verify that setExternalClientKeystoreFilename correctly handles edge cases where the keystore path is null or points to a non-existent file. Both cases should result in the default keystore being used, per the method's documented contract. These tests ensure that: 1. Null paths do not modify the builder's keystore configuration 2. Non-existent file paths do not modify the builder's keystore configuration 3. The behavior matches the JavaDoc promise of falling back to the default keystore at src/main/resources/clientKeystore.properties Addresses Sourcery code review suggestion for test coverage of documented fallback behavior. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
|
@claude review |
|
Claude finished @D3V41's task in 2m 13s —— View job Code Review - Comprehensive Analysis
✅ Overall Assessment: APPROVEDThis PR successfully fixes a critical race condition in EDT client keystore configuration and addresses all review feedback comprehensively. The implementation is sound, well-tested, and follows project standards. 📋 Review SummaryCore Fix (Commit 2320607)Problem Identified: Static Solution Implemented: ✅ Correct
Impact: Each EDT service (MCEDT, HCV) now maintains independent keystore configuration, eliminating the race condition. 🎯 Follow-up Improvements (Commits 94cebab through 73efa77)1. Documentation Fix (Commit 94cebab) ✅What: Corrected 2. Bug Fix: Empty/Blank String Handling (Commit 8b6c0bd) ✅What: Added
Why: Prevents 3. Regression Test (Commit f2de883) ✅What: Test verifies keystore configuration isolation between builder instances 4. Edge Case Tests (Commit 73efa77) ✅What: Tests for null and non-existent path handling
Status: Comprehensive edge case coverage matching JavaDoc contract 🔍 Code Quality AnalysisSecurity ✅
Documentation ✅
Testing ✅
CLAUDE.md Compliance ✅
🎨 Code ArchitectureDesign Pattern: Instance-Based Configuration ✅The change from static to instance-based configuration follows sound OOP principles:
Call Site Updates ✅All three call sites properly updated to new pattern: // Before (static - WRONG)
EdtClientBuilder.setClientKeystoreFilename(path);
// After (instance - CORRECT)
EdtClientBuilder builder = new EdtClientBuilder(config);
setExternalClientKeystoreFilename(builder, path);
// ... then builder.setClientKeystoreFilename(url);📊 Test Coverage AnalysisRegression ProtectionThe
Edge Case CoverageBoth new edge case tests verify documented contract:
🔧 Minor Observations (Non-blocking)1. Test File LocationNote: The test Location: if (clientKeystorePropertiesPath == null) {
return;
}Should be: if (clientKeystorePropertiesPath == null || clientKeystorePropertiesPath.trim().isEmpty()) {
return;
}Impact: Low - test helper may not handle empty strings the same way as production code 2. Code Duplication (As noted by Sourcery)The three
Current state: Acceptable - each is in a different context (MCEDT service factory, HCV validator, test base) ✨ Strengths
🏁 Final RecommendationAPPROVED ✅ This PR:
Minor follow-up (non-blocking): Consider adding Files Reviewed
|
Summary
clientKeystorefrom static to instance variable inEdtClientBuilderProblem
The
clientKeystorevariable inEdtClientBuilderwas declared asstatic, meaning it was shared across all instances. This caused a critical race condition:/var/lib/mcedt/clientKeystore.properties✅/var/lib/hcv/clientKeystore.propertiesSolution
Made
clientKeystorean instance variable instead of static:Files Modified
EdtClientBuilder.java(src/main/java/ca/openosp/openo/integration/ebs/client/ng/)