[postgis-devel] Re: Implementing support for PostGIS spatial types in JPOX

[Thomas Marti (HSR)]

Hello everyone

Let's continue this discussion...

Markus Schaber wrote:
  >> equals() does not conform to the general contract (throws an
  >> exception when null is given)
  >
  > This is a bug, I'll look into fixing it.

Well, it is already fixed...in the attached patches...

  >> equals() uses 'getClass()' instead of 'instanceof'
  >
  > The reason for this is that I intentionally wanted to avoid having the
  > problem of comparing different subclasses.
  >
  > Say we have an Object a of MyPoint, and an Object b of YourPoint, both
  > extending Point.
  >
  > Now the code has to see that both of them are a subclass of point, and
  > then compare them, having no knowledge about the exact semantics of
  > MyPoint and YourPoint.

Well, to be honest with you. You're trying to solve a problem that
cannot be solved! To quote Joshua Bloch's excellent book 'Effective
Java' (Chapter 3, Item 7):
"There is simply no way to extend an instantiable class and add an
aspect while preserving the equals contract."

In other words: Every class that extends Point and adds fields, whos
values are relevant for equality will automatically break the equals()
contract!

With that in mind, the current implementation isn't really helping
anyone. Not the extending subclass that adds aspects and not the
extending subclass which doens't add aspects (like ours). The problem
is, you can't try to let a superclass "think" for it's subclasses. But
you can provide suitable methods, like equals() implemented with
instanceof, that are open for extension. When a subclass has other
needs, it has to override the method, but that is none of the
superclasses business. We really hope you reconsider this.


  > For your case, you should override the equals method in your subclass
  > with appropriate semantics, and then possibly the equalsIntern method.

This doesn't solve the problem, because we can't ensure symmetry.
Example:

Point p1 = new Point( 1, 1 );
PointWrapper p = new PointWrapper( 1, 1 ); // equals() overridden

p2.equals( p1 ); // true
p1.equals( p2 ); // false, because of getClass()!


  > It seems that you had attached the original, unmodified versions of the
  > classes.
  >

Uhm, actually we didn't. If you look closely you'll see the changes we
made in equals(). We also "killed off" equalsintern(), because we think
it isn't needed.

  > Btw, I prefer a "diff -u" attached as text/plain instead of .zip, as
  > it clearly shows the differences and can be looked at directly from
  > the MUA.

Sorry for that, wanted to save some space. I attached the plain diffs
adn all java classes again in this mail.

[snip - taken from the third mail]
  > I just committed the equals() fix, as well as two getter methods for
  > Geometry, and am looking forward your reply to my other mails.
  >
  > Btw, as the JDBC code is relatively independent from the rest of
  > PostGIS, we should discuss separating releases, e. G. by moving the
  > JDBC part into its own CVS directory, just as the PostgreSQL people
  > moved all non-libpq client libraries into their own projects.
  >
  > This allows for different minor releases of PostGIS and PostGIS-JDBC.
[snip]

Well, you were a bit too fast for us... ;) Because of the scenario
described above we still favour "our version". We also think the code is
bit more compact, while retaining the same functionality. Also, our
approach follows the recommended protocol for implementing equals() as
described in "Effective Java" and elsewhere.

Some more ideas to "beautify" the code:
* JtsGeometry.toString() should have a check for null
* ComposedGeom: use isEmpty() instead of ((subgeoms == null) ||
(subgeoms.length == 0))

About dividing PostGIS-JDBC and the geometry classes: This sounds like a
very good idea to us, but you're the experts who have to decide this.

  >> Is there any way to make them private in future releases?
  >
  > I had thought about doing this, but it will possibly break compatibility
  > with existing software.
  >
  > We should take two steps for this:
  >
  > - enshure that all attributes have proper accessor methods, could be in
  > a minor release
  >
  > - change all non-final member variables to private (or at least
  > protected), this should be a 2.x or at least 1.x release, not in
1.1.x IMHO.

[snip - taken from the second mail]
  > There's another discussion topic here:
  >
  > At least for JTS, their general design approach is to treat all geometry
  > objects as unmodifiable, and instead of modifying a geometry in place,
  > operations create new geometries. I know that their code does not 100%
  > reflect his, but it is what they advise.
  >
  > Having a similar model for PostGIS would mean to make all public fields
  > final, or to only provide getter methods and no setter methods. How
  > would this interfere with your plans?
[snip]

We're very glad, that you brought this up.

Right now the classes are in a sort of mixed state: You can change any
field in Point and Geometry (because they're public and have setters),
but not in the other classes (because subgeoms is protected and has no
setters). But then again you can get a single Point of a Polygon,
LineString or whatever and change that. So the current implementation
isn't very consistent.

  From a technical point of view, either way (mutable or immutable) is
fine with us, but it has to be consistent! From a design point of view,
we would strongly favour an immutable implementation across all Geometry
classes, because we feel this makes the most sense and also is
consistent with JTS.


  >> 3.) How far developed is JTS-Support in the PostGIS-JDBC-Driver? The
  >> todo-file is rather sparse and says only 'Finish JTS support'. Can
  >> anyone state how complete the implementation approximately is?
  >> 20% or 80%?
  >
  > It is fully working, and in productional use at our site.
  >
  > However, the regression tests are not extensive enough, the build
  > process is suboptimal, and I still have to rework the whole
  > GeometryFactory / SRID thing to avoid the strange geometry factory
  > cache, now as JTS does not deprecate the SRID fields any more.
  >
  > And there are bugs in code pathes I did not use in my own projects (as
  > the one you found below), due to the lack of user feedback and
  > regression tests.

OK, it definitely sounds like a "worthy candidate" for our project... ;)


  >> 4.) What is the difference between JtsWrapper and JtsGisWrapper? The
  >> only things we found, when comparing the source code was:
  >>     * the POSTGIS_PROTOCOL differs: "jdbc:postgresql_JTS:" vs.
  >> "jdbc:postgres_jts:"
  >>     * JtsGisWrapper adds 'geometry', 'box2d' and 'box3d' as datatypes.
  >> JtsWrapper adds only 'geometry'. But where is the difference in the
  >> implementation?
  >
  > There is no other difference, the JtsWrapper only registers a handler
  > to map the JTS classes on the PostGIS geometry PostgreSQL datatype,
  > whereas the JtsGisWrapper additionally uses the plain PostGIS
  > box2d/box3d classes to handle bounding boxes.

Hmm, then maybe setting a more descriptive name, rather than just use
'jts' in upper and lower case, could help prevent confusion in the future...


  >> 5.) We also think that we've found a bug in
  >> org.postgis.jts.JtsGeometry#equals():
  >
  >>             return ((JtsGeometry) obj).getValue().equals(geom);
  >
  > Yes, that's a bug, AFAICS it should have read
  >
  >               return ((JtsGeometry) obj).getGeometry().equals(geom);
  >
  > But that still does not fix the "null" issue, I'll overhaul it and
  > commit the fix as soon as CVS access works again.

Actually it does! The instanceof-operator returns false, if the argument
is null. So, one thing less, that you have to worry about... ;)

I made a patch for this as well, it is attached.


  > Thanks a lot,
  > Markus

Thank YOU... ;)


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Point.java.patch
URL: <http://lists.osgeo.org/pipermail/postgis-devel/attachments/20060509/9a80cbe2/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ComposedGeom.java.patch
URL: <http://lists.osgeo.org/pipermail/postgis-devel/attachments/20060509/9a80cbe2/attachment-0001.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Geometry.java.patch
URL: <http://lists.osgeo.org/pipermail/postgis-devel/attachments/20060509/9a80cbe2/attachment-0002.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: JtsGeometry.java.patch
URL: <http://lists.osgeo.org/pipermail/postgis-devel/attachments/20060509/9a80cbe2/attachment-0003.ksh>