db-jdo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Bouschen <mbo.t...@spree.de>
Subject Re: Fix for JDO-77
Date Sun, 03 Jul 2005 21:28:43 GMT
Hi Craig,

this looks good, no remarks from my side.

Regards Michael

> Hi Michael,
>
> On Jul 2, 2005, at 11:58 AM, Michael Bouschen wrote:
>
>> Hi Craig,
>>
>> the changes look good!
>>
>> I would like to propose a few improvements of the method mangleObject 
>> you changed. Maybe this should be an extra check in, because they are 
>> not related to the fix for JDO-77.
>> (1) We plan to add configuration running the TCK with a security 
>> manager. Then the reflection access of method mangleObject should be 
>> enclosed in a doPivileged block.
>
>
> Done.
>
>> (2) The method checks for fields of specific types. Should we add 
>> date, BigDecimal, BigInteger, float, double, Float and Double?
>
>
> Later. None of the identity classes uses these types.
>
>> (3) We could use 'else if' when checking the field types.
>
>
> Done.
>
> Please check the attached.
>
> Thanks,
>
> Craig
>
> =
>
>------------------------------------------------------------------------
>
>Index: test/java/org/apache/jdo/tck/JDO_Test.java
>===================================================================
>--- test/java/org/apache/jdo/tck/JDO_Test.java	(revision 208860)
>+++ test/java/org/apache/jdo/tck/JDO_Test.java	(working copy)
>@@ -20,10 +20,18 @@
> import java.io.FileInputStream;
> import java.io.IOException;
> import java.io.InputStream;
>+
> import java.lang.reflect.Field;
>+import java.lang.reflect.Modifier;
>+
>+import java.security.AccessController;
>+import java.security.PrivilegedAction;
>+
>+import java.util.ArrayList;
> import java.util.Collection;
> import java.util.Iterator;
> import java.util.LinkedList;
>+import java.util.List;
> import java.util.Properties;
> import java.util.Vector;
> 
>@@ -749,54 +757,75 @@
>         return NUM_STATES;
>     }
> 
>-    /** This method mangles an object by changing all its public fields
>+    /** This method mangles an object by changing all its non-static, 
>+     *  non-final fields.
>+     *  It returns true if the object was mangled, and false if there
>+     *  are no fields to mangle.
>      */
>-    protected void mangleObject (Object oid) 
>+    protected boolean mangleObject (Object oid) 
>         throws Exception {
>-        Class oidClass = oid.getClass();
>-        Field[] fields = oidClass.getFields();
>+        Field[] fields = getModifiableFields(oid);
>+        if (fields.length == 0) return false;
>         for (int i = 0; i < fields.length; ++i) {
>             Field field = fields[i];
>-            field.setAccessible(true);
>-            if (debug) 
>-                logger.debug("field" + i + " has name: " + field.getName() +
>-                             " type: " + field.getType());
>             Class fieldType = field.getType();
>             if (fieldType == long.class) {
>-                field.setLong(oid, 10000L);
>+                field.setLong(oid, 10000L + field.getLong(oid));
>+            } else if (fieldType == int.class) {
>+                field.setInt(oid, 10000 + field.getInt(oid));
>+            } else if (fieldType == short.class) {
>+                field.setShort(oid, (short)(10000 + field.getShort(oid)));
>+            } else if (fieldType == byte.class) {
>+                field.setByte(oid, (byte)(100 + field.getByte(oid)));
>+            } else if (fieldType == char.class) {
>+                field.setChar(oid, (char)(10 + field.getChar(oid)));
>+            } else if (fieldType == String.class) {
>+                field.set(oid, "This is certainly a challenge" + (String)field.get(oid));
>+            } else if (fieldType == Integer.class) {
>+                field.set(oid, new Integer(10000 + ((Integer)field.get(oid)).intValue()));
>+            } else if (fieldType == Long.class) {
>+                field.set(oid, new Long(10000L + ((Long)field.get(oid)).longValue()));
>+            } else if (fieldType == Short.class) {
>+                field.set(oid, new Short((short)(10000 + ((Short)field.get(oid)).shortValue())));
>+            } else if (fieldType == Byte.class) {
>+                field.set(oid, new Byte((byte)(100 + ((Byte)field.get(oid)).byteValue())));
>+            } else if (fieldType == Character.class) {
>+                field.set(oid, new Character((char)(10 + ((Character)(field.get(oid))).charValue())));
>             }
>-            if (fieldType == int.class) {
>-                field.setInt(oid, 10000);
>-            }
>-            if (fieldType == short.class) {
>-                field.setShort(oid, (short)10000);
>-            }
>-            if (fieldType == byte.class) {
>-                field.setByte(oid, (byte)100);
>-            }
>-            if (fieldType == char.class) {
>-                field.setChar(oid, '0');
>-            }
>-            if (fieldType == String.class) {
>-                field.set(oid, "This is certainly a challenge");
>-            }
>-            if (fieldType == Integer.class) {
>-                field.set(oid, new Integer(10000));
>-            }
>-            if (fieldType == Long.class) {
>-                field.set(oid, new Long(10000L));
>-            }
>-            if (fieldType == Short.class) {
>-                field.set(oid, new Short((short)10000));
>-            }
>-            if (fieldType == Byte.class) {
>-                field.set(oid, new Byte((byte)100));
>-            }
>-            if (fieldType == Character.class) {
>-                field.set(oid, new Character('0'));
>-            }
>         }
>+        return true;
>     }
>+    
>+    /** Returns modifiable Fields of the class of the parameter. 
>+     * Fields are considered modifiable if they are not static or final.
>+     * This method requires several permissions in order to run with
>+     * a SecurityManager, hence the doPrivileged block:
>+     * <ul>
>+     * <li>ReflectPermission("suppressAccessChecks")</li>
>+     * <li>RuntimePermission("accessDeclaredMembers")</li>
>+     * </ul>
>+     */
>+    protected Field[] getModifiableFields(final Object obj) {
>+        return (Field[])AccessController.doPrivileged(
>+            new PrivilegedAction () {
>+                public Object run () {
>+                    Class cls = obj.getClass();
>+                    List result = new ArrayList();
>+                    Field[] fields = cls.getFields();
>+                    for (int i = 0; i < fields.length; ++i) {
>+                        Field field = fields[i];
>+                        int modifiers = field.getModifiers();
>+                        if (Modifier.isFinal(modifiers) ||
>+                                Modifier.isStatic(modifiers))
>+                            continue;
>+                        field.setAccessible(true);
>+                        result.add(field);
>+                    }
>+                    return result.toArray(new Field[result.size()]);
>+                }
>+            }
>+        );
>+    }
> 
>     /**
>      * Returns <code>true</code> if the current test runs with application
>Index: test/java/org/apache/jdo/tck/lifecycle/ObjectIdNotModifiedWhenObjectIdInstanceModified.java
>===================================================================
>--- test/java/org/apache/jdo/tck/lifecycle/ObjectIdNotModifiedWhenObjectIdInstanceModified.java
(revision 208860)
>+++ test/java/org/apache/jdo/tck/lifecycle/ObjectIdNotModifiedWhenObjectIdInstanceModified.java
(working copy)
>@@ -16,7 +16,9 @@
>  
> package org.apache.jdo.tck.lifecycle;
> 
>+import java.util.ArrayList;
> import java.util.Iterator;
>+import java.util.List;
> 
> import javax.jdo.Extent;
> import javax.jdo.Transaction;
>@@ -76,16 +78,45 @@
>     				 "Extent for StateTransitionObj should not be empty");
>     		}
>     		extent.close(iter);
>-    
>-    		for (int i=0; i<NUM_OBJECTS; i++)
>-    		{
>-    			Object objId=pm.getObjectId(obj[i]);
>-    			mangleObject(objId);
>-    			Object objId2 = pm.getObjectId(obj[i]); // get another ObjectId copy
>-    			if (objId.equals(objId2))
>-    				fail(ASSERTION_FAILED,
>-    					 "object Id has been changed");
>+            int failures = 0;
>+            StringBuffer report = new StringBuffer("Failures comparing oids.\n");
>+    		for (int i=0; i<NUM_OBJECTS; i++) {
>+    			Object objId1=pm.getObjectId(obj[i]);
>+                String before=objId1.toString();
>+                int objId1HashCode = objId1.hashCode();
>+                Object objId2=pm.getObjectId(obj[i]);
>+    			if (!mangleObject(objId2)) {
>+                    /* The object id class is immutable, so the test succeeds. */
>+                    break;
>+                }
>+                int objId2HashCode = objId2.hashCode();
>+    			Object objId3 = pm.getObjectId(obj[i]); // get another ObjectId copy
>+    			if (!(objId1.equals(objId3) && objId1HashCode != objId2HashCode)) {
>+                    /* The object id obtained after mangling the second object id
>+                     * must equal the original object id, and the mangling must
>+                     * have changed the mangled id.
>+                     */
>+                    report.append("Index= ");
>+                    report.append(i);
>+                    report.append("\n");
>+                    report.append(" before= ");
>+                    report.append(before);
>+                    report.append("\n");
>+                    report.append("mangled= ");
>+                    report.append(objId2.toString());
>+                    report.append("\n");
>+                    report.append("  after= ");
>+                    report.append(objId3.toString());
>+                    report.append("\n");
>+                    ++failures;
>+                }
>     		}
>+            if (failures != 0) {
>+                if (debug) {
>+                    logger.debug(report.toString());
>+                }
>+                fail(ASSERTION_FAILED, "Failed to compare " + failures + " object ids.");
>+            }
>             pm.currentTransaction().commit();
>         } finally {
>             if (pm!=null && pm.currentTransaction().isActive()) {
>
>
> ------------------------------------------------------------------------
>
>
>>
>> Regards Michael
>>
>>
>>> Hi,
>>>
>>> Please review this patch. It fixes an illegal access exception when 
>>> mangleObject tries to change a final field. The final field was 
>>> added to the PCPoint$Oid class.
>>>
>>> The fix checks for final and static modifiers on the field before it 
>>> changes it.
>>>
>>> I also changed the method to actually change each field, instead of 
>>> simply changing its value to a constant.
>>>
>>> Thanks,
>>>
>>> Craig
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: test/java/org/apache/jdo/tck/JDO_Test.java
>>> ===================================================================
>>> --- test/java/org/apache/jdo/tck/JDO_Test.java    (revision 208860)
>>> +++ test/java/org/apache/jdo/tck/JDO_Test.java    (working copy)
>>> @@ -757,43 +757,47 @@
>>>         Field[] fields = oidClass.getFields();
>>>         for (int i = 0; i < fields.length; ++i) {
>>>             Field field = fields[i];
>>> +            int modifiers = field.getModifiers();
>>> +            if (java.lang.reflect.Modifier.isFinal(modifiers) ||
>>> +                    java.lang.reflect.Modifier.isStatic(modifiers))
>>> +                break;
>>>             field.setAccessible(true);
>>>             if (debug)                 logger.debug("field" + i + " 
>>> has name: " + field.getName() +
>>>                              " type: " + field.getType());
>>>             Class fieldType = field.getType();
>>>             if (fieldType == long.class) {
>>> -                field.setLong(oid, 10000L);
>>> +                field.setLong(oid, 10000L + field.getLong(oid));
>>>             }
>>>             if (fieldType == int.class) {
>>> -                field.setInt(oid, 10000);
>>> +                field.setInt(oid, 10000 + field.getInt(oid));
>>>             }
>>>             if (fieldType == short.class) {
>>> -                field.setShort(oid, (short)10000);
>>> +                field.setShort(oid, (short)(10000 + 
>>> field.getShort(oid)));
>>>             }
>>>             if (fieldType == byte.class) {
>>> -                field.setByte(oid, (byte)100);
>>> +                field.setByte(oid, (byte)(100 + field.getByte(oid)));
>>>             }
>>>             if (fieldType == char.class) {
>>> -                field.setChar(oid, '0');
>>> +                field.setChar(oid, (char)(10 + field.getChar(oid)));
>>>             }
>>>             if (fieldType == String.class) {
>>> -                field.set(oid, "This is certainly a challenge");
>>> +                field.set(oid, "This is certainly a challenge" + 
>>> (String)field.get(oid));
>>>             }
>>>             if (fieldType == Integer.class) {
>>> -                field.set(oid, new Integer(10000));
>>> +                field.set(oid, new Integer(10000 + 
>>> ((Integer)field.get(oid)).intValue()));
>>>             }
>>>             if (fieldType == Long.class) {
>>> -                field.set(oid, new Long(10000L));
>>> +                field.set(oid, new Long(10000L + 
>>> ((Long)field.get(oid)).longValue()));
>>>             }
>>>             if (fieldType == Short.class) {
>>> -                field.set(oid, new Short((short)10000));
>>> +                field.set(oid, new Short((short)(10000 + 
>>> ((Short)field.get(oid)).shortValue())));
>>>             }
>>>             if (fieldType == Byte.class) {
>>> -                field.set(oid, new Byte((byte)100));
>>> +                field.set(oid, new Byte((byte)(100 + 
>>> ((Byte)field.get(oid)).byteValue())));
>>>             }
>>>             if (fieldType == Character.class) {
>>> -                field.set(oid, new Character('0'));
>>> +                field.set(oid, new Character((char)(10 + 
>>> ((Character)(field.get(oid))).charValue())));
>>>             }
>>>         }
>>>     }
>>>
>>>  
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>> Craig Russell
>>>
>>> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>>>
>>> 408 276-5638 mailto:Craig.Russell@sun.com
>>>
>>> P.S. A good JDO? O, Gasp!
>>>
>>>
>>>
>>
>>
>> -- 
>> Michael Bouschen        Tech@Spree Engineering GmbH
>> mailto:mbo.tech@spree.de    http://www.tech.spree.de/
>> Tel.:++49/30/235 520-33        Buelowstr. 66            
>> Fax.:++49/30/2175 2012        D-10783 Berlin            
>>
>>
>
> Craig Russell
>
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
>
> 408 276-5638 mailto:Craig.Russell@sun.com
>
> P.S. A good JDO? O, Gasp!
>
>
> =



-- 
Michael Bouschen		Tech@Spree Engineering GmbH
mailto:mbo.tech@spree.de	http://www.tech.spree.de/
Tel.:++49/30/235 520-33		Buelowstr. 66			
Fax.:++49/30/2175 2012		D-10783 Berlin			


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message