This is an automated email from the ASF dual-hosted git repository. desruisseaux pushed a commit to branch geoapi-4.0 in repository https://gitbox.apache.org/repos/asf/sis.git
commit 311d7ea64a8f6eba63ad64f4917b53265ef3a6f3 Author: Martin Desruisseaux <martin.desruisse...@geomatys.com> AuthorDate: Wed Sep 1 18:09:36 2021 +0200 Add a `StoreMetadata.isLowPriority` boolean for testing a provider last. This is used when a provider is too generic and we want to allow more specialized providers to be tested first. --- .../apache/sis/internal/gui/ResourceLoader.java | 12 --- .../sis/storage/netcdf/NetcdfStoreProvider.java | 3 +- .../apache/sis/internal/storage/StoreMetadata.java | 12 +++ .../storage/folder/FolderStoreProvider.java | 16 ++-- .../sis/internal/storage/folder/package-info.java | 2 +- .../org/apache/sis/storage/DataStoreRegistry.java | 85 +++++++++++++++++----- .../org/apache/sis/storage/StorageConnector.java | 18 +++-- .../org.apache.sis.storage.DataStoreProvider | 1 + .../sis/internal/storage/folder/StoreTest.java | 5 +- 9 files changed, 102 insertions(+), 52 deletions(-) diff --git a/application/sis-javafx/src/main/java/org/apache/sis/internal/gui/ResourceLoader.java b/application/sis-javafx/src/main/java/org/apache/sis/internal/gui/ResourceLoader.java index 988a820..d18916c 100644 --- a/application/sis-javafx/src/main/java/org/apache/sis/internal/gui/ResourceLoader.java +++ b/application/sis-javafx/src/main/java/org/apache/sis/internal/gui/ResourceLoader.java @@ -21,7 +21,6 @@ import java.net.URL; import java.io.File; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.Files; import java.io.IOException; import java.net.URISyntaxException; import java.util.function.Consumer; @@ -36,7 +35,6 @@ import org.apache.sis.storage.DataStoreException; import org.apache.sis.storage.DataStores; import org.apache.sis.util.collection.Cache; import org.apache.sis.util.resources.Vocabulary; -import org.apache.sis.internal.storage.folder.FolderStoreProvider; import org.apache.sis.internal.storage.io.IOUtilities; import org.apache.sis.storage.DataStore; @@ -154,16 +152,6 @@ public final class ResourceLoader extends Task<Resource> { * Loads the resource after we verified that it is not in the cache. */ private DataStore load() throws DataStoreException { - Object input = source; - if (input instanceof StorageConnector) { - input = ((StorageConnector) input).getStorage(); - } - if ((input instanceof File && ((File) input).isDirectory()) || - (input instanceof Path && Files.isDirectory((Path) input))) - { - return FolderStoreProvider.INSTANCE.open((source instanceof StorageConnector) - ? (StorageConnector) source : new StorageConnector(input)); - } return DataStores.open(source); } diff --git a/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/NetcdfStoreProvider.java b/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/NetcdfStoreProvider.java index 9459c54..50e97a9 100644 --- a/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/NetcdfStoreProvider.java +++ b/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/NetcdfStoreProvider.java @@ -74,7 +74,8 @@ import org.apache.sis.util.Version; @StoreMetadata(formatName = NetcdfStoreProvider.NAME, fileSuffixes = "nc", capabilities = Capability.READ, - resourceTypes = {Aggregate.class, FeatureSet.class, GridCoverageResource.class}) + resourceTypes = {Aggregate.class, FeatureSet.class, GridCoverageResource.class}, + isLowPriority = true) public class NetcdfStoreProvider extends DataStoreProvider { /** * The format name. diff --git a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/StoreMetadata.java b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/StoreMetadata.java index 292d9c8..7be37c6 100644 --- a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/StoreMetadata.java +++ b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/StoreMetadata.java @@ -92,4 +92,16 @@ public @interface StoreMetadata { * @return information about the expected resource types which might be encounter with this format. */ Class<? extends Resource>[] resourceTypes() default {}; + + /** + * Returns {@code true} if the data store should be tested last when searching for a data store capable + * to open a given file. This method should return {@code true} if the data store claims to be able to + * open a wide variety of files, in order to allow specialized data stores to be tested before generic + * data stores. + * + * <p>If many data stores have low priority, the ordering between them is unspecified.</p> + * + * @return {@code true} if this data store should be tested after all "normal priority" data stores. + */ + boolean isLowPriority() default false; } diff --git a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/FolderStoreProvider.java b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/FolderStoreProvider.java index c630652..7ae0ade 100644 --- a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/FolderStoreProvider.java +++ b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/FolderStoreProvider.java @@ -52,9 +52,9 @@ import org.apache.sis.setup.OptionKey; /** - * The provider of {@link Store} instances. This provider is intentionally <strong>not</strong> registered - * in {@code META-INF/services/org.apache.sis.storage.DataStoreProvider} because is will open any directory, - * which may conflict with other providers opening only directory with some specific content. + * The provider of {@link Store} instances. This provider is intentionally registered with lowest priority + * because it will open any directory, which may conflict with other providers opening only directory with + * some specific content. * * @author Johann Sorel (Geomatys) * @author Martin Desruisseaux (Geomatys) @@ -64,7 +64,8 @@ import org.apache.sis.setup.OptionKey; */ @StoreMetadata(formatName = FolderStoreProvider.NAME, capabilities = {Capability.READ, Capability.WRITE}, - resourceTypes = {Aggregate.class, FeatureSet.class, GridCoverageResource.class}) + resourceTypes = {Aggregate.class, FeatureSet.class, GridCoverageResource.class}, + isLowPriority = true) public final class FolderStoreProvider extends DataStoreProvider { /** * A short name or abbreviation for the data format. @@ -116,14 +117,9 @@ public final class FolderStoreProvider extends DataStoreProvider { } /** - * The unique instance of this provider. - */ - public static final FolderStoreProvider INSTANCE = new FolderStoreProvider(); - - /** * Creates a new provider. */ - private FolderStoreProvider() { + public FolderStoreProvider() { } /** diff --git a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/package-info.java b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/package-info.java index 9dfa98b..906c57f 100644 --- a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/package-info.java +++ b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/folder/package-info.java @@ -19,7 +19,7 @@ * {@link org.apache.sis.storage.DataStore} implementation for a folder containing an arbitrary amount of data files. * * @author Johann Sorel (Geomatys) - * @version 1.0 + * @version 1.1 * @since 0.8 * @module */ 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..e2c8383 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 @@ -43,7 +43,7 @@ import org.apache.sis.util.ArraysExt; * on the part of the caller. * * @author Martin Desruisseaux (Geomatys) - * @version 1.0 + * @version 1.1 * @since 0.4 * @module */ @@ -139,17 +139,54 @@ final class DataStoreRegistry { } /** + * The kind of providers to test. The provider are divided in 4 categories depending on whether + * the file suffix matches the suffix expected by the provider, and whether the provider should + * be tested last for giving a chance to specialized providers to open the file. + */ + private enum Category { + /** The provider can be tested now and the file suffix matches. */ + SUFFIX_MATCH(true, false), + + /** The provider could be tested now but the file suffix does not match. */ + IGNORE_SUFFIX(false, false), + + /** The provider should be tested last, but the file suffix matches. */ + DEFERRED(true, true), + + /** The provider should be tested last because it is too generic. */ + DEFERRED_IGNORE_SUFFIX(false, true); + + /** Whether this category checks if suffix matches. */ + final boolean useSuffix; + + /** Whether this category is for providers to test last. */ + final boolean isLowPriority; + + /** Creates a new enumeration value. */ + private Category(final boolean useSuffix, final boolean isLowPriority) { + this.useSuffix = useSuffix; + this.isLowPriority = isLowPriority; + } + + /** Returns the categories, optionally ignoring file suffix. */ + static Category[] values(final boolean useSuffix) { + return useSuffix ? values() : new Category[] {IGNORE_SUFFIX, DEFERRED_IGNORE_SUFFIX}; + } + } + + /** * Implementation of {@link #probeContentType(Object)} and {@link #open(Object)}. * * @param storage the input/output object as a URL, file, image input stream, <i>etc.</i>. * @param open {@code true} for creating a {@link DataStore}, or {@code false} if not needed. + * @return the result, or {@code null} if the format is not recognized and {@code open} is {@code false}. * @throws UnsupportedStorageException if no {@link DataStoreProvider} is found for a given storage object. * @throws DataStoreException if an error occurred while opening the storage. * * @todo Iterate on {@code ServiceLoader.Provider.type()} on JDK9. */ private ProbeProviderPair lookup(final Object storage, final boolean open) throws DataStoreException { - StorageConnector connector; + StorageConnector connector; // Will be reset to `null` if it shall not be closed. if (storage instanceof StorageConnector) { connector = (StorageConnector) storage; } else { @@ -158,20 +195,23 @@ final class DataStoreRegistry { /* * If we can get a filename extension from the given storage (file, URL, etc.), then we may perform two iterations * on the provider list. The first iteration will use only the providers which declare capability to read files of - * that suffix (matchCondition = TRUE). Only if no provider has been able to read that file, we will do a second - * iteration on other providers (matchCondition = FALSE). The intent is to avoid DataStoreProvider.probeContent(…) + * that suffix (Category.SUFFIX_MATCH). Only if no provider has been able to read that file, we will do a second + * iteration on other providers (Category.IGNORE_SUFFIX). The intent is to avoid DataStoreProvider.probeContent(…) * invocations loading large dependencies. */ - final String extension = connector.getFileExtension(); - Boolean matchCondition = (extension != null && !extension.isEmpty()) ? Boolean.TRUE : null; + final String extension = connector.getFileExtension(); + final boolean useSuffix = !(extension == null || extension.isEmpty()); + final Category[] categories = Category.values(useSuffix); + ProbeProviderPair selected = null; final List<ProbeProviderPair> needMoreBytes = new LinkedList<>(); - ProbeProviderPair selected = null; try { -search: do { +search: for (int ci=0; ci < categories.length; ci++) { + final Category category = categories[ci]; /* - * All usages of 'loader' and its 'providers' iterator must be protected in a synchronized block, + * All usages of `loader` and its `providers` iterator must be protected in a synchronized block, * because ServiceLoader is not thread-safe. We try to keep the synhronization block as small as - * possible for less contention. In particular, the probeContent(connector) method call may be costly. + * possible for less contention. In particular, the `probeContent(connector)` method call may be + * costly. */ final Iterator<DataStoreProvider> providers; DataStoreProvider provider; @@ -180,10 +220,20 @@ search: do { provider = providers.hasNext() ? providers.next() : null; } while (provider != null) { - boolean accept = true; - if (matchCondition != null) { - final StoreMetadata md = provider.getClass().getAnnotation(StoreMetadata.class); - accept = (md != null && ArraysExt.containsIgnoreCase(md.fileSuffixes(), extension)) == matchCondition; + /* + * Check if the provider should be tested in current iteration. + * The criteria for testing a provider is determined by comparing + * provider metadata with the category tested in current iteration. + */ + boolean accept; + final StoreMetadata md = provider.getClass().getAnnotation(StoreMetadata.class); + if (md == null) { + accept = (ci == 0); // If no metadata, test only in first iteration. + } else { + accept = (md.isLowPriority() == category.isLowPriority); + if (accept & useSuffix) { + accept = ArraysExt.containsIgnoreCase(md.fileSuffixes(), extension) == category.useSuffix; + } } if (accept) { final ProbeResult probe = provider.probeContent(connector); @@ -219,7 +269,7 @@ search: do { } } /* - * If any provider did not had enough bytes for answering the 'probeContent(…)' question, + * If any provider did not had enough bytes for answering the `probeContent(…)` question, * get more bytes and try again. We try to prefetch more bytes only if we have no choice * in order to avoid latency on network connection. */ @@ -242,10 +292,9 @@ search: do { /* * If we filtered providers by the file extension without finding a suitable provider, * try again with all other providers (even if they are for another file extension). - * We do that by changing 'matchCondition' from TRUE to FALSE. In all other cases, - * we stop the search. + * We do that by changing moving to the next `Category`. */ - } while (matchCondition != null && (matchCondition = !matchCondition) == false); + } /* * If a provider has been found, or if a provider returned UNDETERMINED, use that one * for opening a DataStore. Note that if more than one provider returned UNDETERMINED, diff --git a/storage/sis-storage/src/main/java/org/apache/sis/storage/StorageConnector.java b/storage/sis-storage/src/main/java/org/apache/sis/storage/StorageConnector.java index e04113f..b0b9e5f 100644 --- a/storage/sis-storage/src/main/java/org/apache/sis/storage/StorageConnector.java +++ b/storage/sis-storage/src/main/java/org/apache/sis/storage/StorageConnector.java @@ -1091,14 +1091,16 @@ public class StorageConnector implements Serializable { c = getView(ByteBuffer.class); if (reset(c)) { // reset(c) as a matter of principle, but (c != null) would have worked. final ByteBuffer buffer = (ByteBuffer) c.view; - final int p = buffer.limit(); - final long mark = input.getStreamPosition(); - input.seek(Math.addExact(mark, p)); - final int n = input.read(buffer.array(), p, buffer.capacity() - p); - input.seek(mark); - if (n > 0) { - buffer.limit(p + n); - return true; + if (buffer != null) { + final int p = buffer.limit(); + final long mark = input.getStreamPosition(); + input.seek(Math.addExact(mark, p)); + final int n = input.read(buffer.array(), p, buffer.capacity() - p); + input.seek(mark); + if (n > 0) { + buffer.limit(p + n); + return true; + } } } } diff --git a/storage/sis-storage/src/main/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider b/storage/sis-storage/src/main/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider index 62b5285..5c1a70f 100644 --- a/storage/sis-storage/src/main/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider +++ b/storage/sis-storage/src/main/resources/META-INF/services/org.apache.sis.storage.DataStoreProvider @@ -1,3 +1,4 @@ org.apache.sis.internal.storage.xml.StoreProvider org.apache.sis.internal.storage.wkt.StoreProvider org.apache.sis.internal.storage.csv.StoreProvider +org.apache.sis.internal.storage.folder.FolderStoreProvider diff --git a/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/folder/StoreTest.java b/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/folder/StoreTest.java index 236116a..1f04666 100644 --- a/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/folder/StoreTest.java +++ b/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/folder/StoreTest.java @@ -43,7 +43,7 @@ import static org.junit.Assume.assumeTrue; * * @author Johann Sorel (Geomatys) * @author Martin Desruisseaux (Geomatys) - * @version 1.0 + * @version 1.1 * @since 0.8 * @module */ @@ -89,11 +89,12 @@ public final strictfp class StoreTest extends TestCase { */ @Test public void testSearchProviderParameter() throws URISyntaxException, DataStoreException, IOException { + final FolderStoreProvider provider = new FolderStoreProvider(); final Set<String> identifiers = new HashSet<>(Arrays.asList("Sample 1", "Sample 2", "Sample 3", "data4")); final ParameterValueGroup params = FolderStoreProvider.PARAMETERS.createValue(); params.parameter("location").setValue(testDirectory()); params.parameter("format").setValue("XML"); - try (Store store = (Store) FolderStoreProvider.INSTANCE.open(params)) { + try (Store store = (Store) provider.open(params)) { assertEquals("Expected one less data store.", 3, store.components().size()); verifyContent(store, identifiers); }