Mercurial > hg > release > thermostat-1.0

changeset 1398:044868dbf9ad

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Improve 403/500 response handling in WebStorage. Reviewed-by: vanaltj Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-May/009839.html PR1783
author Severin Gehwolf <sgehwolf@redhat.com>
date Wed, 21 May 2014 15:26:25 +0200
parents c406ad5b66f1
children 8f05f8a277d9
files web/client/src/main/java/com/redhat/thermostat/web/client/internal/EntityConsumingIOException.java web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java web/client/src/test/java/com/redhat/thermostat/web/client/internal/EntityConsumingIOExceptionTest.java web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java
diffstat 4 files changed, 231 insertions(+), 14 deletions(-) [+]
line wrap: on
line diff
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/web/client/src/main/java/com/redhat/thermostat/web/client/internal/EntityConsumingIOException.java	Wed May 21 15:26:25 2014 +0200
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2012-2014 Red Hat, Inc.
+ *
+ * This file is part of Thermostat.
+ *
+ * Thermostat is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2, or (at your
+ * option) any later version.
+ *
+ * Thermostat is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Thermostat; see the file COPYING.  If not see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Linking this code with other modules is making a combined work
+ * based on this code.  Thus, the terms and conditions of the GNU
+ * General Public License cover the whole combination.
+ *
+ * As a special exception, the copyright holders of this code give
+ * you permission to link this code with independent modules to
+ * produce an executable, regardless of the license terms of these
+ * independent modules, and to copy and distribute the resulting
+ * executable under terms of your choice, provided that you also
+ * meet, for each linked independent module, the terms and conditions
+ * of the license of that module.  An independent module is a module
+ * which is not derived from or based on this code.  If you modify
+ * this code, you may extend this exception to your version of the
+ * library, but you are not obligated to do so.  If you do not wish
+ * to do so, delete this exception statement from your version.
+ */
+package com.redhat.thermostat.web.client.internal;
+
+import java.io.IOException;
+
+import org.apache.http.HttpEntity;
+import org.apache.http.util.EntityUtils;
+
+/**
+ * Should be used for to-be-thrown IOExceptions which need to consume an
+ * {@link HttpEntity} prior to getting thrown. This avoids resource leaks.
+ *
+ */
+class EntityConsumingIOException extends IOException {
+
+    private static final long serialVersionUID = 6822662292793556315L;
+    
+    EntityConsumingIOException(HttpEntity entity, String msg) {
+        super(msg);
+        consumeEntity(entity);
+    }
+    
+    private void consumeEntity(HttpEntity entity) {
+        try {
+            EntityUtils.consume(entity);
+        } catch (IOException e) {
+            // ignore, consuming failure since we are about to
+            // throw an IOException anyway.
+        }
+    }
+
+}
--- a/web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java	Tue Apr 22 17:01:48 2014 +0200
+++ b/web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java	Wed May 21 15:26:25 2014 +0200
@@ -112,8 +112,8 @@
 public class WebStorage implements Storage, SecureStorage {
 
     private static final String HTTPS_PREFIX = "https";
-    final Logger logger = LoggingUtils.getLogger(WebStorage.class);
-
+    static final Logger logger = LoggingUtils.getLogger(WebStorage.class);
+    
     private static class CloseableHttpEntity implements Closeable, HttpEntity {
 
         private HttpEntity entity;
@@ -378,7 +378,7 @@
             client.getCredentialsProvider().setCredentials(scope, creds);
         }
     }
-
+    
     private void ping() throws StorageException {
         post(endpoint + "/ping", (HttpEntity) null).close();
     }
@@ -429,7 +429,12 @@
             // Let calling code handle SC_OK
             break;
         default:
-            throw new IOException("Server returned status: " + status);
+            // Properly consume the entity, thus closing the content stream,
+            // by throwing this IOException sub-class. This is important for the
+            // 403 and 500 status code cases. See:
+            // http://hc.apache.org/httpcomponents-core-4.3.x/httpcore/apidocs/org/apache/http/util/EntityUtils.html#consume%28org.apache.http.HttpEntity%29
+            throw new EntityConsumingIOException(response.getEntity(), 
+                    "Server returned status: " + status);
         }
 
         return new CloseableHttpEntity(response.getEntity(), responseCode);
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/web/client/src/test/java/com/redhat/thermostat/web/client/internal/EntityConsumingIOExceptionTest.java	Wed May 21 15:26:25 2014 +0200
@@ -0,0 +1,61 @@
+/*
+ * Copyright 2012-2014 Red Hat, Inc.
+ *
+ * This file is part of Thermostat.
+ *
+ * Thermostat is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2, or (at your
+ * option) any later version.
+ *
+ * Thermostat is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Thermostat; see the file COPYING.  If not see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Linking this code with other modules is making a combined work
+ * based on this code.  Thus, the terms and conditions of the GNU
+ * General Public License cover the whole combination.
+ *
+ * As a special exception, the copyright holders of this code give
+ * you permission to link this code with independent modules to
+ * produce an executable, regardless of the license terms of these
+ * independent modules, and to copy and distribute the resulting
+ * executable under terms of your choice, provided that you also
+ * meet, for each linked independent module, the terms and conditions
+ * of the license of that module.  An independent module is a module
+ * which is not derived from or based on this code.  If you modify
+ * this code, you may extend this exception to your version of the
+ * library, but you are not obligated to do so.  If you do not wish
+ * to do so, delete this exception statement from your version.
+ */
+package com.redhat.thermostat.web.client.internal;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.verify;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import org.apache.http.HttpEntity;
+import org.junit.Test;
+
+public class EntityConsumingIOExceptionTest {
+
+    @Test
+    public void verifyEntityIsConsumed() throws IllegalStateException, IOException {
+        HttpEntity entity = mock(HttpEntity.class);
+        InputStream stream = mock(InputStream.class);
+        when(entity.getContent()).thenReturn(stream);
+        when(entity.isStreaming()).thenReturn(true);
+        
+        // this should trigger consumption of entity
+        new EntityConsumingIOException(entity, "testing-consume");
+        verify(stream).close();
+    }
+}
--- a/web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java	Tue Apr 22 17:01:48 2014 +0200
+++ b/web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java	Wed May 21 15:26:25 2014 +0200
@@ -85,13 +85,12 @@
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
 import com.redhat.thermostat.shared.config.SSLConfiguration;
-import com.redhat.thermostat.storage.config.StartupConfiguration;
 import com.redhat.thermostat.storage.core.AuthToken;
+import com.redhat.thermostat.storage.core.BackingStorage;
 import com.redhat.thermostat.storage.core.Categories;
 import com.redhat.thermostat.storage.core.Category;
 import com.redhat.thermostat.storage.core.Connection.ConnectionListener;
 import com.redhat.thermostat.storage.core.Connection.ConnectionStatus;
-import com.redhat.thermostat.storage.core.BackingStorage;
 import com.redhat.thermostat.storage.core.Cursor;
 import com.redhat.thermostat.storage.core.DescriptorParsingException;
 import com.redhat.thermostat.storage.core.IllegalDescriptorException;
@@ -102,6 +101,7 @@
 import com.redhat.thermostat.storage.core.StatementDescriptor;
 import com.redhat.thermostat.storage.core.StatementExecutionException;
 import com.redhat.thermostat.storage.core.StorageCredentials;
+import com.redhat.thermostat.storage.core.StorageException;
 import com.redhat.thermostat.storage.model.Pojo;
 import com.redhat.thermostat.test.FreePortFinder;
 import com.redhat.thermostat.test.FreePortFinder.TryPort;
@@ -308,6 +308,53 @@
     }
     
     @Test
+    public void forbiddenExecuteQueryThrowsConsumingExcptn() throws UnsupportedEncodingException, IOException {
+        Gson gson = new GsonBuilder().registerTypeHierarchyAdapter(PreparedParameter.class, new PreparedParameterSerializer())
+                .registerTypeAdapter(WebPreparedStatement.class, new WebPreparedStatementSerializer())
+                .registerTypeAdapter(WebQueryResponse.class, new WebQueryResponseSerializer<>())
+                .registerTypeAdapter(Pojo.class, new ThermostatGSONConverter())
+                .create();
+
+        String strDesc = "QUERY test WHERE 'property1' = ?s";
+        StatementDescriptor<TestObj> desc = new StatementDescriptor<>(category, strDesc);
+        PreparedStatement<TestObj> stmt = null;
+        
+        int fakePrepStmtId = 5;
+        WebPreparedStatementResponse fakeResponse = new WebPreparedStatementResponse();
+        fakeResponse.setNumFreeVariables(1);
+        fakeResponse.setStatementId(fakePrepStmtId);
+        prepareServer(gson.toJson(fakeResponse));
+        try {
+            stmt = storage.prepareStatement(desc);
+        } catch (DescriptorParsingException e) {
+            // descriptor should parse fine and is trusted
+            fail(e.getMessage());
+        }
+        assertTrue(stmt instanceof WebPreparedStatement);
+        WebPreparedStatement<TestObj> webStmt = (WebPreparedStatement<TestObj>)stmt;
+        assertEquals(fakePrepStmtId, webStmt.getStatementId());
+        PreparedParameters params = webStmt.getParams();
+        assertEquals(1, params.getParams().length);
+        assertNull(params.getParams()[0]);
+        
+        // now set a parameter
+        stmt.setString(0, "fluff");
+        assertEquals("fluff", params.getParams()[0].getValue());
+        assertEquals(String.class, params.getParams()[0].getType());
+        
+        prepareServer(HttpServletResponse.SC_FORBIDDEN);
+        try {
+            stmt.executeQuery();
+            fail("Forbidden should have thrown an exception!");
+        } catch (StatementExecutionException e) {
+            Throwable t = e.getCause();
+            assertTrue(t instanceof StorageException);
+            t = t.getCause();
+            assertTrue("Wanted EntityConsumingIOException as root cause", t instanceof EntityConsumingIOException);
+        }
+    }
+    
+    @Test
     public void canPrepareAndExecuteQuery() throws UnsupportedEncodingException, IOException {
         TestObj obj1 = new TestObj();
         obj1.setProperty1("fluffor1");
@@ -424,6 +471,52 @@
         
         assertEquals(PreparedStatementResponseCode.WRITE_GENERIC_FAILURE, response);
     }
+    
+    @Test
+    public void forbiddenExecuteWriteReturnsGenericWriteFailure() {
+        Gson gson = new GsonBuilder().registerTypeAdapter(PreparedParameter.class, new PreparedParameterSerializer())
+                .registerTypeAdapter(WebPreparedStatement.class, new WebPreparedStatementSerializer())
+                .registerTypeHierarchyAdapter(Pojo.class, new ThermostatGSONConverter())
+                .create();
+
+        String strDesc = "ADD test SET 'property1' = ?s";
+        StatementDescriptor<TestObj> desc = new StatementDescriptor<>(category, strDesc);
+        PreparedStatement<TestObj> stmt = null;
+        
+        int fakePrepStmtId = 3;
+        WebPreparedStatementResponse fakeResponse = new WebPreparedStatementResponse();
+        fakeResponse.setNumFreeVariables(1);
+        fakeResponse.setStatementId(fakePrepStmtId);
+        prepareServer(gson.toJson(fakeResponse));
+        try {
+            stmt = storage.prepareStatement(desc);
+        } catch (DescriptorParsingException e) {
+            // descriptor should parse fine and is trusted
+            fail(e.getMessage());
+        }
+        assertTrue(stmt instanceof WebPreparedStatement);
+        WebPreparedStatement<TestObj> webStmt = (WebPreparedStatement<TestObj>)stmt;
+        assertEquals(fakePrepStmtId, webStmt.getStatementId());
+        PreparedParameters params = webStmt.getParams();
+        assertEquals(1, params.getParams().length);
+        assertNull(params.getParams()[0]);
+        
+        // now set a parameter
+        stmt.setString(0, "fluff");
+        assertEquals("fluff", params.getParams()[0].getValue());
+        assertEquals(String.class, params.getParams()[0].getType());
+        
+        prepareServer(HttpServletResponse.SC_FORBIDDEN);
+        try {
+            stmt.execute();
+            fail("Forbidden should have thrown an exception!");
+        } catch (StatementExecutionException e) {
+            Throwable t = e.getCause();
+            assertTrue(t instanceof StorageException);
+            t = t.getCause();
+            assertTrue("Wanted EntityConsumingIOException as root cause", t instanceof EntityConsumingIOException);
+        }
+    }
 
     @Test
     public void testSaveFile() {
@@ -522,14 +615,6 @@
 
     @Test
     public void canSSLEnableClient() {
-        // This test doesn't use the class-wide storage+server setup.
-        StartupConfiguration config = new StartupConfiguration() {
-            
-            @Override
-            public String getDBConnectionString() {
-                return "https://onlyHttpsPrefixUsed.example.com";
-            }
-        };
         SSLConfiguration sslConf = mock(SSLConfiguration.class);
         WebStorage storage = new WebStorage("https://onlyHttpsPrefixUsed.example.com",
                 new TrivialStorageCredentials(null, null), sslConf);