From 60dc53edca9ae2bcd6703e02b26acd47ef3d61a8 Mon Sep 17 00:00:00 2001
From: Chris Jaekl
Date: Sun, 21 Dec 2014 23:13:24 -0500
Subject: [PATCH] Improve error reporting. New error page that gives a bit
more of an explanation for InvalideResponseExceptions.
---
WEB-INF/classes/frank.properties | 6 +
prod/frank.properties | 6 +
prod/net/jaekl/frank/ErrorHandler.java | 105 ++++++++++++++++++
prod/net/jaekl/frank/FrankBundle.java | 6 +
prod/net/jaekl/frank/Schedule.java | 20 +---
prod/net/jaekl/frank/Style.java | 23 ++++
prod/net/jaekl/frank/ViewSchedule.java | 41 ++-----
.../qd/http/InvalidResponseException.java | 1 +
test/net/jaekl/frank/ErrorHandlerTest.java | 102 +++++++++++++++++
test/net/jaekl/frank/ScheduleTest.java | 32 +-----
test/net/jaekl/frank/StyleTest.java | 45 ++++++++
test/net/jaekl/frank/ViewScheduleTest.java | 30 -----
12 files changed, 310 insertions(+), 107 deletions(-)
create mode 100644 prod/net/jaekl/frank/ErrorHandler.java
create mode 100644 prod/net/jaekl/frank/Style.java
create mode 100644 test/net/jaekl/frank/ErrorHandlerTest.java
create mode 100644 test/net/jaekl/frank/StyleTest.java
diff --git a/WEB-INF/classes/frank.properties b/WEB-INF/classes/frank.properties
index e508d3f..4f35303 100644
--- a/WEB-INF/classes/frank.properties
+++ b/WEB-INF/classes/frank.properties
@@ -1,3 +1,4 @@
+answer.received=Answer received:
data.collected=Data collected 0m 0s ago at {0}.
destination=Destination
error.page=Error Page
@@ -5,8 +6,13 @@ eta=ETA
frank=Frank
gps.off=GPS off
gps.read=GPS Read
+invalid.response=Frank requested information from the OC Transpo server, but received an unexpected response.
m=m
+maybe.server.problem=This may indicate a problem with the server that was contacted, but it is also possible that you've uncovered a bug in Frank.
remain=Remain
+request.made=Request made:
route=Route
s=s
unexpected.error=Unexpected Error
+unexpected.exception=An unexpected exception has been raised. This probably indicates a bug in Frank.
+url.contacted=URL contacted:
diff --git a/prod/frank.properties b/prod/frank.properties
index e508d3f..4f35303 100644
--- a/prod/frank.properties
+++ b/prod/frank.properties
@@ -1,3 +1,4 @@
+answer.received=Answer received:
data.collected=Data collected 0m 0s ago at {0}.
destination=Destination
error.page=Error Page
@@ -5,8 +6,13 @@ eta=ETA
frank=Frank
gps.off=GPS off
gps.read=GPS Read
+invalid.response=Frank requested information from the OC Transpo server, but received an unexpected response.
m=m
+maybe.server.problem=This may indicate a problem with the server that was contacted, but it is also possible that you've uncovered a bug in Frank.
remain=Remain
+request.made=Request made:
route=Route
s=s
unexpected.error=Unexpected Error
+unexpected.exception=An unexpected exception has been raised. This probably indicates a bug in Frank.
+url.contacted=URL contacted:
diff --git a/prod/net/jaekl/frank/ErrorHandler.java b/prod/net/jaekl/frank/ErrorHandler.java
new file mode 100644
index 0000000..c1d72bd
--- /dev/null
+++ b/prod/net/jaekl/frank/ErrorHandler.java
@@ -0,0 +1,105 @@
+package net.jaekl.frank;
+
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.io.PrintWriter;
+import java.util.Locale;
+
+import net.jaekl.qd.http.InvalidResponseException;
+
+public class ErrorHandler {
+ static final String JAVASCRIPT =
+ "";
+
+ void writeScript(PrintWriter pw) {
+ pw.println(JAVASCRIPT);
+ }
+
+ void explain(PrintWriter pw, Throwable t, FrankBundle bundle) {
+ Throwable cause = t;
+ if (t instanceof FrankException) {
+ if (null != t.getCause()) {
+ cause = t.getCause();
+ }
+ }
+
+ pw.println("
");
+
+ explain(pw, t, bundle);
+
+ // Note that, if we cared about security, we would log this stack trace to a
+ // server log, and only report a cross-reference to the log file back to the
+ // end user's browser, to avoid potentially exposing internal info that we
+ // don't want to share.
+ // At least at this point, we don't care (that much), and trade off a
+ // potential information leak in favour of reducing our code complexity
+ // and the administrator's workload.
+
+ pw.println("");
+ pw.println("
");
-
- // Note that, if we cared about security, we would log this stack trace to a
- // server log, and only report a cross-reference to the log file back to the
- // end user's browser, to avoid potentially exposing internal info that we
- // don't want to share.
- // At least at this point, we don't care (that much), and trade off a
- // potential information leak in favour of reducing our code complexity
- // and the administrator's workload.
- t.printStackTrace(ps);
- String stackTrace = baos.toString();
- pw.println(stackTrace);
-
- pw.println("
");
- pw.println("");
- }
-
-
}
diff --git a/prod/net/jaekl/qd/http/InvalidResponseException.java b/prod/net/jaekl/qd/http/InvalidResponseException.java
index 7d0bdc8..5240f15 100644
--- a/prod/net/jaekl/qd/http/InvalidResponseException.java
+++ b/prod/net/jaekl/qd/http/InvalidResponseException.java
@@ -19,6 +19,7 @@ public class InvalidResponseException extends QDException {
public String getUrl() { return m_url; }
public String getMethod() { return m_method; }
+ public String getResponse() { return m_response; }
@Override
public String toString() {
diff --git a/test/net/jaekl/frank/ErrorHandlerTest.java b/test/net/jaekl/frank/ErrorHandlerTest.java
new file mode 100644
index 0000000..4db0602
--- /dev/null
+++ b/test/net/jaekl/frank/ErrorHandlerTest.java
@@ -0,0 +1,102 @@
+package net.jaekl.frank;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.sql.SQLException;
+import java.util.Locale;
+
+import net.jaekl.qd.QDException;
+import net.jaekl.qd.http.InvalidResponseException;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ErrorHandlerTest {
+ static final String GATEWAY = "http://www.jaekl.net/api";
+ static final String METHOD = "SampleApiMethod";
+ static final String RESPONSE = "Required parameter not specified.";
+
+ ByteArrayOutputStream m_baos;
+ PrintWriter m_pw;
+
+ @Before
+ public void setUp() {
+ m_baos = new ByteArrayOutputStream();
+ m_pw = new PrintWriter(m_baos);
+ }
+
+ @After
+ public void tearDown() throws IOException {
+ m_pw.close();
+ }
+
+ @Test
+ public void testWriteErrorPage_basicBehaviourWithVariousExceptions() {
+ ErrorHandler eh = new ErrorHandler();
+
+ Throwable[] throwables = {
+ new NullPointerException(),
+ new QDException(),
+ new SQLException(),
+ new InvalidResponseException(new NullPointerException(), GATEWAY, METHOD, RESPONSE)
+ };
+
+ for (Throwable t : throwables) {
+ m_baos.reset();
+ eh.writeErrorPage(m_pw, t, Locale.CANADA);
+ m_pw.flush();
+
+ String actual = m_baos.toString();
+ Assert.assertTrue(actual.contains("Frank: Error Page"));
+ Assert.assertTrue(actual.contains(t.toString()));
+ }
+ }
+
+ @Test
+ public void testExplain_unexpectedException() {
+ Locale[] locales = { Locale.CANADA, Locale.FRANCE, Locale.JAPAN, Locale.CHINA};
+
+ ErrorHandler eh = new ErrorHandler();
+
+ for (Locale locale : locales) {
+ FrankBundle bundle = FrankBundle.getInst(locale);
+
+ m_baos.reset();
+ eh.writeErrorPage(m_pw, new NullPointerException(), Locale.CANADA);
+ m_pw.flush();
+
+ String actual = m_baos.toString();
+ Assert.assertTrue(actual.contains(bundle.get(FrankBundle.UNEXPECTED_EXCEPTION)));
+ }
+ }
+
+ @Test
+ public void testExplain_wrappedInvalidResponseException() {
+ Locale[] locales = { Locale.CANADA, Locale.FRANCE, Locale.JAPAN, Locale.CHINA};
+
+ ErrorHandler eh = new ErrorHandler();
+ InvalidResponseException ire = new InvalidResponseException(null, GATEWAY, METHOD, RESPONSE);
+ FrankException fe = new FrankException(ire);
+
+ for (Locale locale : locales) {
+ FrankBundle bundle = FrankBundle.getInst(locale);
+
+ m_baos.reset();
+ eh.writeErrorPage(m_pw, fe, Locale.CANADA);
+ m_pw.flush();
+
+ String actual = m_baos.toString();
+
+ Assert.assertTrue(actual.contains(bundle.get(FrankBundle.INVALID_RESPONSE)));
+
+ Assert.assertTrue(actual.contains(bundle.get(FrankBundle.URL_CONTACTED)));
+ Assert.assertTrue(actual.contains(bundle.get(FrankBundle.REQUEST_MADE)));
+ Assert.assertTrue(actual.contains(bundle.get(FrankBundle.ANSWER_RECEIVED)));
+
+ Assert.assertTrue(actual.contains(bundle.get(FrankBundle.MAYBE_SERVER_PROBLEM)));
+ }
+ }
+}
diff --git a/test/net/jaekl/frank/ScheduleTest.java b/test/net/jaekl/frank/ScheduleTest.java
index 2fc9ac2..7f30680 100644
--- a/test/net/jaekl/frank/ScheduleTest.java
+++ b/test/net/jaekl/frank/ScheduleTest.java
@@ -17,15 +17,8 @@ import org.junit.Before;
import org.junit.Test;
public class ScheduleTest {
- static final String EXPECTED_STYLE = "\n";
static final String TITLE_PREFIX = "\n\n";
- static final String TITLE_SUFFIX = "\n" + EXPECTED_STYLE + "\n";
+ static final String TITLE_SUFFIX = "\n" + Style.CSS + "\n\n";
ByteArrayOutputStream m_baos;
@@ -44,25 +37,6 @@ public class ScheduleTest {
}
}
- // Confirm that writeStyle's output does not vary with the locale
- @Test
- public void test_writeStyle() {
- String actual;
-
- Locale[] locales = { Locale.CANADA, Locale.CANADA_FRENCH, Locale.JAPAN };
-
- for (Locale locale : locales) {
- m_baos.reset();
-
- Schedule schedule = new Schedule(locale);
- schedule.writeStyle(m_pw);
- m_pw.flush();
-
- actual = m_baos.toString();
- Assert.assertEquals(EXPECTED_STYLE, actual);
- }
- }
-
@Test
public void test_writeHeader() {
Locale[] locales = { Locale.CANADA, Locale.CANADA_FRENCH, Locale.JAPAN };
@@ -102,7 +76,7 @@ public class ScheduleTest {
String actual = m_baos.toString();
- Assert.assertTrue(actual.contains(EXPECTED_STYLE));
+ Assert.assertTrue(actual.contains(Style.CSS));
String expectedTitle = TITLE_PREFIX + "Frank: " + stopName + " (" + stopNo + ")" + TITLE_SUFFIX;
Assert.assertTrue(actual.contains(expectedTitle));
@@ -178,7 +152,7 @@ public class ScheduleTest {
// Some rudimentary validation of the result.
// Should really go through more permutations, and examine them more closely, here.
- Assert.assertTrue(actual.contains(EXPECTED_STYLE));
+ Assert.assertTrue(actual.contains(Style.CSS));
String expectedTitle = TITLE_PREFIX + "Frank: " + stopName + " (" + stopNo + ")" + TITLE_SUFFIX;
Assert.assertTrue(actual.contains(expectedTitle));
diff --git a/test/net/jaekl/frank/StyleTest.java b/test/net/jaekl/frank/StyleTest.java
new file mode 100644
index 0000000..f08f649
--- /dev/null
+++ b/test/net/jaekl/frank/StyleTest.java
@@ -0,0 +1,45 @@
+package net.jaekl.frank;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.PrintWriter;
+
+import junit.framework.Assert;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class StyleTest {
+ ByteArrayOutputStream m_baos;
+ PrintWriter m_pw;
+
+ @Before
+ public void setUp() {
+ m_baos = new ByteArrayOutputStream();
+ m_pw = new PrintWriter(m_baos);
+ }
+
+ @After
+ public void tearDown() throws IOException {
+ if (null != m_baos) {
+ m_baos.close();
+ }
+ }
+
+ // Should develop something here that is a bit smarter about what
+ // it's looking for, and less brittle than a simple compare-against-master...
+ @Test
+ public void test_writeStyle() {
+ String actual;
+
+ m_baos.reset();
+
+ Style style = new Style();
+ style.writeStyle(m_pw);
+ m_pw.flush();
+
+ actual = m_baos.toString();
+ Assert.assertEquals(Style.CSS + "\n", actual);
+ }
+}
diff --git a/test/net/jaekl/frank/ViewScheduleTest.java b/test/net/jaekl/frank/ViewScheduleTest.java
index eec671f..673b343 100644
--- a/test/net/jaekl/frank/ViewScheduleTest.java
+++ b/test/net/jaekl/frank/ViewScheduleTest.java
@@ -1,12 +1,7 @@
package net.jaekl.frank;
-import java.io.ByteArrayOutputStream;
-import java.io.PrintWriter;
-import java.sql.SQLException;
import java.util.HashMap;
-import java.util.Locale;
-import net.jaekl.qd.QDException;
import net.jaekl.qd.http.HttpServletRequestMock;
import org.junit.Assert;
@@ -72,29 +67,4 @@ public class ViewScheduleTest {
value = vs.getParamString(reqMock, "notPresent");
Assert.assertEquals(null, value);
}
-
- @Test
- public void testWriteErrorPage() {
- ByteArrayOutputStream baos = new ByteArrayOutputStream();
- PrintWriter pw = new PrintWriter(baos);
-
- ViewSchedule vs = new ViewSchedule();
-
- Throwable[] throwables = {
- new NullPointerException(),
- new QDException(),
- new SQLException()
- };
-
- for (Throwable t : throwables) {
- baos.reset();
- vs.writeErrorPage(pw, t, Locale.CANADA); // TODO: test translations
- pw.flush();
-
- String actual = baos.toString();
- Assert.assertTrue(actual.contains("Frank: Error Page"));
- Assert.assertTrue(actual.contains(t.toString()));
- }
- }
-
}
--
2.39.2