[GEOS-10824] gs-flatgeobuf extension can clash with "directory of sha…

…pefiles" datastores (geoserver#7782)

* [GEOS-10824] gs-flatgeobuf extension can clash with "directory of shapefiles" datastores

* [GEOS-10824] gs-flatgeobuf extension can clash with "directory of shapefiles" datastores. Do not share the test store with all test modules

src/extension/app-schema/app-schema-test/src/test/java/org/geoserver/test/RestconfigWfsTest.java

@@ -88,6 +88,7 @@ protected void onTearDown(SystemTestData testData) throws Exception {
                     + "<workspace>" //
                     + "<name>gsml</name>" //
                     + "</workspace>" //
+                    + "<type>Application Schema DataAccess</type>" //
                     + "<connectionParameters>" //
                     + "<entry key='dbtype'>app-schema</entry>" //
                     + "<entry key='url'>file:workspaces/gsml/MappedFeature/MappedFeature.xml</entry>" //

src/main/src/main/java/org/geoserver/catalog/ResourcePool.java

@@ -705,15 +705,12 @@ private void disableStoreInfoIfNeeded(
         connectionParameters =
                 ResourcePool.getParams(connectionParameters, catalog.getResourceLoader());
 
-        // obtain the factory
+        // obtain the factory, either using the "type", or if not found, using the parameters
         DataAccessFactory factory = null;
         try {
             factory = getDataStoreFactory(info);
         } catch (IOException e) {
-            throw new IOException(
-                    "Failed to find the datastore factory for "
-                            + info.getName()
-                            + ", did you forget to install the store extension jar?");
+            // ignoring since the error message is the same as for the null factory, see line below
         }
         if (factory == null) {
             throw new IOException(
@@ -725,9 +722,8 @@ private void disableStoreInfoIfNeeded(
 
         // ensure that the namespace parameter is set for the datastore
         if (!connectionParameters.containsKey("namespace") && params != null) {
-            // if we grabbed the factory, check that the factory actually supports
-            // a namespace parameter, if we could not get the factory, assume that
-            // it does
+            // if we grabbed the factory, check that the factory actually supports a namespace
+            // parameter, if we could not get the factory, assume that it does
             boolean supportsNamespace = false;
 
             for (Param p : params) {
@@ -749,8 +745,7 @@ private void disableStoreInfoIfNeeded(
             }
         }
 
-        // see if the store has a repository param, if so, pass the one wrapping
-        // the store
+        // see if the store has a repository param, if so, pass the one wrapping the store
         if (params != null) {
             for (Param p : params) {
                 if (Repository.class.equals(p.getType())) {
@@ -772,7 +767,19 @@ private void disableStoreInfoIfNeeded(
             }
         }
 
-        dataStore = DataStoreUtils.getDataAccess(connectionParameters);
+        // use the factory obtained through the lookup first
+        try {
+            dataStore = DataStoreUtils.getDataAccess(factory, connectionParameters);
+        } catch (IOException e) {
+            LOGGER.log(
+                    Level.INFO,
+                    String.format(
+                            "Failed to create the store using the configured factory (%s), will try a generic lookup now.",
+                            factory.getClass()),
+                    e);
+            dataStore = DataStoreUtils.getDataAccess(connectionParameters);
+        }
+
         if (dataStore == null) {
             /*
              * Preserve DataStore retyping behaviour by calling

src/main/src/main/java/org/geoserver/catalog/util/LegacyCatalogImporter.java

@@ -325,6 +325,12 @@ void importDataStores(CatalogFactory factory, Map dataStores) {
             dataStore.getConnectionParameters().put("namespace", ns.getURI());
 
             dataStore.setEnabled((Boolean) map.get("enabled"));
+
+            // some tolerance for legacy app-schema tests
+            if ("app-schema".equals(connectionParams.get("dbtype"))) {
+                dataStore.setType("Application Schema DataAccess");
+            }
+
             catalog.add(dataStore);
 
             if (dataStore.isEnabled()) {

src/main/src/main/java/org/vfny/geoserver/util/DataStoreUtils.java

@@ -62,6 +62,16 @@ public abstract class DataStoreUtils {
             return null;
         }
 
+        return getDataAccess(factory, params);
+    }
+
+    /**
+     * Creates a {@link DataAccess} using the given params and factory, verbatim, and then
+     * eventually wraps it into a renaming wrapper so that feature type names are good ones from the
+     * wfs point of view (that is, no ":" in the type names)
+     */
+    public static DataAccess<? extends FeatureType, ? extends Feature> getDataAccess(
+            DataAccessFactory factory, Map<String, Serializable> params) throws IOException {
         DataAccess<? extends FeatureType, ? extends Feature> store =
                 factory.createDataStore(params);
         if (store == null) {

src/main/src/test/java/org/geoserver/catalog/ResourcePoolTest.java

@@ -41,6 +41,7 @@
 import java.net.URI;
 import java.net.URL;
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
@@ -77,6 +78,8 @@
 import org.geotools.api.coverage.grid.GridCoverageReader;
 import org.geotools.api.data.DataAccess;
 import org.geotools.api.data.DataStore;
+import org.geotools.api.data.DataStoreFactorySpi;
+import org.geotools.api.data.DataStoreFinder;
 import org.geotools.api.data.FeatureSource;
 import org.geotools.api.data.Query;
 import org.geotools.api.data.SimpleFeatureLocking;
@@ -102,6 +105,8 @@
 import org.geotools.coverage.grid.io.GridCoverage2DReader;
 import org.geotools.coverage.grid.io.StructuredGridCoverage2DReader;
 import org.geotools.coverage.util.CoverageUtilities;
+import org.geotools.data.directory.DirectoryDataStore;
+import org.geotools.data.shapefile.ShapefileDirectoryFactory;
 import org.geotools.data.simple.SimpleFeatureCollection;
 import org.geotools.data.wfs.WFSDataStoreFactory;
 import org.geotools.factory.CommonFactoryFinder;
@@ -121,13 +126,18 @@
 import org.geotools.util.SoftValueHashMap;
 import org.geotools.util.URLs;
 import org.geotools.util.Version;
+import org.geotools.util.factory.FactoryRegistry;
 import org.geotools.util.factory.GeoTools;
 import org.geotools.util.factory.Hints;
+import org.hamcrest.Matchers;
+import org.junit.AfterClass;
 import org.junit.Assert;
 import org.junit.Assume;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.locationtech.jts.geom.Point;
+import org.springframework.test.util.ReflectionTestUtils;
 import org.w3c.dom.Element;
 import org.xml.sax.SAXException;
 
@@ -146,6 +156,8 @@ public class ResourcePoolTest extends GeoServerSystemTestSupport {
     private static final String HUMANS = "humans";
 
     private static final String BAD_CONN_DATASTORE = "bad_conn_data_store";
+    public static final DataStoreFactorySpi TEST_DIRECTORY_STORE_FACTORY_SPI =
+            new TestDirectoryStoreFactorySpi();
 
     static {
         System.setProperty("ALLOW_ENV_PARAMETRIZATION", "true");
@@ -158,6 +170,17 @@ public class ResourcePoolTest extends GeoServerSystemTestSupport {
 
     private static final String EXTERNAL_ENTITIES = "externalEntities";
 
+    @BeforeClass
+    public static void registerTestDirectoryStore() {
+        // a "catch-all" datastore that will use any File without requiring a filetype/dbtype
+        DataStoreFinder.registerFactrory(TEST_DIRECTORY_STORE_FACTORY_SPI);
+    }
+
+    @AfterClass
+    public static void deregisterTestDirectoryStore() {
+        DataStoreFinder.deregisterFactrory(TEST_DIRECTORY_STORE_FACTORY_SPI);
+    }
+
     @Override
     protected void onSetUp(SystemTestData testData) throws Exception {
         super.onSetUp(testData);
@@ -199,6 +222,7 @@ protected void onSetUp(SystemTestData testData) throws Exception {
         }
         CatalogBuilder cb = new CatalogBuilder(catalog);
         DataStoreInfo dataStoreInfo = cb.buildDataStore("mini-states");
+        dataStoreInfo.setType("Shapefile");
         dataStoreInfo.getConnectionParameters().put("url", "file:data/mini-states/mini-states.shp");
         catalog.add(dataStoreInfo);
         DataStore ds = (DataStore) dataStoreInfo.getDataStore(null);
@@ -1376,6 +1400,7 @@ public void testAutodisableOnConnfailure() {
         ds.setWorkspace(ws);
         ds.setEnabled(true);
         ds.setDisableOnConnFailure(true);
+        ds.setType("H2");
         Map<String, Serializable> params = ds.getConnectionParameters();
         params.put("dbtype", "h2");
         params.put("database", "");
@@ -1527,4 +1552,44 @@ public void testCustomizeAttributesCRS() throws Exception {
         CoordinateReferenceSystem crs = schema.getCoordinateReferenceSystem();
         assertEquals(CRS.decode("EPSG:4326", true), crs);
     }
+
+    @Test
+    public void testAcceptAllStore() throws Exception {
+        // check we have both the shapefile directory and test store accepting URL without a dbtype
+        ShapefileDirectoryFactory shapeDirectorFactory = null;
+        TestDirectoryStoreFactorySpi testDirectoryFactory = null;
+        Iterator<DataStoreFactorySpi> factoryIterator = DataStoreFinder.getAllDataStores();
+        while (factoryIterator.hasNext()) {
+            DataStoreFactorySpi spi = factoryIterator.next();
+            if (spi instanceof TestDirectoryStoreFactorySpi)
+                testDirectoryFactory = (TestDirectoryStoreFactorySpi) spi;
+            else if (spi instanceof ShapefileDirectoryFactory)
+                shapeDirectorFactory = (ShapefileDirectoryFactory) spi;
+        }
+        assertNotNull(shapeDirectorFactory);
+        assertNotNull(testDirectoryFactory);
+
+        // Using reflection to grab the registry. The synchronization is there because the method
+        // is using assertions to check it's being called in a synchronized block (assertions
+        // are enabled when running tests with Maven)
+        FactoryRegistry registry;
+        synchronized (DataStoreFinder.class) {
+            registry =
+                    ReflectionTestUtils.invokeMethod(DataStoreFinder.class, "getServiceRegistry");
+        }
+        registry.setOrdering(DataStoreFactorySpi.class, testDirectoryFactory, shapeDirectorFactory);
+
+        // now create a store that would be caught by the test factory, if it wasn't for the type
+        // being used to lookup the shape factory
+        Catalog catalog = getCatalog();
+        CatalogBuilder cb = new CatalogBuilder(catalog);
+        DataStoreInfo dataStoreInfo = cb.buildDataStore("mini-states-dir");
+        dataStoreInfo.setType("Directory of spatial files (shapefiles)");
+        dataStoreInfo.getConnectionParameters().put("url", "file:data/mini-states");
+        catalog.add(dataStoreInfo);
+
+        // check this is the right type of store
+        DataStore ds = (DataStore) dataStoreInfo.getDataStore(null);
+        assertThat(ds, Matchers.instanceOf(DirectoryDataStore.class));
+    }
 }

src/main/src/test/java/org/geoserver/catalog/TestDirectoryStoreFactorySpi.java

@@ -0,0 +1,61 @@
+/* (c) 2024 Open Source Geospatial Foundation - all rights reserved
+ * This code is licensed under the GPL 2.0 license, available at the root
+ * application directory.
+ */
+package org.geoserver.catalog;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.Map;
+import org.geotools.api.data.DataStore;
+import org.geotools.api.data.DataStoreFactorySpi;
+import org.geotools.data.memory.MemoryDataStore;
+
+/**
+ * A fake store that returns nothing, used to similate the FGB store interaction with directory data
+ * store
+ */
+public class TestDirectoryStoreFactorySpi implements DataStoreFactorySpi {
+
+    public static final Param URL_PARAM =
+            new Param("url", URL.class, "A file or directory", true, null);
+
+    @Override
+    public DataStore createDataStore(Map<String, ?> params) throws IOException {
+        return new TestDirectoryStore();
+    }
+
+    @Override
+    public String getDisplayName() {
+        return "Test store accepting a simple URL";
+    }
+
+    @Override
+    public String getDescription() {
+        return "A test factory with no parameters and one fixed data type";
+    }
+
+    @Override
+    public Param[] getParametersInfo() {
+        return new Param[] {URL_PARAM};
+    }
+
+    @Override
+    public boolean isAvailable() {
+        return true;
+    }
+
+    @Override
+    public DataStore createNewDataStore(Map<String, ?> params) throws IOException {
+        return createDataStore(params);
+    }
+
+    /**
+     * Empty subclass, just to have a clearer name on test failures, e.g., "failed to cast to
+     * TestDirectoryStore" is better than "failed to case to MemoryDataStore", e.g., it would lead
+     * immediately to this file. In case you're seeing this, it's likely that you created a
+     * DataStoreInfo without setting the "type" attribute to the desired data store factory
+     * displayName.
+     */
+    public static class TestDirectoryStore extends MemoryDataStore {}
+}

src/restconfig/src/main/java/org/geoserver/rest/catalog/DataStoreController.java

@@ -146,14 +146,16 @@ public ResponseEntity<String> dataStorePost(
         }
 
         // attempt to set the datastore type
-        try {
-            DataAccessFactory factory =
-                    DataStoreUtils.aquireFactory(dataStore.getConnectionParameters());
-            dataStore.setType(factory.getDisplayName());
-        } catch (Exception e) {
-            LOGGER.warning("Unable to determine datastore type from connection parameters");
-            if (LOGGER.isLoggable(Level.FINE)) {
-                LOGGER.log(Level.FINE, "", e);
+        if (dataStore.getType() == null) {
+            try {
+                DataAccessFactory factory =
+                        DataStoreUtils.aquireFactory(dataStore.getConnectionParameters());
+                dataStore.setType(factory.getDisplayName());
+            } catch (Exception e) {
+                LOGGER.warning("Unable to determine datastore type from connection parameters");
+                if (LOGGER.isLoggable(Level.FINE)) {
+                    LOGGER.log(Level.FINE, "", e);
+                }
             }
         }
 

src/restconfig/src/test/java/org/geoserver/rest/catalog/DataStoreControllerTest.java

@@ -6,6 +6,7 @@
 
 import static org.custommonkey.xmlunit.XMLAssert.assertXpathEvaluatesTo;
 import static org.custommonkey.xmlunit.XMLAssert.assertXpathExists;
+import static org.geoserver.catalog.ResourcePoolTest.TEST_DIRECTORY_STORE_FACTORY_SPI;
 import static org.geoserver.rest.RestBaseController.ROOT_PATH;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.endsWith;
@@ -33,10 +34,13 @@
 import org.geoserver.data.test.SystemTestData;
 import org.geoserver.rest.RestBaseController;
 import org.geotools.api.data.DataStore;
+import org.geotools.api.data.DataStoreFinder;
 import org.geotools.api.feature.type.FeatureType;
 import org.geotools.data.property.PropertyDataStore;
 import org.hamcrest.Matchers;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.springframework.http.MediaType;
 import org.springframework.mock.web.MockHttpServletResponse;
@@ -46,6 +50,17 @@
 
 public class DataStoreControllerTest extends CatalogRESTTestSupport {
 
+    @BeforeClass
+    public static void registerTestDirectoryStore() {
+        // a "catch-all" datastore that will use any File without requiring a filetype/dbtype
+        DataStoreFinder.registerFactrory(TEST_DIRECTORY_STORE_FACTORY_SPI);
+    }
+
+    @AfterClass
+    public static void deregisterTestDirectoryStore() {
+        DataStoreFinder.deregisterFactrory(TEST_DIRECTORY_STORE_FACTORY_SPI);
+    }
+
     @Before
     public void addDataStores() throws IOException {
         removeStore("sf", "newDataStore"); // may have been created by other tests