From aa879bccee0eec113edac51ef3bf4482b5793abf Mon Sep 17 00:00:00 2001 From: Fabian Steeg Date: Thu, 20 Sep 2018 11:15:35 +0200 Subject: [PATCH 1/2] Handle errors on invalid queries See https://github.com/hbz/lobid-gnd/issues/156 --- app/controllers/HomeController.java | 5 ++++- app/modules/IndexComponent.java | 12 +++++++++--- test/controllers/HomeControllerTest.java | 1 + test/modules/IndexQueryTest.java | 5 +++++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/controllers/HomeController.java b/app/controllers/HomeController.java index 7993dd7..68c639c 100644 --- a/app/controllers/HomeController.java +++ b/app/controllers/HomeController.java @@ -361,7 +361,7 @@ public ByteString next() { private Result htmlSearch(String q, String type, int from, int size, String format, SearchResponse response) { return ok(views.html.search.render(q, type, from, size, returnAsJson(q, response), - response.getHits().getTotalHits())); + response == null ? 0 : response.getHits().getTotalHits())); } private static Result withCallback(final String json) { @@ -434,6 +434,9 @@ public static String currentUri() { } private static String returnAsJson(String q, SearchResponse queryResponse) { + if (queryResponse == null) { + return Json.newObject().toString(); + } List> hits = Arrays.asList(queryResponse.getHits().getHits()).stream() .map(hit -> hit.getSource()).collect(Collectors.toList()); ObjectNode object = Json.newObject(); diff --git a/app/modules/IndexComponent.java b/app/modules/IndexComponent.java index 6c8e1e1..58c6730 100644 --- a/app/modules/IndexComponent.java +++ b/app/modules/IndexComponent.java @@ -259,9 +259,15 @@ public SearchResponse query(String q, String filter, int from, int size) { for (String a : HomeController.AGGREGATIONS) { requestBuilder.addAggregation(AggregationBuilders.terms(a).field(a).size(1000)); } - Logger.debug("Search request: {}", requestBuilder); - SearchResponse response = requestBuilder.get(); - return response; + try { + Logger.debug("Search request: {}", requestBuilder); + SearchResponse response = requestBuilder.get(); + return response; + } catch (Throwable t) { + Logger.error("Could not execute request: {}, cause: {}", t.getMessage(), t.getCause().getMessage()); + Logger.debug("Could not execute request", t); + return null; + } } @Override diff --git a/test/controllers/HomeControllerTest.java b/test/controllers/HomeControllerTest.java index ccecb1a..292a58a 100644 --- a/test/controllers/HomeControllerTest.java +++ b/test/controllers/HomeControllerTest.java @@ -33,6 +33,7 @@ public static Collection data() { { routes.HomeController.search("*", "", 0, 10, "json").toString(), Status.OK }, { routes.HomeController.search("*", "", 0, 10, "jsonl").toString(), Status.OK }, { routes.HomeController.search("*", "", 0, 10, "json:suggest").toString(), Status.OK }, + { routes.HomeController.search("++test", "", 0, 10, "html").toString(), Status.OK }, { routes.HomeController.search("*", "", 0, 10, "jsonfoo").toString(), Status.UNSUPPORTED_MEDIA_TYPE }, { routes.HomeController.search("*", "", 0, 10, "ttl").toString(), Status.UNSUPPORTED_MEDIA_TYPE }, { routes.HomeController.search("*", "", 0, 10, "rdf").toString(), Status.UNSUPPORTED_MEDIA_TYPE }, diff --git a/test/modules/IndexQueryTest.java b/test/modules/IndexQueryTest.java index 2e5e02c..ba695ef 100644 --- a/test/modules/IndexQueryTest.java +++ b/test/modules/IndexQueryTest.java @@ -122,4 +122,9 @@ public void testNoStemming() { Assert.assertEquals(1, index.query("namenlosen").getHits().getTotalHits()); } + @Test + public void testInvalidQuery() { + Assert.assertNull(index.query("++test")); + } + } From 68ce911a102f10bebbee174a8d5604d8ba0d2505 Mon Sep 17 00:00:00 2001 From: Fabian Steeg Date: Thu, 20 Sep 2018 14:47:55 +0200 Subject: [PATCH 2/2] Return proper error code, embed message in error template See https://github.com/hbz/lobid-gnd/issues/156 --- app/controllers/HomeController.java | 58 ++++++++++++--------- app/modules/IndexComponent.java | 12 ++--- app/views/error.scala.html | 10 ++++ test/controllers/AcceptIntegrationTest.java | 8 +-- test/controllers/HomeControllerTest.java | 2 +- test/modules/IndexQueryTest.java | 5 +- 6 files changed, 53 insertions(+), 42 deletions(-) create mode 100644 app/views/error.scala.html diff --git a/app/controllers/HomeController.java b/app/controllers/HomeController.java index 68c639c..32ff84f 100644 --- a/app/controllers/HomeController.java +++ b/app/controllers/HomeController.java @@ -178,8 +178,8 @@ public Result authority(String id, String format) { Format responseFormat = Accept.formatFor(format, request().acceptedTypes()); if (responseFormat == null || responseFormat == Accept.Format.JSON_LINES || format != null && format.contains(":")) { - return unsupportedMediaType(String.format("Unsupported for single resource: format=%s, accept=%s", format, - request().acceptedTypes())); + return unsupportedMediaType(views.html.error.render(id, String.format( + "Unsupported for single resource: format=%s, accept=%s", format, request().acceptedTypes()))); } try { switch (responseFormat) { @@ -199,7 +199,7 @@ public Result authority(String id, String format) { } } catch (Exception e) { Logger.error("Could not create response", e); - return internalServerError(e.getMessage()); + return internalServerError(views.html.error.render(id, e.getMessage())); } } @@ -297,31 +297,37 @@ public Result search(String q, String filter, int from, int size, String format) Format responseFormat = Accept.formatFor(format, request().acceptedTypes()); if (responseFormat == null || Stream.of(RdfFormat.values()).map(RdfFormat::getParam) .anyMatch(f -> f.equals(responseFormat.queryParamString))) { - return unsupportedMediaType( - String.format("Unsupported for search: format=%s, accept=%s", format, request().acceptedTypes())); + return unsupportedMediaType(views.html.error.render(q, + String.format("Unsupported for search: format=%s, accept=%s", format, request().acceptedTypes()))); } String queryString = (q == null || q.isEmpty()) ? "*" : q; - SearchResponse response = index.query(queryString, filter, from, size); - response().setHeader("Access-Control-Allow-Origin", "*"); - String[] formatAndConfig = format == null ? new String[] {} : format.split(":"); - boolean returnSuggestions = formatAndConfig.length == 2; - if (returnSuggestions) { - List> hits = Arrays.asList(response.getHits().getHits()).stream() - .map(hit -> hit.getSource()).collect(Collectors.toList()); - return withCallback(toSuggestions(Json.toJson(hits), formatAndConfig[1])); - } - switch (responseFormat) { - case HTML: { - return htmlSearch(q, filter, from, size, responseFormat.queryParamString, response); - } - case JSON_LINES: { - response().setHeader("Content-Disposition", - String.format("attachment; filename=\"lobid-gnd-bulk-%s.jsonl\"", System.currentTimeMillis())); - return jsonLines(queryString, filter, response); - } - default: { - return ok(returnAsJson(q, response)).as(config("index.content")); - } + try { + SearchResponse response = index.query(queryString, filter, from, size); + response().setHeader("Access-Control-Allow-Origin", "*"); + String[] formatAndConfig = format == null ? new String[] {} : format.split(":"); + boolean returnSuggestions = formatAndConfig.length == 2; + if (returnSuggestions) { + List> hits = Arrays.asList(response.getHits().getHits()).stream() + .map(hit -> hit.getSource()).collect(Collectors.toList()); + return withCallback(toSuggestions(Json.toJson(hits), formatAndConfig[1])); + } + switch (responseFormat) { + case HTML: { + return htmlSearch(q, filter, from, size, responseFormat.queryParamString, response); + } + case JSON_LINES: { + response().setHeader("Content-Disposition", + String.format("attachment; filename=\"lobid-gnd-bulk-%s.jsonl\"", System.currentTimeMillis())); + return jsonLines(queryString, filter, response); + } + default: { + return ok(returnAsJson(q, response)).as(config("index.content")); + } + } + } catch (Throwable t) { + String message = t.getMessage() + (t.getCause() != null ? ", cause: " + t.getCause().getMessage() : ""); + Logger.error("Error: {}", message); + return internalServerError(views.html.error.render(q, "Error: " + message)); } } diff --git a/app/modules/IndexComponent.java b/app/modules/IndexComponent.java index 58c6730..6c8e1e1 100644 --- a/app/modules/IndexComponent.java +++ b/app/modules/IndexComponent.java @@ -259,15 +259,9 @@ public SearchResponse query(String q, String filter, int from, int size) { for (String a : HomeController.AGGREGATIONS) { requestBuilder.addAggregation(AggregationBuilders.terms(a).field(a).size(1000)); } - try { - Logger.debug("Search request: {}", requestBuilder); - SearchResponse response = requestBuilder.get(); - return response; - } catch (Throwable t) { - Logger.error("Could not execute request: {}, cause: {}", t.getMessage(), t.getCause().getMessage()); - Logger.debug("Could not execute request", t); - return null; - } + Logger.debug("Search request: {}", requestBuilder); + SearchResponse response = requestBuilder.get(); + return response; } @Override diff --git a/app/views/error.scala.html b/app/views/error.scala.html new file mode 100644 index 0000000..037612f --- /dev/null +++ b/app/views/error.scala.html @@ -0,0 +1,10 @@ +@* Copyright 2018 Fabian Steeg, hbz. Licensed under the GPLv2 *@ + +@(q: String, message: String) + +@main(q, "Error") { +
+
Fehler
+
@message
+
+} diff --git a/test/controllers/AcceptIntegrationTest.java b/test/controllers/AcceptIntegrationTest.java index 74d91b0..12344a9 100644 --- a/test/controllers/AcceptIntegrationTest.java +++ b/test/controllers/AcceptIntegrationTest.java @@ -49,8 +49,8 @@ public static Collection data() { { fakeRequest(GET, "/gnd/search?q=*").header("Accept", "*/*"), /*->*/ "application/json" }, { fakeRequest(GET, "/gnd/search?q=*&format="), /*->*/ "application/json" }, { fakeRequest(GET, "/gnd/search?q=*&format=json"), /*->*/ "application/json" }, - { fakeRequest(GET, "/gnd/search?q=*&format=whatever"), /*->*/ "text/plain" }, - { fakeRequest(GET, "/gnd/search?q=*").header("Accept", "text/plain"), /*->*/ "text/plain" }, + { fakeRequest(GET, "/gnd/search?q=*&format=whatever"), /*->*/ "text/html" }, + { fakeRequest(GET, "/gnd/search?q=*").header("Accept", "text/plain"), /*->*/ "text/html" }, // search, bulk format: JSON lines { fakeRequest(GET, "/gnd/search?q=*").header("Accept", "application/x-jsonlines"), /*->*/ "application/x-jsonlines" }, { fakeRequest(GET, "/gnd/search?format=jsonl"), /*->*/ "application/x-jsonlines" }, @@ -65,8 +65,8 @@ public static Collection data() { { fakeRequest(GET, "/gnd/118820591"), /*->*/ "application/json" }, { fakeRequest(GET, "/gnd/118820591?format="), /*->*/ "application/json" }, { fakeRequest(GET, "/gnd/118820591?format=json"), /*->*/ "application/json" }, - { fakeRequest(GET, "/gnd/118820591?format=whatever"), /*->*/ "text/plain" }, - { fakeRequest(GET, "/gnd/118820591?format=whatever").header("Accept", "text/html"), /*->*/ "text/plain" }, + { fakeRequest(GET, "/gnd/118820591?format=whatever"), /*->*/ "text/html" }, + { fakeRequest(GET, "/gnd/118820591?format=whatever").header("Accept", "text/html"), /*->*/ "text/html" }, { fakeRequest(GET, "/gnd/118820591").header("Accept", "text/plain"), /*->*/ "application/n-triples" }, // get, other formats as query param: { fakeRequest(GET, "/gnd/118820591?format=html"), /*->*/ "text/html" }, diff --git a/test/controllers/HomeControllerTest.java b/test/controllers/HomeControllerTest.java index 292a58a..67f07ef 100644 --- a/test/controllers/HomeControllerTest.java +++ b/test/controllers/HomeControllerTest.java @@ -33,7 +33,7 @@ public static Collection data() { { routes.HomeController.search("*", "", 0, 10, "json").toString(), Status.OK }, { routes.HomeController.search("*", "", 0, 10, "jsonl").toString(), Status.OK }, { routes.HomeController.search("*", "", 0, 10, "json:suggest").toString(), Status.OK }, - { routes.HomeController.search("++test", "", 0, 10, "html").toString(), Status.OK }, + { routes.HomeController.search("++test", "", 0, 10, "html").toString(), Status.INTERNAL_SERVER_ERROR }, { routes.HomeController.search("*", "", 0, 10, "jsonfoo").toString(), Status.UNSUPPORTED_MEDIA_TYPE }, { routes.HomeController.search("*", "", 0, 10, "ttl").toString(), Status.UNSUPPORTED_MEDIA_TYPE }, { routes.HomeController.search("*", "", 0, 10, "rdf").toString(), Status.UNSUPPORTED_MEDIA_TYPE }, diff --git a/test/modules/IndexQueryTest.java b/test/modules/IndexQueryTest.java index ba695ef..55d170b 100644 --- a/test/modules/IndexQueryTest.java +++ b/test/modules/IndexQueryTest.java @@ -7,6 +7,7 @@ import java.io.FileNotFoundException; import java.util.Set; +import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.junit.Assert; import org.junit.Test; @@ -122,9 +123,9 @@ public void testNoStemming() { Assert.assertEquals(1, index.query("namenlosen").getHits().getTotalHits()); } - @Test + @Test(expected = SearchPhaseExecutionException.class) public void testInvalidQuery() { - Assert.assertNull(index.query("++test")); + index.query("++test"); } }