[#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

jOOQ/src/main/java/org/jooq/QueryPartInternal.java

@@ -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);
 
     /**

jOOQ/src/main/java/org/jooq/impl/AbstractBindContext.java

@@ -252,6 +252,7 @@ public final BindContext literal(String literal) {
     /**
      * Subclasses may override this method to achieve different behaviour
      */
+    @SuppressWarnings("deprecation")
     protected void bindInternal(QueryPartInternal internal) {
         internal.accept(this);
     }

jOOQ/src/main/java/org/jooq/impl/AbstractQuery.java

@@ -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 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 @@ public final void close() {
                 statement = null;
             }
             catch (SQLException e) {
-                throw Utils.translate(sql, e);
+                throw Utils.translate(rendered.sql, e);
             }
         }
     }
@@ -267,7 +268,7 @@ public final void cancel() {
                 statement.cancel();
             }
             catch (SQLException e) {
-                throw Utils.translate(sql, e);
+                throw Utils.translate(rendered.sql, e);
             }
         }
     }
@@ -291,7 +292,7 @@ public final int execute() {
 
                 // [#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 @@ public final int execute() {
                 // [#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 @@ public final int execute() {
                     !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 @@ public final int execute() {
 
                 if (!keepStatement()) {
                     statement = null;
-                    sql = null;
+                    rendered = null;
                 }
             }
         }
@@ -434,8 +436,22 @@ public boolean isExecutable() {
         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 @@ private final String getSQL0(ExecuteContext ctx) {
 
         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));
         }
 
 

jOOQ/src/main/java/org/jooq/impl/AbstractQueryPart.java

@@ -70,28 +70,28 @@ Configuration configuration() {
     }
 
     // -------------------------------------------------------------------------
-    // 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

jOOQ/src/main/java/org/jooq/impl/BetweenCondition.java

@@ -117,12 +117,12 @@ public final Condition and(Field f) {
 
     @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) {

jOOQ/src/main/java/org/jooq/impl/CustomCondition.java

@@ -42,25 +42,19 @@
 
 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 @@ protected CustomCondition() {}
     // -------------------------------------------------------------------------
 
     /**
-     * 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.
-     * <hr/>
-     * {@inheritDoc}
-     */
-    @Override
-    public void accept(Context<?> ctx) {
-        if (ctx instanceof RenderContext)
-            toSQL((RenderContext) ctx);
-        else
-            bind((BindContext) ctx);
-    }
-
-    /**
-     * Subclasses may implement this method
+     * Subclasses must 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

jOOQ/src/main/java/org/jooq/impl/CustomField.java

@@ -42,25 +42,21 @@
 
 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 @@ protected CustomField(String name, DataType<T> type) {
      * 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