Some fixes and documentation from Zell.

src/java/com/samskivert/jdbc/depot/DepotMarshaller.java

@@ -251,22 +251,54 @@ public class DepotMarshaller<T>
     }
 
     /**
-     * Returns a key configured with the primary key of the supplied object.  Throws an exception
+     * Returns a key configured with the primary key of the supplied object. If all the fields are
-     * if the persistent object did not declare a primary key.
+     * null, this method returns null. An exception is thrown if some of the fields are null and
+     * some are not, or if the object does not declare a primary key.
      */
     public Key<T> getPrimaryKey (Object object)
+    {
+        return getPrimaryKey(object, true);
+    }
+
+    /**
+     * Returns a key configured with the primary key of the supplied object. If all the fields are
+     * null, this method returns null. If some of the fields are null and some are not, an
+     * exception is thrown. If the object does not declare a primary key and the second argument is
+     * true, this method throws an exception; if it's false, the method returns null.
+     */
+    public Key<T> getPrimaryKey (Object object, boolean requireKey)
     {
         if (!hasPrimaryKey()) {
-            throw new UnsupportedOperationException(
+            if (requireKey) {
-                _pclass.getName() + " does not define a primary key");
+                throw new UnsupportedOperationException(
+                    _pclass.getName() + " does not define a primary key");
+            }
+            return null;
         }
+
         try {
             Comparable[] values = new Comparable[_pkColumns.size()];
+            boolean hasNulls = false;
             for (int ii = 0; ii < _pkColumns.size(); ii++) {
                 FieldMarshaller field = _pkColumns.get(ii);
                 values[ii] = (Comparable) field.getField().get(object);
+                if (values[ii] == null || Integer.valueOf(0).equals(values[ii])) {
+                    // if this is the first null we see but not the first field, freak out
+                    if (!hasNulls && ii > 0) {
+                        throw new IllegalArgumentException(
+                            "Persistent object's primary key fields are mixed null and non-null.");
+                    }
+                    hasNulls = true;
+                } else if (hasNulls) {
+                    // if this is a non-null field and we've previously seen nulls, also freak
+                    throw new IllegalArgumentException(
+                        "Persistent object's primary key fields are mixed null and non-null.");
+                }
             }
-            return makePrimaryKey(values);
+
+            // if all the fields were null, return null, else build a key
+            return hasNulls ? null : makePrimaryKey(values);
+
         } catch (IllegalAccessException iae) {
             throw new RuntimeException(iae);
         }

src/java/com/samskivert/jdbc/depot/DepotRepository.java

@@ -189,7 +189,9 @@ public class DepotRepository
         throws PersistenceException
     {
         final DepotMarshaller marsh = _ctx.getMarshaller(record.getClass());
-        return _ctx.invoke(new CachingModifier<T>(null, null) {
+        final Key key = marsh.getPrimaryKey(record, false);
+        // key will be null if record was supplied without a primary key
+        return _ctx.invoke(new CachingModifier<T>(record, key, key) {
             public int invoke (Connection conn) throws SQLException {
                 // update our modifier's key so that it can cache our results
                 updateKey(marsh.assignPrimaryKey(conn, record, false));
@@ -198,7 +200,6 @@ public class DepotRepository
                     int mods = stmt.executeUpdate();
                     // check again in case we have a post-factum key generator
                     updateKey(marsh.assignPrimaryKey(conn, record, true));
-                    setInstance(record);
                     return mods;
                 } finally {
                     JDBCUtil.close(stmt);
@@ -218,7 +219,10 @@ public class DepotRepository
     {
         final DepotMarshaller marsh = _ctx.getMarshaller(record.getClass());
         final Key key = marsh.getPrimaryKey(record);
-        return _ctx.invoke(new CachingModifier<T>(key, key) {
+        if (key == null) {
+            throw new IllegalArgumentException("Can't update record with null primary key.");
+        }
+        return _ctx.invoke(new CachingModifier<T>(record, key, key) {
             public int invoke (Connection conn) throws SQLException {
                 PreparedStatement stmt = marsh.createUpdate(conn, record, key);
                 try {
@@ -241,7 +245,10 @@ public class DepotRepository
     {
         final DepotMarshaller marsh = _ctx.getMarshaller(record.getClass());
         final Key key = marsh.getPrimaryKey(record);
-        return _ctx.invoke(new CachingModifier<T>(key, key) {
+        if (key == null) {
+            throw new IllegalArgumentException("Can't update record with null primary key.");
+        }
+        return _ctx.invoke(new CachingModifier<T>(record, key, key) {
             public int invoke (Connection conn) throws SQLException {
                 PreparedStatement stmt = marsh.createUpdate(conn, record, key, modifiedFields);
                 try {
@@ -408,13 +415,13 @@ public class DepotRepository
     {
         final DepotMarshaller marsh = _ctx.getMarshaller(record.getClass());
         final Key key = marsh.hasPrimaryKey() ? marsh.getPrimaryKey(record) : null;
-        return _ctx.invoke(new CachingModifier<T>(key, key) {
+        return _ctx.invoke(new CachingModifier<T>(record, key, key) {
             public int invoke (Connection conn) throws SQLException {
                 PreparedStatement stmt = null;
                 try {
-                    // if our primary key is null or is the integer 0, assume the record has never
+                    // if our primary key isn't null, update rather than insert the record
                     // before been persisted and insert
-                    if (key != null && !Integer.valueOf(0).equals(key)) {
+                    if (key != null) {
                         stmt = marsh.createUpdate(conn, record, key);
                         int mods = stmt.executeUpdate();
                         if (mods > 0) {
@@ -429,7 +436,6 @@ public class DepotRepository
                     stmt = marsh.createInsert(conn, record);
                     int mods = stmt.executeUpdate();
                     updateKey(marsh.assignPrimaryKey(conn, record, true));
-                    setInstance(record);
                     return mods;
 
                 } finally {
@@ -450,6 +456,9 @@ public class DepotRepository
     {
         @SuppressWarnings("unchecked") Class<T> type = (Class<T>)record.getClass();
         Key<T> primaryKey = _ctx.getMarshaller(type).getPrimaryKey(record);
+        if (primaryKey == null) {
+            throw new IllegalArgumentException("Can't delete record with null primary key.");
+        }
         return deleteAll(type, primaryKey, primaryKey);
     }
 

src/java/com/samskivert/jdbc/depot/Modifier.java

@@ -53,23 +53,29 @@ public abstract class Modifier
     }
 
     /**
-     * A simple modifier that updates the cache with its modified object on completion. The derived
+     * A convenience modifier that can perform cache updates in addition to invalidation:
-     * class must call {@link #setInstance} to inform the modifier of its object at some point
+     * - Before {@link #invoke(Connection)}, the {@link CacheInvalidator} is run, if given.
-     * during the modification operation.
+     * - After {@link #invoke(Connection)}, the cache is updated with the modified object,
+     * presuming both _key and _result are non-null. These variables may be set or modified
+     * during execution in addition to being supplied to the constructor.
      */
     public static abstract class CachingModifier<T> extends Modifier
     {
-        protected CachingModifier (CacheKey key, CacheInvalidator invalidator)
+        /**
+         * Construct a new CachingModifier with the given result, cache key, and invalidator,
+         * all of which are optional, and may also be set during execution.
+         */
+        protected CachingModifier (T result, CacheKey key, CacheInvalidator invalidator)
         {
             super(invalidator);
+            _result = result;
             _key = key;
         }
 
-        protected void setInstance (T result)
+        /**
-        {
+         * Update this {@link CachingModifier}'s cache key, e.g. during insertion when a
-            _result = result;
+         * persistent object first receives a generated key.
-        }
+         */
-
         protected void updateKey (CacheKey key)
         {
             if (key != null) {
@@ -77,11 +83,12 @@ public abstract class Modifier
             }
         }
 
-        @Override
+        @Override // from Modifier
         public void cacheUpdate (PersistenceContext ctx)
         {
             super.cacheUpdate(ctx);
-            if (_key != null) {
+            // if we have both a key and a record, cache
+            if (_key != null && _result != null) {
                 ctx.cacheStore(_key, _result);
             }
         }
@@ -90,13 +97,23 @@ public abstract class Modifier
         protected T _result;
     }
 
+    /**
+     * Overriden to perform the actual database modifications represented by this object;
+     * should return the number of modified rows.
+     */
     public abstract int invoke (Connection conn) throws SQLException;
 
+    /**
+     * Constructs a {@link Modifier} without a cache invalidator.
+     */
     public Modifier ()
     {
         this(null);
     }
 
+    /**
+     * Constructs a {@link Modifier} with the given cache invalidator.
+     */
     public Modifier (CacheInvalidator invalidator)
     {
         _invalidator = invalidator;