Fix batch geocoding request handling #148

captainbarbosa · 2018-06-22T23:41:35Z

Fixes #147.

Improves handling of logic around setting the desired Mapbox Geocoding API endpoint when using MBGeocodeOptions.

cc @kbauhaus

1ec5 · 2018-06-26T21:11:27Z

MapboxGeocoder/MBGeocodeOptions.swift

+    /**
+     The query mode of the forward or reverse geocoding request.
+    */
+    internal var mode: String = "mapbox.places"


Nit: String can be inferred from the value on the right-hand side, so it can be removed.

1ec5 · 2018-06-26T21:12:49Z

MapboxGeocoder/MBGeocoder.swift

-        }
+        let mode = options.mode
+
+        assert(options.queries.count <= 50, "Too many queries in a single request.")


Assuming the Geocoding API returns an error when putting over 50 queries in a single batch, we should remove this assertion so that the more graceful error handler can kick in.

captainbarbosa · 2018-06-26T23:54:50Z

Things seem to be operational now, but I'll need to add a good chunk of tests before calling this PR finished.

captainbarbosa · 2018-06-26T23:55:34Z

MapboxGeocoder/MBGeocoder.swift

-                DispatchQueue.main.async {
-                    errorHandler(error as NSError)
+                // Handle single & single batch geocoding queries
+                do {


Per chat with @1ec5 we may want to consider doing a refactor of this gnarly do/try/catch block.

1ec5

Great work! We’ll be ready to merge once there are tests of forward and reverse batch geocoding, including of small-batch geocoding. The diff for this PR will be a little nicer once I merge #149 and this PR is rebased atop that one.

1ec5 · 2018-06-27T00:39:08Z

MapboxGeocoder/MBGeocoder.swift

+                do {
+                    let result = try decoder.decode(GeocodeAPIResult.self, from: data)
+                    // Check if geocoding query failed
+                    guard result.message == nil else {


Nit: We can also turn this guard statement into an if let so that the line below doesn’t have to force-unwrap result.message.

1ec5 · 2018-06-27T00:41:46Z

MapboxGeocoder/MBGeocoder.swift

+                    result = try decoder.decode([GeocodeResult].self, from: data)
+                } catch {
+                    // Decode single batch geocding queries
+                    result = [try decoder.decode(GeocodeResult.self, from: data)]


As tail work, maybe we should consider refactoring Geocoder’s public methods so that we distinguish between single-serving and batched requests at different places in the call stack.

captainbarbosa · 2018-06-29T23:06:23Z

MapboxGeocoderTests/BatchForwardGeocodingTests.swift

+        }
+    }
+
+    func testInvalidForwardSingleBatchGeocode() {


@1ec5 both testInvalidForwardSingleBatchGeocode / testInvalidForwardMultipleBatchGeocode tests are commented out because they are failing. I'm actually seeing the same error the original issue described - Expected to decode Array<Any> but found a dictionary instead. 🤔

Which decode step is failing? If the step that tries to look for a message in a dictionary in an array ([GeocoderAPIResult]) is failing, then we need to make sure the fallback that looks for message in a dictionary (GeocoderAPIResult) is working. If the step that tries to decode an array of individual results ([GeocoderResult]) is failing, then we should make sure the fallback that looks for a bare result (GeocoderResult) is working.

captainbarbosa · 2018-07-04T02:34:14Z

@1ec5 thanks for helping me get unstuck on the failing invalid batch geocoding test. For those following along, it seems like OHHTTP already encodes queries so there was no need to encode it before passing it in.

I was able to add the rest of the tests for batch geocoding and also tested token validity. As a next step, I'll start the reverse batch geocoding tests, which should be fairly similar. I've also rebased this on master now that #149 has merged.

captainbarbosa · 2018-07-05T22:53:26Z

MapboxGeocoderTests/BatchGeocodingTests.swift

+        _ = stub(condition: isHost("api.mapbox.com")
+
+           // && isPath("/geocoding/v5/mapbox.places-permanent/100,100.json")
+              // The above line is causing the test fo fail for unknown reasons


@1ec5 I've added the reverse batch geocoding tests but I'm seeing the weird behavior with OHTTPS isPath again - I get test failures if I include the line above for both reverse batch geocoding tests that return no results. The tests pass if I remove this line though 🤕

Does the URL match exactly? Or does urlForGeocoding(_:) translate CLLocationCoordinate2DMake(100, 100) into something else, such as 100.0,100.0?

Oh! Yeah, it looks like urlForGeocoding(_:) is turning this into 100.00000,100.00000.

captainbarbosa · 2018-07-11T23:16:00Z

@1ec5 I feel good about this now, would you mind doing a final review?

1ec5

Looks good, assuming the build failures are only due to the missing file reference.

1ec5 · 2018-07-12T00:22:44Z

MapboxGeocoderTests/BatchGeocodingTests.swift

+        _ = stub(condition: isHost("api.mapbox.com")
+            && isPath("/geocoding/v5/mapbox.places-permanent/85+2nd+st+san+francisco.json")
+            && containsQueryParams(["access_token": invalidToken])) { _ in
+                let path = Bundle(for: type(of: self)).path(forResource: "permanent_invalid_token", ofType: "json")


Tests are failing:

Testing failed: error: /Users/Desktop/permanent_invalid_token.json: No such file or directory error: /Users/Desktop/permanent_reverse_multiple_no_results.json: No such file or directory

Did you add these files to the iOS and tvOS test targets as well? (There’s no watchOS test target.)

Looks like I forgot to Copy items if needed when adding these fixtures to the project 😬

1ec5 · 2018-07-12T00:48:13Z

MapboxGeocoder.xcodeproj/project.pbxproj

@@ -170,6 +206,18 @@
 /* End PBXCopyFilesBuildPhase section */

 /* Begin PBXFileReference section */
+		0711239220E59DE90043CB51 /* permanent_forward_single_valid.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; name = permanent_forward_single_valid.json; path = ../../../../.Trash/permanent_forward_single_valid.json; sourceTree = "<group>"; };


This file is located in the Trash on your Mac, which Bitrise has no access to.

captainbarbosa added the bug label Jun 22, 2018

captainbarbosa requested a review from 1ec5 June 22, 2018 23:41

1ec5 reviewed Jun 26, 2018

View reviewed changes

captainbarbosa commented Jun 26, 2018

View reviewed changes

1ec5 reviewed Jun 27, 2018

View reviewed changes

captainbarbosa commented Jun 29, 2018

View reviewed changes

captainbarbosa and others added 13 commits July 3, 2018 19:35

Allowing batch geocoding of at least one query

ecde0a1

Refactor mode override

3ef3160

Handle multiple geocoding queries

265370d

Add clarifying comments

bb21f69

Minor feedback edits

4d82f65

Set up single forward batch test case

202c3da

Add test for valid fwd batch geocoding with multiple queries

8723c07

Add test for failed single forward batch geocode

7a433ff

Add test for failed multiple forward batch geocode

ec597f1

Add debug notes

3c4ba37

Add no-results returned forward batch geocoding tests

d475a79

Rename/cleanup tests & comments

9fbb49a

Switch from guard to if/let

ddda4f5

captainbarbosa force-pushed the batch-bugfix-nb branch from 725a1c2 to ddda4f5 Compare July 4, 2018 02:36

Add reverse batch geocode tests

e7185e4

captainbarbosa commented Jul 5, 2018

View reviewed changes

Match precision for path-matching tests

6b83082

1ec5 approved these changes Jul 12, 2018

View reviewed changes

1ec5 reviewed Jul 12, 2018

View reviewed changes

Fix test fixture file paths

08b2f25

1ec5 changed the title ~~[WIP] Fix batch geocoding request handling~~ Fix batch geocoding request handling Jul 12, 2018

🔮

f7ef9ba

captainbarbosa merged commit 75e3744 into master Jul 12, 2018

captainbarbosa deleted the batch-bugfix-nb branch July 12, 2018 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix batch geocoding request handling #148

Fix batch geocoding request handling #148

captainbarbosa commented Jun 22, 2018

1ec5 Jun 26, 2018

1ec5 Jun 26, 2018

captainbarbosa commented Jun 26, 2018

captainbarbosa Jun 26, 2018

1ec5 left a comment

1ec5 Jun 27, 2018

1ec5 Jun 27, 2018

captainbarbosa Jun 29, 2018

1ec5 Jun 30, 2018

captainbarbosa commented Jul 4, 2018 •

edited

Loading

captainbarbosa Jul 5, 2018

1ec5 Jul 9, 2018

captainbarbosa Jul 11, 2018

captainbarbosa commented Jul 11, 2018

1ec5 left a comment

1ec5 Jul 12, 2018

captainbarbosa Jul 12, 2018

1ec5 Jul 12, 2018

Fix batch geocoding request handling #148

Fix batch geocoding request handling #148

Conversation

captainbarbosa commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainbarbosa commented Jun 26, 2018

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainbarbosa commented Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainbarbosa commented Jul 11, 2018

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainbarbosa commented Jul 4, 2018 •

edited

Loading