From 5159d74c7dd9272cf75571f7ba7323bc8112e772 Mon Sep 17 00:00:00 2001 From: Gabriel Roldan Date: Tue, 14 Oct 2025 12:10:28 -0300 Subject: [PATCH] Rollback persisted configuration on blob store add/modify failures When adding or modifying a blob store, if an exception occurs after the configuration has been persisted to disk, the stored XML file could be left in an invalid state that prevents application startup on restart. This prevents GeoServer to store invalid blob store configurations, for as long as the BlobStore throws UnsuitableStorageException, in order not to change the current logic. This fix ensures that if save() or handleAddBlobStore()/handleModifyBlobStore() fail, the persisted configuration is rolled back by re-saving the previous valid state. Makes loadConfiguration() package-private with @VisibleForTesting to allow tests to verify the persisted state matches expectations after rollback. --- .../geowebcache/config/XMLConfiguration.java | 51 +++++- .../config/XMLConfigurationTest.java | 153 +++++++++++++++++- 2 files changed, 198 insertions(+), 6 deletions(-) diff --git a/geowebcache/core/src/main/java/org/geowebcache/config/XMLConfiguration.java b/geowebcache/core/src/main/java/org/geowebcache/config/XMLConfiguration.java index b203948fa..7c330fb78 100644 --- a/geowebcache/core/src/main/java/org/geowebcache/config/XMLConfiguration.java +++ b/geowebcache/core/src/main/java/org/geowebcache/config/XMLConfiguration.java @@ -15,6 +15,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.annotations.VisibleForTesting; import com.thoughtworks.xstream.XStream; import com.thoughtworks.xstream.io.xml.DomReader; import java.io.FileNotFoundException; @@ -290,7 +291,8 @@ public void setDefaultValues(TileLayer layer) { } } - private GeoWebCacheConfiguration loadConfiguration() throws ConfigurationException { + @VisibleForTesting + GeoWebCacheConfiguration loadConfiguration() throws ConfigurationException { Assert.isTrue(resourceProvider.hasInput(), "Resource provider must have an input"); try { try (InputStream in = resourceProvider.in()) { @@ -969,19 +971,44 @@ public synchronized void addBlobStore(BlobStoreInfo info) { } catch (IOException ioe) { // save failed, roll back the add blobStores.remove(info); - throw new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), ioe); + ConfigurationPersistenceException thrown = + new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), ioe); + + try { // reverting the stored config by re-saving + save(); + } catch (IOException suppressed) { + log.log( + Level.WARNING, + "Unable to revert stored configuration after failure to store BlobStore " + info, + suppressed); + thrown.addSuppressed(thrown); + } + throw thrown; } try { blobStoreListeners.safeForEach(listener -> { listener.handleAddBlobStore(info); }); } catch (IOException | GeoWebCacheException e) { + ConfigurationPersistenceException thrown; if (ExceptionUtils.isOrSuppresses(e, UnsuitableStorageException.class)) { // Can't store here, roll back blobStores.remove(info); - throw new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), e); + thrown = new ConfigurationPersistenceException("Unable to add BlobStoreInfo \"%s\"".formatted(info), e); + try { // reverting the stored config by re-saving + save(); + log.log(Level.CONFIG, "Configuration reverted after failure to add BlobStore " + info); + } catch (IOException suppress) { + log.log( + Level.WARNING, + "Unexpected error saving the reverted configuration after failure to add BlobStore " + info, + suppress); + thrown.addSuppressed(thrown); + } + } else { + thrown = new ConfigurationPersistenceException(e); } - throw new ConfigurationPersistenceException(e); + throw thrown; } } @@ -1050,7 +1077,21 @@ public synchronized void modifyBlobStore(BlobStoreInfo info) { // Can't store here, roll back blobStores.remove(info); blobStores.add(infoToRemove); - throw new ConfigurationPersistenceException("Unable to modify BlobStoreInfo \"%s\"".formatted(info), e); + ConfigurationPersistenceException thrown = new ConfigurationPersistenceException( + "Unable to modify BlobStoreInfo \"%s\"".formatted(info), e); + try { + // roll-back stored config, leaving an invalid stored config will prevent application startup + save(); + log.log(Level.CONFIG, "Configuration reverted after failure to modify BlobStore " + info); + } catch (IOException suppress) { + log.log( + Level.WARNING, + "Unexpected error saving the reverted configuration after failure to modify BlobStore " + + info, + suppress); + thrown.addSuppressed(suppress); + } + throw thrown; } throw new ConfigurationPersistenceException(e); } diff --git a/geowebcache/core/src/test/java/org/geowebcache/config/XMLConfigurationTest.java b/geowebcache/core/src/test/java/org/geowebcache/config/XMLConfigurationTest.java index 3b3151116..e921fad22 100644 --- a/geowebcache/core/src/test/java/org/geowebcache/config/XMLConfigurationTest.java +++ b/geowebcache/core/src/test/java/org/geowebcache/config/XMLConfigurationTest.java @@ -16,6 +16,7 @@ import static org.easymock.EasyMock.createMock; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.replay; +import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.greaterThan; @@ -31,12 +32,15 @@ import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.io.File; import java.io.FileInputStream; +import java.io.FilterOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.net.URL; import java.util.ArrayList; import java.util.Arrays; @@ -67,13 +71,16 @@ import org.geowebcache.grid.SRS; import org.geowebcache.layer.TileLayer; import org.geowebcache.layer.wms.WMSLayer; +import org.geowebcache.storage.UnsuitableStorageException; import org.geowebcache.util.TestUtils; import org.junit.Assume; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.mockito.Mockito; import org.springframework.context.ApplicationContext; +import org.springframework.web.context.WebApplicationContext; import org.xml.sax.SAXParseException; public class XMLConfigurationTest { @@ -477,7 +484,7 @@ public void testNoBlobStores() throws Exception { } @Test - public void testSaveBlobStores() throws Exception { + public void testAddBlobStores() throws Exception { FileBlobStoreInfo store1 = new FileBlobStoreInfo(); store1.setName("store1"); store1.setDefault(true); @@ -517,6 +524,150 @@ public void testSaveBlobStores() throws Exception { assertEquals(store2, stores.get(1)); } + @Test + public void testAddBlobStoreExceptionSaving() throws Exception { + + XMLFileResourceProvider resourceProvider = + new XMLFileResourceProvider( + XMLConfiguration.DEFAULT_CONFIGURATION_FILE_NAME, + (WebApplicationContext) null, + this.configDir.getAbsolutePath(), + null) { + + // throw an ioexception the first time close() is called, the second time is the roll-back + private boolean thrown = false; + + @Override + public OutputStream out() throws IOException { + OutputStream real = super.out(); + return new FilterOutputStream(real) { + + @Override + public void close() throws IOException { + real.close(); + if (thrown) { + return; + } + thrown = true; + throw new IOException("forced io exception"); + } + }; + } + }; + + config = new XMLConfiguration(null, resourceProvider); + config.setGridSetBroker(gridSetBroker); + + FileBlobStoreInfo store1 = new FileBlobStoreInfo(); + store1.setName("store1"); + store1.setDefault(true); + store1.setEnabled(true); + store1.setFileSystemBlockSize(8096); + store1.setBaseDirectory("/tmp/test"); + + assertThrows(ConfigurationPersistenceException.class, () -> config.addBlobStore(store1)); + assertEquals(0, config.getBlobStoreCount()); + GeoWebCacheConfiguration configuration = config.loadConfiguration(); + assertTrue("store shouldn't be saved", configuration.getBlobStores().isEmpty()); + } + + /** + * Verifies the blobstore configuration is rolled back from the persisted configuration if a + * {@link BlobStoreConfigurationListener#handleAddBlobStore} throws an {@link UnsuitableStorageException}. + * + *

Note I'm not sure why XMLConfiguration.addBlobStore() only rolls-back on UnsuitableStorageException and not on + * IOException or GeoWebCacheException + */ + @Test + public void testAddBlobStoreExceptionFromListener() throws Exception { + FileBlobStoreInfo store1 = new FileBlobStoreInfo(); + store1.setName("store1"); + store1.setDefault(true); + store1.setEnabled(true); + store1.setFileSystemBlockSize(8096); + store1.setBaseDirectory("/tmp/test"); + + BlobStoreConfigurationListener listener; + + listener = mock(BlobStoreConfigurationListener.class); + doThrow(new UnsuitableStorageException("fake")).when(listener).handleAddBlobStore(Mockito.any()); + config.addBlobStoreListener(listener); + + assertAddBlobStoreFails(store1, UnsuitableStorageException.class); + + // note, I'm not sure why XMLConfiguration.addBlobStore() only rolls-back on UnsuitableStorageException and not + // on IOException or GeoWebCacheException + // doThrow(new IOException("fake")).when(listener).handleAddBlobStore(Mockito.any()); + // assertAddBlobStoreFails(store1, IOException.class); + // + // doThrow(new GeoWebCacheException("fake")).when(listener).handleAddBlobStore(Mockito.any()); + // assertAddBlobStoreFails(store1, GeoWebCacheException.class); + } + + private void assertAddBlobStoreFails(FileBlobStoreInfo store, Class expectedCause) + throws ConfigurationException { + ConfigurationPersistenceException expected; + expected = assertThrows(ConfigurationPersistenceException.class, () -> config.addBlobStore(store)); + assertThat(expected.getCause(), instanceOf(expectedCause)); + assertEquals(0, config.getBlobStoreCount()); + GeoWebCacheConfiguration configuration = config.loadConfiguration(); + assertTrue("store shouldn't be saved", configuration.getBlobStores().isEmpty()); + } + + /** + * Verifies the blobstore configuration is rolled back from the persisted configuration if a + * {@link BlobStoreConfigurationListener#handleModifyBlobStore(BlobStoreInfo)} throws an + * {@link UnsuitableStorageException}. + * + *

Note I'm not sure why XMLConfiguration.modifyBobstore() only rolls-back on UnsuitableStorageException and not + * on IOException or GeoWebCacheException + */ + @Test + public void testModifyBlobStoreExceptionFromListener() throws Exception { + FileBlobStoreInfo original = new FileBlobStoreInfo(); + original.setName("store1"); + original.setDefault(true); + original.setEnabled(true); + original.setFileSystemBlockSize(8096); + original.setBaseDirectory("/tmp/test"); + + config.addBlobStore(original); + + BlobStoreConfigurationListener listener; + + listener = mock(BlobStoreConfigurationListener.class); + doThrow(new UnsuitableStorageException("fake")).when(listener).handleModifyBlobStore(Mockito.any()); + config.addBlobStoreListener(listener); + + assertModifyBlobStoreFails(original, UnsuitableStorageException.class); + + // note, I'm not sure why XMLConfiguration.addBlobStore() only rolls-back on UnsuitableStorageException and not + // on IOException or GeoWebCacheException + // doThrow(new IOException("fake")).when(listener).handleModifyBlobStore(Mockito.any()); + // assertModifyBlobStoreFails(original, IOException.class); + // + // doThrow(new GeoWebCacheException("fake")).when(listener).handleModifyBlobStore(Mockito.any()); + // assertModifyBlobStoreFails(original, GeoWebCacheException.class); + } + + private void assertModifyBlobStoreFails(FileBlobStoreInfo original, Class expectedCause) + throws ConfigurationException { + + FileBlobStoreInfo modified = (FileBlobStoreInfo) original.clone(); + modified.setBaseDirectory("/tmp/test2"); + + assertEquals(1, config.getBlobStoreCount()); + + ConfigurationPersistenceException expected; + expected = assertThrows(ConfigurationPersistenceException.class, () -> config.modifyBlobStore(modified)); + assertThat(expected.getCause(), instanceOf(expectedCause)); + GeoWebCacheConfiguration reloaded = config.loadConfiguration(); + assertEquals(1, reloaded.getBlobStores().size()); + + BlobStoreInfo stored = reloaded.getBlobStores().get(0); + assertEquals("store shouldn't be saved", original, stored); + } + @Test public void testSaveCurrentVersion() throws Exception {