[GEOS-10824] Replace TestDirectoryStoreFactorySpi by a DataAccessFactoryProducer

[groldan]

gs-main:tests is used a lot, so we better don't have a DataStoreFactorySpi used for a single test case laying around always.

Since ResourcePool calls DataStoreUtils.getDataAccess(), which in turn finds DataAccessFactoryProducer extensions, better eat our own dog food and use the extensions to provide additional data access factory producers.

With this patch, DataStoreUtils stops calling
DataAccessFinder.getAvailableDataStores() and that becomes the DefaultDataAccessFactoryProducer instead, while at the same time the DataAccessFactoryProducer interface extends ExtensionPriority.

[groldan]

Compilation errors:

Error: 9,197 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project gs-ysld: Compilation failure: Compilation failure: 
Error: 9,197 [ERROR] /home/runner/work/geoserver/geoserver/src/extension/ysld/src/main/java/org/geoserver/ysld/YsldHandler.java:[83,52] incompatible types: org.geotools.ysld.parse.WellKnownZoomContextFinder cannot be converted to org.geotools.ysld.parse.ZoomContextFinder
Error: 9,197 [ERROR] /home/runner/work/geoserver/geoserver/src/extension/ysld/src/main/java/org/geoserver/ysld/GWCZoomContextFinder.java:[35,20] incompatible types: org.geoserver.ysld.GWCZoomContextFinder.GWCZoomContext cannot be converted to org.geotools.ysld.parse.ZoomContext

you'll need to rebase on top of main

[aaime]

I like that the test gets better isolated, at the same time, the DataAccessFactoryProducer is a leftover of the GeoScript times that has not been used in years, and this patch basically makes it the primary lookup mechanism. On the other end, I don't see any significant harm in using it the way you propose.

Having the default producer be at lowest priority seemed odd/wrong, but again, it seems necessary for the test to be significant, otherwise, the default stores would always get priority and the new one would not be found at all.

@jodygarnett can we have a second opinion on the approach here?

[groldan]

Having the default producer be at lowest priority seemed odd/wrong, but again, it seems necessary for the test to be significant, otherwise, the default stores would always get priority and the new one would not be found at all.

Our ExtensionPriority interface has no default priority and the range 0-100 for min/max seems rather random anyways. My intention was to follow the same logic as Spring's Ordered or more precisely, the @Order annotation where the default priority is Ordered.LOWEST_PRECEDENCE. But yeah, the default one could be the mid-range value for example, no preference really.

[jodygarnett]

I think the provider is left over from Eclipse / uDig times where we wanted to allow another plugin system (not only FactorySPI to be able to inject functionality into GeoTools).

Let me have a look at this PR; if I understand correctly you want to have a small speed up by avoiding SPI?

[jodygarnett]

Gabe can we solve this at the GeoTools level please? Something was added for the Eclipse OSGi plugin system to allow additional factories to be mixed into the library. Solving this kind of thing here in GeoServer seems really odd ...

So whatever test needs org.geoserver.catalog.TestDirectoryStoreFactorySpi can have a beforeTest method:

@BeforeClass
public void init(){
    DataStoreFinder.getServiceRegistry().registerFactory( TestDirectoryStoreFactorySpi.INSTANCE,DataStore.class);
}
@AfterClass
public void deinit(){
    DataStoreFinder.getServiceRegistry().deregisterFactory( TestDirectoryStoreFactorySpi.INSTANCE,DataStore.class );
}

This would allow you to remove src/main/src/test/java/META-INF/services/org.geotools.api.data.DataStoreFactorySpi ?

[aaime]

I did not think about it... but we'll need to make the service registry getter public, that method is now private (most finders keep their internal registries under wraps).

[jodygarnett]

I did not think about it... but we'll need to make the service registry getter public, that method is now private (most finders keep their internal registries under wraps).

That makes some sense if they are caching anything I suppose.

Reading the docs:

• addFactoryIteratorProvider(...)
• removeFactoryIteratorProvider(...)

It appears the minimal change is to add these methods to DataStoreFinder, or DataAccessProvider...

That approach is functionally equivalent, however for test cases I expect DataStoreFinder.registerFactory(...) and DataStoreFinder.deregisterFactory(...) to be kinder.

What do you think @groldan ?

[aaime]

Applied the approach suggested by @jodygarnett
I would also suggest to remove the DataAccessFactoryProducer, it's dangling, the ability to register programmatically stores is more general and addresses the same need.