refactor: use EventEncodingException for encoding #412
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to use a custom EventEncodingException for all encoding/decoding operations instead of the generic JsonProcessingException. This change provides more specific exception handling for Nostr protocol-related encoding failures and improves error context by wrapping underlying Jackson exceptions with descriptive messages.
Key changes include:
- Replacing
JsonProcessingExceptionwithEventEncodingExceptionin method signatures across message classes and codecs - Adding try-catch blocks to wrap Jackson exceptions with meaningful error messages
- Updating integration tests to handle the new exception type
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| BaseMessage.java | Updated abstract encode method signature to throw EventEncodingException |
| IDecoder.java | Removed JsonProcessingException from interface, simplifying decoder contract |
| Multiple message classes | Added exception wrapping in encode/decode methods with descriptive error messages |
| Codec classes | Updated to throw EventEncodingException with proper error context |
| ApiEventIT.java | Updated integration test to handle new exception type |
| } catch (EventEncodingException e) { | ||
| throw new IllegalStateException("Failed to encode event", e); | ||
| return ENCODER_MAPPER_BLACKBIRD.writeValueAsString(arrayNode); | ||
| } catch (JsonProcessingException e) { |
There was a problem hiding this comment.
The BaseEventEncoder.encode() method now throws EventEncodingException, but this call is not wrapped in a try-catch block. This will cause a compilation error since the encode() method is called within a try block that only catches JsonProcessingException.
| } catch (EventEncodingException e) { | ||
| throw new IllegalStateException("Failed to encode canonical authentication event", e); | ||
| } catch (JsonProcessingException e) { | ||
| throw new EventEncodingException("Failed to encode canonical authentication message", e); |
There was a problem hiding this comment.
The BaseEventEncoder.encode() method now throws EventEncodingException, but this call is not wrapped in a try-catch block. This will cause a compilation error since the encode() method is called within a try block that only catches JsonProcessingException.
| ObjectMapper I_DECODER_MAPPER_BLACKBIRD | ||
| = JsonMapper.builder().addModule(new BlackbirdModule()).build().configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, true); | ||
| T decode(String str) throws JsonProcessingException; | ||
| T decode(String str); |
There was a problem hiding this comment.
The IDecoder interface signature has been changed to not throw any exceptions, but many implementing classes in the codebase now throw EventEncodingException. This creates an inconsistency where implementations cannot declare checked exceptions that aren't in the interface.
| T decode(String str); | |
| T decode(String str) throws EventEncodingException; |
Summary
Testing
mvn -q verify(fails: Previous attempts to find a Docker environment failed. Will not retry.)https://chatgpt.com/codex/tasks/task_b_68a812eddc248331a9df050b0ea234ad