-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial commit of an HTTP writer for SNTMetricSets. #621
Conversation
This PR adds support for shipping serialized SNTMetricSets to an HTTP server via POSTs.
Rearrange block declaration for readability
@@ -81,7 +81,7 @@ - (NSDictionary *)normalize:(NSDictionary *)metrics { | |||
if (![NSJSONSerialization isValidJSONObject:normalizedMetrics]) { | |||
if (err != nil) { | |||
*err = [[NSError alloc] | |||
initWithDomain:@"SNTMetricRawJSONFileWriter" | |||
initWithDomain:@"com.google.santa.metricservice.formatters.rawjson" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be com.google.santa.metricservice.formats.rawjson
.
@@ -16,6 +17,9 @@ @interface SNTMetricServiceTest : XCTestCase | |||
@property id mockConfigurator; | |||
@property NSString *tempDir; | |||
@property NSURL *jsonURL; | |||
@property id mockSession; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were moved to be properties to avoid a bug an issue with OCMock and releasing the blocks too many times.
OCMStub([self.mockConfigurator metricFormat]).andReturn(SNTMetricFormatTypeRawJSON); | ||
OCMStub([self.mockConfigurator metricURL]).andReturn(url); | ||
|
||
self.mockSession = [OCMockObject niceMockForClass:[NSURLSession class]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the macros for this:
self.mockSession = OCMClassMock([NSURLSession class]);
self.mockSessionDataTask = OCMClassMock([NSURLSessionDataTask class]);
self.mockMOLAuthenticatingURLSession = OCMClassMock([MOLAuthenticatingURLSession class]);
[[[self.mockMOLAuthenticatingURLSession stub] andReturn:self.mockMOLAuthenticatingURLSession] alloc]; | ||
[[[self.mockMOLAuthenticatingURLSession stub] andReturn:self.mockSession] session]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for this:
OCMStub([self.mockMOLAuthenticatingURLSession alloc]).andReturn(self.mockMOLAuthenticatingURLSession);
OCMStub([self.moclMOLAuthenticatingURLSession session]).andReturn(self.mockSession);
passedBlock(nil, response, nil); | ||
[responseCallback fulfill]; | ||
}; | ||
[(NSURLSessionDataTask *)[[self.mockSessionDataTask stub] andDo:callCompletionHandler] resume]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should work too:
OCMStub([self.mockSessionDataTask resume]).andDo(callCompletionhandler);
[[[[self.mockSession stub] andDo:getCompletionHandler] andReturn:self.mockSessionDataTask] | ||
dataTaskWithRequest:[OCMArg any] | ||
completionHandler:[OCMArg any]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OCMStub([self.mockSession dataTaskWithRequest:[OCMArg any]
completionHandler:[OCMArg any]).andDo(getCompletionHandler).andReturn(self.mockSessionDataTask);
_request = [[NSMutableURLRequest alloc] init]; | ||
_request.HTTPMethod = @"POST"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never seems to be used?
[request setValue:@"application/json" forHTTPHeaderField:@"Content-Type"]; | ||
|
||
_authSession.serverHostname = url.host; | ||
_session = _authSession.session; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no real reason to store this as an ivar, you're getting a new one each time in this method anyway.
_session = _authSession.session; | ||
|
||
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); | ||
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for this dispatch_async? Each call to [dataTaskWithRequest:completionHandler: resume]
should be async anyway so the semaphore here is signalling only after the call to start the request occurs.
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER); | ||
|
||
if (_blockError != nil) { | ||
if (error != nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will lose the error if error
is nil. I'm assuming you probably wanted to set it if error
is nil and log/merge the errors if error
is nonnil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case if error is nil it means the caller passed in nil/NULL to ignore the errors.
*stop = YES; | ||
} | ||
|
||
if (response == nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break these out into short-circuit cases? ex
if (err != nil) {
_blockError = err;
*stop = YES;
return;
}
if (response == nil) {
// also create an NSError for _blockError here
*stop = YES;
return;
}
if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
...
* Post serialzied metrics to the specified URL one object at a time. | ||
**/ | ||
- (BOOL)write:(NSArray<NSData *> *)metrics toURL:(NSURL *)url error:(NSError **)error { | ||
// open the file and write it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orphaned comment?
|
||
__block void (^passedBlock)(NSData *, NSURLResponse *, NSError *); | ||
|
||
void (^getCompletionHandler)(NSInvocation *) = ^(NSInvocation *invocation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rearrange here too
BOOL result = [httpWriter write:@[ JSONdata ] toURL:url error:&err]; | ||
XCTAssertEqual(YES, result); | ||
XCTAssertNil(err); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add test/assert on the response handler here to check if the data is still intact when exported?
I'm realizing that I screwed up in a refactoring on Friday and forgot to mark this PR as draft. Essentially I wanted that handler to continue so that it could mark the semaphore as finished at the end of it. |
This PR adds support for shipping serialized SNTMetricSets to an HTTP server via POSTs.
Testing is accomplished via mocks.
This is part of the work needed to support #563