Repository: influxdata/influxdb-java (java)

Issue:
Add support for parameter binding to built queries
Currently, parameter binding is only supported through `BoundParameterQuery`, which takes a plain text query. It would nice to use built queries with parameters.

I've implemented this. Should I post a PR?


Pull request:
Add support for parameter binding to built queries
Pulled up bound parameter support from `BoundParameterQuery` to `Query`, so that they can be used with built queries, and added support for that.

Also:
- Decoupled `requiresPost` from parameters, as they are unrelated
- Made `InfluxDBService` more consistent, by making all `query` methods use `GET`, and all `postQuery` methods use `POST`

Fixes #1009.

Patch:
diff --git a/QUERY_BUILDER.md b/QUERY_BUILDER.md
index 5f500b8..d84e6d2 100644
--- a/QUERY_BUILDER.md
+++ b/QUERY_BUILDER.md
@@ -588,3 +588,19 @@ Query select = select().raw("an expression on select").from(dbName, "cpu").where
 ```sqlite-psql
 SELECT an expression on select FROM h2o_feet WHERE an expression as condition;
 ```
+
+Binding parameters
+
+If your Query is based on user input, it is good practice to use parameter binding to avoid [injection attacks](https://en.wikipedia.org/wiki/SQL_injection).
+You can create queries with parameter binding:
+
+```java
+Query query = select().from(DATABASE,"h2o_feet").where(gt("water_level", FunctionFactory.placeholder("level")))
+                      .bindParameter("level", 8);
+```
+
+```sqlite-psql
+SELECT * FROM h2o_feet WHERE water_level > $level;
+```
+
+The values of bindParameter() calls are bound to the placeholders in the query (`level`).
diff --git a/src/main/java/org/influxdb/dto/BoundParameterQuery.java b/src/main/java/org/influxdb/dto/BoundParameterQuery.java
index 0c7b08b..1f19728 100644
--- a/src/main/java/org/influxdb/dto/BoundParameterQuery.java
+++ b/src/main/java/org/influxdb/dto/BoundParameterQuery.java
@@ -1,77 +1,9 @@
 package org.influxdb.dto;
 
-import com.squareup.moshi.JsonWriter;
-import java.io.IOException;
-import java.nio.charset.Charset;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Map.Entry;
-
-import org.influxdb.InfluxDBIOException;
-
-import okio.Buffer;
-
 public final class BoundParameterQuery extends Query {
 
-  private final Map<String, Object> params = new HashMap<>();
-
   private BoundParameterQuery(final String command, final String database) {
-    super(command, database, true);
-  }
-
-  public String getParameterJsonWithUrlEncoded() {
-    try {
-      String jsonParameterObject = createJsonObject(params);
-      String urlEncodedJsonParameterObject = encode(jsonParameterObject);
-      return urlEncodedJsonParameterObject;
-    } catch (IOException e) {
-      throw new InfluxDBIOException(e);
-    }
-  }
-
-  private String createJsonObject(final Map<String, Object> parameterMap) throws IOException {
-    Buffer b = new Buffer();
-    JsonWriter writer = JsonWriter.of(b);
-    writer.beginObject();
-    for (Entry<String, Object> pair : parameterMap.entrySet()) {
-      String name = pair.getKey();
-      Object value = pair.getValue();
-      if (value instanceof Number) {
-        Number number = (Number) value;
-        writer.name(name).value(number);
-      } else if (value instanceof String) {
-        writer.name(name).value((String) value);
-      } else if (value instanceof Boolean) {
-        writer.name(name).value((Boolean) value);
-      } else {
-        writer.name(name).value(String.valueOf(value));
-      }
-    }
-    writer.endObject();
-    return b.readString(Charset.forName("utf-8"));
-  }
-
-  @Override
-  public int hashCode() {
-    final int prime = 31;
-    int result = super.hashCode();
-    result = prime * result + params.hashCode();
-    return result;
-  }
-
-  @Override
-  public boolean equals(final Object obj) {
-    if (this == obj) {
-      return true;
-    }
-    if (!super.equals(obj)) {
-      return false;
-    }
-    BoundParameterQuery other = (BoundParameterQuery) obj;
-    if (!params.equals(other.params)) {
-      return false;
-    }
-    return true;
+    super(command, database);
   }
 
   public static class QueryBuilder {
@@ -93,7 +25,7 @@ public final class BoundParameterQuery extends Query {
       if (query == null) {
           query = new BoundParameterQuery(influxQL, null);
       }
-      query.params.put(placeholder, value);
+      query.bindParameter(placeholder, value);
       return this;
     }
 
diff --git a/src/main/java/org/influxdb/dto/Query.java b/src/main/java/org/influxdb/dto/Query.java
index 5c4921b..ebed08e 100644
--- a/src/main/java/org/influxdb/dto/Query.java
+++ b/src/main/java/org/influxdb/dto/Query.java
@@ -1,8 +1,18 @@
 package org.influxdb.dto;
 
+import com.squareup.moshi.JsonWriter;
+import okio.Buffer;
+import org.influxdb.InfluxDBIOException;
+import org.influxdb.querybuilder.Appendable;
+
+import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
+import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
 
 /**
  * Represents a Query against Influxdb.
@@ -15,6 +25,7 @@ public class Query {
   private final String command;
   private final String database;
   private final boolean requiresPost;
+  protected final Map<String, Object> params = new HashMap<>();
 
   /**
    * @param command the query command
@@ -68,38 +79,43 @@ public class Query {
     return requiresPost;
   }
 
-  @SuppressWarnings("checkstyle:avoidinlineconditionals")
-  @Override
-  public int hashCode() {
-    final int prime = 31;
-    int result = 1;
-    result = prime * result + ((command == null) ? 0 : command.hashCode());
-    result = prime * result
-        + ((database == null) ? 0 : database.hashCode());
-    return result;
+  public Query bindParameter(final String placeholder, final Object value) {
+    params.put(placeholder, value);
+    return this;
+  }
+
+  public boolean hasBoundParameters() {
+    return !params.isEmpty();
+  }
+
+  public String getParameterJsonWithUrlEncoded() {
+    try {
+      String jsonParameterObject = createJsonObject(params);
+      String urlEncodedJsonParameterObject = encode(jsonParameterObject);
+      return urlEncodedJsonParameterObject;
+    } catch (IOException e) {
+      throw new InfluxDBIOException(e);
+    }
   }
 
-  @SuppressWarnings("checkstyle:needbraces")
   @Override
-  public boolean equals(final Object obj) {
-    if (this == obj)
-      return true;
-    if (obj == null)
-      return false;
-    if (getClass() != obj.getClass())
-      return false;
-    Query other = (Query) obj;
-    if (command == null) {
-      if (other.command != null)
-        return false;
-    } else if (!command.equals(other.command))
+  public boolean equals(final Object o) {
+    if (o == null || getClass() != o.getClass()) {
       return false;
-    if (database == null) {
-      if (other.database != null)
-        return false;
-    } else if (!database.equals(other.database))
-      return false;
-    return true;
+    }
+
+    Query query = (Query) o;
+    return Objects.equals(command, query.command) && Objects.equals(database, query.database) && params.equals(
+            query.params);
+  }
+
+  @Override
+  public int hashCode() {
+    final int prime = 31;
+    int result = Objects.hashCode(command);
+    result = prime * result + Objects.hashCode(database);
+    result = prime * result + params.hashCode();
+    return result;
   }
 
   /**
@@ -115,4 +131,30 @@ public class Query {
       throw new IllegalStateException("Every JRE must support UTF-8", e);
     }
   }
+
+  private String createJsonObject(final Map<String, Object> parameterMap) throws IOException {
+    Buffer b = new Buffer();
+    JsonWriter writer = JsonWriter.of(b);
+    writer.beginObject();
+    for (Map.Entry<String, Object> pair : parameterMap.entrySet()) {
+      String name = pair.getKey();
+      Object value = pair.getValue();
+      if (value instanceof Number) {
+        Number number = (Number) value;
+        writer.name(name).value(number);
+      } else if (value instanceof String) {
+        writer.name(name).value((String) value);
+      } else if (value instanceof Boolean) {
+        writer.name(name).value((Boolean) value);
+      } else if (value instanceof Appendable) {
+        StringBuilder stringBuilder = new StringBuilder();
+        ((Appendable) value).appendTo(stringBuilder);
+        writer.name(name).value(stringBuilder.toString());
+      } else {
+        writer.name(name).value(String.valueOf(value));
+      }
+    }
+    writer.endObject();
+    return b.readString(Charset.forName("utf-8"));
+  }
 }
diff --git a/src/main/java/org/influxdb/impl/InfluxDBImpl.java b/src/main/java/org/influxdb/impl/InfluxDBImpl.java
index 825e070..23427a2 100644
--- a/src/main/java/org/influxdb/impl/InfluxDBImpl.java
+++ b/src/main/java/org/influxdb/impl/InfluxDBImpl.java
@@ -16,7 +16,6 @@ import org.influxdb.InfluxDB;
 import org.influxdb.InfluxDBException;
 import org.influxdb.InfluxDBIOException;
 import org.influxdb.dto.BatchPoints;
-import org.influxdb.dto.BoundParameterQuery;
 import org.influxdb.dto.Point;
 import org.influxdb.dto.Pong;
 import org.influxdb.dto.Query;
@@ -637,13 +636,17 @@ public class InfluxDBImpl implements InfluxDB {
   public void query(final Query query, final int chunkSize, final BiConsumer<Cancellable, QueryResult> onNext,
                     final Runnable onComplete, final Consumer<Throwable> onFailure) {
     Call<ResponseBody> call;
-    if (query instanceof BoundParameterQuery) {
-      BoundParameterQuery boundParameterQuery = (BoundParameterQuery) query;
-      call = this.influxDBService.query(getDatabase(query), query.getCommandWithUrlEncoded(), chunkSize,
-          boundParameterQuery.getParameterJsonWithUrlEncoded());
+    if (query.hasBoundParameters()) {
+      if (query.requiresPost()) {
+        call = this.influxDBService.postQuery(getDatabase(query), query.getCommandWithUrlEncoded(), chunkSize,
+                                          query.getParameterJsonWithUrlEncoded());
+      } else {
+        call = this.influxDBService.query(getDatabase(query), query.getCommandWithUrlEncoded(), chunkSize,
+                                          query.getParameterJsonWithUrlEncoded());
+      }
     } else {
       if (query.requiresPost()) {
-        call = this.influxDBService.query(getDatabase(query), query.getCommandWithUrlEncoded(), chunkSize, null);
+        call = this.influxDBService.postQuery(getDatabase(query), query.getCommandWithUrlEncoded(), chunkSize);
       } else {
         call = this.influxDBService.query(getDatabase(query), query.getCommandWithUrlEncoded(), chunkSize);
       }
@@ -716,18 +719,21 @@ public class InfluxDBImpl implements InfluxDB {
   @Override
   public QueryResult query(final Query query, final TimeUnit timeUnit) {
     Call<QueryResult> call;
-    if (query instanceof BoundParameterQuery) {
-        BoundParameterQuery boundParameterQuery = (BoundParameterQuery) query;
-        call = this.influxDBService.query(getDatabase(query),
-                TimeUtil.toTimePrecision(timeUnit), query.getCommandWithUrlEncoded(),
-                boundParameterQuery.getParameterJsonWithUrlEncoded());
+    if (query.hasBoundParameters()) {
+      if (query.requiresPost()) {
+        call = this.influxDBService.postQuery(getDatabase(query), TimeUtil.toTimePrecision(timeUnit),
+                                          query.getCommandWithUrlEncoded(), query.getParameterJsonWithUrlEncoded());
+      } else {
+        call = this.influxDBService.query(getDatabase(query), TimeUtil.toTimePrecision(timeUnit),
+                                          query.getCommandWithUrlEncoded(), query.getParameterJsonWithUrlEncoded());
+      }
     } else {
         if (query.requiresPost()) {
-          call = this.influxDBService.query(getDatabase(query),
-                  TimeUtil.toTimePrecision(timeUnit), query.getCommandWithUrlEncoded(), null);
+          call = this.influxDBService.postQuery(getDatabase(query),
+                  TimeUtil.toTimePrecision(timeUnit), query.getCommandWithUrlEncoded());
         } else {
           call = this.influxDBService.query(getDatabase(query),
-                  TimeUtil.toTimePrecision(timeUnit), query.getCommandWithUrlEncoded());
+                  TimeUtil.toTimePrecision(timeUnit), query.getCommandWithUrlEncoded(), null);
         }
     }
     return executeQuery(call);
@@ -788,10 +794,14 @@ public class InfluxDBImpl implements InfluxDB {
    */
   private Call<QueryResult> callQuery(final Query query) {
     Call<QueryResult> call;
-    if (query instanceof BoundParameterQuery) {
-        BoundParameterQuery boundParameterQuery = (BoundParameterQuery) query;
+    if (query.hasBoundParameters()) {
+      if (query.requiresPost()) {
         call = this.influxDBService.postQuery(getDatabase(query), query.getCommandWithUrlEncoded(),
-                boundParameterQuery.getParameterJsonWithUrlEncoded());
+                                              query.getParameterJsonWithUrlEncoded());
+      } else {
+        call = this.influxDBService.query(getDatabase(query), null, query.getCommandWithUrlEncoded(),
+                                              query.getParameterJsonWithUrlEncoded());
+      }
     } else {
         if (query.requiresPost()) {
           call = this.influxDBService.postQuery(getDatabase(query), query.getCommandWithUrlEncoded());
diff --git a/src/main/java/org/influxdb/impl/InfluxDBService.java b/src/main/java/org/influxdb/impl/InfluxDBService.java
index ce7a811..061a766 100644
--- a/src/main/java/org/influxdb/impl/InfluxDBService.java
+++ b/src/main/java/org/influxdb/impl/InfluxDBService.java
@@ -47,12 +47,7 @@ interface InfluxDBService {
 
   @GET("query")
   public Call<QueryResult> query(@Query(DB) String db,
-      @Query(EPOCH) String epoch, @Query(value = Q, encoded = true) String query);
-
-  @POST("query")
-  @FormUrlEncoded
-  public Call<QueryResult> query(@Query(DB) String db,
-          @Query(EPOCH) String epoch, @Field(value = Q, encoded = true) String query,
+          @Query(EPOCH) String epoch, @Query(value = Q, encoded = true) String query,
           @Query(value = PARAMS, encoded = true) String params);
 
   @GET("query")
@@ -66,9 +61,26 @@ interface InfluxDBService {
 
   @POST("query")
   @FormUrlEncoded
-  public Call<QueryResult> postQuery(@Query(DB) String db,
+  public Call<QueryResult> postQuery(@Query(DB) String db, @Query(EPOCH) String epoch,
+      @Field(value = Q, encoded = true) String query);
+
+  @POST("query")
+  @FormUrlEncoded
+  public Call<QueryResult> postQuery(@Query(DB) String db, @Query(EPOCH) String epoch,
           @Field(value = Q, encoded = true) String query, @Query(value = PARAMS, encoded = true) String params);
 
+  @Streaming
+  @POST("query?chunked=true")
+  @FormUrlEncoded
+  public Call<ResponseBody> postQuery(@Query(DB) String db, @Field(value = Q, encoded = true) String query,
+         @Query(CHUNK_SIZE) int chunkSize);
+
+  @Streaming
+  @POST("query?chunked=true")
+  @FormUrlEncoded
+  public Call<ResponseBody> postQuery(@Query(DB) String db, @Field(value = Q, encoded = true) String query,
+         @Query(CHUNK_SIZE) int chunkSize, @Query(value = PARAMS, encoded = true) String params);
+
   @POST("query")
   @FormUrlEncoded
   public Call<QueryResult> postQuery(@Field(value = Q, encoded = true) String query);
@@ -79,8 +91,7 @@ interface InfluxDBService {
       @Query(CHUNK_SIZE) int chunkSize);
 
   @Streaming
-  @POST("query?chunked=true")
-  @FormUrlEncoded
-  public Call<ResponseBody> query(@Query(DB) String db, @Field(value = Q, encoded = true) String query,
+  @GET("query?chunked=true")
+  public Call<ResponseBody> query(@Query(DB) String db, @Query(value = Q, encoded = true) String query,
           @Query(CHUNK_SIZE) int chunkSize, @Query(value = PARAMS, encoded = true) String params);
 }
diff --git a/src/main/java/org/influxdb/querybuilder/Appender.java b/src/main/java/org/influxdb/querybuilder/Appender.java
index 3dab5c0..8c7e34b 100644
--- a/src/main/java/org/influxdb/querybuilder/Appender.java
+++ b/src/main/java/org/influxdb/querybuilder/Appender.java
@@ -62,6 +62,8 @@ public final class Appender {
       stringBuilder.append(')');
     } else if (value instanceof Column) {
       appendName(((Column) value).getName(), stringBuilder);
+    } else if (value instanceof Placeholder) {
+      stringBuilder.append('$').append(((Placeholder) value).getName());
     } else if (value instanceof String) {
       stringBuilder.append("'").append(value).append("'");
     } else if (value != null) {
diff --git a/src/main/java/org/influxdb/querybuilder/FunctionFactory.java b/src/main/java/org/influxdb/querybuilder/FunctionFactory.java
index 19541c4..ba5bfab 100644
--- a/src/main/java/org/influxdb/querybuilder/FunctionFactory.java
+++ b/src/main/java/org/influxdb/querybuilder/FunctionFactory.java
@@ -61,6 +61,10 @@ public final class FunctionFactory {
     return new Column(name);
   }
 
+  public static Object placeholder(final String name) {
+    return new Placeholder(name);
+  }
+
   private static void convertToColumns(final Object... arguments) {
     for (int i = 0; i < arguments.length; i++) {
       arguments[i] = convertToColumn(arguments[i]);
diff --git a/src/main/java/org/influxdb/querybuilder/Placeholder.java b/src/main/java/org/influxdb/querybuilder/Placeholder.java
new file mode 100644
index 0000000..8b21cd8
--- /dev/null
+++ b/src/main/java/org/influxdb/querybuilder/Placeholder.java
@@ -0,0 +1,14 @@
+package org.influxdb.querybuilder;
+
+public class Placeholder {
+
+  private final String name;
+
+  Placeholder(final String name) {
+    this.name = name;
+  }
+
+  public String getName() {
+    return name;
+  }
+}


Test patch:
diff --git a/src/test/java/org/influxdb/querybuilder/api/BuiltQueryTest.java b/src/test/java/org/influxdb/querybuilder/api/BuiltQueryTest.java
index 2f9565a..1fcadfd 100644
--- a/src/test/java/org/influxdb/querybuilder/api/BuiltQueryTest.java
+++ b/src/test/java/org/influxdb/querybuilder/api/BuiltQueryTest.java
@@ -10,10 +10,14 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import org.influxdb.dto.Query;
+import org.influxdb.querybuilder.FunctionFactory;
 import org.influxdb.querybuilder.RawText;
 import org.influxdb.querybuilder.Where;
 import org.junit.jupiter.api.Test;
+import org.junit.platform.runner.JUnitPlatform;
+import org.junit.runner.RunWith;
 
+@RunWith(JUnitPlatform.class)
 public class BuiltQueryTest {
 
   private static final String DATABASE = "testdb";
@@ -973,4 +977,12 @@ public class BuiltQueryTest {
     assertEquals(query.getDatabase(), select.getDatabase());
   }
 
+  @Test
+  public void testBoundParameters() {
+    Query query = select().column("a").from(DATABASE, "b")
+                           .where(eq("c", FunctionFactory.placeholder("d"))).bindParameter("d", 3);
+    assertEquals("SELECT a FROM b WHERE c = $d;", query.getCommand());
+    assertEquals(Query.encode("{\"d\":3}"), query.getParameterJsonWithUrlEncoded());
+    assertEquals(DATABASE, query.getDatabase());
+  }
 }