This is an automated email from the ASF dual-hosted git repository. amanin pushed a commit to branch refactor/failure_upon_store_discovery in repository https://gitbox.apache.org/repos/asf/sis.git
commit 53eefbb393ef76d5fc381e06d23da7f5efc4c2cf Author: Alexis Manin <[email protected]> AuthorDate: Thu Mar 4 14:37:31 2021 +0100 refactor(Storage): minor rework of error management upon datastore discovery With both various possible input format and drivers, it is very complicated to ensure that content probing never launch unexpected errors. To mitigate such problem, we modify current fail-fast strategy to try other providers beforeraising errors. This has multiple advantages: instead of getting one error at once, if multiple providers are unstable, a report with all catched errors is returned at once, allowing to spot multiple bugs (and therefore, allow to fix them) at once. Also, if an unstable provider cause an error, the operation can still succeed if the required provider is tested after the failing one. --- .../org/apache/sis/storage/DataStoreRegistry.java | 32 +++++++++++++++++-- .../org/apache/sis/storage/DataStoresTest.java | 37 ++++++++++++++++++++++ .../org/apache/sis/storage/FailingProvider.java | 35 ++++++++++++++++++++ .../org.apache.sis.storage.DataStoreProvider | 1 + 4 files changed, 102 insertions(+), 3 deletions(-) diff --git a/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreRegistry.java b/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreRegistry.java index 524880e..f2595bf 100644 --- a/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreRegistry.java +++ b/storage/sis-storage/src/main/java/org/apache/sis/storage/DataStoreRegistry.java @@ -16,11 +16,13 @@ */ package org.apache.sis.storage; +import java.util.ArrayList; import java.util.List; import java.util.LinkedList; import java.util.Set; import java.util.Iterator; import java.util.ServiceLoader; +import java.util.stream.Collectors; import org.apache.sis.internal.storage.Resources; import org.apache.sis.internal.storage.StoreMetadata; import org.apache.sis.internal.system.DefaultFactories; @@ -166,6 +168,7 @@ final class DataStoreRegistry { Boolean matchCondition = (extension != null && !extension.isEmpty()) ? Boolean.TRUE : null; final List<ProbeProviderPair> needMoreBytes = new LinkedList<>(); ProbeProviderPair selected = null; + List<RuntimeException> providerErrors = new ArrayList<>(4); try { search: do { /* @@ -186,7 +189,13 @@ search: do { accept = (md != null && ArraysExt.containsIgnoreCase(md.fileSuffixes(), extension)) == matchCondition; } if (accept) { - final ProbeResult probe = provider.probeContent(connector); + ProbeResult probe; + try { + probe = provider.probeContent(connector); + } catch (RuntimeException e) { + providerErrors.add(e); + probe = ProbeResult.UNSUPPORTED_STORAGE; + } if (probe.isSupported()) { /* * Stop at the first provider claiming to be able to read the storage. @@ -226,7 +235,12 @@ search: do { while (!needMoreBytes.isEmpty() && connector.prefetch()) { for (final Iterator<ProbeProviderPair> it = needMoreBytes.iterator(); it.hasNext();) { final ProbeProviderPair p = it.next(); - p.probe = p.provider.probeContent(connector); + try { + p.probe = p.provider.probeContent(connector); + } catch (RuntimeException e) { + providerErrors.add(e); + p.probe = ProbeResult.UNSUPPORTED_STORAGE; + } if (p.probe.isSupported()) { selected = p; break search; @@ -265,7 +279,19 @@ search: do { if (open && selected == null) { @SuppressWarnings("null") final String name = connector.getStorageName(); - throw new UnsupportedStorageException(null, Resources.Keys.UnknownFormatFor_1, name); + final UnsupportedStorageException mainError = new UnsupportedStorageException(null, Resources.Keys.UnknownFormatFor_1, name); + if (!providerErrors.isEmpty()) { + for (Exception providerError : providerErrors) { + mainError.addSuppressed(providerError); + } + } + throw mainError; + } else if (selected == null && !providerErrors.isEmpty()) { + // No valid datastore found, but some errors have been post-poned until now + final Iterator<RuntimeException> it = providerErrors.iterator(); + final RuntimeException promotedError = it.next(); + while (it.hasNext()) promotedError.addSuppressed(it.next()); + throw promotedError; } return selected; } diff --git a/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoresTest.java b/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoresTest.java index ccae2ff..a03da18 100644 --- a/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoresTest.java +++ b/storage/sis-storage/src/test/java/org/apache/sis/storage/DataStoresTest.java @@ -16,8 +16,11 @@ */ package org.apache.sis.storage; +import java.io.ByteArrayInputStream; +import java.io.InputStream; import java.io.StringReader; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -31,6 +34,7 @@ import org.apache.sis.internal.storage.xml.StoreTest; import org.apache.sis.test.DependsOn; import org.apache.sis.test.TestCase; import org.junit.Test; +import org.junit.jupiter.api.Assertions; import static org.apache.sis.test.Assert.*; @@ -94,6 +98,39 @@ public final strictfp class DataStoresTest extends TestCase { } /** + * Ensure that when no datastore is available for a given input, intermediate errors launched by providers (in case + * of a bug) are accessible by the caller, and not completely hidden. + * + * @throws Exception Any error not planned to occur. + */ + @Test + public void discovery_must_be_resilient_to_arbitrary_provider_errors() throws Exception { + final InputStream mockInput = new ByteArrayInputStream(new byte[0]); + final DataStoreRegistry dsr = new DataStoreRegistry(DataStoresTest.class.getClassLoader()); + try { + dsr.probeContentType(mockInput); + fail("We should not have found any matching datastore"); + } catch (RuntimeException e) { + Assertions.assertTrue( + e instanceof FailingProvider.SystematicFailure || + Arrays.stream(e.getSuppressed()) + .anyMatch(err -> err instanceof FailingProvider.SystematicFailure), + "Arbitrary provider errors should not be completely hidden" + ); + } + + try (DataStore dataStore = dsr.open(mockInput)) { + fail("We should not have found any matching datastore"); + } catch (UnsupportedStorageException e) { + Assertions.assertTrue( + Arrays.stream(e.getSuppressed()) + .anyMatch(err -> err instanceof FailingProvider.SystematicFailure), + "Arbitrary provider errors should not be completely hidden" + ); + } + } + + /** * Starts the specified amount of worker threads where each thread ask for the set of providers. * * @param nbWorkers number of concurrent threads. diff --git a/storage/sis-storage/src/test/java/org/apache/sis/storage/FailingProvider.java b/storage/sis-storage/src/test/java/org/apache/sis/storage/FailingProvider.java new file mode 100644 index 0000000..0fe3305 --- /dev/null +++ b/storage/sis-storage/src/test/java/org/apache/sis/storage/FailingProvider.java @@ -0,0 +1,35 @@ +package org.apache.sis.storage; + +import org.apache.sis.internal.storage.URIDataStore; +import org.opengis.parameter.ParameterDescriptorGroup; + +public class FailingProvider extends DataStoreProvider { + + private static final String NAME = "ALWAYS_FAIL"; + + @Override + public String getShortName() { + return NAME; + } + + @Override + public ParameterDescriptorGroup getOpenParameters() { + return URIDataStore.Provider.descriptor(NAME); + } + + @Override + public ProbeResult probeContent(StorageConnector connector) throws DataStoreException { + throw new SystematicFailure("I always fail. I'm here to ensure Datastore discovery is robust to arbitrary provider errors"); + } + + @Override + public DataStore open(StorageConnector connector) throws DataStoreException { + throw new UnsupportedOperationException("Not meant to be supported"); + } + + public static class SystematicFailure extends RuntimeException { + SystematicFailure(String message) { + super(message); + } + } +} diff --git a/storage/sis-storage/src/test/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider b/storage/sis-storage/src/test/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider new file mode 100644 index 0000000..3b7aa05 --- /dev/null +++ b/storage/sis-storage/src/test/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider @@ -0,0 +1 @@ +org.apache.sis.storage.FailingProvider
