Add Gisgraphy Geocoder #34

gisgraphy · 2018-02-14T10:28:58Z

Add Gisgraphy Geocoder as a new provider (for geocoding forward and reverse and autocomplete)
Documentation need to be updated.
Some technical choice can be discussed as

the CountryInfo class
what to do if the queryType is not correct (we then consider that it is geocoding), maybe throw an exception can be better
put the DTO in a specific package can improve lisibility.

A try in real environement can also be necessary

Every comments are welcomed

karussell · 2018-02-14T11:56:28Z

Thanks a lot! Will have a look, especially in the choices you mean.

boldtrn · 2018-02-15T06:47:22Z

src/main/java/com/graphhopper/converter/ConverterConfiguration.java

+	}
+
+    @JsonProperty
+	public void setGisgraphyGeocodingUrl(String gisgraphyGeocodingUrl) {


Here URL is Url, but in the converter.yml it is URL. Also the reverseGeocodingURL is all in upper case as well. Since the other services use Url, it might be the best to have everything as "Url"? We could also rename everything "URL", since URL is the common acronym.

It's interesting that this wasn't picked up the tests. Maybe we need better integration tests? When I tried to start the server locally it wouldn't start due to that

ok, I rename all to Upper.

boldtrn · 2018-02-15T06:56:14Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+			@QueryParam("limit") @DefaultValue("5") Integer limit,
+			@QueryParam("querytype") @DefaultValue("geocoding") String queryType) {
+		limit = fixLimit(limit);
+


here it might be worthwhile to check if the parameter input is valid to sort out completely wrong input, like we do in AbstractConverterResource#checkInvalidParameter.

For example if you pass an empty q to a regular geocoding search, it will send a request to https://services.gisgraphy.com/geocoding/?address=&format=json&apikey=..., which will return in a 500 -> which returns "The geocoding provider responded with an unexpected error." to the user.

BTW: In this case I would have expected the server to return a 400 error code "Bad Request" and not a 500 "Internal Server Error". WDYT?

Hi

It make sense for the 400 status code. I need to update this on server side (later). Feel free to provide an issue with a tag 'enhancement' on Gisgraphy github repo

boldtrn · 2018-02-15T07:07:41Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+						+ target.getUri() + " the provider returned: "
+						+ status.code + " - " + status.message);
+				throw new BadRequestException(
+						"error deserializing geocoding feed");


Can we avoid the code duplication here?

yes I can :)

boldtrn · 2018-02-15T07:10:55Z

what to do if the queryType is not correct (we then consider that it is geocoding), maybe throw an exception can be better

I think it might be better to have it similar to the GraphHopper api with reverse=true|false and maybe add another parameter with autocomplete=true|false. That way users don't necessarily rewrite the request when using provider=gisgraphy.

boldtrn · 2018-02-15T07:17:00Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+	@Timed
+	public Response handle(@QueryParam("q") @DefaultValue("") String query,
+			@QueryParam("lat") @DefaultValue("") String lat,
+			@QueryParam("lng") @DefaultValue("") String lng,


The GraphHopper API uses point instead of lat,lng, do you think it makes sense to point instead? I think it would make it easier to integrate this for our users?

why not, but I propose to put default value to "" instead of false as in ConverterResourceNominatim#handle

If autocomplete is true and so does reverse , do we
1/ force reverse to false
2/ ignore autocomplete and consider it is a reverse query
3 / throws an exception
4 / other

Good question. Maybe if autocomplete==true and reverse==true then we could do an autocomplete with location bias :) ?

boldtrn · 2018-02-15T07:20:30Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+				throw new BadRequestException(
+						"error deserializing geocoding feed");
+			}
+		}


As you mentioned, I think here should be indeed another else that throws and exception

boldtrn · 2018-02-15T07:25:42Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+public class ConverterResourceGisgraphy extends AbstractConverterResource {
+
+	// all webservices
+	public static final String ADDRESS_PARAMETER = "address";


Do these need to be public?

boldtrn · 2018-02-15T07:30:47Z

src/main/java/com/graphhopper/converter/api/GisgraphySearchEntry.java

+
+	@JsonProperty("label")
+	public String getLabel() {
+		StringBuffer addressFormated = new StringBuffer();


Would it make more sense to use formatedFull instead?

no formatedFull won't have the house number. Gisgraphy search for street and a street with house number is return. we don't store address in SolR but streets (it allow address interpolation and more)

Could you explain this a bit more? I tried to get a real live example and couldn't find any house numbers via gisgraphy?

I tried for example Universitätsbibliothek, Standort Stadtmitte, in OSM it contains a housenumber. When I looked it up via Gisgraphy fulltext, there is no house number. That doesn't trouble me, but I would like to avoid business logic in the geocoding-converter, if it's avoidable. The simpler the better :). This also means we don't have to update the geocoding-converter if any geocoding provider changes details about their implementation.

That's why I was asking about using formattedFull or for fulltext it seems to be fully_qualified_name.

boldtrn · 2018-02-15T07:34:23Z

src/main/java/com/graphhopper/converter/api/GisgraphyAddressEntry.java

+		this.formatedPostal = formatedPostal;
+	}
+
+	public String getDisplayName() {


Might it be better to use formattedFull?

boldtrn

Thank you very much for this PR! I added a couple of comments.

gisgraphy · 2018-02-15T13:45:59Z

If it is ambiguous for us, it will be ambiguous for the users, I suggest to notify user by an exception that says that something is not logic in the query and propose to set reverse or autocomplete to false; it is not our role to choose. Le 15/02/2018 à 13:07, Peter a écrit :

…

***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java <#34 (comment)>: > + + public ConverterResourceGisgraphy(String geocodingUrl, + String reverseGeocodingUrl, String searchURL, String apiKey, + Client jerseyClient) { + this.geocodingUrl = geocodingUrl; + this.reverseGeocodingUrl = reverseGeocodingUrl; + this.searchURL = searchURL; + this.jerseyClient = jerseyClient; + this.apiKey = apiKey; + } + + @get + @timed + public Response ***@***.***("q") @DefaultValue("") String query, + @QueryParam("lat") @DefaultValue("") String lat, + @QueryParam("lng") @DefaultValue("") String lng, Good question. Maybe if autocomplete==true and reverse==true then we could do an autocomplete with location bias :) ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHS1SGrmWUrmiYyMwN8No6DX-uEwqY8_ks5tVB3kgaJpZM4SFF3o>.

karussell · 2018-02-15T13:50:08Z

ok, true.

Remove duplicate code change Gisgraphy Ressource API (reverse and autocomplete instead of query type, point instead of lat/long) update tests

gisgraphy · 2018-02-15T15:37:48Z

modifications done ! let me know if it is ok to be merged

devemux86 · 2018-02-15T17:11:45Z

Also could use StringBuilder instead of StringBuffer where possible?

use string builder where possible

gisgraphy · 2018-02-15T21:54:55Z

I have done changes to use string builder. Some changes has also been done to display zipcode in the label.

I remove WIP from the name of the PR ;-)

gisgraphy · 2018-02-15T21:55:54Z

There is still some docs and README to update

gisgraphy · 2018-02-15T22:12:00Z

README updated for Gisgraphy.

Everything ok to be merged ?

boldtrn · 2018-02-15T23:51:47Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+	}
+
+	private void checkParameters(String query, boolean reverse, boolean autocomplete, String lat,String lng) {
+		if (query!=null && query.trim().isEmpty() && !reverse){


Nice, the trim is a nice idea, haven't used that in AbstractConverterResource#checkInvalidParameter. Do you think it would make sense to reuse AbstractConverterResource#checkInvalidParameter and just add the autocomplete part to the check? I would like to avoid code duplication.

private checkInvalidParameter(boolean reverse, String query, String point, boolean autocomplete) { super.checkInvalidParameter(reverse, query, point); if (reverse && autocomplete){ throw new BadRequestException("autocomplete is not available in reverse geocoding request, set reverse or autocomplete to false but not both"); }

We could then move this code after the check, as this could create a index out of bounds exception for non empty points that don't contain a ' ,'?:

String lat = null; String lng = null; if (!point.isEmpty()){ String[] cords = point.split(","); lat = cords[0]; lng = cords[1]; }

ok but the AbstractConverterResource#checkInvalidParameter has not the same parameters as ConverterResourceGisgraphy##checkInvalidParameter. Do you want me to modify the signature of AbstractConverterResource ?I don't think add the boolean autocomplete make sense since it means nothing for the AbstractConverterResource. but tell me (we are not force to use the autocomplete parameter)

has been done on server side to calculate the lable instead of client side) improve checkparameters (remove code de-duplication)

…ter.git

boldtrn · 2018-02-20T05:14:07Z

BTW: I have just seen that you used tabs instead of spaces. The whole codebase uses spaces, which will lead to ugly changelogs with the next autoformat. Most editors nowadays use spaces by default and also most language specs recommend using spaces. If it's easy for to changes this, it would be nice if you could do this, otherwise I can do this in another commit before merging.

boldtrn · 2018-02-20T05:17:40Z

src/main/java/com/graphhopper/converter/ConverterApplication.java

+		new ConverterApplication().run(args);
+	}
+
+


Would you mind to check the formatting? Sometimes there are 2 or 3 lines between methods. Is there a reason for this or can we just use one empty line?

Also there is a change for this whole file, probably because of tabs vs spaces. It would be great if we could keep the changeset as small as possible.

Please do it separately. I use Eclipse and appreciate if you can do it in another commit. thanks

I done it but not sure it is correct

boldtrn · 2018-02-20T05:20:05Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+		if (point!=null && !point.isEmpty() && point.indexOf(",")>0){
+			String[] cords = point.split(",");
+         lat = cords[0];
+         lng = cords[1];


Would you mind to check the format here? These two lines use white spaces, all other lines use tabs.

Please do the formatting separately.

I have modified settings in eclipse to use space and reformat, hope it will be better.

boldtrn · 2018-02-20T05:33:47Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+			target = target.queryParam(SEARCH_LIMIT_PARAMETER, limit);
+		}
+		return target;
+	}


Would it be possible to reduce the code duplicateion for the 3 buildTarget methods?

All 3 use these lines:

if (!apiKey.isEmpty()) { target = target.queryParam(APIKEY_PARAMETER, apiKey); } if (lat!=null && !lat.isEmpty()) { target = target.queryParam(LAT_PARAMETER, lat); } if (lng!=null && !lng.isEmpty()) { target = target.queryParam(LONG_PARAMETER, lng); }

2 use these lines:

if (!radius.isEmpty()) { target = target.queryParam(RADIUS_PARAMETER, radius); } if (!country.isEmpty()) { target = target.queryParam(COUNTRY_PARAMETER, country); }

boldtrn · 2018-02-20T05:43:08Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+			@QueryParam("country") @DefaultValue("") String country,
+			@QueryParam("limit") @DefaultValue("5") Integer limit,
+			@QueryParam("reverse") @DefaultValue("false") boolean reverse,
+			@QueryParam("autocomplete") @DefaultValue("false") boolean autocomplete) {


Should we add the locale parameter as well? The other 2 converters have a locale parameter.

AFAIK gisgraphy allows locale input only for autocomplete searches. So adding it might not make sense? WDYT?

no 'locale' is not managed for autocomplete (it is for the full-text web service, but not in suggest). There is a lang parameter but it has an impact on returned fields (and those fields are not returned by the suggest web service). This is not managed at all for geocoding and reverse web services. It might be an improvement but needs some code modifications on the server side that are not planned yet. We have to go without this first ;-)

Ok no problem :), that's why I was asking.

boldtrn · 2018-02-20T05:45:31Z

src/main/java/com/graphhopper/converter/data/CountryInfo.java

+ * @author <a href="mailto:[email protected]">David Masclet</a>
+ * 
+ */
+public class CountryInfo {


Regarding this class, AFAIK you want to add the country to Gisgraphy? Should we then even add this class, wait until the country is available in Gisgraphy, or use the country code for now and switch to the country name, once it's available in the Gisgraphy API?

I suggest keeping this code because other geocoder returns the country name and not the iso-3166-2 one. The code can be done but needs time. The goal is to have a first PR that match as best as possible to ease the migration of users that want to use Gisgraphy. Then we will improve it step by step (as for locale parameter). Gisgraphy is not to be a perfect equivalent to other providers, even if I think the proposed changes make sens. Gisgraphy has some fields that are not present in Nominatim (formatedpostal, azimuthStart, azimuthEnd, length,...) and I am not sure they modify their feed to match Gisgraphy. :-). hopefully, there are some improvements in Gisgraphy that can be done ;-)

WDYT @karussell, should we just use the country code or should we convert the country code into the actual country name?

I'm good with what @gisgraphy said 👍 (offering country name via this converter or did I misunderstood sth :)?)

offering country name via this converter or did I misunderstood sth :)?

No that's correct :). Ok, let's keep it as it is.

boldtrn · 2018-02-20T05:49:18Z

src/main/java/com/graphhopper/converter/core/Converter.java

+
+
+
+


Why are these lines added?

boldtrn · 2018-02-20T05:53:45Z

src/test/java/com/graphhopper/converter/resource/ConverterResourceGisgraphyTest.java

+
+
+
+


Why are these lines added?

removed (bad habits to add few lines before adding code)

I do that as well ;). I try to always check before committing that I remove them again.

boldtrn · 2018-02-20T05:56:08Z

Is there something missing to merge ?

Sorry for the multiple review rounds, I know it's tedious :). I added a couple more comments.

gisgraphy · 2018-02-20T09:44:48Z

no problem, I appreciate review since it makes sense :) hope it will be ok.

clean code (remove empty lines) refactor code to avoid duplicate code

boldtrn · 2018-02-20T21:12:00Z

src/main/java/com/graphhopper/converter/api/GisgraphySearchEntry.java

+    }
+
+
+    @JsonProperty("house_number")


Double empty lines above and below the getCountry :)

boldtrn · 2018-02-20T21:12:26Z

src/main/java/com/graphhopper/converter/api/GisgraphySearchResponse.java

+public class GisgraphySearchResponse {
+
+
+    private Integer numFound = 0;


Double Empty line

boldtrn · 2018-02-20T21:12:41Z

src/main/java/com/graphhopper/converter/api/GisgraphySearchResponseHeader.java

+
+
+
+


boldtrn · 2018-02-20T21:12:53Z

src/main/java/com/graphhopper/converter/api/GisgraphySearchResult.java

+@JsonIgnoreProperties(ignoreUnknown = true)
+public class GisgraphySearchResult {
+
+


boldtrn · 2018-02-20T21:14:54Z

src/main/java/com/graphhopper/converter/data/CountryInfo.java

+ *******************************************************************************/
+package com.graphhopper.converter.data;
+
+import java.io.File;


can we remove the unused import?

boldtrn · 2018-02-20T21:15:26Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+    private static final String SEARCH_LIMIT_PARAMETER = "to";
+    private static final String SEARCH_QUERY_PARAMETER = "q";
+
+


boldtrn · 2018-02-20T21:15:59Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+    }
+
+
+


boldtrn · 2018-02-20T21:29:54Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

+    private WebTarget buildAutocompleteTarget(String query, String lat,
+            String lng, String radius, String country, int limit) {
+        WebTarget target = buildBaseTarget(lat, lng, searchURL);
+        target.queryParam(SEARCH_QUERY_PARAMETER, query)


Here a target = is missing or we have to remove the ; in the line above, also see the failing test.

boldtrn · 2018-02-20T21:33:43Z

Thanks, the formatting looks correct now, you can always check in the files tab. Before there were huge changes in files where you only changed one or two lines.

I think once we fix the failing test, this PR should be pretty much ready to merge. We just need to decide how we handle the country code :).

tests are now ok.

gisgraphy · 2018-02-21T08:59:55Z

modifications are done. For the country field, I plan to do server modifications soon and will propose a new PR when done

do we merge? :)

boldtrn · 2018-02-22T00:09:45Z

src/main/java/com/graphhopper/converter/ConverterApplication.java

    @Override
    public String getName() {
-        return "graphhopper-nominatim-converter";
+        return "graphhopper-geocoder-converter";


Ah, good catch, thanks :)

boldtrn

LGTM. Thanks for this nice PR. Will merge this now.

karussell · 2018-02-22T12:48:05Z

Thank you all will now bring this into production - exciting :) !

karussell · 2018-03-26T11:17:30Z

@gisgraphy is it already possible to search for parking places through this converter?

gisgraphy · 2018-03-26T11:27:35Z

to search by name, the URL is something like http://services.gisgraphy.com/fulltext/?q=sebastopol&style=long&placetype=parking

or name / GPS : http://services.gisgraphy.com/fulltext/?q=sebastopol&style=long&placetype=parking&lat=48.8566101&lng=2.3514992

fulltext webservice is possible but the placetype parameter is not mapped in the converter

or by GPS position :
http://services.gisgraphy.com/geoloc/search?format=json&lat=48.856610100000005&lng=2.3514992&placetype=Parking

geoloc webservice is not available yet in the converter.

so it need some developpement

gisgraphy · 2018-03-26T11:33:26Z

there is a lot of possibilities in Gisgraphy that are possible but this PR was a first one. maybe add an enhancement/issues to track this. I am currently busy and have no time to do that soon :(

karussell · 2018-03-26T12:00:17Z

Really no problem and no hurry. Will create an issue and maybe @boldtrn or I will create time for this

#46

gisgraphy added 2 commits February 14, 2018 11:19

Add Gisgraphy geocoder

f345ef9

remove unused function

20b2b8a

karussell requested a review from boldtrn February 14, 2018 11:56

boldtrn reviewed Feb 15, 2018

View reviewed changes

boldtrn requested changes Feb 15, 2018

View reviewed changes

Put Config param to URL (in uppercase)

35175dd

Remove duplicate code change Gisgraphy Ressource API (reverse and autocomplete instead of query type, point instead of lat/long) update tests

update dto for better zipcode management for label

dca79af

use string builder where possible

gisgraphy changed the title ~~[WIP] Add Gisgraphy Geocoder~~ Add Gisgraphy Geocoder Feb 15, 2018

Update README.md

6e8588f

This was referenced Feb 15, 2018

Server Status Code 500 vs. 400 gisgraphy/gisgraphy#19

Open

Provide Country Name vs. Country Code gisgraphy/gisgraphy#20

Closed

boldtrn reviewed Feb 15, 2018

View reviewed changes

gisgraphy added 2 commits February 16, 2018 14:02

update label to take the value returned by the server (Some modification

17b219b

has been done on server side to calculate the lable instead of client side) improve checkparameters (remove code de-duplication)

Merge branch 'master' of https://github.com/gisgraphy/geocoder-conver…

fb671a5

…ter.git

boldtrn reviewed Feb 20, 2018

View reviewed changes

reformat code (tabs=>space)

da642c5

clean code (remove empty lines) refactor code to avoid duplicate code

boldtrn reviewed Feb 20, 2018

View reviewed changes

src/main/java/com/graphhopper/converter/api/GisgraphySearchResponseHeader.java

Copy link

Member

boldtrn Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

boldtrn reviewed Feb 20, 2018

View reviewed changes

src/main/java/com/graphhopper/converter/resources/ConverterResourceGisgraphy.java

}

Copy link

Member

boldtrn Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

boldtrn reviewed Feb 20, 2018

View reviewed changes

clean code

4a512aa

tests are now ok.

boldtrn reviewed Feb 22, 2018

View reviewed changes

boldtrn approved these changes Feb 22, 2018

View reviewed changes

boldtrn merged commit c43c773 into graphhopper:master Feb 22, 2018

		public class GisgraphySearchResponse {


		private Integer numFound = 0;

		@JsonIgnoreProperties(ignoreUnknown = true)
		public class GisgraphySearchResult {

		private static final String SEARCH_LIMIT_PARAMETER = "to";
		private static final String SEARCH_QUERY_PARAMETER = "q";

Add Gisgraphy Geocoder #34

Add Gisgraphy Geocoder #34

Conversation

gisgraphy commented Feb 14, 2018

karussell commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boldtrn Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

boldtrn Feb 15, 2018 • edited Loading

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

boldtrn commented Feb 15, 2018

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

boldtrn left a comment

Choose a reason for hiding this comment

gisgraphy commented Feb 15, 2018 via email

karussell commented Feb 15, 2018 • edited Loading

gisgraphy commented Feb 15, 2018

devemux86 commented Feb 15, 2018

gisgraphy commented Feb 15, 2018

gisgraphy commented Feb 15, 2018

gisgraphy commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boldtrn commented Feb 20, 2018

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

boldtrn Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell Feb 20, 2018 • edited Loading

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

boldtrn commented Feb 20, 2018

gisgraphy commented Feb 20, 2018

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

boldtrn Feb 20, 2018 • edited Loading

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

boldtrn commented Feb 20, 2018

gisgraphy commented Feb 21, 2018

Choose a reason for hiding this comment

boldtrn Feb 15, 2018 •

edited

Loading

boldtrn Feb 15, 2018 •

edited

Loading

karussell commented Feb 15, 2018 •

edited

Loading

boldtrn Feb 20, 2018 •

edited

Loading

karussell Feb 20, 2018 •

edited

Loading

boldtrn Feb 20, 2018 •

edited

Loading

karussell commented Mar 26, 2018 •

edited

Loading