Avoid unexpected array mutation in CoordinateArraySequence creation

[dr-jts]

The CoordinateArraySequence enforceArrayConsistency method can create or mutate coordinates in the client Coordinate array.

I'm not sure this is good behaviour because:

• It hides errors due to uninitialized null coordinates
• It mutates the passed-in coordinate array, which may introduce unexpected aliasing errors in the client
• It also creates a throw-away Coordinate object, which is slightly inefficient

It would be better to check Coordinate consistency and throw an exception if they are not consistent.

[dr-jts]

@jodygarnett @jnh5y what are your thoughts?

[jodygarnett]

This is only being called in a constructor right? It may be slightly more functional to call this method on the array before assigning to this.coordiantes. It is a protected method (not static) so that subclasses can be in charge of their own coordinate representation.

• It does hide error due to null coordinates, would you prefer to throw an exception, or to record null in the resulting array? What was the prior functionality ...
• It does mutate the provided array, the array is being provided to the object to be used directly is it not? I do not think we would like to make a copy due to overhead ...
• As for the throw-away sample ... we would need a new protected method allowing subclasses to return the class of coordiante they support and then use type isAssignableFrom ?

[dr-jts]

Yes, the enforce method is only being called in the constructor.
CoordinateArraySequence is a concrete class, and usually used directly, so would be best to fix this behaviour.

• Previous behaviour was to not check for null coords, and just let subsequent methods throw (which they generally will). This is easy to replicate - just don't check for nulls.
• I think the minimum thing this method needs to do is to check that all coordinate objects have the same class, if they are present. Could also check that they have the expected number of dimensions (which only needs to be done once, on the first coordinate instance).

[dr-jts]

BTW, I came across this error when I mistakenly omitted to initialize an element of a Coordinate array. I then called Geometry.getLength(), and was puzzled when the answer was incorrect. This is because the enforceArrayConsistency method silently replaces nulls with Coordinate(0,0). Previously this would have thrown at some point - which IMO is better behaviour.

[dr-jts]

@jodygarnett I guess a further question is whether the CoordinateArraySequence should check to see that the coordinates in the provided array actually match the dimension/measure values provided.

[jodygarnett]

update from chat:

• Martin is going to prep a PR toning this down to a check consistency
• Going to run JTS and GeoTools tests (Jody remembers a test-case producing inconsistent results with a coordinate array made up of mixed coordinates)

[jodygarnett]

Agreed, do whatever checks you can, and then we can run the tests

[dr-jts]

See PR #374

[jodygarnett]

This has been somewhat addressed for the 1.16.1 release using
#376