From cfd208368a285a20daa5f8c40f761bbd2733b8af Mon Sep 17 00:00:00 2001 From: Holger Bruch Date: Fri, 12 Oct 2018 15:29:27 +0200 Subject: [PATCH 1/6] Add bbox parameter Signed-off-by: Holger Bruch # Conflicts: # src/main/java/de/komoot/photon/query/PhotonRequestFactory.java --- README.md | 6 +++++ .../photon/query/FilteredPhotonRequest.java | 5 ++-- .../photon/query/PhotonQueryBuilder.java | 15 ++++++++++++ .../de/komoot/photon/query/PhotonRequest.java | 9 +++++++- .../photon/query/PhotonRequestFactory.java | 23 +++++++++++++++---- .../photon/query/ReverseQueryBuilder.java | 6 +++++ .../photon/query/TagFilterQueryBuilder.java | 8 +++++++ .../FilteredPhotonRequestHandler.java | 3 ++- .../searcher/SimplePhotonRequestHandler.java | 3 ++- .../query/FilteredPhotonRequestTest.java | 12 +++++----- .../query/PhotonRequestFactoryTest.java | 19 +++++++++++++++ 11 files changed, 94 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index b791ac248..f4ecfd177 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,7 @@ Photon software is open source and licensed under [Apache License, Version 2.0]( - location bias - typo tolerance - filter by osm tag and value +- filter by bounding box - reverse geocode a coordinate to an address - OSM data import (built upon [Nominatim](https://github.com/twain47/Nominatim)) inclusive continuous updates @@ -134,6 +135,11 @@ http://localhost:2322/api?q=berlin&limit=2 http://localhost:2322/api?q=berlin&lang=it ``` +#### Filter results by bounding box +``` +http://localhost:2322/api?q=berlin&bbox=9.5,51.5,11.5,53.5 +``` + #### Filter results by [tags and values](http://taginfo.openstreetmap.org/projects/nominatim#tags) *Note: not all tags on [link in the title](http://taginfo.openstreetmap.org/projects/nominatim#tags) are supported. Please see [nominatim source](https://github.com/openstreetmap/osm2pgsql/blob/master/output-gazetteer.cpp#L81) for an accurate list.* If one or many query parameters named ```osm_tag``` are present, photon will attempt to filter results by those tags. In general, here is the expected format (syntax) for the value of osm_tag request parameters. diff --git a/src/main/java/de/komoot/photon/query/FilteredPhotonRequest.java b/src/main/java/de/komoot/photon/query/FilteredPhotonRequest.java index 1712b8af1..77c079383 100644 --- a/src/main/java/de/komoot/photon/query/FilteredPhotonRequest.java +++ b/src/main/java/de/komoot/photon/query/FilteredPhotonRequest.java @@ -1,5 +1,6 @@ package de.komoot.photon.query; +import com.vividsolutions.jts.geom.Envelope; import com.vividsolutions.jts.geom.Point; import java.util.HashMap; @@ -20,8 +21,8 @@ public class FilteredPhotonRequest extends PhotonRequest { private Map> excludeTags = new HashMap>(3); private Map> excludeTagValues = new HashMap>(3); - FilteredPhotonRequest(String query, int limit, Point locationForBias, double locBiasScale, String language) { - super(query, limit, locationForBias, locBiasScale, language); + FilteredPhotonRequest(String query, int limit, Envelope bbox, Point locationForBias, double locBiasScale, String language) { + super(query, limit, bbox, locationForBias, locBiasScale, language); } public Set keys() { diff --git a/src/main/java/de/komoot/photon/query/PhotonQueryBuilder.java b/src/main/java/de/komoot/photon/query/PhotonQueryBuilder.java index 1fd7210a2..0084677f2 100644 --- a/src/main/java/de/komoot/photon/query/PhotonQueryBuilder.java +++ b/src/main/java/de/komoot/photon/query/PhotonQueryBuilder.java @@ -2,6 +2,7 @@ import com.google.common.collect.ImmutableSet; +import com.vividsolutions.jts.geom.Envelope; import com.vividsolutions.jts.geom.Point; import org.elasticsearch.common.lucene.search.function.CombineFunction; import org.elasticsearch.common.lucene.search.function.FiltersFunctionScoreQuery.ScoreMode; @@ -51,6 +52,8 @@ public class PhotonQueryBuilder implements TagFilterQueryBuilder { private MatchQueryBuilder languageMatchQueryBuilder; + private GeoBoundingBoxQueryBuilder bboxQueryBuilder; + private QueryBuilder m_finalQueryBuilder; protected ArrayList m_alFilterFunction4QueryBuilder = new ArrayList<>(1); @@ -133,6 +136,15 @@ public TagFilterQueryBuilder withLocationBias(Point point, double scale) { .boostMode(CombineFunction.MULTIPLY); return this; } + + @Override + public TagFilterQueryBuilder withBoundingBox(Envelope bbox) { + if (bbox == null) return this; + bboxQueryBuilder = new GeoBoundingBoxQueryBuilder("coordinate"); + bboxQueryBuilder.setCorners(bbox.getMaxY(), bbox.getMinX(), bbox.getMinY(), bbox.getMaxX()); + + return this; + } @Override public TagFilterQueryBuilder withTags(Map> tags) { @@ -314,6 +326,9 @@ public QueryBuilder buildQuery() { m_queryBuilderForTopLevelFilter.must(andQueryBuilderForExcludeTagFiltering); } + + if (bboxQueryBuilder != null) + m_queryBuilderForTopLevelFilter.filter(bboxQueryBuilder); state = State.FINISHED; diff --git a/src/main/java/de/komoot/photon/query/PhotonRequest.java b/src/main/java/de/komoot/photon/query/PhotonRequest.java index a3bdd30bf..86bb18831 100644 --- a/src/main/java/de/komoot/photon/query/PhotonRequest.java +++ b/src/main/java/de/komoot/photon/query/PhotonRequest.java @@ -1,5 +1,6 @@ package de.komoot.photon.query; +import com.vividsolutions.jts.geom.Envelope; import com.vividsolutions.jts.geom.Point; import java.io.Serializable; @@ -13,13 +14,15 @@ public class PhotonRequest implements Serializable { private Point locationForBias; private String language; private final double scale; + private Envelope bbox; - public PhotonRequest(String query, int limit, Point locationForBias, double scale, String language) { + public PhotonRequest(String query, int limit, Envelope bbox, Point locationForBias, double scale, String language) { this.query = query; this.limit = limit; this.locationForBias = locationForBias; this.scale = scale; this.language = language; + this.bbox = bbox; } public String getQuery() { @@ -29,6 +32,10 @@ public String getQuery() { public Integer getLimit() { return limit; } + + public Envelope getBbox() { + return bbox; + } public Point getLocationForBias() { return locationForBias; diff --git a/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java b/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java index 5b8b61f83..e116357ae 100644 --- a/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java +++ b/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java @@ -1,5 +1,7 @@ package de.komoot.photon.query; +import com.vividsolutions.jts.geom.Coordinate; +import com.vividsolutions.jts.geom.Envelope; import com.vividsolutions.jts.geom.Point; import spark.QueryParamsMap; import spark.Request; @@ -17,8 +19,7 @@ public class PhotonRequestFactory { private final static LocationParamConverter optionalLocationParamConverter = new LocationParamConverter(false); protected static HashSet m_hsRequestQueryParams = new HashSet<>(Arrays.asList("lang", "q", "lon", "lat", - "limit", "osm_tag", "location_bias_scale", "debug")); - + "limit", "osm_tag", "location_bias_scale", "bbox", "debug")); public PhotonRequestFactory(Set supportedLanguages) { this.languageChecker = new LanguageChecker(supportedLanguages); } @@ -42,7 +43,21 @@ public R create(Request webRequest) throws BadRequestE } catch (NumberFormatException e) { limit = 15; } + Point locationForBias = optionalLocationParamConverter.apply(webRequest); + + Envelope bbox = null; + try { + String bboxParam = webRequest.queryParams("bbox"); + String[] bboxCoords = bboxParam.split(","); + bbox = new Envelope( + Double.valueOf(bboxCoords[0]), + Double.valueOf(bboxCoords[2]), + Double.valueOf(bboxCoords[1]), + Double.valueOf(bboxCoords[3])); + } catch (Exception nfe) { + //ignore + } // don't use too high default value, see #306 double scale = 1.6; @@ -56,9 +71,9 @@ public R create(Request webRequest) throws BadRequestE QueryParamsMap tagFiltersQueryMap = webRequest.queryMap("osm_tag"); if (!new CheckIfFilteredRequest().execute(tagFiltersQueryMap)) { - return (R) new PhotonRequest(query, limit, locationForBias, scale, language); + return (R) new PhotonRequest(query, limit, bbox, locationForBias, scale, language); } - FilteredPhotonRequest photonRequest = new FilteredPhotonRequest(query, limit, locationForBias, scale, language); + FilteredPhotonRequest photonRequest = new FilteredPhotonRequest(query, limit, bbox, locationForBias, scale, language); String[] tagFilters = tagFiltersQueryMap.values(); setUpTagFilters(photonRequest, tagFilters); diff --git a/src/main/java/de/komoot/photon/query/ReverseQueryBuilder.java b/src/main/java/de/komoot/photon/query/ReverseQueryBuilder.java index 65fa8ae23..e5f56b99c 100644 --- a/src/main/java/de/komoot/photon/query/ReverseQueryBuilder.java +++ b/src/main/java/de/komoot/photon/query/ReverseQueryBuilder.java @@ -1,6 +1,7 @@ package de.komoot.photon.query; import com.google.common.collect.ImmutableSet; +import com.vividsolutions.jts.geom.Envelope; import com.vividsolutions.jts.geom.Point; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.index.query.BoolQueryBuilder; @@ -45,6 +46,11 @@ public TagFilterQueryBuilder withLocationBias(Point point, double scale) { throw new RuntimeException(new NoSuchMethodException("this method is not implemented (NOOP)")); } + @Override + public TagFilterQueryBuilder withBoundingBox(Envelope bbox) { + throw new RuntimeException(new NoSuchMethodException("this method is not implemented (NOOP)")); + } + @Override public TagFilterQueryBuilder withTags(Map> tags) { throw new RuntimeException(new NoSuchMethodException("this method is not implemented (NOOP)")); diff --git a/src/main/java/de/komoot/photon/query/TagFilterQueryBuilder.java b/src/main/java/de/komoot/photon/query/TagFilterQueryBuilder.java index a801c94be..42477fcac 100644 --- a/src/main/java/de/komoot/photon/query/TagFilterQueryBuilder.java +++ b/src/main/java/de/komoot/photon/query/TagFilterQueryBuilder.java @@ -1,5 +1,6 @@ package de.komoot.photon.query; +import com.vividsolutions.jts.geom.Envelope; import com.vividsolutions.jts.geom.Point; import org.elasticsearch.index.query.QueryBuilder; @@ -21,6 +22,13 @@ public interface TagFilterQueryBuilder { */ TagFilterQueryBuilder withLimit(Integer limit); + /** + * Search results will be filtered to places contained in the bounding box (WGS84) provided in the argument. + * + * @param bbox Geographical {@link Envelope} + */ + TagFilterQueryBuilder withBoundingBox(Envelope bbox); + /** * Location bias for query. By setting this, places found will be sorted by proximity to this point. * diff --git a/src/main/java/de/komoot/photon/searcher/FilteredPhotonRequestHandler.java b/src/main/java/de/komoot/photon/searcher/FilteredPhotonRequestHandler.java index 1e6b21c2d..1a51cebc8 100644 --- a/src/main/java/de/komoot/photon/searcher/FilteredPhotonRequestHandler.java +++ b/src/main/java/de/komoot/photon/searcher/FilteredPhotonRequestHandler.java @@ -35,7 +35,8 @@ public TagFilterQueryBuilder buildQuery(FilteredPhotonRequest photonRequest) { withoutKeys(excludeKeys). withoutValues(excludeValues). withTagsNotValues(excludeTagValues). - withLocationBias(photonRequest.getLocationForBias(), photonRequest.getScaleForBias()); + withLocationBias(photonRequest.getLocationForBias(), photonRequest.getScaleForBias()). + withBoundingBox(photonRequest.getBbox()); } } diff --git a/src/main/java/de/komoot/photon/searcher/SimplePhotonRequestHandler.java b/src/main/java/de/komoot/photon/searcher/SimplePhotonRequestHandler.java index 8904aca80..ab15dd21a 100644 --- a/src/main/java/de/komoot/photon/searcher/SimplePhotonRequestHandler.java +++ b/src/main/java/de/komoot/photon/searcher/SimplePhotonRequestHandler.java @@ -15,6 +15,7 @@ public SimplePhotonRequestHandler(ElasticsearchSearcher elasticsearchSearcher) { @Override public TagFilterQueryBuilder buildQuery(PhotonRequest photonRequest) { return PhotonQueryBuilder.builder(photonRequest.getQuery(), photonRequest.getLanguage()). - withLocationBias(photonRequest.getLocationForBias(), photonRequest.getScaleForBias()); + withLocationBias(photonRequest.getLocationForBias(), photonRequest.getScaleForBias()). + withBoundingBox(photonRequest.getBbox()); } } diff --git a/src/test/java/de/komoot/photon/query/FilteredPhotonRequestTest.java b/src/test/java/de/komoot/photon/query/FilteredPhotonRequestTest.java index 7823642d9..d4fd6ee3f 100644 --- a/src/test/java/de/komoot/photon/query/FilteredPhotonRequestTest.java +++ b/src/test/java/de/komoot/photon/query/FilteredPhotonRequestTest.java @@ -14,7 +14,7 @@ public class FilteredPhotonRequestTest { @Test public void testNotKey() { - FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, 0, null); + FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, null, 0, null); filteredPhotonRequest.notKeys("exclude"); filteredPhotonRequest.notKeys("exclude"); filteredPhotonRequest.notKeys("anotherExclude"); @@ -27,7 +27,7 @@ public void testNotKey() { @Test public void testNotTag() { - FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, 0, null); + FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, null, 0, null); filteredPhotonRequest.notTags("aKey", ImmutableSet.of("aValue")); filteredPhotonRequest.notTags("anotherKey", ImmutableSet.of("anotherValue")); Map> excludeTags = filteredPhotonRequest.notTags(); @@ -37,7 +37,7 @@ public void testNotTag() { @Test public void testNotValue() { - FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, 0, null); + FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, null, 0, null); filteredPhotonRequest.notValues("exclude"); filteredPhotonRequest.notValues("exclude"); filteredPhotonRequest.notValues("anotherExclude"); @@ -48,7 +48,7 @@ public void testNotValue() { @Test public void testKey() { - FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, 0, null); + FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, null, 0, null); filteredPhotonRequest.keys("keyToInclude"); filteredPhotonRequest.keys("keyToInclude"); filteredPhotonRequest.keys("anotherKeyToInclude"); @@ -57,7 +57,7 @@ public void testKey() { @Test public void testTag() { - FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, 0, null); + FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, null, 0, null); filteredPhotonRequest.tags("aKey", ImmutableSet.of("aValue")); filteredPhotonRequest.tags("anotherKey", ImmutableSet.of("anotherValue")); Map> includeTags = filteredPhotonRequest.tags(); @@ -68,7 +68,7 @@ public void testTag() { @Test public void testValue() { - FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, 0, null); + FilteredPhotonRequest filteredPhotonRequest = new FilteredPhotonRequest(null, 0, null, null, 0, null); filteredPhotonRequest.values("keyToInclude"); filteredPhotonRequest.values("keyToInclude"); filteredPhotonRequest.values("anotherKeyToInclude"); diff --git a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java index da9bf202f..a2ed34f0f 100644 --- a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java +++ b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java @@ -2,6 +2,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.vividsolutions.jts.geom.Envelope; + import org.junit.Assert; import org.junit.Test; import org.mockito.Mockito; @@ -202,4 +204,21 @@ public void testWithExcludeValueFilter() throws Exception { Mockito.verify(mockOsmTagQueryParm, Mockito.times(2)).values(); Assert.assertEquals(ImmutableSet.of("aValue"), filteredPhotonRequest.notValues()); } + + @Test + public void testWithBboxFilter() throws Exception { + Request mockRequest = Mockito.mock(Request.class); + Mockito.when(mockRequest.queryParams("q")).thenReturn("hanover"); + QueryParamsMap mockQueryParamsMap = Mockito.mock(QueryParamsMap.class); + Mockito.when(mockRequest.queryMap("osm_tag")).thenReturn(mockQueryParamsMap); + Mockito.when(mockRequest.queryParams("bbox")).thenReturn("9.6,52.4,9.8,52.3"); + + PhotonRequestFactory photonRequestFactory = new PhotonRequestFactory(ImmutableSet.of("en")); + PhotonRequest photonRequest = photonRequestFactory.create(mockRequest); + + Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); + Assert.assertEquals(new Envelope(9.6, 9.8, 52.3, 52.4), photonRequest.getBbox()); + + } + } \ No newline at end of file From f6f5dfbb1cb8cc3fdddd2850842eb77c43e8497f Mon Sep 17 00:00:00 2001 From: Holger Bruch Date: Sat, 13 Oct 2018 06:55:19 +0200 Subject: [PATCH 2/6] Add documentation and validation of bbox coords order Signed-off-by: Holger Bruch # Conflicts: # src/main/java/de/komoot/photon/query/PhotonRequestFactory.java --- README.md | 1 + .../query/BoundingBoxParamConverter.java | 45 +++++++++++++++++++ .../photon/query/PhotonRequestFactory.java | 16 ++----- .../query/PhotonRequestFactoryTest.java | 18 ++++++++ 4 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java diff --git a/README.md b/README.md index f4ecfd177..8e3f63d17 100644 --- a/README.md +++ b/README.md @@ -136,6 +136,7 @@ http://localhost:2322/api?q=berlin&lang=it ``` #### Filter results by bounding box +Expected format is minLon,minLat,maxLon,maxLat. ``` http://localhost:2322/api?q=berlin&bbox=9.5,51.5,11.5,53.5 ``` diff --git a/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java b/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java new file mode 100644 index 000000000..8a36d5198 --- /dev/null +++ b/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java @@ -0,0 +1,45 @@ +package de.komoot.photon.query; + +import com.vividsolutions.jts.geom.Envelope; + +import de.komoot.photon.utils.Function; +import spark.Request; + +/** + * Converts the bbox parameter into an Envelope and performs format checking. + * Created by Holger Bruch on 10/13/2018. + */ +public class BoundingBoxParamConverter implements Function { + + private static final String INVALID_BBOX_ERROR_MESSAGE = "invalid search term 'bbox', expected format is: minLon,minLat,maxLon,maxLat"; + + @Override + public Envelope apply(Request webRequest) throws BadRequestException { + String bboxParam = webRequest.queryParams("bbox"); + if (bboxParam == null) { + return null; + } + Envelope bbox = null; + try { + String[] bboxCoords = bboxParam.split(","); + if (bboxCoords.length != 4) { + throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); + } + Double minLon = Double.valueOf(bboxCoords[0]); + Double minLat = Double.valueOf(bboxCoords[2]); + Double maxLon = Double.valueOf(bboxCoords[1]); + Double maxLat = Double.valueOf(bboxCoords[3]); + if (minLon > 180.0 || minLon < -180.00 || maxLon > 180.0 || maxLon < -180.00) { + throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); + } + if (minLat > 90.0 || minLat < -90.00 || maxLat > 90.0 || maxLat < -90.00) { + throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); + } + bbox = new Envelope(minLon, minLat, maxLon, maxLat); + } catch (NumberFormatException nfe) { + throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); + } + + return bbox; + } +} diff --git a/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java b/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java index e116357ae..bf42f7728 100644 --- a/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java +++ b/src/main/java/de/komoot/photon/query/PhotonRequestFactory.java @@ -17,11 +17,13 @@ public class PhotonRequestFactory { private final LanguageChecker languageChecker; private final static LocationParamConverter optionalLocationParamConverter = new LocationParamConverter(false); + private final BoundingBoxParamConverter bboxParamConverter; protected static HashSet m_hsRequestQueryParams = new HashSet<>(Arrays.asList("lang", "q", "lon", "lat", "limit", "osm_tag", "location_bias_scale", "bbox", "debug")); public PhotonRequestFactory(Set supportedLanguages) { this.languageChecker = new LanguageChecker(supportedLanguages); + this.bboxParamConverter = new BoundingBoxParamConverter(); } public R create(Request webRequest) throws BadRequestException { @@ -45,19 +47,7 @@ public R create(Request webRequest) throws BadRequestE } Point locationForBias = optionalLocationParamConverter.apply(webRequest); - - Envelope bbox = null; - try { - String bboxParam = webRequest.queryParams("bbox"); - String[] bboxCoords = bboxParam.split(","); - bbox = new Envelope( - Double.valueOf(bboxCoords[0]), - Double.valueOf(bboxCoords[2]), - Double.valueOf(bboxCoords[1]), - Double.valueOf(bboxCoords[3])); - } catch (Exception nfe) { - //ignore - } + Envelope bbox = bboxParamConverter.apply(webRequest); // don't use too high default value, see #306 double scale = 1.6; diff --git a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java index a2ed34f0f..37e19338c 100644 --- a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java +++ b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java @@ -218,7 +218,25 @@ public void testWithBboxFilter() throws Exception { Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); Assert.assertEquals(new Envelope(9.6, 9.8, 52.3, 52.4), photonRequest.getBbox()); + } + + @Test + public void testWithBadBboxFilter() throws Exception { + Request mockRequest = Mockito.mock(Request.class); + Mockito.when(mockRequest.queryParams("q")).thenReturn("hanover"); + QueryParamsMap mockQueryParamsMap = Mockito.mock(QueryParamsMap.class); + Mockito.when(mockRequest.queryMap("osm_tag")).thenReturn(mockQueryParamsMap); + Mockito.when(mockRequest.queryParams("bbox")).thenReturn("9.6,91,9.8,93"); + try { + PhotonRequestFactory photonRequestFactory = new PhotonRequestFactory(ImmutableSet.of("en")); + photonRequestFactory.create(mockRequest); + Assert.fail(); + } catch (BadRequestException e) { + Assert.assertEquals("invalid search term 'bbox', expected format is: minLon,minLat,maxLon,maxLat", e.getMessage()); + } + Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); } + } \ No newline at end of file From 56c299b1c22bb508e4b77ef6a11c2167afc88ca3 Mon Sep 17 00:00:00 2001 From: Pascal Blunk Date: Fri, 14 Jun 2019 16:42:16 +0200 Subject: [PATCH 3/6] slightly improved readability The previous test request was not consistent with the spec given in the documentation. I just swapped the latitude values. --- .../java/de/komoot/photon/query/PhotonRequestFactoryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java index 37e19338c..170c53304 100644 --- a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java +++ b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java @@ -211,7 +211,7 @@ public void testWithBboxFilter() throws Exception { Mockito.when(mockRequest.queryParams("q")).thenReturn("hanover"); QueryParamsMap mockQueryParamsMap = Mockito.mock(QueryParamsMap.class); Mockito.when(mockRequest.queryMap("osm_tag")).thenReturn(mockQueryParamsMap); - Mockito.when(mockRequest.queryParams("bbox")).thenReturn("9.6,52.4,9.8,52.3"); + Mockito.when(mockRequest.queryParams("bbox")).thenReturn("9.6,52.3,9.8,52.4"); PhotonRequestFactory photonRequestFactory = new PhotonRequestFactory(ImmutableSet.of("en")); PhotonRequest photonRequest = photonRequestFactory.create(mockRequest); From b729e93902a1a037e3e1bf53bb5cb5a7295aad19 Mon Sep 17 00:00:00 2001 From: Pascal Blunk Date: Fri, 14 Jun 2019 17:33:38 +0200 Subject: [PATCH 4/6] fixed broken bounding box creation from input params --- .../de/komoot/photon/query/BoundingBoxParamConverter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java b/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java index 8a36d5198..8b8569626 100644 --- a/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java +++ b/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java @@ -26,8 +26,8 @@ public Envelope apply(Request webRequest) throws BadRequestException { throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); } Double minLon = Double.valueOf(bboxCoords[0]); - Double minLat = Double.valueOf(bboxCoords[2]); - Double maxLon = Double.valueOf(bboxCoords[1]); + Double minLat = Double.valueOf(bboxCoords[1]); + Double maxLon = Double.valueOf(bboxCoords[2]); Double maxLat = Double.valueOf(bboxCoords[3]); if (minLon > 180.0 || minLon < -180.00 || maxLon > 180.0 || maxLon < -180.00) { throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); @@ -35,7 +35,7 @@ public Envelope apply(Request webRequest) throws BadRequestException { if (minLat > 90.0 || minLat < -90.00 || maxLat > 90.0 || maxLat < -90.00) { throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); } - bbox = new Envelope(minLon, minLat, maxLon, maxLat); + bbox = new Envelope(minLon, maxLon, minLat, maxLat); } catch (NumberFormatException nfe) { throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); } From a51ff2903c908d9434621876e4039dd38cf480ef Mon Sep 17 00:00:00 2001 From: Pascal Blunk Date: Fri, 14 Jun 2019 19:46:44 +0200 Subject: [PATCH 5/6] added test for wrong longitude --- .../query/PhotonRequestFactoryTest.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java index 170c53304..723d7503e 100644 --- a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java +++ b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java @@ -237,6 +237,23 @@ public void testWithBadBboxFilter() throws Exception { } Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); } - + + @Test + public void testWithBadBboxFilter180() throws Exception { + Request mockRequest = Mockito.mock(Request.class); + Mockito.when(mockRequest.queryParams("q")).thenReturn("hanover"); + QueryParamsMap mockQueryParamsMap = Mockito.mock(QueryParamsMap.class); + Mockito.when(mockRequest.queryMap("osm_tag")).thenReturn(mockQueryParamsMap); + Mockito.when(mockRequest.queryParams("bbox")).thenReturn("181,27,9.8,87"); + + try { + PhotonRequestFactory photonRequestFactory = new PhotonRequestFactory(ImmutableSet.of("en")); + photonRequestFactory.create(mockRequest); + Assert.fail(); + } catch (BadRequestException e) { + Assert.assertEquals("invalid search term 'bbox', expected format is: minLon,minLat,maxLon,maxLat", e.getMessage()); + } + Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); + } } \ No newline at end of file From 9b773f97515bd1bf1364764d8af3aace9f087502 Mon Sep 17 00:00:00 2001 From: Pascal Blunk Date: Fri, 14 Jun 2019 20:15:51 +0200 Subject: [PATCH 6/6] added proper tests and new exception description --- .../query/BoundingBoxParamConverter.java | 7 ++-- .../query/PhotonRequestFactoryTest.java | 39 ++++++++++++++----- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java b/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java index 8b8569626..d29b4fc90 100644 --- a/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java +++ b/src/main/java/de/komoot/photon/query/BoundingBoxParamConverter.java @@ -11,7 +11,8 @@ */ public class BoundingBoxParamConverter implements Function { - private static final String INVALID_BBOX_ERROR_MESSAGE = "invalid search term 'bbox', expected format is: minLon,minLat,maxLon,maxLat"; + public static final String INVALID_BBOX_ERROR_MESSAGE = "invalid number of supplied coordinates for parameter 'bbox', expected format is: minLon,minLat,maxLon,maxLat"; + public static final String INVALID_BBOX_BOUNDS_MESSAGE = "Invalid bounds for parameter bbox, expected values minLat, maxLat element [-90,90], minLon, maxLon element [-180,180]"; @Override public Envelope apply(Request webRequest) throws BadRequestException { @@ -30,10 +31,10 @@ public Envelope apply(Request webRequest) throws BadRequestException { Double maxLon = Double.valueOf(bboxCoords[2]); Double maxLat = Double.valueOf(bboxCoords[3]); if (minLon > 180.0 || minLon < -180.00 || maxLon > 180.0 || maxLon < -180.00) { - throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); + throw new BadRequestException(400, INVALID_BBOX_BOUNDS_MESSAGE); } if (minLat > 90.0 || minLat < -90.00 || maxLat > 90.0 || maxLat < -90.00) { - throw new BadRequestException(400, INVALID_BBOX_ERROR_MESSAGE); + throw new BadRequestException(400, INVALID_BBOX_BOUNDS_MESSAGE); } bbox = new Envelope(minLon, maxLon, minLat, maxLat); } catch (NumberFormatException nfe) { diff --git a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java index 723d7503e..372411f53 100644 --- a/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java +++ b/src/test/java/de/komoot/photon/query/PhotonRequestFactoryTest.java @@ -219,41 +219,60 @@ public void testWithBboxFilter() throws Exception { Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); Assert.assertEquals(new Envelope(9.6, 9.8, 52.3, 52.4), photonRequest.getBbox()); } - + @Test - public void testWithBadBboxFilter() throws Exception { + public void testWithBboxFilterWrongNumberOfInputs() throws Exception { Request mockRequest = Mockito.mock(Request.class); Mockito.when(mockRequest.queryParams("q")).thenReturn("hanover"); QueryParamsMap mockQueryParamsMap = Mockito.mock(QueryParamsMap.class); Mockito.when(mockRequest.queryMap("osm_tag")).thenReturn(mockQueryParamsMap); - Mockito.when(mockRequest.queryParams("bbox")).thenReturn("9.6,91,9.8,93"); - + Mockito.when(mockRequest.queryParams("bbox")).thenReturn("9.6,52.3,9.8"); + try { PhotonRequestFactory photonRequestFactory = new PhotonRequestFactory(ImmutableSet.of("en")); - photonRequestFactory.create(mockRequest); + photonRequestFactory.create(mockRequest); Assert.fail(); } catch (BadRequestException e) { - Assert.assertEquals("invalid search term 'bbox', expected format is: minLon,minLat,maxLon,maxLat", e.getMessage()); + Assert.assertEquals(BoundingBoxParamConverter.INVALID_BBOX_ERROR_MESSAGE, e.getMessage()); } Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); } + + @Test + public void testWithBadBboxFilterMinLat90() throws Exception { + testBoundingBoxResponse(9.6,-92,9.8,14); + } + + @Test + public void testWithBadBboxFilterMaxLat90() throws Exception { + testBoundingBoxResponse(9.6,14,9.8,91); + } + + @Test + public void testWithBadBboxFilterMinLon180() throws Exception { + testBoundingBoxResponse(-181, 9, 4, 12); + } @Test - public void testWithBadBboxFilter180() throws Exception { + public void testWithBadBboxFilterMaxLon180() throws Exception { + testBoundingBoxResponse(12, 9, 181, 12); + } + + + public void testBoundingBoxResponse(double minLon, double minLat, double maxLon, double maxLat) { Request mockRequest = Mockito.mock(Request.class); Mockito.when(mockRequest.queryParams("q")).thenReturn("hanover"); QueryParamsMap mockQueryParamsMap = Mockito.mock(QueryParamsMap.class); Mockito.when(mockRequest.queryMap("osm_tag")).thenReturn(mockQueryParamsMap); - Mockito.when(mockRequest.queryParams("bbox")).thenReturn("181,27,9.8,87"); + Mockito.when(mockRequest.queryParams("bbox")).thenReturn(minLon + "," + minLat + "," + maxLon + "," + maxLat); try { PhotonRequestFactory photonRequestFactory = new PhotonRequestFactory(ImmutableSet.of("en")); photonRequestFactory.create(mockRequest); Assert.fail(); } catch (BadRequestException e) { - Assert.assertEquals("invalid search term 'bbox', expected format is: minLon,minLat,maxLon,maxLat", e.getMessage()); + Assert.assertEquals(BoundingBoxParamConverter.INVALID_BBOX_BOUNDS_MESSAGE, e.getMessage()); } Mockito.verify(mockRequest, Mockito.times(1)).queryParams("bbox"); } - } \ No newline at end of file