Fix certificate and secret key version check and messages #202
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jnmt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an inconsistency in version validation between the client and ledger sides for certificates and secret keys. It updates the client-side validation to ensure that certificate and secret key versions are strictly greater than zero, aligning with the existing ledger-side requirements. This change prevents unexpected errors for users by throwing an IllegalArgumentException earlier in the client if an invalid version (zero) is provided.
Highlights
- Version Validation Alignment: Updated client-side validation for certificate and secret key versions to require values strictly greater than zero (
> 0), aligning with the existing ledger-side requirements. This prevents unexpected errors for users by throwing anIllegalArgumentExceptionearlier in the client if an invalid version (zero) is provided. - Improved Error Messaging: Modified error messages in
CommonError.javato clearly indicate that certificate and secret key versions must be greater than zero, enhancing clarity for users encountering validation failures. - Certificate Version Prioritization: Adjusted the client configuration loading logic in
ClientConfig.javato prioritize the newDS_CERT_VERSIONover the deprecatedCERT_VERSIONfor digital signature certificates, ensuring the correct and non-deprecated version is used with higher precedence. - Enhanced Test Coverage: Added new unit tests in
ClientConfigTest.javato specifically verify that anIllegalArgumentExceptionis thrown when a version of zero is provided for both certificate and secret keys, covering scenarios with both new and deprecated configuration properties.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR fixes certificate and secret key version validation to align client-side validation with server-side requirements. The changes ensure that certificate and secret key versions must be greater than zero rather than greater than or equal to zero, preventing users from seeing unexpected errors instead of proper IllegalArgumentExceptions.
- Updated version validation logic to require values greater than zero for both certificate and secret key versions
- Refactored certificate version priority logic to always use non-deprecated configuration with higher precedence
- Updated error messages and enum names to reflect the new validation requirements
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CommonError.java | Updates error enum names and messages to reflect "greater than zero" requirement |
| HmacIdentityConfig.java | Changes secret key version validation from >= 0 to > 0 |
| DigitalSignatureIdentityConfig.java | Changes certificate version validation from >= 0 to > 0 |
| ClientConfig.java | Simplifies certificate version loading logic to prioritize non-deprecated config |
| ClientConfigTest.java | Adds comprehensive test coverage for version validation edge cases |
Comments suppressed due to low confidence (2)
client/src/test/java/com/scalar/dl/client/config/ClientConfigTest.java:592
- This test is adding a cert version of 0 but the test name suggests it's testing HMAC authentication with no secret key. The cert version 0 addition seems unrelated to the test's intended purpose and may cause confusion about what the test is validating.
props.put(ClientConfig.CERT_VERSION, "0");
client/src/test/java/com/scalar/dl/client/config/ClientConfigTest.java:775
- [nitpick] The test method name 'constructor_NewWrongCertVersionAndDeprecatedCorrectCertVersionGiven_ShouldThrowIllegalArgumentException' is overly verbose and unclear. Consider renaming to something more concise like 'constructor_NewCertVersionTakesPrecedenceOverDeprecated_ShouldThrowWhenNewVersionInvalid'.
public void
There was a problem hiding this comment.
Code Review
This pull request fixes the validation for certificate and secret key versions, aligning the client with the ledger's behavior by ensuring they must be greater than zero. The changes prioritize newer configuration properties over deprecated ones, improving clarity. The new logic is supported by unit tests. A minor suggestion is made to enhance the readability of a test case by removing a redundant line.
|
@yu2scalar I've added you as a reviewer for your reference. |
Description
This PR fixes the certificate and secret key version check and messages. Since we only accept the versions greater than zero (cf. certificate version and secret key version) on the Ledger side, we should have the same rules on the client side. Without fixing this issue, users will see an unexpected error instead of
IllegalArgumentExceptionin the client.Related issues and/or PRs
N/A
Changes made
Checklist
Additional notes (optional)
The error code docs should be fixed after this PR is merged.
Release notes
Fixed certificate and secret key version check and messages.