Parameter binding in InfluxQL (influxdata/influxdb-java#274) #429

hampelratte · 2018-03-15T18:41:37Z

This is a simple implementation for "prepared statements" (#274) using regular expressions.

I added a new query class BoundParameterQuery, which can be used for
"prepared statements". The constructor accepts an InfluxQL expression
with placeholders, a DB name and a varags list to bind the parameters to
the placeholders. I also extended the InfluxDBService, so that the HTTP
requests contain the "params" parameter. The InfluxDBImpl now
differentiates between a Query and a BoundParameterQuery. This is not
the cleanest solution, because this had to be done at a few locations,
but I didn't want to change too much of the code.

It has limitations, because it does not use a parser/grammar for InfluxQL. So you could for example have a placeholder in a string value of a field/tag and this implementation would fail, but at least it is a start and better than risking injections.

Feel free to delete this pull request, if you don't like it at all ;)

I added a new query class BoundParameterQuery, which can be used for "prepared statements". The constructor accepts a InfluxQL expression with placeholders, a DB name and a varags list to bind the parameters to the placeholders. I also extended the InfluxDBService, so that the HTTP requests contain the "params" parameter. The InfluxDBImpl now differentiates between a Query and a BoundParameterQuery. This is not the cleanest solution, because this has to be done at a few locations, but I didn't want to change too much of the code.

codecov-io · 2018-03-15T19:19:42Z

Codecov Report

Merging #429 into master will increase coverage by 0.79%.
The diff coverage is 89.33%.

@@             Coverage Diff              @@
##             master     #429      +/-   ##
============================================
+ Coverage     84.98%   85.78%   +0.79%     
- Complexity      274      291      +17     
============================================
  Files            19       20       +1     
  Lines          1192     1259      +67     
  Branches        121      130       +9     
============================================
+ Hits           1013     1080      +67     
  Misses          118      118              
  Partials         61       61

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/influxdb/impl/InfluxDBImpl.java	`85.67% <79.31%> (+0.65%)`	`64 <3> (+3)`	⬆️
...ain/java/org/influxdb/dto/BoundParameterQuery.java	`95.65% <95.65%> (ø)`	`12 <12> (?)`
src/main/java/org/influxdb/impl/TimeUtil.java	`68% <0%> (+4%)`	`6% <0%> (+1%)`	⬆️
src/main/java/org/influxdb/dto/Query.java	`63.15% <0%> (+5.26%)`	`12% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90f02ea...2fc928e. Read the comment docs.

majst01 · 2018-03-16T10:26:21Z

Hi @hampelratte thanks for the effort, we always appreciate contribution.

I think this is a good starting Point, but i would like to have a fluent API without a very wide Constructor. I can think of something like:

query = BoundParameterQuery("SELECT * FROM abc WHERE a > $a AND b < $b")
                  .with("a",0)
                  .with("b",10)
                  .create()

This Builder avoid very long constructor calls and also makes it more obvious, which value is bound to which placeholder.

Removed bind method from BoundParameterQuery, because this functionality is now implemented in the QueryBuilder

Fixed checkstyle issues and formatted according to project standard (hopefully)

Added a test which executes a BoundParameterQuery against an actual database.

fmachado · 2018-03-17T17:28:28Z

src/main/java/org/influxdb/dto/BoundParameterQuery.java

+  }
+
+  private void assurePlaceholdersAreBound(final List<String> placeholders, final Map<String, Object> params) {
+    if (placeholders.size() != params.size()) {


This check will cause a valid query to fail. For example:

BoundParameterQuery query = QueryBuilder.newQuery("SELECT * FROM cpu WHERE atag = $mytag ; SELECT * FROM cpu WHERE atag != $mytag") .forDatabase(dbName) .bind("mytag", "test") .create();

Is there any reason to have it?

Relevant code has been removed

fmachado · 2018-03-17T17:50:57Z

src/main/java/org/influxdb/dto/BoundParameterQuery.java

+    for (Entry<String, Object> pair : parameterMap.entrySet()) {
+      String name = pair.getKey();
+      Object value = pair.getValue();
+      if (value instanceof Number) {


The moshi library has a specific behavior when deserializing numbers: #153 (comment)

I hope casting to Number is enough to keep the data type on the JSON object being created here.

I had a look at the moshi code and tested it. This works as intended.

fmachado · 2018-03-17T17:53:24Z

src/main/java/org/influxdb/dto/BoundParameterQuery.java

+      } else if (value instanceof Boolean) {
+        writer.name(name).value((Boolean) value);
+      } else {
+        writer.name(name).value(value.toString());


This may cause a NullPointerException.

I would change the logic here to:

if (... Number) { //... } else if (... Boolean) { //... } else { writer.name(name).value(String.valueOf(value)); }

fmachado · 2018-03-17T17:59:40Z

src/main/java/org/influxdb/dto/BoundParameterQuery.java

+
+  private List<String> parsePlaceHolders(final String command) {
+    List<String> placeHolders = new ArrayList<>();
+    Pattern p = Pattern.compile("\\s+\\$(\\w+?)(?:\\s|$)");


Pattern is immutable and should be declared as class scoped static final.

Relevant code has been removed

fmachado · 2018-03-17T18:04:49Z

src/test/java/org/influxdb/dto/BoundParameterQueryTest.java

+  }
+
+  @Test
+  public void testEqualsAndHashCode() {


Is this really necessary? Did you create your own hashcode/equals implementation or you are using the one provided by your IDE?

There is a test for hashcode and equals in Query. So I just did it for BoundParameterQuery, too.

fmachado · 2018-03-17T18:12:57Z

src/test/java/org/influxdb/dto/BoundParameterQueryTest.java

+        .bind("b", 10)
+        .create();
+    Map<String, Object> params = readObject(decode(query.getParameterJsonWithUrlEncoded()));
+    Assert.assertEquals(2, params.size());


I really appreciate your detailed and extensive test but I think it's too much: you are testing multiple times the same logic over and over.

Could you please test query.getParameterJsonWithUrlEncoded() only once, covering all data types supported by InfluxDB?

Relevant code has been removed

fmachado · 2018-03-17T18:16:01Z

src/main/java/org/influxdb/dto/BoundParameterQuery.java

+
+  public String getParameterJsonWithUrlEncoded() {
+    try {
+      List<String> placeholders = parsePlaceHolders(getCommand());


parsePlaceHolders is only used to create a list for assurePlaceholdersAreBound.
Could you please move parsePlaceHolders to be called from inside assurePlaceholdersAreBound?

Relevant code has been removed

fmachado · 2018-03-17T18:29:12Z

src/main/java/org/influxdb/dto/BoundParameterQuery.java

+
+    for (String placeholder : placeholders) {
+      if (params.get(placeholder) == null) {
+        throw new RuntimeException("Placeholder $" + placeholder + " is not bound");


Personally I don't like to throw RuntimeExceptions (check why here https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable ).

Also, this exception will be thrown only when the query is about to be executed instead of the moment when I'm executing the .create() from the Builder. Would be possible to move it to the builder?

Another question: what about throwing IllegalStateException from the .create() method?

Relevant code has been removed

fmachado · 2018-03-17T18:31:40Z

@hampelratte and before I forget, thanks for your contribution. :)

hampelratte · 2018-03-17T19:16:10Z

@fmachado Good point. I was thinking, that a check for the placeholders might be a good thing, but obviously InfluxDB does that already and returns reasonable error messages. So there is no reason to do that in the connector. I'm going to remove that and as a result the majority of your annotations will get resolved, too.

The checks in BoundParameterQuery have been removed, because InfluxDB does that already and returns appropriate error messages in case of an invalid request.

fmachado · 2018-03-20T06:38:50Z

@majst01 do you want to take a look?

fmachado · 2018-03-20T06:45:31Z

@hampelratte could you please update README.md with an explanation and example of this new feature? Also, would be nice to have a link to the this PR on CHANGELOG.md under a new section "2.10 [unreleased]".

majst01 · 2018-03-20T08:27:46Z

Hi,
yes, once we have a bit documentation and a changelogentry i will merge this.
Again, thanks @hampelratte for contribution !

Added short paragraph in README, which describes the parameter binding for queries. Also added an entry for the parameter binding pull request to the changelog.

tekbot · 2018-04-20T01:17:23Z

Hi @majst01 !

We're eagerly awaiting the 2.10 release for this very feature. Do you have an estimated release date by chance?

tekbot · 2018-05-11T21:04:57Z

I just added this to our project -- Thanks! It did require that I include OKIO (https://github.com/square/okio) as a dependency. Trying to query against an influxDB instance threw a Buffer exception without it. Easy enough to resolve but it might be worth adding it on your side or updating your documentation. On the documentation topic, you still list 2.9 as your latest version for the maven dependency configuration in your docs.

majst01 · 2018-05-13T06:55:13Z

Hi,
Readme.md ist fixed.
If you are using maven or gradle to build and package your application, then okio is part of the transitive dependencies, if not you must of course care by yourself.

hampelratte added 2 commits March 15, 2018 19:14

Added another test for BoundParameterQuery

658fa1d

hampelratte added 7 commits March 16, 2018 18:40

Implemented Builder mechanism for BoundParameterQuery

d71f316

This Builder avoid very long constructor calls and also makes it more obvious, which value is bound to which placeholder.

Removed unnecessary method

929cfde

Removed bind method from BoundParameterQuery, because this functionality is now implemented in the QueryBuilder

Added test for the different parameter types

7eb0b5a

Added equals and hashcode

f03fa37

Code cleanup

966d781

Fixed checkstyle issues and formatted according to project standard (hopefully)

Added test for BoundParameterQuery

be08bcd

Added a test which executes a BoundParameterQuery against an actual database.

Improved test coverage for BoundParameterQuery

58cd9eb

fmachado requested review from fmachado and majst01 March 17, 2018 17:24

fmachado added the kind/enhancement label Mar 17, 2018

fmachado suggested changes Mar 17, 2018

View reviewed changes

Removed unnecessary placeholder checks

f761e61

The checks in BoundParameterQuery have been removed, because InfluxDB does that already and returns appropriate error messages in case of an invalid request.

fmachado approved these changes Mar 20, 2018

View reviewed changes

Added paragraph for parameter binding in queries in README

2fc928e

Added short paragraph in README, which describes the parameter binding for queries. Also added an entry for the parameter binding pull request to the changelog.

majst01 merged commit aca3701 into influxdata:master Mar 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameter binding in InfluxQL (influxdata/influxdb-java#274) #429

Parameter binding in InfluxQL (influxdata/influxdb-java#274) #429

hampelratte commented Mar 15, 2018

codecov-io commented Mar 15, 2018 •

edited

Loading

majst01 commented Mar 16, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado Mar 17, 2018

hampelratte Mar 18, 2018

fmachado commented Mar 17, 2018

hampelratte commented Mar 17, 2018

fmachado commented Mar 20, 2018

fmachado commented Mar 20, 2018

majst01 commented Mar 20, 2018

tekbot commented Apr 20, 2018

tekbot commented May 11, 2018

majst01 commented May 13, 2018

Parameter binding in InfluxQL (influxdata/influxdb-java#274) #429

Parameter binding in InfluxQL (influxdata/influxdb-java#274) #429

Conversation

hampelratte commented Mar 15, 2018

codecov-io commented Mar 15, 2018 • edited Loading

Codecov Report

majst01 commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmachado commented Mar 17, 2018

hampelratte commented Mar 17, 2018

fmachado commented Mar 20, 2018

fmachado commented Mar 20, 2018

majst01 commented Mar 20, 2018

tekbot commented Apr 20, 2018

tekbot commented May 11, 2018

majst01 commented May 13, 2018

codecov-io commented Mar 15, 2018 •

edited

Loading