Обсуждение: Patch to improve Cloneable implementation on classes which extend PGobject.

The attached patch aims to improve the current implementation of the
clone() method on classes which extend PGobject.  I believe that the
current implementation has a few deficiencies, namely

1. They are implemented by calling the objects constructors which means
that each derived class must re-implement all of the logic of the parent
in order to clone itself correctly.

2. When extending an object, the default behavior of the clone() method
is to return an object of the wrong type where most developers expect
that if the derived class is Cloneable and doesn't add any mutable
fields the base classes clone() implementation should suffice in cloning
the object and it should return an object of the same type as the
derived class.

I think that this patch provides implementations of the clone() method
which falls more in line with the behavior that most developers expect
from the method.  Thanks for considering the patch for inclusion in the
driver.  If you have any questions or problems with it, please feel free
to ask.

Sincerely,
Russ Francis
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGbox.java pgjdbc/org/postgresql/geometric/PGbox.java
--- pgjdbc.orig/org/postgresql/geometric/PGbox.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGbox.java    2007-04-28 23:12:58.000000000 -0400
@@ -130,9 +130,17 @@
         return point[0].hashCode() ^ point[1].hashCode();
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGbox((PGpoint)point[0].clone(), (PGpoint)point[1].clone());
+        PGbox newPGbox = (PGbox) super.clone();
+        if( newPGbox.point != null )
+        {
+            newPGbox.point = (PGpoint[]) newPGbox.point.clone();
+            for( int i = 0; i < newPGbox.point.length; ++i )
+                if( newPGbox.point[i] != null )
+                    newPGbox.point[i] = (PGpoint) newPGbox.point[i].clone();
+        }
+        return newPGbox;
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGcircle.java pgjdbc/org/postgresql/geometric/PGcircle.java
--- pgjdbc.orig/org/postgresql/geometric/PGcircle.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGcircle.java    2007-04-28 23:05:11.000000000 -0400
@@ -114,9 +114,12 @@
         return (int) (center.hashCode() ^ v ^ (v >>> 32));
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGcircle((PGpoint)center.clone(), radius);
+        PGcircle newPGcircle = (PGcircle) super.clone();
+        if( newPGcircle.center != null )
+            newPGcircle.center = (PGpoint) newPGcircle.center.clone();
+        return newPGcircle;
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGline.java pgjdbc/org/postgresql/geometric/PGline.java
--- pgjdbc.orig/org/postgresql/geometric/PGline.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGline.java    2007-04-28 23:13:35.000000000 -0400
@@ -104,9 +104,17 @@
         return point[0].hashCode() ^ point[1].hashCode();
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGline((PGpoint)point[0].clone(), (PGpoint)point[1].clone());
+        PGline newPGline = (PGline) super.clone();
+        if( newPGline.point != null )
+        {
+            newPGline.point = (PGpoint[]) newPGline.point.clone();
+            for( int i = 0; i < newPGline.point.length; ++i )
+                if( newPGline.point[i] != null )
+                    newPGline.point[i] = (PGpoint) newPGline.point[i].clone();
+        }
+        return newPGline;
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGlseg.java pgjdbc/org/postgresql/geometric/PGlseg.java
--- pgjdbc.orig/org/postgresql/geometric/PGlseg.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGlseg.java    2007-04-28 23:05:21.000000000 -0400
@@ -102,9 +102,17 @@
         return point[0].hashCode() ^ point[1].hashCode();
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGlseg((PGpoint)point[0].clone(), (PGpoint)point[1].clone());
+        PGlseg newPGlseg = (PGlseg) super.clone();
+        if( newPGlseg.point != null )
+        {
+            newPGlseg.point = (PGpoint[]) newPGlseg.point.clone();
+            for( int i = 0; i < newPGlseg.point.length; ++i )
+                if( newPGlseg.point[i] != null )
+                    newPGlseg.point[i] = (PGpoint) newPGlseg.point[i].clone();
+        }
+        return newPGlseg;
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGpath.java pgjdbc/org/postgresql/geometric/PGpath.java
--- pgjdbc.orig/org/postgresql/geometric/PGpath.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGpath.java    2007-04-28 23:15:32.000000000 -0400
@@ -124,12 +124,16 @@
         return hash;
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        PGpoint ary[] = new PGpoint[points.length];
-        for (int i = 0;i < points.length;i++)
-            ary[i] = (PGpoint)points[i].clone();
-        return new PGpath(ary, open);
+        PGpath newPGpath = (PGpath) super.clone();
+        if( newPGpath.points != null )
+        {
+            newPGpath.points = (PGpoint[]) newPGpath.points.clone();
+            for( int i = 0; i < newPGpath.points.length; ++i )
+                newPGpath.points[i] = (PGpoint) newPGpath.points[i].clone();
+        }
+        return newPGpath;
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGpoint.java pgjdbc/org/postgresql/geometric/PGpoint.java
--- pgjdbc.orig/org/postgresql/geometric/PGpoint.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGpoint.java    2007-04-28 23:05:11.000000000 -0400
@@ -107,9 +107,9 @@
         return (int) (v1 ^ v2 ^ (v1 >>> 32) ^ (v2 >>> 32));
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGpoint(x, y);
+        return super.clone();
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/geometric/PGpolygon.java pgjdbc/org/postgresql/geometric/PGpolygon.java
--- pgjdbc.orig/org/postgresql/geometric/PGpolygon.java    2005-01-11 03:25:45.000000000 -0500
+++ pgjdbc/org/postgresql/geometric/PGpolygon.java    2007-04-28 23:05:11.000000000 -0400
@@ -98,12 +98,17 @@
         return hash;
     }

-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        PGpoint ary[] = new PGpoint[points.length];
-        for (int i = 0;i < points.length;i++)
-            ary[i] = (PGpoint)points[i].clone();
-        return new PGpolygon(ary);
+        PGpolygon newPGpolygon = (PGpolygon) super.clone();
+        if( newPGpolygon.points != null )
+        {
+            newPGpolygon.points = (PGpoint[]) newPGpolygon.points.clone();
+            for( int i = 0; i < newPGpolygon.points.length; ++i )
+                if( newPGpolygon.points[i] != null )
+                    newPGpolygon.points[i] = (PGpoint) newPGpolygon.points[i].clone();
+        }
+        return newPGpolygon;
     }

     /**
diff -ruN pgjdbc.orig/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java
pgjdbc/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java
--- pgjdbc.orig/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java    2007-04-16 12:36:41.000000000 -0400
+++ pgjdbc/org/postgresql/jdbc2/AbstractJdbc2ResultSet.java    2007-04-28 23:04:06.000000000 -0400
@@ -13,6 +13,7 @@
 import java.io.InputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.io.Serializable;
 import java.io.UnsupportedEncodingException;
 import java.io.ByteArrayInputStream;
 import java.math.BigDecimal;
@@ -2838,7 +2839,7 @@
     // values but retains column type info.
     //

-    class NullObject extends PGobject {
+    class NullObject extends PGobject implements Serializable, Cloneable {
         NullObject(String type) {
             setType(type);
         }
@@ -2846,6 +2847,11 @@
         public String getValue() {
             return null;
         }
+
+        public Object clone() throws CloneNotSupportedException
+        {
+            return super.clone();
+        }
     };
 }

diff -ruN pgjdbc.orig/org/postgresql/util/PGInterval.java pgjdbc/org/postgresql/util/PGInterval.java
--- pgjdbc.orig/org/postgresql/util/PGInterval.java    2007-04-11 04:31:51.000000000 -0400
+++ pgjdbc/org/postgresql/util/PGInterval.java    2007-04-28 23:05:53.000000000 -0400
@@ -442,9 +442,9 @@
      *
      * @return Cloned interval
      */
-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGInterval(years, months, days, hours, minutes, seconds);
+        return super.clone();
     }

 }
diff -ruN pgjdbc.orig/org/postgresql/util/PGmoney.java pgjdbc/org/postgresql/util/PGmoney.java
--- pgjdbc.orig/org/postgresql/util/PGmoney.java    2005-01-11 03:25:49.000000000 -0500
+++ pgjdbc/org/postgresql/util/PGmoney.java    2007-04-28 23:05:53.000000000 -0400
@@ -89,9 +89,9 @@
     /*
      * This must be overidden to allow the object to be cloned
      */
-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        return new PGmoney(val);
+        return super.clone();
     }

     public String getValue()
diff -ruN pgjdbc.orig/org/postgresql/util/PGobject.java pgjdbc/org/postgresql/util/PGobject.java
--- pgjdbc.orig/org/postgresql/util/PGobject.java    2005-01-11 03:25:49.000000000 -0500
+++ pgjdbc/org/postgresql/util/PGobject.java    2007-04-28 22:47:29.000000000 -0400
@@ -86,12 +86,9 @@
     /**
      * This must be overidden to allow the object to be cloned
      */
-    public Object clone()
+    public Object clone() throws CloneNotSupportedException
     {
-        PGobject obj = new PGobject();
-        obj.type = type;
-        obj.value = value;
-        return obj;
+        return super.clone();
     }

     /**

On Sun, 24 Jun 2007, Russell Francis wrote:

> The attached patch aims to improve the current implementation of the
> clone() method on classes which extend PGobject.

Makes sense, but why are you providing clone implementations in point,
money, interval and NullObject instances?  Shouldn't this already be
handled by PGObject's implementation?

Kris Jurka

Kris Jurka wrote:
>
> On Sun, 24 Jun 2007, Russell Francis wrote:
>
>> The attached patch aims to improve the current implementation of the
>> clone() method on classes which extend PGobject.
>
> Makes sense, but why are you providing clone implementations in point,
> money, interval and NullObject instances?  Shouldn't this already be
> handled by PGObject's implementation?

Yes, in all of those cases the clone() implementation in PGobject would
do what is expected.  I suppose I provided the method declarations and
trivial implementations for clarity but they are not needed.

Thanks,
Russ

On Mon, 16 Jul 2007, Russell Francis wrote:

> Kris Jurka wrote:
>>
>> On Sun, 24 Jun 2007, Russell Francis wrote:
>>
>>> The attached patch aims to improve the current implementation of the
>>> clone() method on classes which extend PGobject.
>>
>> Makes sense, but why are you providing clone implementations in point,
>> money, interval and NullObject instances?  Shouldn't this already be
>> handled by PGObject's implementation?
>
> Yes, in all of those cases the clone() implementation in PGobject would
> do what is expected.  I suppose I provided the method declarations and
> trivial implementations for clarity but they are not needed.
>

OK, patch applied.

Kris Jurka