Return proper error code, embed message in error template

See #156
hbz · Sep 20, 2018 · 68ce911 · 68ce911
1 parent aa879bc
commit 68ce911
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 42 deletions.
diff --git 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<Map<String, Object>> 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<Map<String, Object>> 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
@@ -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
@@ -0,0 +1,10 @@
+@* Copyright 2018 Fabian Steeg, hbz. Licensed under the GPLv2 *@
+
+@(q: String, message: String)
+
+@main(q, "Error") { 
+	<div class="panel panel-danger footer">
+		<div class='panel-heading'>Fehler</div>
+		<div class='panel-body'>@message</div>
+	</div>
+}
diff --git a/test/controllers/AcceptIntegrationTest.java b/test/controllers/AcceptIntegrationTest.java
@@ -49,8 +49,8 @@ public static Collection<Object[]> 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<Object[]> 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
@@ -33,7 +33,7 @@ public static Collection<Object[]> 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
@@ -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");
 	}
 
 }