Skip to content

[Backport 1.28.x] Rollback persisted configuration on blob store add/modify failures #1441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
groldan merged 1 commit into from
Oct 14, 2025
Merged

[Backport 1.28.x] Rollback persisted configuration on blob store add/modify failures #1441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 46 additions & 5 deletions geowebcache/core/src/main/java/org/geowebcache/config/XMLConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
}
Expand Down
153 changes: 152 additions & 1 deletion geowebcache/core/src/test/java/org/geowebcache/config/XMLConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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}.
*
* <p>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<? extends Exception> 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}.
*
* <p>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<? extends Exception> 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 {

Expand Down