Fix successful request log event not correctly printing finalized data #71
Conversation
…> instead of just Any
brendanlensink
changed the title
nbrooke
left a comment
nbrooke
left a comment
There was a problem hiding this comment.
I don't think you can make the argument that the request has succeeded if doesn't finalize, it's essentially the same thing as a decoding error, you are going to have a object you can use, and the completion is going to get called with an error. I'd say the semantics of the original code was wrong and this is correct.
We should (at least consider) the one tweak mentioned inline, though.
Netable/Netable/Netable.swift
Outdated
| case .success(let finalizedResult): | ||
| self.log(.requestSuccess(request: requestInfo, taskTime: time, statusCode: response.statusCode, responseData: data, finalizedResult: finalizedResult)) | ||
| case .failure(let error): | ||
| self.log(.requestFailed(request: requestInfo, taskTime: time, error: error)) |
There was a problem hiding this comment.
I know it's not new to this change (because previously it was not handling those errors at all and just passing any error straight to the completion handler), but while we are tweaking this, should this maybe be a throw error instead? Right now the semantics of failure here are subtly different that other decoding errors, because it doesn't go to the regular error handling path these errors CAN'T be retried. Although I think we could argue that decoding errors are of a slightly different character than network errors and maybe shouldn't be retried as much (since you are unlikely to get different results) we DO currently have the ability to retry JSON decoding errors, and it seems like we should at least be consistent between JSON decoding and finalization errors, since they are pretty much the same thing.
There was a problem hiding this comment.
Oh great question... I think it's a little strange to allow for retries since I can't really come up with a plausible scenario where you'd expect the retry to succeed but I think it's slightly more important to be consistent, so lets go with that.
There was a problem hiding this comment.
Yeah, I'm kind in the same boat, retrying decoding errors seems not super useful in practice, but being inconsistent between JSON and finalization errors seems worse, and it being impossible for the user to decide to retry those errors also is not great.
We should probably consider having some default set of errors that are not retryable (including JSON decoding errors and these), as long as there was some way to turn that off. Maybe there's a default implementation of allowRetry that get called if you don't specify one that suppresses both of them by default, but if you implement your own you've go the option to allow them?
That would also potentially solve the crumminess with timeout errors. The potential consequences of retying those (default timeouts are long enough that allowing retries could make a request take minutes) was bad enough that I decided to explicitly suppress those when implementing retries. However, It WOULD be safe, and possibly desirable, to retry timeouts if you knew that you had a relatively short timeout window (or were interacting with a server that you know timed out a lot), but there's no way to turn them on.
I'll file a bug for that.
Netable/Netable/Netable.swift
Outdated
| if case let .success(finalizedResult) = finalizedData { | ||
| self.log(.requestSuccess(request: requestInfo, taskTime: time, statusCode: response.statusCode, responseData: data, finalizedResult: finalizedResult)) | ||
| } else if case let .failure(finalizedError) = finalizedData { | ||
| throw NetableError.decodingError(finalizedError, data) |
There was a problem hiding this comment.
Do we want to wrap this up in a decoding error? Decoding error is a wrapper mainly because it's holding a system error that happened during decoding, where if you pass the raw error through it would not be clear that the error happened in the decoding step. It seems like (if people are doing things right) this should already be a Netable resourceExtractionError (or at the very least, another known-to-the-user type, since any type here is thrown from user code), so it might not be as valuable to wrap it.
There was a problem hiding this comment.
Good point, updated
Netable/Netable/Netable.swift
Outdated
| } else if case let .failure(finalizedError) = finalizedData { | ||
| throw NetableError.decodingError(finalizedError, data) | ||
| } else { | ||
| throw NetableError.resourceExtractionError("Failed to parse the result of `Finalize`.") |
There was a problem hiding this comment.
Can it ever actually fall though into this case. It seems like success and failure should be exhaustive and I thought that those parameters were non optional?
Maybe the issue is if the user makes them optional, like sets the FinalResource type to Int? (or whatever), HOWEVER, it seems like in that case we DON'T want to treat a nil as a failure. if the user makes the FinalResource type Int? and then passes back .success(nil) we probably should NOT treat that as a resource extraction failure, which I think this code might be doing.
There was a problem hiding this comment.
Yeah you're right, we can do this a lot more simply with a switch. Updated.
LogEvent.requestSuccessexpectsfinalizedResult: Any, but we were passing inResult<Any, NetableError>which was causing problems when we were trying to unwrap and print the finalized result.I think there might be a more elegant way to do this, where we pass the whole result into
.requestSuccessand then print a different message depending on whether the decoding was a success or not, because I think you could make an argument the request has succeeded either way, but I couldn't get it to work and I'm not sure it's worth dwelling on.