Backport to branch(3.11) : Fix NullPointerException when a client is misconfigured with digital signature #315
Conversation
…l signature for HMAC (#302)
Summary of ChangesHello @feeblefakie, 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 backport addresses a Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request backports a fix for a NullPointerException that occurs when a client is misconfigured for digital signatures. The main change involves modifying SecretRegistry.lookup to throw a MissingSecretException instead of returning null when a secret is not found, which is a good practice to prevent NPEs. The changes are consistent across the codebase, with related error codes, exceptions, and tests being updated accordingly. I've provided a few suggestions to improve code clarity and testing practices.
| public void register(SecretEntry entry) { | ||
| SecretEntry existing = registry.lookup(entry.getKey()); | ||
| if (existing != null) { | ||
| try { | ||
| registry.lookup(entry.getKey()); | ||
| throw new DatabaseException(CommonError.SECRET_ALREADY_REGISTERED); | ||
| } catch (MissingSecretException e) { | ||
| registry.bind(entry); | ||
| } | ||
| registry.bind(entry); | ||
| } |
There was a problem hiding this comment.
Using a try-catch block for control flow, as done here, can make the code harder to understand. The expected path for registration (where the secret does not yet exist) is handled in the catch block, which is unconventional.
A clearer approach would be to introduce an exists() method in the SecretRegistry interface. This would make the intent of the code more explicit and improve readability.
For example, you could change the register method to:
public void register(SecretEntry entry) {
if (registry.exists(entry.getKey())) {
throw new DatabaseException(CommonError.SECRET_ALREADY_REGISTERED);
}
registry.bind(entry);
}This would require adding an exists() method to the SecretRegistry interface and its implementations, but it would represent a more idiomatic way to handle this check.
| Result result; | ||
| try { | ||
| return storage.get(get).map(this::toSecretEntry).orElse(null); | ||
| result = | ||
| storage | ||
| .get(get) | ||
| .orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND)); | ||
| } catch (ExecutionException e) { | ||
| throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage()); | ||
| } | ||
|
|
||
| return toSecretEntry(result); |
There was a problem hiding this comment.
This implementation can be made more concise by combining the variable declaration and assignment, and returning the result of toSecretEntry from within the try block. This improves readability by reducing the scope of the result variable.
| Result result; | |
| try { | |
| return storage.get(get).map(this::toSecretEntry).orElse(null); | |
| result = | |
| storage | |
| .get(get) | |
| .orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND)); | |
| } catch (ExecutionException e) { | |
| throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage()); | |
| } | |
| return toSecretEntry(result); | |
| try { | |
| Result result = | |
| storage | |
| .get(get) | |
| .orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND)); | |
| return toSecretEntry(result); | |
| } catch (ExecutionException e) { | |
| throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage()); | |
| } |
| MissingSecretException toThrow = mock(MissingSecretException.class); | ||
| when(registry.lookup(entry.getKey())).thenThrow(toThrow); |
There was a problem hiding this comment.
It's generally better to avoid mocking simple data-holder classes like exceptions. You can directly instantiate the exception. This simplifies the test setup and avoids unnecessary mocking.
| MissingSecretException toThrow = mock(MissingSecretException.class); | |
| when(registry.lookup(entry.getKey())).thenThrow(toThrow); | |
| when(registry.lookup(entry.getKey())).thenThrow(new MissingSecretException("secret not found")); |
2 participants
This is an automated backport of the following:
Please merge this PR after all checks have passed.