search

parse-community / Parse-SDK-iOS-OSX

The Apple SDK for Parse Platform (iOS, macOS, watchOS, tvOS)
https://parseplatform.org
Other
2.81k stars 871 forks source link

Possible batching bug depending on number of objects batched #1588

Open cbaker6 opened 3 years ago

cbaker6 commented 3 years ago

I suspect there's a bug in arrayBySplittingArray, particularly: https://github.com/parse-community/Parse-SDK-iOS-OSX/blob/5dad4f224d5ae4b36ead891acbdfc15b86d527ae/Parse/Parse/Internal/PFInternalUtils.m#L245

I think there are two issues, but this is only a suspicion as there are no test cases for arrayBySplittingArray. I don't recall, how range works for objective-c, but if it's similar to swift, I'm sure there's an issue. Basically, batches can be missed when the number of objects go over the default batch limit of 50. I suspect most people are batching with < 50 objects so the issue won't show up there.

  1. The endpoint of the subarray should grow dynamically while iterating through the while loop, it currently isn't doing this properly. Should be index+length.
  2. If range works the same way as Swift, then it should be < the endpoint of the array or else it can jump out-of-bounds.

If someone wants to take this on, replicating the test cases I wrote for ParseSwift should detect the problem .

The ParseSwift equivalent for arrayBySplittingArray is here which can be used to fix the bug: https://github.com/parse-community/Parse-Swift/blob/f1de7e4a95d937bcc008bc5ef040ebba3ced7adb/Sources/ParseSwift/API/BatchUtils.swift#L35-L49

shlusiak commented 3 years ago

The NSMakeRange method takes a location and a length (not an end point), so passing in index and length here seems correct. https://developer.apple.com/documentation/foundation/1417188-nsmakerange?language=objc

length is either the batch size of 50 or the number of remaining elements in the array, whichever is smaller. I cannot see any off-by one situations in that algorithm either, could be give me an example in numbers where there would be an out-off-bounds access to array?

You linked https://github.com/parse-community/Parse-SDK-Android/blob/a5cfa1f9c6654281c76367ad8b3252e2499bb821/parse/src/main/java/com/parse/Lists.java#L67 in the other PR, but I can't see any issue in Android's implementation either. Android's subList takes a start index (inclusive) and an end index (exclusive) so that also looks correct.

The test case you linked is giving me a 404 error.

cbaker6 commented 3 years ago

The branch I linked was recently merged and deleted, the location on the main branch is here: https://github.com/parse-community/Parse-Swift/blob/main/Tests/ParseSwiftTests/BatchUtilsTests.swift

The linked file with the swift version is here: https://github.com/parse-community/Parse-Swift/blob/f124d0a45c079dba0324b9603e9e106f51ecb24e/Sources/ParseSwift/API/BatchUtils.swift#L34-L50

If what you mentioned is how range works for objective-c, then there may not be any issues.