From b8fec6cab5ee452a4052b4e0f9e934d673aeaa72 Mon Sep 17 00:00:00 2001 From: Michael Bayne Date: Sat, 5 Feb 2011 00:22:53 +0000 Subject: [PATCH] Fixed a problem with binding transformed values in updatePartial. When we go to bind values in an update statement, we either have: - field names and a pojo from which to extract them, in which case transformation happens naturally when we extract the current value from the pojo - value expressions (like 1, or "bob") which we were previously just binding directly to the statement, which was wrong because they need to first be transformed in the case where the field in question has an @Transform annotation; now we get the field marshaller for the field in question and pass the raw value to it, so that it can do the necessary transformations in writeToStatement() - other expressions (like LiteralExp("true") or or IntervalExp or anything else where the database is involved in computing the final value to be assigned to the column). This last case still holds the potential for badness with regard to transformed fields. We can't magically transform the value to be stored into a field if the database is computing the ultimate value. Say, for a contrived example, that you had a record where you stored an int column as a String for shits and giggles: public class MyRecord { @Transform(IntToStringTransformer.class) public int someValue; } someValue will be a string column in the database, owing to the IntToStringTransformer converting the int to a String. Now you come along and think to yourself, "Hey, I want to store the current minute in my 'int' field." and you write something like: updatePartial(MyRecord.class, MyRecord.SOME_VALUE, DateFuncs.minute(Exps.literal(new Timestamp(System.currentTimeMillis())))); Well, that's going to end up trying to stuff an int-valued expression (computed by the database) into a String field, and the shit will hit the fan. I can't really think of a non-contrived situation where this is likely to bite us in the ass, but I don't especially like grass covered pits like this lying around, waiting for someone to unsuspectingly step into them. I could fail if you try to update a transformed field with anything other than a ValueExp (or the current value from a pojo), but that would prevent you from doing something potentially useful and safe like copying one field to another, which both use the same transformation. --- .../samskivert/depot/impl/BuildVisitor.java | 92 ++++++++++--------- .../com/samskivert/depot/TransformTest.java | 13 +++ 2 files changed, 63 insertions(+), 42 deletions(-) diff --git a/src/main/java/com/samskivert/depot/impl/BuildVisitor.java b/src/main/java/com/samskivert/depot/impl/BuildVisitor.java index 635263a..23937c5 100644 --- a/src/main/java/com/samskivert/depot/impl/BuildVisitor.java +++ b/src/main/java/com/samskivert/depot/impl/BuildVisitor.java @@ -467,7 +467,8 @@ public abstract class BuildVisitor implements FragmentVisitor _builder.append(" = "); if (pojo != null) { bindField(pClass, fields[ii], pojo); - + } else if (values[ii] instanceof ValueExp) { + bindFieldValue(pClass, fields[ii], (ValueExp)values[ii]); } else { values[ii].accept(this); } @@ -747,18 +748,61 @@ public abstract class BuildVisitor implements FragmentVisitor return null; } - protected Void bindValue (Object object) + protected Void bindValue (final Object value) { - _bindables.add(newBindable(object)); + _bindables.add(new Bindable() { + public void doBind (Connection conn, PreparedStatement stmt, int argIx) + throws Exception { + // TODO: how can we abstract this fieldless marshalling + if (value instanceof ByteEnum) { + // byte enums require special conversion + stmt.setByte(argIx, ((ByteEnum)value).toByte()); + } else if (value instanceof Enum) { + // enums are converted to strings + stmt.setString(argIx, ((Enum)value).name()); + } else if (value instanceof int[]) { + // int arrays require conversion to byte arrays + int[] data = (int[])value; + ByteBuffer bbuf = ByteBuffer.allocate(data.length * 4); + bbuf.asIntBuffer().put(data); + stmt.setObject(argIx, bbuf.array()); + } else { + stmt.setObject(argIx, value); + } + } + }); _builder.append("?"); return null; } protected Void bindField ( - Class pClass, ColumnExp field, Object pojo) + Class pClass, ColumnExp field, final Object pojo) { - final DepotMarshaller marshaller = _types.getMarshaller(pClass); - _bindables.add(newBindable(marshaller, field, pojo)); + final FieldMarshaller fmarsh = + _types.getMarshaller(pClass).getFieldMarshaller(field.name); + _bindables.add(new Bindable() { + public void doBind (Connection conn, PreparedStatement stmt, int argIx) + throws Exception { + fmarsh.getAndWriteToStatement(stmt, argIx, pojo); + } + }); + _builder.append("?"); + return null; + } + + protected Void bindFieldValue ( + Class pClass, ColumnExp field, final ValueExp value) + { + // we know that the Ts match in FieldMarshaller and ValueExp, but it's hard + // to convince the type system of that + final @SuppressWarnings("unchecked") FieldMarshaller fmarsh = + (FieldMarshaller)_types.getMarshaller(pClass).getFieldMarshaller(field.name); + _bindables.add(new Bindable() { + public void doBind (Connection conn, PreparedStatement stmt, int argIx) + throws Exception { + fmarsh.writeToStatement(stmt, argIx, value.getValue()); + } + }); _builder.append("?"); return null; } @@ -936,42 +980,6 @@ public abstract class BuildVisitor implements FragmentVisitor void doBind (Connection conn, PreparedStatement stmt, int argIx) throws Exception; } - protected static Bindable newBindable ( - final DepotMarshaller marshaller, final ColumnExp field, final Object pojo) - { - return new Bindable() { - public void doBind (Connection conn, PreparedStatement stmt, int argIx) - throws Exception { - marshaller.getFieldMarshaller(field.name).getAndWriteToStatement(stmt, argIx, pojo); - } - }; - } - - protected static Bindable newBindable (final Object value) - { - return new Bindable() { - public void doBind (Connection conn, PreparedStatement stmt, int argIx) - throws Exception { - // TODO: how can we abstract this fieldless marshalling - if (value instanceof ByteEnum) { - // byte enums require special conversion - stmt.setByte(argIx, ((ByteEnum)value).toByte()); - } else if (value instanceof Enum) { - // enums are converted to strings - stmt.setString(argIx, ((Enum)value).name()); - } else if (value instanceof int[]) { - // int arrays require conversion to byte arrays - int[] data = (int[])value; - ByteBuffer bbuf = ByteBuffer.allocate(data.length * 4); - bbuf.asIntBuffer().put(data); - stmt.setObject(argIx, bbuf.array()); - } else { - stmt.setObject(argIx, value); - } - } - }; - } - protected DepotTypes _types; /** For each SQL parameter ? we add an {@link Comparable} to bind to this list. */ diff --git a/src/test/java/com/samskivert/depot/TransformTest.java b/src/test/java/com/samskivert/depot/TransformTest.java index 0f8381a..3f5f6b1 100644 --- a/src/test/java/com/samskivert/depot/TransformTest.java +++ b/src/test/java/com/samskivert/depot/TransformTest.java @@ -240,6 +240,19 @@ public class TransformTest extends TestBase delete(in); } + @Test public void testUpdatePartial () + { + TransformRecord in = createAndInsert(new String[] { "one", "two", "three" }); + + EnumSet nbobs = EnumSet.of(ExtraOrdinal.ONE, ExtraOrdinal.THREE); + _repo.updatePartial(TransformRecord.getKey(in.recordId), TransformRecord.BOBS, nbobs); + + TransformRecord in2 = _repo.load(TransformRecord.getKey(in.recordId)); + assertEquals(nbobs, in2.bobs); + + delete(in); + } + protected TransformRecord createAndInsert (String[] strings) { TransformRecord in = new TransformRecord();