[#3323] [#4650] [#4976] Various fixes

- [#3323] Make QueryPartInternal.toSQL() and bind() final in favour of the new accept() method
- [#4650] Collect bind variables while generating SQL, instead of traversing the AST twice
- [#4976] jOOQ's internals should not call QueryPartInternal.accept(Context), but Context.visit(QueryPart) instead
This commit is contained in:
lukaseder 2016-01-23 15:20:53 +01:00
parent 2285d55427
commit e6f9368a39
24 changed files with 151 additions and 208 deletions

View File

@ -55,7 +55,12 @@ public interface QueryPartInternal extends QueryPart {
/**
* This {@link QueryPart} can <code>accept</code> a {@link Context} object
* in order to render a SQL string or to bind its variables.
*
* @deprecated - Calling {@link #accept(Context)} directly on a
* {@link QueryPart} is almost always a mistake. Instead,
* {@link Context#visit(QueryPart)} should be called.
*/
@Deprecated
void accept(Context<?> ctx);
/**

View File

@ -252,6 +252,7 @@ abstract class AbstractBindContext extends AbstractContext<BindContext> implemen
/**
* Subclasses may override this method to achieve different behaviour
*/
@SuppressWarnings("deprecation")
protected void bindInternal(QueryPartInternal internal) {
internal.accept(this);
}

View File

@ -42,6 +42,7 @@
package org.jooq.impl;
import static java.util.Arrays.asList;
import static org.jooq.Constants.FULL_VERSION;
import static org.jooq.ExecuteType.DDL;
// ...
// ...
@ -64,11 +65,11 @@ import java.util.Map;
import org.jooq.AttachableInternal;
import org.jooq.Configuration;
import org.jooq.Constants;
import org.jooq.ExecuteContext;
import org.jooq.ExecuteListener;
import org.jooq.Param;
import org.jooq.Query;
import org.jooq.QueryPart;
import org.jooq.RenderContext;
import org.jooq.Select;
import org.jooq.conf.ParamType;
@ -90,7 +91,7 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
private int timeout;
private boolean keepStatement;
private transient PreparedStatement statement;
private transient String sql;
private transient Rendered rendered;
AbstractQuery(Configuration configuration) {
this.configuration = configuration;
@ -255,7 +256,7 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
statement = null;
}
catch (SQLException e) {
throw Utils.translate(sql, e);
throw Utils.translate(rendered.sql, e);
}
}
}
@ -267,7 +268,7 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
statement.cancel();
}
catch (SQLException e) {
throw Utils.translate(sql, e);
throw Utils.translate(rendered.sql, e);
}
}
}
@ -291,7 +292,7 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
// [#385] If a statement was previously kept open
if (keepStatement() && statement != null) {
ctx.sql(sql);
ctx.sql(rendered.sql);
ctx.statement(statement);
// [#3191] Pre-initialise the ExecuteContext with a previous connection, if available.
@ -301,10 +302,10 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
// [#385] First time statement preparing
else {
listener.renderStart(ctx);
ctx.sql(getSQL0(ctx));
rendered = getSQL0(ctx);
ctx.sql(rendered.sql);
listener.renderEnd(ctx);
sql = ctx.sql();
rendered.sql = ctx.sql();
// [#3234] Defer initialising of a connection until the prepare step
// This optimises unnecessary ConnectionProvider.acquire() calls when
@ -337,7 +338,8 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
!Boolean.TRUE.equals(ctx.data(DATA_FORCE_STATIC_STATEMENT))) {
listener.bindStart(ctx);
using(c).bindContext(ctx.statement()).visit(this);
if (rendered.bindValues != null)
using(c).bindContext(ctx.statement()).visit(rendered.bindValues);
listener.bindEnd(ctx);
}
@ -368,7 +370,7 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
if (!keepStatement()) {
statement = null;
sql = null;
rendered = null;
}
}
}
@ -434,8 +436,22 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
return true;
}
private final String getSQL0(ExecuteContext ctx) {
String result;
private static class Rendered {
String sql;
QueryPart bindValues;
Rendered(String sql) {
this(sql, null);
}
Rendered(String sql, QueryPart bindValues) {
this.sql = sql;
this.bindValues = bindValues;
}
}
private final Rendered getSQL0(ExecuteContext ctx) {
Rendered result;
@ -449,17 +465,18 @@ abstract class AbstractQuery extends AbstractQueryPart implements Query, Attacha
if (executePreparedStatements(configuration().settings())) {
try {
RenderContext render = new DefaultRenderContext(configuration);
DefaultRenderContext render = new DefaultRenderContext(configuration);
render.data(DATA_COUNT_BIND_VALUES, true);
result = render.visit(this).render();
render.visit(this);
result = new Rendered(render.render(), render.bindValues());
}
catch (DefaultRenderContext.ForceInlineSignal e) {
ctx.data(DATA_FORCE_STATIC_STATEMENT, true);
result = getSQL(INLINED);
result = new Rendered(getSQL(INLINED));
}
}
else {
result = getSQL(INLINED);
result = new Rendered(getSQL(INLINED));
}

View File

@ -70,28 +70,28 @@ abstract class AbstractQueryPart implements QueryPartInternal {
}
// -------------------------------------------------------------------------
// The QueryPart and QueryPart internal API
// Deprecated API
// -------------------------------------------------------------------------
/**
* @deprecated - 3.4.0 - [#2694] - Use
* {@link QueryPartInternal#accept(Context)} instead.
*/
@Override
@Deprecated
public void toSQL(RenderContext ctx) {
accept(ctx);
}
@Override
public void accept(Context<?> ctx) {
if (ctx instanceof RenderContext)
toSQL((RenderContext) ctx);
else
bind((BindContext) ctx);
}
public final void toSQL(RenderContext context) {}
/**
* @deprecated - 3.4.0 - [#2694] - Use
* {@link QueryPartInternal#accept(Context)} instead.
*/
@Override
@Deprecated
public void bind(BindContext ctx) throws DataAccessException {
accept(ctx);
}
public final void bind(BindContext context) throws DataAccessException {}
// -------------------------------------------------------------------------
// The QueryPart and QueryPart internal API
// -------------------------------------------------------------------------
/**
* Subclasses may override this

View File

@ -117,12 +117,12 @@ class BetweenCondition<T> extends AbstractCondition implements BetweenAndStep<T>
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {

View File

@ -42,25 +42,19 @@ package org.jooq.impl;
import static org.jooq.Clause.CUSTOM;
import org.jooq.BindContext;
import org.jooq.Clause;
import org.jooq.Condition;
import org.jooq.Context;
import org.jooq.RenderContext;
import org.jooq.exception.DataAccessException;
/**
* A base class for custom {@link Condition} implementations in client code.
* <p>
* Client code may provide proper {@link Condition} implementations extending
* this useful base class. All necessary parts of the {@link Condition}
* interface are already implemented. Only these two methods need further
* implementation:
* <ul>
* <li>{@link #toSQL(org.jooq.RenderContext)}</li>
* <li>{@link #bind(org.jooq.BindContext)}</li>
* </ul>
* Refer to those methods' Javadoc for further details about their expected
* interface are already implemented. Only this method needs further
* implementation: {@link #accept(Context)}.
* <p>
* Refer to that methods' Javadoc for further details about their expected
* behaviour.
*
* @author Lukas Eder
@ -80,43 +74,12 @@ public abstract class CustomCondition extends AbstractCondition {
// -------------------------------------------------------------------------
/**
* Subclasses must implement this method
* <hr/>
* {@inheritDoc}
*
* @deprecated - 3.4.0 - [#2694] - Use {@link #accept(Context)} instead.
*/
@Override
@Deprecated
public void toSQL(RenderContext context) {}
// -------------------------------------------------------------------------
// Implementation optional
// -------------------------------------------------------------------------
/**
* Subclasses may implement this method.
* Subclasses must implement this method.
* <hr/>
* {@inheritDoc}
*/
@Override
public void accept(Context<?> ctx) {
if (ctx instanceof RenderContext)
toSQL((RenderContext) ctx);
else
bind((BindContext) ctx);
}
/**
* Subclasses may implement this method
* <hr/>
* {@inheritDoc}
*
* @deprecated - 3.4.0 - [#2694] - Use {@link #accept(Context)} instead.
*/
@Override
@Deprecated
public void bind(BindContext context) throws DataAccessException {}
public abstract void accept(Context<?> ctx);
// -------------------------------------------------------------------------
// No further overrides allowed

View File

@ -42,25 +42,21 @@ package org.jooq.impl;
import static org.jooq.Clause.CUSTOM;
import org.jooq.BindContext;
import org.jooq.Clause;
import org.jooq.Condition;
import org.jooq.Context;
import org.jooq.DataType;
import org.jooq.Field;
import org.jooq.RenderContext;
import org.jooq.exception.DataAccessException;
/**
* A base class for custom {@link Field} implementations in client code.
* <p>
* Client code may provide proper {@link Field} implementations extending this
* useful base class. All necessary parts of the {@link Field} interface are
* already implemented. Only these two methods need further implementation:
* <ul>
* <li>{@link #toSQL(org.jooq.RenderContext)}</li>
* <li>{@link #bind(org.jooq.BindContext)}</li>
* </ul>
* Refer to those methods' Javadoc for further details about their expected
* Client code may provide proper {@link Condition} implementations extending
* this useful base class. All necessary parts of the {@link Condition}
* interface are already implemented. Only this method needs further
* implementation: {@link #accept(Context)}.
* <p>
* Refer to that methods' Javadoc for further details about their expected
* behaviour.
*
* @author Lukas Eder
@ -85,39 +81,9 @@ public abstract class CustomField<T> extends AbstractField<T> {
* Subclasses must implement this method.
* <hr/>
* {@inheritDoc}
* @deprecated - 3.4.0 - [#2694] - Use {@link #accept(Context)} instead.
*/
@Deprecated
@Override
public void toSQL(RenderContext context) {}
// -------------------------------------------------------------------------
// Implementation optional
// -------------------------------------------------------------------------
/**
* Subclasses may implement this method.
* <hr/>
* {@inheritDoc}
*/
@Override
public void accept(Context<?> ctx) {
if (ctx instanceof RenderContext)
toSQL((RenderContext) ctx);
else
bind((BindContext) ctx);
}
/**
* Subclasses may implement this method.
* <hr/>
* {@inheritDoc}
* @deprecated - 3.4.0 - [#2694] - Use {@link #accept(Context)} instead.
*/
@Deprecated
@Override
public void bind(BindContext context) throws DataAccessException {
}
public abstract void accept(Context<?> ctx);
// -------------------------------------------------------------------------
// No further overrides allowed

View File

@ -42,25 +42,20 @@ package org.jooq.impl;
import static org.jooq.Clause.CUSTOM;
import org.jooq.BindContext;
import org.jooq.Clause;
import org.jooq.Condition;
import org.jooq.Context;
import org.jooq.QueryPart;
import org.jooq.RenderContext;
import org.jooq.exception.DataAccessException;
/**
* A base class for custom {@link QueryPart} implementations in client code.
* <p>
* Client code may provide proper {@link QueryPart} implementations extending
* this useful base class. All necessary parts of the {@link QueryPart}
* interface are already implemented. Only these two methods need further
* implementation:
* <ul>
* <li>{@link #toSQL(org.jooq.RenderContext)}</li>
* <li>{@link #bind(org.jooq.BindContext)}</li>
* </ul>
* Refer to those methods' Javadoc for further details about their expected
* Client code may provide proper {@link Condition} implementations extending
* this useful base class. All necessary parts of the {@link Condition}
* interface are already implemented. Only this method needs further
* implementation: {@link #accept(Context)}.
* <p>
* Refer to that methods' Javadoc for further details about their expected
* behaviour.
* <p>
* Such custom <code>QueryPart</code> implementations can be useful in any of
@ -92,42 +87,12 @@ public abstract class CustomQueryPart extends AbstractQueryPart {
// -------------------------------------------------------------------------
/**
* Subclasses must implement this method
* <hr/>
* {@inheritDoc}
* @deprecated - 3.4.0 - [#2694] - Use {@link #accept(Context)} instead.
*/
@Deprecated
@Override
public void toSQL(RenderContext context) {}
// -------------------------------------------------------------------------
// Implementation optional
// -------------------------------------------------------------------------
/**
* Subclasses may implement this method.
* Subclasses must implement this method.
* <hr/>
* {@inheritDoc}
*/
@Override
public void accept(Context<?> ctx) {
if (ctx instanceof RenderContext)
toSQL((RenderContext) ctx);
else
bind((BindContext) ctx);
}
/**
* Subclasses may implement this method
* <hr/>
* {@inheritDoc}
* @deprecated - 3.4.0 - [#2694] - Use {@link #accept(Context)} instead.
*/
@Deprecated
@Override
public void bind(BindContext context) throws DataAccessException {
}
public abstract void accept(Context<?> ctx);
// -------------------------------------------------------------------------
// No further overrides allowed

View File

@ -82,23 +82,24 @@ import org.jooq.tools.StringUtils;
*/
class DefaultRenderContext extends AbstractContext<RenderContext> implements RenderContext {
private static final JooqLogger log = JooqLogger.getLogger(DefaultRenderContext.class);
private static final JooqLogger log = JooqLogger.getLogger(DefaultRenderContext.class);
private static final Pattern IDENTIFIER_PATTERN = Pattern.compile("[A-Za-z][A-Za-z0-9_]*");
private static final Pattern NEWLINE = Pattern.compile("[\\n\\r]");
private static final Set<String> SQLITE_KEYWORDS;
private static final Pattern IDENTIFIER_PATTERN = Pattern.compile("[A-Za-z][A-Za-z0-9_]*");
private static final Pattern NEWLINE = Pattern.compile("[\\n\\r]");
private static final Set<String> SQLITE_KEYWORDS;
private final StringBuilder sql;
private int params;
private int alias;
private int indent;
private Deque<Integer> indentLock;
private int printMargin = 80;
private final StringBuilder sql;
private final QueryPartList<Param<?>> bindValues;
private int params;
private int alias;
private int indent;
private Deque<Integer> indentLock;
private int printMargin = 80;
// [#1632] Cached values from Settings
RenderKeywordStyle cachedRenderKeywordStyle;
RenderNameStyle cachedRenderNameStyle;
boolean cachedRenderFormatted;
RenderKeywordStyle cachedRenderKeywordStyle;
RenderNameStyle cachedRenderNameStyle;
boolean cachedRenderFormatted;
DefaultRenderContext(Configuration configuration) {
super(configuration, null);
@ -106,6 +107,7 @@ class DefaultRenderContext extends AbstractContext<RenderContext> implements Ren
Settings settings = configuration.settings();
this.sql = new StringBuilder();
this.bindValues = new QueryPartList<Param<?>>();
this.cachedRenderKeywordStyle = settings.getRenderKeywordStyle();
this.cachedRenderFormatted = Boolean.TRUE.equals(settings.isRenderFormatted());
this.cachedRenderNameStyle = settings.getRenderNameStyle();
@ -136,6 +138,10 @@ class DefaultRenderContext extends AbstractContext<RenderContext> implements Ren
throw new UnsupportedOperationException();
}
final QueryPart bindValues() {
return bindValues;
}
// ------------------------------------------------------------------------
// RenderContext API
// ------------------------------------------------------------------------
@ -379,21 +385,24 @@ class DefaultRenderContext extends AbstractContext<RenderContext> implements Ren
return visit(part);
}
@SuppressWarnings("deprecation")
@Override
protected final void visit0(QueryPartInternal internal) {
checkForceInline(internal);
collectBindVariable(internal);
internal.accept(this);
}
private final void checkForceInline(QueryPart part) throws ForceInlineSignal {
private final void collectBindVariable(QueryPart part) throws ForceInlineSignal {
if (paramType == INLINED)
return;
if (part instanceof Param) {
if (((Param<?>) part).isInline())
Param<?> param = (Param<?>) part;
if (param.isInline())
return;
switch (configuration().dialect().family()) {
bindValues.add(param);
switch (configuration().family()) {
@ -426,8 +435,8 @@ class DefaultRenderContext extends AbstractContext<RenderContext> implements Ren
}
private final void checkForceInline(int max) throws ForceInlineSignal {
if (Boolean.TRUE.equals(data(DATA_COUNT_BIND_VALUES)))
if (++params > max)
if (bindValues.size() > max)
if (Boolean.TRUE.equals(data(DATA_COUNT_BIND_VALUES)))
throw new ForceInlineSignal();
}

View File

@ -74,7 +74,7 @@ class FetchCount extends AbstractResultQuery<Record1<Integer>> {
}
private final QueryPart delegate(Configuration configuration) {
switch (configuration.dialect().family()) {
switch (configuration.family()) {

View File

@ -43,6 +43,7 @@ package org.jooq.impl;
import static org.jooq.impl.DSL.condition;
import static org.jooq.impl.DSL.inline;
import org.jooq.Clause;
import org.jooq.Configuration;
import org.jooq.Context;
import org.jooq.Field;
@ -65,7 +66,12 @@ class FieldCondition extends AbstractCondition {
@Override
public void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {

View File

@ -369,7 +369,7 @@ class Fields<R extends Record> extends AbstractQueryPart implements RecordType<R
@Override
public final void accept(Context<?> ctx) {
new QueryPartList<Field<?>>(fields).accept(ctx);
ctx.visit(new QueryPartList<Field<?>>(fields));
}
@Override

View File

@ -45,6 +45,7 @@ import static org.jooq.impl.DSL.name;
import static org.jooq.impl.DSL.one;
import static org.jooq.impl.DSL.table;
import org.jooq.Clause;
import org.jooq.Configuration;
import org.jooq.Context;
import org.jooq.Field;
@ -77,6 +78,11 @@ class GenerateSeries extends AbstractTable<Record1<Integer>> {
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return null;
}
private final QueryPart delegate(Configuration configuration) {
switch (configuration.family()) {
case CUBRID:

View File

@ -102,12 +102,12 @@ class IsDistinctFrom<T> extends AbstractCondition {
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
/**

View File

@ -116,7 +116,7 @@ class QuantifiedSelectImpl<R extends Record> extends AbstractQueryPart implement
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration ctx) {

View File

@ -42,6 +42,7 @@ package org.jooq.impl;
import static org.jooq.impl.DSL.field;
import org.jooq.Clause;
import org.jooq.Configuration;
import org.jooq.Context;
import org.jooq.FieldOrRow;
@ -69,8 +70,13 @@ class Rollup extends AbstractField<Object> {
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return null;
}
private final QueryPart delegate(Configuration configuration) {
switch (configuration.dialect()) {
switch (configuration.family()) {
case CUBRID:
case MARIADB:
case MYSQL:

View File

@ -699,12 +699,12 @@ implements
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {
@ -744,7 +744,7 @@ implements
private static final long serialVersionUID = 2915703568738921575L;
@Override
public final void toSQL(RenderContext context) {
public final void accept(Context<?> context) {
context.visit(row);
if (not) context.sql(" ").keyword("not");
context.sql(" ").keyword("between");
@ -754,11 +754,6 @@ implements
context.sql(" ").visit(maxValue);
}
@Override
public final void bind(BindContext context) {
context.visit(row).visit(minValue).visit(maxValue);
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return not ? symmetric ? CLAUSES_NOT_BETWEEN_SYMMETRIC

View File

@ -101,12 +101,12 @@ class RowCondition extends AbstractCondition {
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {

View File

@ -95,12 +95,12 @@ class RowInCondition extends AbstractCondition {
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {

View File

@ -94,12 +94,12 @@ class RowIsNull extends AbstractCondition {
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {

View File

@ -89,12 +89,12 @@ class RowOverlapsCondition<T1, T2> extends AbstractCondition {
@Override
public final void accept(Context<?> ctx) {
delegate(ctx.configuration()).accept(ctx);
ctx.visit(delegate(ctx.configuration()));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx.configuration()).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Configuration configuration) {

View File

@ -112,12 +112,12 @@ class RowSubqueryCondition extends AbstractCondition {
@Override
public final void accept(Context<?> ctx) {
delegate(ctx).accept(ctx);
ctx.visit(delegate(ctx));
}
@Override
public final Clause[] clauses(Context<?> ctx) {
return delegate(ctx).clauses(ctx);
return null;
}
private final QueryPartInternal delegate(Context<?> ctx) {

View File

@ -132,7 +132,7 @@ public class TableImpl<R extends Record> extends AbstractTable<R> {
@Override
public final void accept(Context<?> ctx) {
if (alias != null) {
alias.accept(ctx);
ctx.visit(alias);
}
else {

View File

@ -122,6 +122,7 @@ import org.jooq.QueryPart;
import org.jooq.Record;
import org.jooq.RecordType;
import org.jooq.RenderContext;
import org.jooq.RenderContext.CastMode;
import org.jooq.Result;
import org.jooq.Results;
import org.jooq.Row;
@ -1437,7 +1438,10 @@ final class Utils {
render.visit(substitute);
}
else {
render.sql(sqlChars[i]);
CastMode previous = render.castMode();
render.castMode(CastMode.NEVER)
.visit(substitute)
.castMode(previous);
}
if (bind != null) {