refactor: return Optional from state(), KeyEvents.get(), and Config.get()#75
refactor: return Optional from state(), KeyEvents.get(), and Config.get()#75yashhzd wants to merge 2 commits intocardano-foundation:mainfrom
Conversation
…et() Complete the Optional refactoring started in cardano-foundation#57 for the remaining methods that still threw exceptions for not-found conditions: - SignifyClient.state() returns Optional<State> instead of throwing IllegalArgumentException on 404 - SignifyClient.connect() uses orElseThrow() for clear error messaging - Coring.KeyEvents.get() returns Optional<Object> with 404 check - Coring.Config.get() returns Optional<Object> with 404 check - TestUtils.getOrCreateClient() uses state().isEmpty() instead of try/catch flow control Closes cardano-foundation#49
| HttpResponse<String> res = this.client.fetch(path, method, null); | ||
| return Utils.fromJson(res.body(), Object.class); | ||
|
|
||
| if (res.statusCode() == HttpURLConnection.HTTP_NOT_FOUND) { |
There was a problem hiding this comment.
This isn't possible. The endpoint instead returns [] if not found.
There was a problem hiding this comment.
You're right, I should have checked the actual KERIA behavior instead of assuming a 404. Reverted KeyEvents.get() back to returning plain Object since the endpoint always responds with a list (empty or populated). Pushed in c98d226.
| HttpResponse<String> res = this.client.fetch(path, method, null); | ||
| return Utils.fromJson(res.body(), Object.class); | ||
|
|
||
| if (res.statusCode() == HttpURLConnection.HTTP_NOT_FOUND) { |
There was a problem hiding this comment.
Same, there is no 404 from this endpoint on KERIA
There was a problem hiding this comment.
Same fix applied here — reverted to plain Object return. Thanks for flagging this.
| throw new RuntimeException("State not initialized"); | ||
| } | ||
| State state = state() | ||
| .orElseThrow(() -> new IllegalArgumentException("Agent not booted — call boot() first")); |
There was a problem hiding this comment.
This change makes sense to me. It would be nice to keep the caid in the error message though for debugging reasons while developing. Maybe we can include that if we hit the throw block
There was a problem hiding this comment.
Good call. Updated the error message to include the controller identifier:
\n
Should make it easier to spot which controller is missing its agent during multi-client setups.
- Revert KeyEvents.get() and Config.get() to return plain Object since KERIA endpoints return [] instead of 404 for not-found - Include controller identifier (caid) in connect() error message for easier debugging during development - Update test files to match reverted signatures
| if (state == null) { | ||
| throw new RuntimeException("State not initialized"); | ||
| } | ||
| String caid = controller != null ? controller.getPre() : "unknown"; |
There was a problem hiding this comment.
controller is never null, actually. It gets initalized without the up-to-date state from KERIA in the constructor, and this function re-initializes it with the state (which is kind of weird, but inherited from Signify-TS poor design).
Perhaps we can tidy up any controller != null checks in this file! getPre() should always be work too.
2 participants
Summary
Completes the Optional refactoring started in #57 for the remaining methods that still threw exceptions or returned raw types for not-found conditions.
Production code:
SignifyClient.state()— returnsOptional<State>instead of throwingIllegalArgumentExceptionon 404SignifyClient.connect()— usesorElseThrow()with a clear error messageCoring.KeyEvents.get()— returnsOptional<Object>with 404 handlingCoring.Config.get()— returnsOptional<Object>with 404 handlingTest code:
TestUtils.getOrCreateClient()— replaces try/catch flow control withstate().isEmpty()check (the exact anti-pattern described in Only throw exceptions for error conditions #49)RandyTestandSaltyTests— updatedevents.get()calls to use.orElseThrow()All existing unit tests pass with no modifications needed.
Closes #49