From 1bee1d0351595e97417bcd8f36246c52d0943ac6 Mon Sep 17 00:00:00 2001 From: Michael Bayne Date: Thu, 16 May 2013 18:42:16 +0000 Subject: [PATCH] Revamp identity value generator to use Statement getGeneratedKeys. Most JDBC drivers have supported this for ages. No longer do we need the hackery of turning around and doing another query to obtain generated key values. --- .../com/samskivert/depot/DepotRepository.java | 34 +++++++++++-------- .../depot/impl/DepotMarshaller.java | 10 +++--- .../depot/impl/IdentityValueGenerator.java | 14 ++++++-- .../com/samskivert/depot/impl/SQLBuilder.java | 3 +- .../depot/impl/TableValueGenerator.java | 3 +- .../samskivert/depot/impl/ValueGenerator.java | 5 ++- 6 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/samskivert/depot/DepotRepository.java b/src/main/java/com/samskivert/depot/DepotRepository.java index 6c47f42..3be1556 100644 --- a/src/main/java/com/samskivert/depot/DepotRepository.java +++ b/src/main/java/com/samskivert/depot/DepotRepository.java @@ -5,6 +5,7 @@ package com.samskivert.depot; import java.sql.Connection; +import java.sql.PreparedStatement; import java.sql.SQLException; import java.sql.Timestamp; @@ -317,15 +318,16 @@ public abstract class DepotRepository Set identityFields = Collections.emptySet(); if (_key == null) { // set any auto-generated column values - identityFields = marsh.generateFieldValues(conn, liaison, _result, false); + identityFields = marsh.generateFieldValues(conn, liaison, null, _result, false); updateKey(marsh.getPrimaryKey(_result, false)); } builder.newQuery(new InsertClause(pClass, _result, identityFields)); - int mods = builder.prepare(conn).executeUpdate(); + PreparedStatement stmt = builder.prepare(conn); + int mods = stmt.executeUpdate(); // run any post-factum value generators and potentially generate our key if (_key == null) { - marsh.generateFieldValues(conn, liaison, _result, true); + marsh.generateFieldValues(conn, liaison, stmt, _result, true); updateKey(marsh.getPrimaryKey(_result, false)); } return mods; @@ -572,17 +574,19 @@ public abstract class DepotRepository Set identityFields = Collections.emptySet(); if (_key == null) { // first, set any auto-generated column values - identityFields = marsh.generateFieldValues(conn, liaison, _result, false); + identityFields = marsh.generateFieldValues( + conn, liaison, null, _result, false); // update our modifier's key so that it can cache our results updateKey(marsh.getPrimaryKey(_result, false)); } builder.newQuery(new InsertClause(pClass, _result, identityFields)); - int mods = builder.prepare(conn).executeUpdate(); + PreparedStatement stmt = builder.prepare(conn); + int mods = stmt.executeUpdate(); // run any post-factum value generators and potentially generate our key if (_key == null) { - marsh.generateFieldValues(conn, liaison, _result, true); + marsh.generateFieldValues(conn, liaison, stmt, _result, true); updateKey(marsh.getPrimaryKey(_result, false)); } created[0] = true; @@ -595,16 +599,16 @@ public abstract class DepotRepository }); } catch (DuplicateKeyException dke) { - // If we got this then the insert failed. - // Another node must have done the insert already. - // A simple solution here would be to just ignore the DKE and return, because by - // definition we're in a race condition and we can just pretend we got in first - // but that the other caller did an update afterwards. + // If we got this then the insert failed. Another node must have done the insert + // already. A simple solution here would be to just ignore the DKE and return, because + // by definition we're in a race condition and we can just pretend we got in first but + // that the other caller did an update afterwards. + // But: what if non-symmetrical code is being run on the nodes? What if the other node - // specifically called insert()? In that case, the other node is expecting - // a possible DKE, but this node isn't, and if the other node always calls insert() - // then this node would expect its store() to always work and never be overwritten - // by the other node. We need to attempt to complete the operation. + // specifically called insert()? In that case, the other node is expecting a possible + // DKE, but this node isn't, and if the other node always calls insert() then this node + // would expect its store() to always work and never be overwritten by the other node. + // We need to attempt to complete the operation. if (key == null) { throw dke; // how would this even happen? } diff --git a/src/main/java/com/samskivert/depot/impl/DepotMarshaller.java b/src/main/java/com/samskivert/depot/impl/DepotMarshaller.java index 930afb8..383caff 100644 --- a/src/main/java/com/samskivert/depot/impl/DepotMarshaller.java +++ b/src/main/java/com/samskivert/depot/impl/DepotMarshaller.java @@ -15,6 +15,7 @@ import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; +import java.sql.Statement; import java.util.List; import java.util.Map; import java.util.Set; @@ -653,21 +654,22 @@ public class DepotMarshaller implements QueryMarshal * eventual SQL). */ public Set generateFieldValues ( - Connection conn, DatabaseLiaison liaison, Object po, boolean postFactum) + Connection conn, DatabaseLiaison liaison, Statement stmt, Object po, boolean postFactum) { Set idFields = Sets.newHashSet(); for (ValueGenerator vg : _valueGenerators.values()) { + Field field = vg.getFieldMarshaller().getField(); if (!postFactum && vg instanceof IdentityValueGenerator) { - idFields.add(vg.getFieldMarshaller().getField().getName()); + idFields.add(field.getName()); } if (vg.isPostFactum() != postFactum) { continue; } try { - int nextValue = vg.nextGeneratedValue(conn, liaison); - vg.getFieldMarshaller().getField().set(po, nextValue); + int nextValue = vg.nextGeneratedValue(conn, liaison, stmt); + field.set(po, nextValue); } catch (Exception e) { throw new IllegalStateException( diff --git a/src/main/java/com/samskivert/depot/impl/IdentityValueGenerator.java b/src/main/java/com/samskivert/depot/impl/IdentityValueGenerator.java index 82eaf26..76d6cce 100644 --- a/src/main/java/com/samskivert/depot/impl/IdentityValueGenerator.java +++ b/src/main/java/com/samskivert/depot/impl/IdentityValueGenerator.java @@ -5,7 +5,9 @@ package com.samskivert.depot.impl; import java.sql.Connection; +import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Statement; import com.samskivert.jdbc.DatabaseLiaison; import com.samskivert.depot.annotation.GeneratedValue; @@ -34,10 +36,18 @@ public class IdentityValueGenerator extends ValueGenerator } @Override // from ValueGenerator - public int nextGeneratedValue (Connection conn, DatabaseLiaison liaison) + public int nextGeneratedValue (Connection conn, DatabaseLiaison liaison, Statement stmt) throws SQLException { - return liaison.lastInsertedId(conn, _dm.getTableName(), _fm.getColumnName()); + String column = _fm.getColumnName(); + // if this JDBC driver supports getGeneratedKeys, use it! + if (stmt != null && conn.getMetaData().supportsGetGeneratedKeys()) { + ResultSet rs = stmt.getGeneratedKeys(); + if (rs.next()) { + return rs.getInt(column); + } + } + return liaison.lastInsertedId(conn, _dm.getTableName(), column); } @Override // from ValueGenerator diff --git a/src/main/java/com/samskivert/depot/impl/SQLBuilder.java b/src/main/java/com/samskivert/depot/impl/SQLBuilder.java index 1e6c4be..303c339 100644 --- a/src/main/java/com/samskivert/depot/impl/SQLBuilder.java +++ b/src/main/java/com/samskivert/depot/impl/SQLBuilder.java @@ -64,7 +64,8 @@ public abstract class SQLBuilder { checkState(_buildVisitor != null, "Cannot prepare query until it's been built."); - PreparedStatement stmt = conn.prepareStatement(_buildVisitor.getQuery()); + PreparedStatement stmt = conn.prepareStatement( + _buildVisitor.getQuery(), PreparedStatement.RETURN_GENERATED_KEYS); int argIx = 1; for (BuildVisitor.Bindable bindable : _buildVisitor.getBindables()) { diff --git a/src/main/java/com/samskivert/depot/impl/TableValueGenerator.java b/src/main/java/com/samskivert/depot/impl/TableValueGenerator.java index ff288e8..aa0fdd0 100644 --- a/src/main/java/com/samskivert/depot/impl/TableValueGenerator.java +++ b/src/main/java/com/samskivert/depot/impl/TableValueGenerator.java @@ -8,6 +8,7 @@ import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Statement; import java.util.Arrays; import com.samskivert.depot.annotation.GeneratedValue; @@ -86,7 +87,7 @@ public class TableValueGenerator extends ValueGenerator } @Override // from ValueGenerator - public int nextGeneratedValue (Connection conn, DatabaseLiaison liaison) + public int nextGeneratedValue (Connection conn, DatabaseLiaison liaison, Statement stmt) throws SQLException { // TODO: Make this lockless! diff --git a/src/main/java/com/samskivert/depot/impl/ValueGenerator.java b/src/main/java/com/samskivert/depot/impl/ValueGenerator.java index 64bfad3..ff59db1 100644 --- a/src/main/java/com/samskivert/depot/impl/ValueGenerator.java +++ b/src/main/java/com/samskivert/depot/impl/ValueGenerator.java @@ -46,8 +46,11 @@ public abstract class ValueGenerator /** * Fetch/generate the next primary key value. + * + * @param stmt if post-factum, the statement that was used to perform the insert which resulted + * in the auto-generation of a value, otherwise null. */ - public abstract int nextGeneratedValue (Connection conn, DatabaseLiaison liaison) + public abstract int nextGeneratedValue (Connection conn, DatabaseLiaison liaison, Statement stmt) throws SQLException; /**