Handle Error Pointers according to Cocoa Convention #3653

tclementdev · 2016-08-19T13:01:00Z

This is a Cocoa convention.

codecov-io · 2016-08-19T13:29:04Z

Current coverage is 87.44% (diff: 100%)

Merging #3653 into master will increase coverage by 0.81%

@@             master      #3653   diff @@
==========================================
  Files            44         45     +1   
  Lines          6067       6198   +131   
  Methods        1079       1096    +17   
  Messages          0          0          
  Branches        407        412     +5   
==========================================
+ Hits           5256       5420   +164   
+ Misses          808        775    -33   
  Partials          3          3

Powered by Codecov. Last update e337471...f28654c

tclementdev · 2016-09-19T16:00:32Z

I think you missed the point. The return value of the method call needs to be tested before looking at the error. This is what is done in the sample code from Apple you provided and this is not what is done in the AFNetworking code.

On 19 Sep 2016, at 17:31, Kevin Harwood notifications@github.com wrote:

Hi @tclementdev https://github.com/tclementdev! Thanks for the PR.

Apple's example docs in Dealing with Errors https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/ErrorHandling/ErrorHandling.html uses the standard we are currently using in this library, so I'm going to stick with that for now.
NSError *anyError;
BOOL success = [receivedData writeToURL:someLocalFileURL
                                options:0
                                  error:&anyError];
if (!success) {
    NSLog(@"Write failed with error: %@", anyError);
    // present error to user
}
🍻

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #3653 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ABWubYuOFTgfx9rynV9nLWZhSOhu7AQtks5qrqrBgaJpZM4Joc7m.

tclementdev · 2016-09-19T16:07:04Z

@kcharwood

0xced · 2016-09-30T22:07:35Z

I don’t understand why this PR was closed, @tclementdev point is perfectly valid.

From Cocoa's Error Handling Programming Guide

Important: Success or failure is indicated by the return value of the method. Although Cocoa methods that indirectly return error objects in the Cocoa error domain are guaranteed to return such objects if the method indicates failure by directly returning nil or NO, you should always check that the return value is nil or NO before attempting to do anything with the NSError object.

kcharwood · 2016-10-03T13:29:37Z

Sorry misread the original intent! Reopened here.

Has a full audit of the codebase been done for this particular issue?

tclementdev · 2016-10-03T14:40:07Z

Thank you for re-opening this PR. I just pushed some additionnal fixes concerning the same issue in other system API calls.

I could not find any other system API misuse, however it should be noted that the AFNetworking APIs themselves seem to be quite oblivious of this convention, with code seemingly returning non-nil objects while an error occurred internally. There is probably some amount of work to audit and modify the AFNetworking code itself to clarify and address this (this can be done in a separate PR).

kcharwood · 2016-10-05T18:17:45Z

Can you rebase this with master to get Xcode 8 coverage?

…convention.

…s (this is the correct way to handle errors as described in Apple's documentation).

tclementdev · 2016-10-06T11:24:48Z

Done.

kcharwood · 2016-10-06T13:37:53Z

You can see the codecov diff here.

Verifying those nil's should help prevent regressions in the future. After that, should be good to merge!

kcharwood · 2016-10-06T13:35:02Z

AFNetworking/AFURLRequestSerialization.m

+            return nil;
+        }
+
+        [mutableRequest setHTTPBody:jsonData];


Can you add a test verifying the nil behavior here?

kcharwood · 2016-10-06T13:35:30Z

AFNetworking/AFURLRequestSerialization.m

+
+        if (!plistData) {
+            return nil;
+        }


Also a test to verify nil behavior here. Just so we can catch regressions in the future.

kcharwood · 2016-10-06T13:36:09Z

AFNetworking/AFURLResponseSerialization.m

+        if (error) {
+            *error = AFErrorWithUnderlyingError(serializationError, *error);
+        }
+        return nil;


Test verifying nil

kcharwood · 2016-10-06T13:36:18Z

AFNetworking/AFURLResponseSerialization.m

+        if (error) {
+            *error = AFErrorWithUnderlyingError(serializationError, *error);
+        }
+        return nil;


Test verifying nil

kcharwood · 2016-10-06T13:36:28Z

AFNetworking/AFURLResponseSerialization.m

-    if (data) {
-        responseObject = [NSPropertyListSerialization propertyListWithData:data options:self.readOptions format:NULL error:&serializationError];
+    if (!data) {
+        return nil;


Test verifying nil

tclementdev · 2016-10-06T21:44:12Z

Am I good?

kcharwood · 2016-10-10T12:44:24Z

🍻 thanks!

kcharwood closed this Sep 19, 2016

kcharwood reopened this Oct 3, 2016

kcharwood added the needs backmerge/rebase label Oct 5, 2016

kcharwood added this to the 3.2.0 milestone Oct 5, 2016

tclementdev added 2 commits October 6, 2016 12:51

Check return value before doing anything with error. This is a Cocoa …

0be9763

…convention.

Make sure we check the return values before handling the error object…

df5b512

…s (this is the correct way to handle errors as described in Apple's documentation).

tclementdev force-pushed the master branch from 8f8f269 to df5b512 Compare October 6, 2016 10:53

Rewrite code flow to clarify error handling.

2201d2c

kcharwood removed the needs backmerge/rebase label Oct 6, 2016

kcharwood added the needs tests label Oct 6, 2016

Add json/plist serializer tests for better code coverage.

290ec35

kcharwood approved these changes Oct 6, 2016

View reviewed changes

tclementdev added 2 commits October 6, 2016 18:26

Fix class name comment.

95fa906

Add test asserts for non-nil errors.

f28654c

kcharwood added updated and removed needs tests labels Oct 10, 2016

kcharwood changed the title ~~Check return value before doing anything with error.~~ Handle Error Pointers according to Cocoa Convention Oct 10, 2016

kcharwood merged commit e8fde52 into AFNetworking:master Oct 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Error Pointers according to Cocoa Convention #3653

Handle Error Pointers according to Cocoa Convention #3653

tclementdev commented Aug 19, 2016

codecov-io commented Aug 19, 2016 •

edited

tclementdev commented Sep 19, 2016

tclementdev commented Sep 19, 2016

0xced commented Sep 30, 2016

kcharwood commented Oct 3, 2016

tclementdev commented Oct 3, 2016

kcharwood commented Oct 5, 2016

tclementdev commented Oct 6, 2016

kcharwood commented Oct 6, 2016

kcharwood Oct 6, 2016

kcharwood Oct 6, 2016

kcharwood Oct 6, 2016

kcharwood Oct 6, 2016

kcharwood Oct 6, 2016

tclementdev commented Oct 6, 2016

kcharwood commented Oct 10, 2016

Handle Error Pointers according to Cocoa Convention #3653

Handle Error Pointers according to Cocoa Convention #3653

Conversation

tclementdev commented Aug 19, 2016

codecov-io commented Aug 19, 2016 • edited

Current coverage is 87.44% (diff: 100%)

tclementdev commented Sep 19, 2016

tclementdev commented Sep 19, 2016

0xced commented Sep 30, 2016

kcharwood commented Oct 3, 2016

tclementdev commented Oct 3, 2016

kcharwood commented Oct 5, 2016

tclementdev commented Oct 6, 2016

kcharwood commented Oct 6, 2016

kcharwood Oct 6, 2016

Choose a reason for hiding this comment

kcharwood Oct 6, 2016

Choose a reason for hiding this comment

kcharwood Oct 6, 2016

Choose a reason for hiding this comment

kcharwood Oct 6, 2016

Choose a reason for hiding this comment

kcharwood Oct 6, 2016

Choose a reason for hiding this comment

tclementdev commented Oct 6, 2016

kcharwood commented Oct 10, 2016

codecov-io commented Aug 19, 2016 •

edited