# HG changeset patch # User Severin Gehwolf # Date 1400678785 -7200 # Node ID 044868dbf9ad473554da7cd1e8d980c9136f4264 # Parent c406ad5b66f1c988b050146a436201ec5067d567 Improve 403/500 response handling in WebStorage. Reviewed-by: vanaltj Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-May/009839.html PR1783 diff -r c406ad5b66f1 -r 044868dbf9ad web/client/src/main/java/com/redhat/thermostat/web/client/internal/EntityConsumingIOException.java --- /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 + * . + * + * 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. + } + } + +} diff -r c406ad5b66f1 -r 044868dbf9ad web/client/src/main/java/com/redhat/thermostat/web/client/internal/WebStorage.java --- 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); diff -r c406ad5b66f1 -r 044868dbf9ad web/client/src/test/java/com/redhat/thermostat/web/client/internal/EntityConsumingIOExceptionTest.java --- /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 + * . + * + * 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(); + } +} diff -r c406ad5b66f1 -r 044868dbf9ad web/client/src/test/java/com/redhat/thermostat/web/client/internal/WebStorageTest.java --- 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 desc = new StatementDescriptor<>(category, strDesc); + PreparedStatement 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 webStmt = (WebPreparedStatement)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 desc = new StatementDescriptor<>(category, strDesc); + PreparedStatement 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 webStmt = (WebPreparedStatement)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);