Enforce handle_fatal_reject type safety#1058
Conversation
`handle_fatal_reject` is a utility method intended to reduce boiler plate and duplicated code. Changing the method param to `RejectFatal` enforces better type safety and further reduces boiler plate.
Pull Request Test Coverage Report for Build 17648295731Details
💛 - Coveralls |
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
utACK
I think this is still not the right abstraction but no less wrong than what was there before
| ) -> InternalPersistedError<Err, Self::InternalStorageError> | ||
| where | ||
| Err: std::error::Error, | ||
| { | ||
| self.save_event(event).map_err(InternalPersistedError::Storage)?; | ||
| let RejectFatal(event, error) = reject_fatal; | ||
| if let Err(e) = self.save_event(event) { | ||
| return InternalPersistedError::Storage(e); | ||
| } | ||
| // Session is in a terminal state, close it | ||
| self.close().map_err(InternalPersistedError::Storage) | ||
| if let Err(e) = self.close() { | ||
| return InternalPersistedError::Storage(e); | ||
| } | ||
|
|
||
| InternalPersistedError::Fatal(error) |
There was a problem hiding this comment.
InternalPersistedError type on
if let Err(e) = self.close() {
return InternalPersistedError::Storage(e);
}seems wrong to me because this error is never persisted. but no less wrong than what's here right now so the change is fine with me.
There was a problem hiding this comment.
I think something like
Result<InternalPersistedError, Self::InternalStorageError>
Is more like it. Success means you persisted an error. Failure means you have a persistence (Storage) error.
There was a problem hiding this comment.
Which error? InternalPersistedError::Storage(e); ? That is an application produced error. I.e the application is indicating that it cannot store an event(which in this case is error).
There was a problem hiding this comment.
In that case I don't think it makes sense to be part of the PersistedError variant (unless it's really a PersistenceError. Could just use ImplementationError
There was a problem hiding this comment.
This method and its caller can fail in either two ways. API error or a storage error. We can't just use ImplementationError. Perhaps your issue is more with the naming of PersistenceError?
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
This refactor seems premature since the changes for #793 mean that:
- handle_fatal_reject won't be responsible for closing sessions anymore
- Transient errors will also need to be saved before they are returned, so maybe a shared handler would be less boilerplate
Thats a good point. In #793 the suggestons is to close the session after the post the
Why would we save transient error? for auditability? |
yes
Nevermind we wouldn't, I got those mixed up with non-repliable fatal errors |
|
I'm not opposed to merging this anyway if the refactor helps in other PRs in the meantime fwiw |
4 participants
handle_fatal_rejectis a utility method intended to reduce boiler plate and duplicated code. Changing the method param toRejectFatalenforces better type safety and further reduces boiler plate.Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.