Patch from Zell to ensure that Key objects are only constructed with primary

key fields and also don't leave any out.

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

@@ -21,6 +21,7 @@
 package com.samskivert.jdbc.depot;
 
 import java.io.Serializable;
+import java.lang.reflect.Field;
 import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.util.ArrayList;
@@ -28,7 +29,9 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 
+import com.samskivert.jdbc.depot.annotation.Id;
 import com.samskivert.jdbc.depot.clause.Where;
+import com.samskivert.util.StringUtil;
 
 /**
  * A special form of {@link Where} clause that uniquely specifices a single database row and
@@ -68,31 +71,46 @@ public class Key<T extends PersistentRecord> extends Where
 
     /**
      * Constructs a new multi-column {@code Key} with the given values.
-     * 
-     * TODO: There is no reason to store both fields and values here (and doing so probably more
-     * than doubles the space the key consumes in the cache). The primary key fields are known
-     * by the DepotMarshaller; we should simply check tha the given indices match those, and
-     * store the values in the order defined by the DepotMarshaller. The sanity check would be
-     * welcome in any case. The only problem with this is that we don't currently have access to
-     * a {@link PersistenceContext}. It may be that we should make this class internal to
-     * {@link DepotRepository} and only create it ourselves. This would require *lots* more
-     * convenience methods in {@link DepotRepository} though.
      */
     public Key (Class<T> pClass, String[] fields, Comparable[] values)
     {
         // TODO: make Where an interface so we don't have to do this ugly super call
         super(null);
-
+        _pClass = pClass;
+        
         if (fields.length != values.length) {
             throw new IllegalArgumentException("Field and Value arrays must be of equal length.");
         }
-        _pClass = pClass;
+        
-        _map = new HashMap<String, Comparable>();
+        // build a local map of field name -> field value
+        Map<String, Comparable> map = new HashMap<String, Comparable>();
         for (int i = 0; i < fields.length; i ++) {
-            _map.put(fields[i], values[i]);
+            map.put(fields[i], values[i]);
+        }
+        
+        // then introspect on the persistent record and iterate over its actual fields
+        _fields = new ArrayList<String>(fields.length);
+        _values = new ArrayList<Comparable>(fields.length);
+        for (Field field : _pClass.getFields()) {
+            // look for @Id fields
+            if (field.getAnnotation(Id.class) != null) {
+                String fName = field.getName();
+                // make sure we were provided with a value for this primary key field 
+                if (map.containsKey(fName) == false) {
+                    throw new IllegalArgumentException("Missing value for key field: " + fName);
+                }
+                _fields.add(fName);
+                _values.add(map.get(fName));
+                map.remove(fName);
+            }
+        }
+        // finally make sure we were not given any fields that are not in fact primary key fields
+        if (map.size() > 0) {
+            throw new IllegalArgumentException(
+                "Non-key columns given: " +  StringUtil.join(map.keySet().toArray(), ", "));
         }
     }
-
+    
     // from QueryClause
     public Collection<Class<? extends PersistentRecord>> getClassSet ()
     {
@@ -106,15 +124,11 @@ public class Key<T extends PersistentRecord> extends Where
     public void appendClause (ConstructedQuery<?> query, StringBuilder builder)
     {
         builder.append(" where ");
-        boolean first = true;
+        for (int ii = 0; ii < _fields.size(); ii ++) {
-        for (Map.Entry entry : _map.entrySet()) {
+            if (ii > 0) {
-            if (first) {
-                first = false;
-            } else {
                 builder.append(" and ");
             }
-            builder.append(entry.getKey());
+            builder.append(_fields.get(ii)).append(_values.get(ii) == null ? " is null " : " = ? ");
-            builder.append(entry.getValue() == null ? " is null " : " = ? ");
         }
     }
 
@@ -122,9 +136,9 @@ public class Key<T extends PersistentRecord> extends Where
     public int bindArguments (PreparedStatement pstmt, int argIdx)
         throws SQLException
     {
-        for (Map.Entry entry : _map.entrySet()) {
+        for (int ii = 0; ii < _fields.size(); ii ++) {
-            if (entry.getValue() != null) {
+            if (_values.get(ii) != null) {
-                pstmt.setObject(argIdx ++, entry.getValue());
+                pstmt.setObject(argIdx ++, _values.get(ii));
             }
         }
         return argIdx;
@@ -139,7 +153,7 @@ public class Key<T extends PersistentRecord> extends Where
     // from CacheKey
     public Serializable getCacheKey ()
     {
-        return _map;
+        return _values;
     }
 
     // from CacheInvalidator
@@ -151,7 +165,7 @@ public class Key<T extends PersistentRecord> extends Where
     @Override
     public int hashCode ()
     {
-        return _pClass.hashCode() + 31 * _map.hashCode();
+        return _pClass.hashCode() + 31 * _values.hashCode();
     }
 
     @Override
@@ -164,20 +178,22 @@ public class Key<T extends PersistentRecord> extends Where
             return false;
         }
         Key other = (Key) obj;
-        return (_pClass == other._pClass && _map.equals(other._map));
+        return _pClass == other._pClass && _values.equals(other._values);
     }
 
     @Override
     public String toString ()
     {
         StringBuilder builder = new StringBuilder("[Key pClass=" + _pClass.getName());
-        for (Map.Entry entry : _map.entrySet()) {
+        for (int ii = 0; ii < _fields.size(); ii ++) {
-            builder.append(", ").append(entry.getKey()).append("=").append(entry.getValue());
+            builder.append(", ").append(_fields.get(ii)).append("=").append(_values.get(ii));
         }
         builder.append("]");
         return builder.toString();
     }
-    
+
+        
     protected Class<T> _pClass;
-    protected HashMap<String, Comparable> _map;
+    protected ArrayList<String> _fields;
+    protected ArrayList<Comparable> _values;
 }