From 180b663b7304fcc69f6e80594540d786954fe707 Mon Sep 17 00:00:00 2001 From: Par Winzell Date: Mon, 20 Aug 2007 18:35:43 +0000 Subject: [PATCH] Fully respect the complexities of persistent records whenever we expand field references in a query. For example, a WHERE clause that operates on a field that's been overridden will correctly use the override. This is much more complicated than what we had before (which was fundamentally broken in its own way) so this patch could introduce strange behaviour. Please be on the lookout for SQL errors in your logs and/or data fields showing unexpected values. --- .../samskivert/jdbc/depot/BuildVisitor.java | 310 +++++++++++------- .../jdbc/depot/DepotMarshaller.java | 17 +- .../jdbc/depot/DepotRepository.java | 14 - .../samskivert/jdbc/depot/MySQLBuilder.java | 9 +- .../jdbc/depot/PostgreSQLBuilder.java | 19 +- 5 files changed, 211 insertions(+), 158 deletions(-) diff --git a/src/java/com/samskivert/jdbc/depot/BuildVisitor.java b/src/java/com/samskivert/jdbc/depot/BuildVisitor.java index 6ded164..ed99a72 100644 --- a/src/java/com/samskivert/jdbc/depot/BuildVisitor.java +++ b/src/java/com/samskivert/jdbc/depot/BuildVisitor.java @@ -21,6 +21,7 @@ package com.samskivert.jdbc.depot; import java.sql.SQLException; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -83,8 +84,10 @@ public abstract class BuildVisitor implements ExpressionVisitor throws Exception { fieldOverride.getOverride().accept(this); - _builder.append(" as "); - appendField(fieldOverride.getField()); + if (_aliasFields) { + _builder.append(" as "); + appendIdentifier(fieldOverride.getField()); + } } public void visit (WhereCondition whereCondition) @@ -97,13 +100,7 @@ public abstract class BuildVisitor implements ExpressionVisitor if (ii > 0) { _builder.append(" and "); } - // TODO: This cannot work properly until the Depot architecture has been expanded - // so that every field reference is expanded according to context such as shadowOf - // and FieldOverride. For now, this works out of sheer luck - we have no ambiguous - // references to trip us up... but it must be fixed soon. - // appendTableAbbreviation(pClass); - // _builder.append("."); - appendColumn(pClass, keyFields[ii]); + appendRhsColumn(pClass, keyFields[ii]); _builder.append(values[ii] == null ? " is null " : " = ? "); } } @@ -126,13 +123,13 @@ public abstract class BuildVisitor implements ExpressionVisitor } else { _builder.append(" and "); } - appendColumn(key.getPersistentClass(), entry.getKey()); + appendRhsColumn(key.getPersistentClass(), entry.getKey()); _builder.append(entry.getValue() == null ? " is null " : " = ? "); } if (!first) { _builder.append(" and "); } - appendColumn(key.getPersistentClass(), key.getMultiField()); + appendRhsColumn(key.getPersistentClass(), key.getMultiField()); _builder.append(" in ("); Comparable[] values = key.getMultiValues(); @@ -210,9 +207,7 @@ public abstract class BuildVisitor implements ExpressionVisitor public void visit (ColumnExp columnExp) throws Exception { - appendTableAbbreviation(columnExp.getPersistentClass()); - _builder.append("."); - appendColumn(columnExp.getPersistentClass(), columnExp.getField()); + appendRhsColumn(columnExp.getPersistentClass(), columnExp.getField()); } public void visit (Not not) @@ -317,104 +312,76 @@ public abstract class BuildVisitor implements ExpressionVisitor } _builder.append("select "); - Computed entityComputed = pClass.getAnnotation(Computed.class); - - // iterate over the fields we're filling in and figure out whence each one comes - boolean skip = true; - for (String field : selectClause.getFields()) { - if (!skip) { - _builder.append(", "); - } - skip = false; - - // first, see if there's a field override - FieldOverride override = selectClause.lookupOverride(field); - if (override != null) { - override.accept(this); - continue; - } - - // figure out the class we're selecting from unless we're otherwise overriden: - // for a concrete record, simply use the corresponding table; for a computed one, - // default to the shadowed concrete record, or null if there isn't one - - Class tableClass; - if (entityComputed == null) { - tableClass = pClass; - } else if (!PersistentRecord.class.equals(entityComputed.shadowOf())) { - tableClass = entityComputed.shadowOf(); - } else { - tableClass = null; - } - - // handle the field-level @Computed annotation, if there is one - FieldMarshaller fm = _types.getMarshaller(pClass).getFieldMarshaller(field); - - Computed fieldComputed = fm.getComputed(); - if (fieldComputed != null) { - // check if the computed field has a literal SQL definition - if (fieldComputed.fieldDefinition().length() > 0) { - _builder.append(fieldComputed.fieldDefinition()).append(" as "); - appendField(field); - continue; - } - - // or if we can simply ignore the field - if (!fieldComputed.required()) { - skip = true; - continue; - } - - // else see if there's an overriding shadowOf definition - if (fieldComputed.shadowOf() != null) { - tableClass = fieldComputed.shadowOf(); - } - } - - // if we get this far we hopefully have a table to select from - if (tableClass != null) { - appendTableAbbreviation(tableClass); - _builder.append("."); - appendColumn(tableClass, field); - continue; - } - - // else owie + if (_overrides.containsKey(pClass)) { throw new IllegalArgumentException( - "Persistent field has no definition [class=" + - selectClause.getPersistentClass() + ", field=" + field + "]"); + "Can not yet nest SELECTs on the same persistent record."); } - if (selectClause.getFromOverride() != null) { - selectClause.getFromOverride().accept(this); + Map overrideMap = new HashMap(); + for (FieldOverride override : selectClause.getFieldOverrides()) { + overrideMap.put(override.getField(), override); + } + _overrides.put(pClass, overrideMap); - } else if (_types.getTableName(pClass) != null) { - _builder.append(" from "); - appendTableName(pClass); - _builder.append(" as "); - appendTableAbbreviation(pClass); + try { + // iterate over the fields we're filling in and figure out whence each one comes + boolean skip = true; - } else { - throw new SQLException("Query on @Computed entity with no FromOverrideClause."); - } + // while expanding column names in the SELECT query, do aliasing + _aliasFields = true; - for (Join clause : selectClause.getJoinClauses()) { - clause.accept(this); - } - if (selectClause.getWhereClause() != null) { - selectClause.getWhereClause().accept(this); - } - if (selectClause.getGroupBy() != null) { - selectClause.getGroupBy().accept(this); - } - if (selectClause.getOrderBy() != null) { - selectClause.getOrderBy().accept(this); - } - if (selectClause.getLimit() != null) { - selectClause.getLimit().accept(this); - } - if (selectClause.getForUpdate() != null) { - selectClause.getForUpdate().accept(this); + for (String field : selectClause.getFields()) { + if (!skip) { + _builder.append(", "); + } + skip = false; + + int len = _builder.length(); + appendRhsColumn(pClass, field); + + // if nothing was added, don't add a comma + if (_builder.length() == len) { + skip = true; + } + } + + // then stop + _aliasFields = false; + + if (selectClause.getFromOverride() != null) { + selectClause.getFromOverride().accept(this); + + } else if (_types.getTableName(pClass) != null) { + _builder.append(" from "); + appendTableName(pClass); + _builder.append(" as "); + appendTableAbbreviation(pClass); + + } else { + throw new SQLException("Query on @Computed entity with no FromOverrideClause."); + } + + for (Join clause : selectClause.getJoinClauses()) { + clause.accept(this); + } + if (selectClause.getWhereClause() != null) { + selectClause.getWhereClause().accept(this); + } + if (selectClause.getGroupBy() != null) { + selectClause.getGroupBy().accept(this); + } + if (selectClause.getOrderBy() != null) { + selectClause.getOrderBy().accept(this); + } + if (selectClause.getLimit() != null) { + selectClause.getLimit().accept(this); + } + if (selectClause.getForUpdate() != null) { + selectClause.getForUpdate().accept(this); + } + + } finally { + _overrides.remove(pClass); } if (isInner) { _builder.append(")"); @@ -440,7 +407,7 @@ public abstract class BuildVisitor implements ExpressionVisitor if (ii > 0) { _builder.append(", "); } - appendColumn(pClass, fields[ii]); + appendLhsColumn(pClass, fields[ii]); _builder.append(" = "); if (pojo != null) { @@ -479,7 +446,7 @@ public abstract class BuildVisitor implements ExpressionVisitor if (ii > 0) { _builder.append(", "); } - appendColumn(pClass, fields[ii]); + appendLhsColumn(pClass, fields[ii]); } _builder.append(") values("); @@ -497,23 +464,134 @@ public abstract class BuildVisitor implements ExpressionVisitor _builder.append(")"); } - protected abstract void appendTableName (Class type); - protected abstract void appendTableAbbreviation (Class type); - protected abstract void appendColumn (Class type, String field); - protected abstract void appendField (String field); + protected abstract void appendIdentifier (String field); + + protected void appendTableName (Class type) + { + appendIdentifier(_types.getTableName(type)); + } + + protected void appendTableAbbreviation (Class type) + { + appendIdentifier(_types.getTableAbbreviation(type)); + } + + // Constructs a name used for assignment in e.g. INSERT/UPDATE. This is the SQL + // equivalent of an lvalue; something that can appear to the left of an equals sign. + // We do not prepend this identifier with a table abbreviation, nor do we expand + // field overrides, shadowOf declarations, or the like: it is just a column name. + protected void appendLhsColumn (Class type, String field) + throws Exception + { + DepotMarshaller dm = _types.getMarshaller(type); + FieldMarshaller fm = dm.getFieldMarshaller(field); + if (dm == null) { + throw new IllegalArgumentException( + "Unknown field on persistent record [record=" + type + ", field=" + field + "]"); + } + + appendIdentifier(fm.getColumnName()); + } + + // Appends an expression for the given field on the given persistent record; this can + // appear in a SELECT list, in WHERE clauses, etc, etc. + protected void appendRhsColumn (Class type, String field) + throws Exception + { + DepotMarshaller dm = _types.getMarshaller(type); + FieldMarshaller fm = dm.getFieldMarshaller(field); + if (dm == null) { + throw new IllegalArgumentException( + "Unknown field on persistent record [record=" + type + ", field=" + field + "]"); + } + + if (!_ignoreOverrides) { + Map fieldOverrides = _overrides.get(type); + if (fieldOverrides != null) { + // first, see if there's a field override + FieldOverride override = fieldOverrides.get(field); + if (override != null) { + // overrides point to a raw column even when that column is overridden + // for the purposes of the query itself + _ignoreOverrides = true; + override.accept(this); + _ignoreOverrides = false; + return; + } + } + } + + Computed entityComputed = dm.getComputed(); + + // figure out the class we're selecting from unless we're otherwise overriden: + // for a concrete record, simply use the corresponding table; for a computed one, + // default to the shadowed concrete record, or null if there isn't one + Class tableClass; + if (entityComputed == null) { + tableClass = type; + + } else if (!PersistentRecord.class.equals(entityComputed.shadowOf())) { + tableClass = entityComputed.shadowOf(); + + } else { + tableClass = null; + } + + // handle the field-level @Computed annotation, if there is one + Computed fieldComputed = fm.getComputed(); + if (fieldComputed != null) { + // check if the computed field has a literal SQL definition + if (fieldComputed.fieldDefinition().length() > 0) { + _builder.append(fieldComputed.fieldDefinition()); + if (_aliasFields) { + _builder.append(" as "); + appendIdentifier(field); + } + return; + } + + // or if we can simply ignore the field + if (!fieldComputed.required()) { + return; + } + + // else see if there's an overriding shadowOf definition + if (fieldComputed.shadowOf() != null) { + tableClass = fieldComputed.shadowOf(); + } + } + + // if we get this far we hopefully have a table to select from + if (tableClass != null) { + appendTableAbbreviation(tableClass); + _builder.append("."); + appendIdentifier(fm.getColumnName()); + return; + } + + // else owie + throw new IllegalArgumentException( + "Persistent field has no definition [class=" + type + ", field=" + field + "]"); + } protected BuildVisitor (DepotTypes types) { _types = types; - _builder = new StringBuilder(); - _innerClause = false; } protected DepotTypes _types; /** A StringBuilder to hold the constructed SQL. */ - protected StringBuilder _builder; + protected StringBuilder _builder = new StringBuilder(); + + /** A mapping of field overrides per persistent record. */ + protected Map, Map> _overrides = + new HashMap, Map>(); /** A flag that's set to true for inner SELECT's */ - protected boolean _innerClause; + protected boolean _innerClause = false; + + protected boolean _ignoreOverrides = false; + + protected boolean _aliasFields = false; } diff --git a/src/java/com/samskivert/jdbc/depot/DepotMarshaller.java b/src/java/com/samskivert/jdbc/depot/DepotMarshaller.java index 6e53be1..60aa1ed 100644 --- a/src/java/com/samskivert/jdbc/depot/DepotMarshaller.java +++ b/src/java/com/samskivert/jdbc/depot/DepotMarshaller.java @@ -31,7 +31,6 @@ import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -75,8 +74,8 @@ public class DepotMarshaller Entity entity = pclass.getAnnotation(Entity.class); // see if this is a computed entity - Computed computed = pclass.getAnnotation(Computed.class); - if (computed == null) { + _computed = pclass.getAnnotation(Computed.class); + if (_computed == null) { // if not, this class has a corresponding SQL table _tableName = _pclass.getName(); _tableName = _tableName.substring(_tableName.lastIndexOf(".")+1); @@ -212,6 +211,15 @@ public class DepotMarshaller return _pclass; } + /** + * Returns the @Computed annotation definition of this entity, or null if none. + */ + + public Computed getComputed () + { + return _computed; + } + /** * Returns the name of the table in which persistent instances of our class are stored. By * default this is the classname of the persistent object without the package. @@ -958,6 +966,9 @@ public class DepotMarshaller /** The name of our persistent object table. */ protected String _tableName; + /** The @Computed annotation of this entity, or null. */ + protected Computed _computed; + /** A field marshaller for each persistent field in our object. */ protected Map _fields = new HashMap(); diff --git a/src/java/com/samskivert/jdbc/depot/DepotRepository.java b/src/java/com/samskivert/jdbc/depot/DepotRepository.java index 653b975..cea2dcd 100644 --- a/src/java/com/samskivert/jdbc/depot/DepotRepository.java +++ b/src/java/com/samskivert/jdbc/depot/DepotRepository.java @@ -150,20 +150,6 @@ public abstract class DepotRepository boolean useExplicit = (marsh.getTableName() == null) || !marsh.hasPrimaryKey() || !_ctx.isUsingCache(); - // TODO: This is a very conservative approach where all complicated queries are handled - // TODO: with the traditional single-pass query. The double pass algorithm can handle a - // TODO: much larger class of queries, but only after some engine upgrades: specifically, - // TODO: the field references used in the WHERE clause of the Entity query in WithCache - // TODO: cannot be the trivial expansion currently used in the Key class, but must rather - // TODO: undergo the same treatment SELECT fields currently do in SQLQueryBuilder. This - // TODO: is probably best done with an additional method in QueryBuilderContext, or perhaps - // TODO: a much larger switch to using a visitor pattern for generating the SQL of the - // TODO: clause hierarchies. - for (QueryClause clause : clauses) { - useExplicit |= (clause instanceof FieldOverride || - clause instanceof Join || - clause instanceof FromOverride); - } return _ctx.invoke(useExplicit ? new FindAllQuery.Explicitly(_ctx, type, clauses) : new FindAllQuery.WithCache(_ctx, type, clauses)); diff --git a/src/java/com/samskivert/jdbc/depot/MySQLBuilder.java b/src/java/com/samskivert/jdbc/depot/MySQLBuilder.java index 6ff33fb..0add1f8 100644 --- a/src/java/com/samskivert/jdbc/depot/MySQLBuilder.java +++ b/src/java/com/samskivert/jdbc/depot/MySQLBuilder.java @@ -29,8 +29,6 @@ import java.sql.SQLException; import java.sql.Statement; import java.sql.Time; import java.sql.Timestamp; -import java.util.ArrayList; -import java.util.List; import java.util.Set; import com.samskivert.Log; @@ -106,12 +104,7 @@ public class MySQLBuilder _builder.append(_types.getTableAbbreviation(type)); } - protected void appendColumn (Class type, String field) - { - _builder.append(_types.getColumnName(type, field)); - } - - protected void appendField (String field) + protected void appendIdentifier (String field) { _builder.append(field); } diff --git a/src/java/com/samskivert/jdbc/depot/PostgreSQLBuilder.java b/src/java/com/samskivert/jdbc/depot/PostgreSQLBuilder.java index 99899fa..4d6cf60 100644 --- a/src/java/com/samskivert/jdbc/depot/PostgreSQLBuilder.java +++ b/src/java/com/samskivert/jdbc/depot/PostgreSQLBuilder.java @@ -62,7 +62,7 @@ public class PostgreSQLBuilder public void visit (FullTextMatch match) throws Exception { - appendField("ftsCol_" + match.getName()); + appendIdentifier("ftsCol_" + match.getName()); _builder.append(" @@ TO_TSQUERY('default', ?)"); } @@ -71,22 +71,7 @@ public class PostgreSQLBuilder super(types); } - protected void appendTableName (Class type) - { - _builder.append("\"").append(_types.getTableName(type)).append("\""); - } - - protected void appendTableAbbreviation (Class type) - { - _builder.append("\"").append(_types.getTableAbbreviation(type)).append("\""); - } - - protected void appendColumn (Class type, String field) - { - _builder.append("\"").append(_types.getColumnName(type, field)).append("\""); - } - - protected void appendField (String field) + protected void appendIdentifier (String field) { _builder.append("\"").append(field).append("\""); }