prevents POST requests when the JSON body couldn't be correctly formed #415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aboedo
commented
Dec 4, 2020
| NSParameterAssert(([HTTPMethod isEqualToString:@"GET"] || [HTTPMethod isEqualToString:@"POST"])); | ||
| NSParameterAssert(!([HTTPMethod isEqualToString:@"GET"] && requestBody)); | ||
| NSParameterAssert(!([HTTPMethod isEqualToString:@"POST"] && !requestBody)); |
There was a problem hiding this comment.
this is a new check, but it looks like we don't have any places where we make POST requests without a body, and I can't think of a reason why you might want to.
Purchases/Networking/RCHTTPClient.m
Outdated
Comment on lines
182
to
189
| RCLog(@"Error creating request JSON: %@", requestBody); | ||
| RCErrorLog(@"Error creating request JSON: %@", requestBody); |
There was a problem hiding this comment.
This seems like a clear case for an error log to me
| @@ -179,16 +186,18 @@ - (NSMutableURLRequest *)createRequestWithMethod:(NSString *)HTTPMethod | |||
| NSError *JSONParseError; | |||
| NSData *body = [NSJSONSerialization dataWithJSONObject:requestBody options:0 error:&JSONParseError]; | |||
| if (JSONParseError) { | |||
| RCLog(@"Error creating request JSON: %@", requestBody); | |||
| RCErrorLog(@"Error creating request JSON: %@", requestBody); | |||
| return nil; | |||
There was a problem hiding this comment.
new behavior: if we can't create a JSON request, don't create the request at all, just call completion with an error
Purchases/Networking/RCHTTPClient.m
Outdated
Comment on lines
92
to
97
| if (!urlRequest) { | ||
| RCErrorLog(@"Could not create request to %@ with body %@", path, requestBody); | ||
| completionHandler(-1, | ||
| nil, | ||
| [RCPurchasesErrorUtils networkErrorWithUnderlyingError:RCPurchasesErrorUtils.unknownError]); | ||
| } |
There was a problem hiding this comment.
there might be a nicer-looking way of doing this. But this is the essence of the PR
aboedo
commented
Dec 7, 2020
Comment on lines
-82
to
-90
| [self assertIsValidRequestWithMethod:httpMethod body:requestBody]; | ||
|
|
||
| NSMutableDictionary *defaultHeaders = self.defaultHeaders.mutableCopy; | ||
| [defaultHeaders addEntriesFromDictionary:headers]; | ||
|
|
||
| NSMutableURLRequest *urlRequest = [self createRequestWithMethod:httpMethod | ||
| path:path | ||
| requestBody:requestBody | ||
| headers:defaultHeaders]; |
There was a problem hiding this comment.
moved some of these checks to happen earlier in the flow, since if they fail then the method doesn't actually do anything
…'t be parsed into JSON
aboedo
force-pushed
the
fix/posts_with_empty_body
branch
from
3f55c9d
to
e658c27
Compare
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're consistently getting a few POST requests on the backend which have empty body.
Thing is, we never create requests with
nilbodies intentionally, so after looking at the code it seems like this might happen if thebodydictionary couldn't be parsed into JSON.It doesn't make sense to still hit the backend if the body of the request couldn't be correctly formed, so this does an early exit before that happens.
Also, when you hit the backend unnecessarily, a panda dies. Don't kill pandas.