Adding support for the Geolocation API call. #164

kwnevarez · 2016-06-17T22:41:36Z

This pull request is to initiate a discussion on the proposed changes to support the Geolocation API. Geolocation uses POST calls so support for POST was implemented in both the OkHttp and GAE handlers.

A simple test case was created, it fails, but I believe that's because I am using assert wrong or creating the result object incorrectly. The call to the server is working and that can be seen by running the tests with the --info flag.

The new code can be exercised with:
$ API_KEY=AIza... ./gradlew test --info --tests com.google.maps.GeolocationApiTest

Output:

com.google.maps.GeolocationApiTest > testSimpleGeolocation[0] STANDARD_ERROR
    Jun 17, 2016 3:22:44 PM com.google.maps.OkHttpRequestHandler handlePost
    INFO: Request: https://www.googleapis.com/geolocation/v1/geolocate?key=AIzaSyB5fznx33Gskw8n0vJx9GCz_3FcASNe09s
    Jun 17, 2016 3:22:44 PM com.google.maps.OkHttpRequestHandler handlePost
    INFO: Request Body: {"homeMobileCountryCode":310,"homeMobileNetworkCode":410,"radioType":"gsm","carrier":"Vodafone","considerIp":true,"wifiAccessPoints":[{"macAddress":"01:23:45:67:89:AB","signalStrength":-65,"age":0,"channel":11,"signalToNoiseRatio":40}]}
    Jun 17, 2016 3:22:44 PM com.google.maps.internal.OkHttpPendingResult parseResponse
    INFO: response body{
     "location": {
      "lat": 37.428433999999996,
      "lng": -122.0723816
     },
     "accuracy": 2855.0
    }

domesticmouse · 2016-06-21T23:51:35Z

src/main/java/com/google/maps/OkHttpRequestHandler.java

@@ -15,13 +15,17 @@

 package com.google.maps;

+import static com.google.appengine.repackaged.com.google.common.base.StringUtil.JsEscapingMode.JSON;


This is going to be an issue, we don't want to require appengine code for users who aren't on GAE

I think this is just a bad import from an IDE. You can drop the com.google.appengine.repackaged. prefix and you should be fine.

domesticmouse · 2016-06-21T23:52:45Z

Hey @markmcd can you please check this PR for sanity? Thanks!

markmcd · 2016-06-22T00:08:03Z

src/main/java/com/google/maps/model/WifiAccessPoint.java

+  /**
+  * {@code signalStrength}: The current signal strength measured in dBm.
+  */
+  public int signalStrength;


Are the int fields optional? If so, is it OK that the API will still be sent the default value for int even if the user doesn't provide it?

Closes #9 Closes #17 Closes #16 Closes #1 Closes #2 Closes #4 Closes #8 Closes #14 Closes #10 Closes #12 Closes #11 Closes #13

Code still contains some extraneous logging messages that will be removed when the builder-izing is done. Closes #22 Closes #21 Closes #20 Closes #19 Closes #15 Closes #7 Closes #3

kwnevarez · 2016-07-01T17:59:12Z

Hello @markmcd, @domesticmouse ,
I need some guidance on how to "builder-ize" the post request and its payload. The most recent push uses a basic builder pattern, but its not the model used by the the other API calls. The class upon which all the other API builders are created, PendingResultBase, is tied specifically to the GET verb and I need it to be flexible enough to also call POST.

The specific line is PendingResultBase.java:78 in the method makeRequest(). I'd like to override makeRequest but it, and the variables it uses are private. Do you have any objections to me making them protected so that I can override makeRequest in my GeolocationApiRequest class?

markmcd · 2016-07-04T23:35:57Z

Can you use ApiConfig to define which verb(s) the API uses?

markmcd · 2016-07-04T23:36:02Z

src/main/java/com/google/maps/GeolocationApi.java

+      }
+      ApiException e;
+      // try and fit the older error codes into the new style geo api error formats
+      if(reason.equals("keyInvalid")) {


Can you move these into the ApiException.from method, where the rest of the string-to-exception mapping occurs?

Builder patter utilized in the model of the other API calls. POST payload serialized GeolocationApiRequest and passed as string to POST handler Error mapping moved to ApiException.from() Closes #27 Closes #26 Closes #24 Closes #18 Closes #6 Closes #28

domesticmouse · 2016-09-22T02:58:45Z

src/main/java/com/google/maps/OkHttpRequestHandler.java

@@ -57,32 +60,36 @@ public OkHttpRequestHandler() {

    return new OkHttpPendingResult<T, R>(req, client, clazz, fieldNamingPolicy, errorTimeout);
  }
+  @Override


Seems that you have dropped a bunch of blank lines here. Can you put them back please?

added by auto indent, but now removed

domesticmouse · 2016-09-27T05:52:24Z

src/main/java/com/google/maps/internal/GeolocationResponseAdapter.java

@@ -0,0 +1,122 @@
+package com.google.maps.internal;


This file appears to be missing it's copyright header. Plz add

domesticmouse · 2016-09-27T05:53:07Z

src/main/java/com/google/maps/model/CellTower.java

@@ -0,0 +1,140 @@
+/*
+ * Copyright 2014 Google Inc. All rights reserved.


I'm guessing this is a new file, so it should have a Copyright year of 2016.

domesticmouse · 2016-09-27T05:54:17Z

src/main/java/com/google/maps/model/GeolocationPayload.java

@@ -0,0 +1,165 @@
+/*
+ * Copyright 2014 Google Inc. All rights reserved.


Incorrect copyright year?

domesticmouse · 2016-09-27T05:55:02Z

src/main/java/com/google/maps/model/GeolocationResult.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright 2014 Google Inc. All rights reserved.


domesticmouse · 2016-09-28T03:07:38Z

src/main/java/com/google/maps/GeolocationApi.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright 2014 Google Inc. All rights reserved.


Oops, missed this date.

domesticmouse · 2016-09-28T03:07:52Z

src/main/java/com/google/maps/GeolocationApiRequest.java

@@ -0,0 +1,111 @@
+/*
+ * Copyright 2014 Google Inc. All rights reserved.


And this one.

domesticmouse · 2016-09-30T01:27:28Z

Thanks for working through all the issues in this PR Kenneth!

Adding support for the Geolocation API call.

77b9cc3

domesticmouse reviewed Jun 21, 2016
View reviewed changes

markmcd reviewed Jun 22, 2016
View reviewed changes

kwnevarez added 2 commits June 23, 2016 14:06

Pull Request Comments low hanging fruit.

b2c718a

Closes #9 Closes #17 Closes #16 Closes #1 Closes #2 Closes #4 Closes #8 Closes #14 Closes #10 Closes #12 Closes #11 Closes #13

Issue Fixes

5281ed9

Code still contains some extraneous logging messages that will be removed when the builder-izing is done. Closes #22 Closes #21 Closes #20 Closes #19 Closes #15 Closes #7 Closes #3

markmcd reviewed Jul 4, 2016
View reviewed changes

kwnevarez added 2 commits July 7, 2016 15:09

API now functions correctly in the App Engine environment.

bb21cf9

domesticmouse suggested changes Sep 22, 2016

View reviewed changes

kwnevarez added 2 commits September 26, 2016 10:18

Added back blank lines between methods

13c7aa1

Removed extraneous spacing on blank lines

536418c

added by auto indent, but now removed

domesticmouse suggested changes Sep 27, 2016

View reviewed changes

kwnevarez added 4 commits September 27, 2016 13:18

Adding copyright header.

53dbe6b

domesticmouse suggested changes Sep 28, 2016

View reviewed changes

kwnevarez added 3 commits September 28, 2016 13:04

Added TODO

bbced5c

domesticmouse approved these changes Sep 30, 2016

View reviewed changes

domesticmouse merged commit 6de4f3e into googlemaps:master Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for the Geolocation API call. #164

Adding support for the Geolocation API call. #164

kwnevarez commented Jun 17, 2016

domesticmouse Jun 21, 2016

markmcd Jun 22, 2016

domesticmouse commented Jun 21, 2016

markmcd Jun 22, 2016

kwnevarez commented Jul 1, 2016

markmcd commented Jul 4, 2016

markmcd Jul 4, 2016

domesticmouse Sep 22, 2016

kwnevarez Sep 26, 2016

domesticmouse Sep 27, 2016

kwnevarez Sep 27, 2016

domesticmouse Sep 27, 2016

kwnevarez Sep 27, 2016

domesticmouse Sep 27, 2016

kwnevarez Sep 27, 2016

domesticmouse Sep 27, 2016

kwnevarez Sep 27, 2016

domesticmouse Sep 28, 2016

kwnevarez Sep 28, 2016

domesticmouse Sep 28, 2016

kwnevarez Sep 28, 2016

domesticmouse commented Sep 30, 2016

		@@ -15,13 +15,17 @@

		package com.google.maps;

		import static com.google.appengine.repackaged.com.google.common.base.StringUtil.JsEscapingMode.JSON;

		@@ -0,0 +1,140 @@
		/*
		* Copyright 2014 Google Inc. All rights reserved.

		@@ -0,0 +1,165 @@
		/*
		* Copyright 2014 Google Inc. All rights reserved.

		@@ -0,0 +1,37 @@
		/*
		* Copyright 2014 Google Inc. All rights reserved.

		@@ -0,0 +1,89 @@
		/*
		* Copyright 2014 Google Inc. All rights reserved.

		@@ -0,0 +1,111 @@
		/*
		* Copyright 2014 Google Inc. All rights reserved.

Adding support for the Geolocation API call. #164

Adding support for the Geolocation API call. #164

Conversation

kwnevarez commented Jun 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domesticmouse commented Jun 21, 2016

Choose a reason for hiding this comment

kwnevarez commented Jul 1, 2016

markmcd commented Jul 4, 2016

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

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

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

domesticmouse commented Sep 30, 2016