changeset 1151:2b3cf603245d

Fix NPE in WebStorageEndPoint (causing server 500 errors) reviewed-by: neugens review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-July/007234.html
author Jon VanAlten <jon.vanalten@redhat.com>
date Wed, 03 Jul 2013 12:12:47 -0600
parents d6f3b3eec520
children 4ba43dcff78e
files integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java web/client/pom.xml web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java
diffstat 5 files changed, 126 insertions(+), 74 deletions(-) [+]
line wrap: on
line diff
--- a/integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java	Mon Jun 24 14:15:01 2013 -0600
+++ b/integration-tests/src/test/java/com/redhat/thermostat/itest/WebAppTest.java	Wed Jul 03 12:12:47 2013 -0600
@@ -43,6 +43,7 @@
 import static com.redhat.thermostat.itest.IntegrationTest.spawnThermostat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.ByteArrayInputStream;
@@ -58,11 +59,7 @@
 import java.util.Properties;
 import java.util.UUID;
 
-import org.eclipse.jetty.security.DefaultUserIdentity;
-import org.eclipse.jetty.security.MappedLoginService;
 import org.eclipse.jetty.server.Server;
-import org.eclipse.jetty.server.UserIdentity;
-import org.eclipse.jetty.util.security.Password;
 import org.eclipse.jetty.webapp.WebAppContext;
 import org.junit.After;
 import org.junit.AfterClass;
@@ -599,11 +596,19 @@
         
         byte[] data = "Hello World".getBytes();
         webStorage.saveFile("test", new ByteArrayInputStream(data));
-        // Note: Apparently, saving the file takes a bit. Without this
-        // waiting, we sometimes get problems on loadFile. There seems
-        // to be no way to synchronize on the operation in Mongo.
-        Thread.sleep(300);
-        InputStream loadStream = webStorage.loadFile("test");
+        // Note: On the server side, the file is saved into mongodb
+        // via GridFS.  The save operation returns before write is
+        // complete, and there is no callback mechanism to find out
+        // when the write is complete.  So, we try a few times to
+        // load it before considering it a failure.
+        InputStream loadStream = null;
+        int loadAttempts = 0;
+        while (loadStream == null && loadAttempts < 3) {
+            Thread.sleep(300);
+            loadStream = webStorage.loadFile("test");
+            loadAttempts++;
+        }
+        assertNotNull(loadStream);
         StringBuilder str = new StringBuilder();
         int i = loadStream.read();
         while (i != -1) {
@@ -612,30 +617,4 @@
         }
         assertEquals("Hello World", str.toString());
     }
-    
-    private static class TestLoginService extends MappedLoginService {
-
-        private final String[] roleNames;
-        private final String username;
-        private final String password;
-
-        private TestLoginService(String username, String password,
-                String[] roleNames) {
-            this.username = username;
-            this.password = password;
-            this.roleNames = roleNames;
-        }
-
-        @Override
-        protected void loadUsers() throws IOException {
-            putUser(username, new Password(password),
-                    roleNames);
-        }
-
-        @Override
-        protected UserIdentity loadUser(String username) {
-            return new DefaultUserIdentity(null, null,
-                    roleNames);
-        }
-    }
 }
--- a/web/client/pom.xml	Mon Jun 24 14:15:01 2013 -0600
+++ b/web/client/pom.xml	Wed Jul 03 12:12:47 2013 -0600
@@ -86,6 +86,12 @@
       <version>${jetty.version}</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>javax.servlet</groupId>
+      <artifactId>servlet-api</artifactId>
+      <version>${javax.servlet.version}</version>
+      <scope>provided</scope>
+    </dependency>
 
     <dependency>
       <groupId>com.google.code.gson</groupId>
--- a/web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java	Mon Jun 24 14:15:01 2013 -0600
+++ b/web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java	Wed Jul 03 12:12:47 2013 -0600
@@ -58,6 +58,7 @@
 import java.util.logging.Logger;
 
 import javax.net.ssl.SSLContext;
+import javax.servlet.http.HttpServletResponse;
 
 import org.apache.commons.beanutils.BeanUtils;
 import org.apache.commons.codec.binary.Base64;
@@ -121,9 +122,11 @@
     private static class CloseableHttpEntity implements Closeable, HttpEntity {
 
         private HttpEntity entity;
+        private int responseCode;
 
-        CloseableHttpEntity(HttpEntity entity) {
+        CloseableHttpEntity(HttpEntity entity, int responseCode) {
             this.entity = entity;
+            this.responseCode = responseCode;
         }
 
         @Override
@@ -181,6 +184,9 @@
             }
         }
 
+        int getResponseCode() {
+            return responseCode;
+        }
     }
 
     private final class WebConnection extends Connection {
@@ -436,11 +442,19 @@
         }
         HttpResponse response = httpClient.execute(httpPost);
         StatusLine status = response.getStatusLine();
-        if (status.getStatusCode() != 200) {
+        int responseCode = status.getStatusCode();
+        switch (responseCode) {
+        case (HttpServletResponse.SC_NO_CONTENT):
+            // Let calling code handle SC_NO_CONTENT
+            break;
+        case (HttpServletResponse.SC_OK):
+            // Let calling code handle SC_OK
+            break;
+        default:
             throw new IOException("Server returned status: " + status);
         }
 
-        return new CloseableHttpEntity(response.getEntity());
+        return new CloseableHttpEntity(response.getEntity(), responseCode);
     }
 
     private static InputStream getContent(HttpEntity entity) {
@@ -527,10 +541,31 @@
         NameValuePair fileParam = new BasicNameValuePair("file", name);
         List<NameValuePair> formparams = Arrays.asList(fileParam);
         CloseableHttpEntity entity = post(endpoint + "/load-file", formparams);
+        if (entity.getResponseCode() == HttpServletResponse.SC_NO_CONTENT) {
+            return null;
+        }
         return new WebDataStream(entity);
     }
 
     @Override
+    public void saveFile(String name, InputStream in) throws StorageException {
+        InputStreamBody body = new InputStreamBody(in, name);
+        MultipartEntity mpEntity = new MultipartEntity();
+        mpEntity.addPart("file", body);
+        // See IcedTea bug #1314. For safe-file we need to do this. However,
+        // doing this for other actions messes up authentication when using
+        // jetty (and possibly others). Hence, do this expect-continue thingy
+        // only for save-file.
+        httpClient.getParams().setParameter("http.protocol.expect-continue", Boolean.TRUE);
+        try {
+            post(endpoint + "/save-file", mpEntity).close();
+        } finally {
+            // FIXME: Not sure if we need this :/
+            httpClient.getParams().removeParameter("http.protocol.expect-continue");
+        }
+    }
+
+    @Override
     public void purge(String agentId) throws StorageException {
         NameValuePair agentIdParam = new BasicNameValuePair("agentId", agentId);
         List<NameValuePair> agentIdParams = Arrays.asList(agentIdParam);
@@ -582,24 +617,6 @@
     }
 
     @Override
-    public void saveFile(String name, InputStream in) throws StorageException {
-        InputStreamBody body = new InputStreamBody(in, name);
-        MultipartEntity mpEntity = new MultipartEntity();
-        mpEntity.addPart("file", body);
-        // See IcedTea bug #1314. For safe-file we need to do this. However,
-        // doing this for other actions messes up authentication when using
-        // jetty (and possibly others). Hence, do this expect-continue thingy
-        // only for save-file.
-        httpClient.getParams().setParameter("http.protocol.expect-continue", Boolean.TRUE);
-        try {
-            post(endpoint + "/save-file", mpEntity).close();
-        } finally {
-            // FIXME: Not sure if we need this :/
-            httpClient.getParams().removeParameter("http.protocol.expect-continue");
-        }
-    }
-
-    @Override
     public void setAgentId(UUID agentId) {
         this.agentId = agentId;
     }
--- a/web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java	Mon Jun 24 14:15:01 2013 -0600
+++ b/web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java	Wed Jul 03 12:12:47 2013 -0600
@@ -119,13 +119,16 @@
 
     private int port;
 
-    private String requestBody;
+    // Set these in prepareServer() to determine server behaviour
+    private int responseStatus = HttpServletResponse.SC_OK;
+    private String responseBody;
 
-    private String responseBody;
+    // These get set by test server handler (anonymous class in startServer())
+    // Check them after WebStorage method call that should interact with server.
+    private String requestBody;
     private Map<String,String> headers;
     private String method;
     private String requestURI;
-    private int responseStatus;
 
     private static Category<TestObj> category;
     private static Key<String> key1;
@@ -151,7 +154,7 @@
 
     @Before
     public void setUp() throws Exception {
-        
+
         port = FreePortFinder.findFreePort(new TryPort() {
             @Override
             public void tryPort(int port) throws Exception {
@@ -170,9 +173,6 @@
         storage.setEndpoint("http://localhost:" + port + "/");
         storage.setAgentId(new UUID(123, 456));
         headers = new HashMap<>();
-        requestURI = null;
-        method = null;
-        responseStatus = HttpServletResponse.SC_OK;
         registerCategory();
         factory = new ExpressionFactory();
     }
@@ -216,6 +216,32 @@
         server.start();
     }
 
+    // Specified status and response body.
+    private void prepareServer(int responseStatus, String responseBody) {
+        this.responseStatus = responseStatus;
+        this.responseBody = responseBody;
+
+        requestBody = null;
+        requestURI = null;
+        method = null;
+        headers.clear();
+    }
+
+    // Specified status and null response body.
+    private void prepareServer(int responseStatus) {
+        prepareServer(responseStatus, null);
+    }
+
+    // OK status and specified response body.
+    private void prepareServer(String responseBody) {
+        prepareServer(HttpServletResponse.SC_OK, responseBody);
+    }
+
+    // OK status and null response body.
+    private void prepareServer() {
+        prepareServer(HttpServletResponse.SC_OK);
+    }
+
     @After
     public void tearDown() throws Exception {
 
@@ -246,7 +272,6 @@
         obj2.setProperty1("fluffor2");
         Gson gson = new GsonBuilder().registerTypeHierarchyAdapter(Expression.class, new ExpressionSerializer())
                 .registerTypeHierarchyAdapter(Operator.class, new OperatorSerializer()).create();
-        responseBody = gson.toJson(Arrays.asList(obj1, obj2));
 
         Key<String> key1 = new Key<>("property1", true);
         Query<TestObj> query = storage.createQuery(category);
@@ -254,7 +279,9 @@
         Expression expr = factory.equalTo(key1, "fluff");
         query.where(expr);
 
+        prepareServer(gson.toJson(Arrays.asList(obj1, obj2)));
         Cursor<TestObj> results = query.execute();
+
         StringReader reader = new StringReader(requestBody);
         BufferedReader bufRead = new BufferedReader(reader);
         String line = URLDecoder.decode(bufRead.readLine(), "UTF-8");
@@ -291,6 +318,8 @@
 
         Put replace = storage.createReplace(category);
         replace.setPojo(obj);
+
+        prepareServer();
         replace.apply();
 
         Gson gson = new Gson();
@@ -332,6 +361,8 @@
     public void testRemovePojo() throws UnsupportedEncodingException, IOException {
         Expression expr = factory.equalTo(key1, "test");
         Remove remove = storage.createRemove().from(category).where(expr);
+
+        prepareServer();
         storage.removePojo(remove);
 
         Gson gson = new GsonBuilder()
@@ -376,6 +407,8 @@
         update.where(expr);
         update.set(key1, "fluff");
         update.set(key2, 42);
+
+        prepareServer();
         update.apply();
 
         Gson gson = new GsonBuilder()
@@ -422,9 +455,7 @@
     @Test
     public void testGetCount() throws UnsupportedEncodingException, IOException {
 
-        Gson gson = new Gson();
-        responseBody = gson.toJson(12345);
-
+        prepareServer(new Gson().toJson(12345));
         long result = storage.getCount(category);
 
         StringReader reader = new StringReader(requestBody);
@@ -440,7 +471,10 @@
     public void testSaveFile() {
         String data = "Hello World";
         ByteArrayInputStream in = new ByteArrayInputStream(data.getBytes());
+
+        prepareServer();
         storage.saveFile("fluff", in);
+
         assertEquals("chunked", headers.get("Transfer-Encoding"));
         String contentType = headers.get("Content-Type");
         assertTrue(contentType.startsWith("multipart/form-data; boundary="));
@@ -458,8 +492,12 @@
 
     @Test
     public void testLoadFile() throws IOException {
-        responseBody = "Hello World";
-        InputStream in = storage.loadFile("fluff");
+        prepareServer(HttpServletResponse.SC_NO_CONTENT);
+        InputStream in = storage.loadFile("no_file_here");
+        assertNull(in);
+
+        prepareServer("Hello World");
+        in = storage.loadFile("fluff");
         assertEquals("file=fluff", requestBody.trim());
         byte[] data = new byte[11];
         int totalRead = 0;
@@ -476,7 +514,10 @@
 
     @Test
     public void testPurge() throws UnsupportedEncodingException, IOException {
+
+        prepareServer();
         storage.purge("fluff");
+
         assertEquals("POST", method);
         assertTrue(requestURI.endsWith("/purge"));
         StringReader reader = new StringReader(requestBody);
@@ -489,9 +530,10 @@
 
     @Test
     public void testGenerateToken() throws UnsupportedEncodingException {
-        responseBody = "flufftoken";
 
         final String actionName = "some action";
+
+        prepareServer("flufftoken");
         AuthToken authToken = storage.generateToken(actionName);
 
         assertTrue(requestURI.endsWith("/generate-token"));
@@ -509,6 +551,7 @@
         assertTrue(Arrays.equals(clientToken, authToken.getClientToken()));
 
         // Send another request and verify that we send a different client-token every time.
+        prepareServer("flufftoken");
         storage.generateToken(actionName);
 
         requestParts = requestBody.split("=");
@@ -521,6 +564,7 @@
 
     @Test
     public void canSSLEnableClient() {
+        // This test doesn't use the class-wide storage+server setup.
         StartupConfiguration config = new StartupConfiguration() {
             
             @Override
@@ -528,7 +572,7 @@
                 return "https://onlyHttpsPrefixUsed.example.com";
             }
         };
-        storage = new WebStorage(config);
+        WebStorage storage = new WebStorage(config);
         HttpClient client = storage.httpClient;
         SchemeRegistry schemeReg = client.getConnectionManager().getSchemeRegistry();
         Scheme scheme = schemeReg.getScheme("https");
@@ -545,8 +589,10 @@
         
         AuthToken authToken = new AuthToken(token, tokenDigest);
 
-        responseStatus = 200;
+        prepareServer();
         boolean ok = storage.verifyToken(authToken, someAction);
+
+        assertTrue(ok);
         assertTrue(requestURI.endsWith("/verify-token"));
         assertEquals("POST", method);
         String[] requestParts = requestBody.split("&");
@@ -565,10 +611,9 @@
         urlDecoded = URLDecoder.decode(authTokenParts[1], "UTF-8");
         String base64decoded = new String(Base64.decodeBase64(urlDecoded));
         assertEquals("stuff", base64decoded);
-        assertTrue(ok);
 
         // Try another one in which verification fails.
-        responseStatus = 401;
+        prepareServer(HttpServletResponse.SC_UNAUTHORIZED);
         ok = storage.verifyToken(authToken, someAction);
         assertFalse(ok);
         
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Mon Jun 24 14:15:01 2013 -0600
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Wed Jul 03 12:12:47 2013 -0600
@@ -271,6 +271,10 @@
         
         String name = req.getParameter("file");
         try (InputStream data = storage.loadFile(name)) {
+            if (data == null) {
+                resp.setStatus(HttpServletResponse.SC_NO_CONTENT);
+                return;
+            }
             OutputStream out = resp.getOutputStream();
             byte[] buffer = new byte[512];
             int read = 0;
@@ -280,6 +284,7 @@
                     out.write(buffer, 0, read);
                 }
             }
+            resp.setStatus(HttpServletResponse.SC_OK);
         }
     }