Read keystore configuration from the toml file #105
Conversation
📝 WalkthroughOverviewThis PR modifies the keystore configuration resolution order in the Cipher Tool to prioritize deployment.toml over carbon.xml. The new priority order is: ChangesConstants.javaAdded new constants to support keystore configuration resolution:
Utils.javaEnhanced configuration resolution logic with the following changes: Modified methods:
New helper methods:
Updated
ImpactThis change enables the Cipher Tool to read keystore configurations from deployment.toml before startup, resolving the issue where the tool would ignore deployment.toml updates on first startup and default to carbon.xml settings instead. WalkthroughThe PR modifies
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
components/ciphertool/src/main/java/org/wso2/ciphertool/utils/Utils.java (2)
454-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
$refparsing duplicated and unguarded.The same
$ref{...}extraction logic is repeated here and ingetValueFromConfigs(Lines 205-209). Consider extracting a shared helper that validates the braces are present before callingsubstring, both to avoid duplication and to prevent an uncaughtStringIndexOutOfBoundsExceptionon malformed references.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ciphertool/src/main/java/org/wso2/ciphertool/utils/Utils.java` around lines 454 - 460, The $ref{...} parsing logic is duplicated in the current method and in the getValueFromConfigs method and lacks validation, risking a StringIndexOutOfBoundsException on malformed references. Create a shared helper method that takes a path string as input and extracts the reference value between the curly braces while validating that both opening and closing braces are present before attempting substring operations. Then replace the inline substring extraction at lines 454-460 (where path.substring is called) and the equivalent code in getValueFromConfigs with calls to this new helper method to ensure consistent and safe reference parsing across both locations.
195-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid relying on
NullPointerExceptionfor control flow, and guard$refparsing.Two concerns in
getValueFromConfigs:
- The carbon.xml fallback is triggered by catching
NullPointerExceptionfromdefaultMap.get(key).toString(). This is fragile and can mask unrelated NPEs originating elsewhere in thetryblock. Prefer an explicit null check on the map lookup.value.substring(value.indexOf('{') + 1, value.indexOf('}'))will throwStringIndexOutOfBoundsException(not caught here) if a value begins with$refbut is missing a{/}, leaving malformed input unhandled.♻️ Suggested approach
- String value = config; - try { - // If the value is empty in deployment.toml, read from default.json. - if (StringUtils.isBlank(value)) { - value = defaultMap.get(key).toString(); - } - // If the value is given as a reference, read from default.json. - if (value.startsWith("$ref")) { - // Read the value between the curly braces as the reference. - // e.g. $ref{<reference>} -> <reference> - String reference = value.substring(value.indexOf('{') + 1, value.indexOf('}')); - return defaultMap.get(reference).toString(); - } - return value; - // Throw NullPointerException if the value is not available in default.json. - } catch (NullPointerException e) { - // Read from carbon.xml if default.json is not available. - System.err.println("Invalid value " + key + " " + e); - return Utils.getValueFromXPath(element, xPath); - } + String value = config; + // If the value is empty in deployment.toml, read from default.json. + if (StringUtils.isBlank(value)) { + Object mapped = defaultMap.get(key); + if (mapped == null) { + // Read from carbon.xml if not available in default.json. + return Utils.getValueFromXPath(element, xPath); + } + value = mapped.toString(); + } + // If the value is given as a reference, resolve it from default.json. + String resolved = resolveReference(value, defaultMap); + return resolved != null ? resolved : value;Where
resolveReferencevalidates the presence of{/}before substring extraction and returnsnullfor non-reference values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ciphertool/src/main/java/org/wso2/ciphertool/utils/Utils.java` around lines 195 - 218, In the getValueFromConfigs method, replace the try-catch block that relies on catching NullPointerException with explicit null checks after defaultMap.get(key) and defaultMap.get(reference) calls to properly handle missing values. Additionally, before parsing the reference in the $ref block using substring with indexOf('{') and indexOf('}'), add validation to ensure both curly braces are present in the value string; if they are missing, throw a descriptive exception rather than allowing StringIndexOutOfBoundsException to occur. Consider extracting the $ref reference parsing logic into a separate helper method to improve code clarity and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ciphertool/src/main/java/org/wso2/ciphertool/utils/Utils.java`:
- Around line 595-609: The getJSONConfiguration method currently only catches
IOException in its try-catch block, but Gson's fromJson method can also throw
JsonSyntaxException when encountering malformed JSON. To fix this, add a catch
clause for JsonSyntaxException alongside the existing IOException catch block so
that both exceptions are handled gracefully, allowing the method to return the
empty map fallback as documented in the comment instead of aborting on invalid
JSON syntax.
---
Nitpick comments:
In `@components/ciphertool/src/main/java/org/wso2/ciphertool/utils/Utils.java`:
- Around line 454-460: The $ref{...} parsing logic is duplicated in the current
method and in the getValueFromConfigs method and lacks validation, risking a
StringIndexOutOfBoundsException on malformed references. Create a shared helper
method that takes a path string as input and extracts the reference value
between the curly braces while validating that both opening and closing braces
are present before attempting substring operations. Then replace the inline
substring extraction at lines 454-460 (where path.substring is called) and the
equivalent code in getValueFromConfigs with calls to this new helper method to
ensure consistent and safe reference parsing across both locations.
- Around line 195-218: In the getValueFromConfigs method, replace the try-catch
block that relies on catching NullPointerException with explicit null checks
after defaultMap.get(key) and defaultMap.get(reference) calls to properly handle
missing values. Additionally, before parsing the reference in the $ref block
using substring with indexOf('{') and indexOf('}'), add validation to ensure
both curly braces are present in the value string; if they are missing, throw a
descriptive exception rather than allowing StringIndexOutOfBoundsException to
occur. Consider extracting the $ref reference parsing logic into a separate
helper method to improve code clarity and maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c146bdd2-b780-43db-b817-1ab232ed981d
📒 Files selected for processing (2)
components/ciphertool/src/main/java/org/wso2/ciphertool/utils/Constants.javacomponents/ciphertool/src/main/java/org/wso2/ciphertool/utils/Utils.java
2 participants
Purpose
Port of #75
Fixes wso2/product-integrator-mi/issues/4942