Remove GeometryServiceProvider.Instance

[airbreather]

This is somewhat related to #30, but that issue is very broad, so I wanted to give this its own arena for discussion.

GeometryServiceProvider.Instance isn't actually used anywhere in GeoAPI itself. Its sole purpose seems to be a common point of reference for downstream consumers.

This sounds neat, but it creates some problems:

1. Downstream consumers need to set it themselves somehow, either by explicitly calling this sometime after being loaded (see Automatically set GeometryServiceProvider.Instance when NTS is loaded. NetTopologySuite#236) or by passing this responsibility to their own consumers.
   • This creates its own friction, since it's not clear to ORM providers whether or not they should be setting this themselves.
2. GeoAPI implementers shouldn't rely too much on this, even if they set it themselves, because some other GeoAPI implementer might set it later.
3. Related challenges that have been overcome in the past year or two (from when this issue was opened).

In the face of all those problems, the practical benefits are questionable. I may have to eat my words on this, but in its current state, I find it difficult to imagine a particularly useful project that depends on GeoAPI.Core without also depending on something more concrete, such as NetTopologySuite.Core. So if there is, in fact, a need for a global default instance, then it could just as easily be NtsGeometryServices.Instance or something similar.

In a post-#30 world, there may be a use for having GeoAPI export a global variable that can be used as a default to reduce the friction of the API, depending on how the architecture looks in the end, but right now I feel that it doesn't add enough value on top of NtsGeometryServices.Instance to be worth keeping around (for reasons other than backwards-compatibility).

[bricelam]

Copying some comments here.

@bricelam said:

We never go through GeoAPI.GeometryServiceProvider.Instance so [calling NetTopologySuiteBootstrapper.Bootstrap() is] not needed. Instead, we register NtsGeometryServices.Instance (as IGeometryServices) in the service provider and pass it into the NTS Reader.

This provides much more flexibility than the GeometryServiceProvider.Instance service locator anti-pattern since you can use different services with different DbContext instances.

@roji said:

I think this is where the SqlServer/Sqlite spatial implementation may actually have an advantage over Npgsql's.

With Npgsql, NTS is supported at the ADO level - which is great because you can easily use it without EF Core. However, unlike with EF Core there's no fancy dependency injection in ADO, so at least at the moment everything is done with the global NTS bootstrapping, which as you say isn't ideal.

[roji]

Just to add a few more comments, continuing the discussion with @bricelam cited above...

I think it may make sense to retain GeometryServiceProvider.Instance. As written above, the Npgsql ADO provider (not the EF Core provider) has no dependency injection of any sort. Now, I think the following are reasonable requirements from the Npgsql NTS plugin:

1. If the user hasn't indicated they're interested in any specific GeoAPI implementation, the plugin should default to using NetTopologySuite (i.e. instantiate NTS objects when reading PostGIS data from the database).
2. However, the plugin should definitely support other GeoAPI implementations. This could be done by:
   a. Receiving a GeometryServiceProvider instance in the plugin's initialization, or
   b. A global static setting as it is now (GeometryServiceProvider.Instance)

While I agree that responsibilities around setting GeometryServiceProvider.Instance can be a bit confusing, it does seem to make sense to allow end users to specify the desired GeoAPI implementation globally once, at the start of the application, and have it picked up by various libraries (e.g. ORMs). However, these libraries should probably also optionally allow a GeometryServiceProvider instance to be passed in, just in case the global setting isn't sufficient.

I may have missed some issues/complexities that GeometryServiceProvider.Instance creates, though...

[airbreather]

I think it may make sense to retain GeometryServiceProvider.Instance. As written above, the Npgsql ADO provider (not the EF Core provider) has no dependency injection of any sort. Now, I think the following are reasonable requirements from the Npgsql NTS plugin:

1. If the user hasn't indicated they're interested in any specific GeoAPI implementation, the plugin should default to using NetTopologySuite (i.e. instantiate NTS objects when reading PostGIS data from the database).
2. However, the plugin should definitely support other GeoAPI implementations. This could be done by:
   a. Receiving a GeometryServiceProvider instance in the plugin's initialization, or
   b. A global static setting as it is now (GeometryServiceProvider.Instance)

The idea behind this one is that I expect everybody who currently uses GeometryServiceProvider.Instance to fall into at least one of two categories (probably often both):

1. They would be better off using an implementation of IoC that's already available in their context to get the IGeometryServices instead (this is what @bricelam was referring to), and/or
2. They can just as easily switch to using NtsGeometryServices.Instance instead.

With #70, the case is even easier to make because the two types are going to be right next to each other.

[FObermaier]

With GeoAPI merged into NetTopologySuite there is no reason to have two public instances of IGeometryServices.