fix sending mutiple queries with single query bug #236

shanexu · 2016-11-08T23:46:29Z

should use @Filed and @FormUrlEncoded, or the arguments will encoded as query string append to request URL.

codecov-io · 2016-11-08T23:51:05Z

Current coverage is 72.30% (diff: 63.63%)

Merging #236 into master will decrease coverage by 0.05%

@@             master       #236   diff @@
==========================================
  Files            11         11          
  Lines           633        639     +6   
  Methods           0          0          
  Messages          0          0          
  Branches         75         75          
==========================================
+ Hits            458        462     +4   
- Misses          142        144     +2   
  Partials         33         33

Powered by Codecov. Last update 5d11e5f...94bd9e2

majst01 · 2016-11-09T06:41:35Z

I already had the same idea, thanks, would you please show the behavior in a unit-test then i will merge.

shanexu · 2016-11-09T07:01:15Z

    @Test
    public void testPostQuery() {
        String dbName = "unittest_" + System.currentTimeMillis();
        this.influxDB.createDatabase(dbName);
        QueryResult result = this.influxDB.query(new Query("SELECT * FROM not_exists;SELECT * FROM not_exists", dbName, true));
        Assert.assertEquals(2, result.getResults().size());
        this.influxDB.deleteDatabase(dbName);
    }

this will fail if use @Query

majst01 · 2016-11-09T07:18:44Z

Yes of course, but include some tests, one for failing and one for working which shows how your code modifications make it work. A note on the README.md regarding multiple Queries in one request would also be helpful.

shanexu · 2016-11-09T08:22:36Z

    @Test
    public void testPostQuery() {
        OkHttpClient.Builder builder = new OkHttpClient.Builder();
        builder.readTimeout(1, TimeUnit.MINUTES);
        builder.connectTimeout(1, TimeUnit.MINUTES);
        builder.writeTimeout(1, TimeUnit.MINUTES);
        this.influxDB = InfluxDBFactory.connect("http://" + TestUtils.getInfluxIP() + ":" + TestUtils.getInfluxPORT(true), "admin", "admin", builder);
        String dbName = "unittest_" + System.currentTimeMillis();
        this.influxDB.createDatabase(dbName);
        String query = "SELECT * FROM not_exists";
        StringBuilder sb = new StringBuilder();
        for (int i = 0; i < 1 << 18; i++) {
            sb.append(query).append(";");
        }
        sb.deleteCharAt(sb.length() - 1);
        this.influxDB.query(new Query(sb.toString(), dbName, true));
        this.influxDB.deleteDatabase(dbName);
    }

this test case should throw exception if use @Query, http header is limited to a fixed size. When using @Query, the arguments will append to request url, and create a http request with large header.
When using @Field, the arugments are encoded into form and append to http request body.

majst01 · 2016-11-12T09:50:58Z

@shanexu can you please rebase and resolve the merge conflict ?
After that i would like to merge it

majst01 · 2016-11-12T13:33:17Z

@shanexu You should rebase, or clone again. The way you did it now does not show what you changed.

Please try again

shanexu · 2016-11-12T13:34:51Z

Now, using @Field will cause

    @Test
    public void testWriteEnableGzip() {

this unit test case fail.
I need to read influxdb's source code to verify the problem.

influxdb does not handle a query request with Content-Encoding: gzip and Content-Type: application/x-www-form-urlencoded, so we should skip compressing for query request.

shanexu · 2016-11-13T00:08:37Z

@majst01 is it OK now?

jiafu1115 · 2016-11-13T00:26:20Z

@shanexu you can squash your commit into one commit to make PR's commit clean:
(I sqush all commits and force push then I only have one commit for one PR)
now your PR had 6 commits.Just one suggestion you can ignore it. Thanks.

majst01 · 2016-11-13T08:15:30Z

@jiafu1115 what do you think, i am fine with this PR

jiafu1115

@shanexu @majst01 are you sure of this:
influxdb does not handle a query request with Content-Encoding: gzip and Content-Type: application/x-www-form-urlencoded, so we should skip compressing for query request.
are you sure that it is influxdb's issue? if so you can skip it after all that gzip's focus is on write.

I provide some tiny suggestions on unit tests. Thanks. You can go ahead!

jiafu1115 · 2016-11-13T13:03:57Z

src/test/java/org/influxdb/InfluxDBTest.java

+		this.influxDB.createDatabase(dbName);
+		QueryResult result = this.influxDB.query(new Query("SELECT * FROM not_exists;SELECT * FROM not_exists", dbName, false));
+		Assert.assertEquals(2, result.getResults().size());
+		this.influxDB.deleteDatabase(dbName);


try...finally to make use tmp database to be removed

jiafu1115 · 2016-11-13T13:05:57Z

src/test/java/org/influxdb/InfluxDBTest.java

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


add java doc or change test's method's name to show what's your test focus?

jiafu1115 · 2016-11-13T13:27:11Z

src/test/java/org/influxdb/InfluxDBTest.java

+		this.influxDB.createDatabase(dbName);
+		QueryResult result = this.influxDB.query(new Query("SELECT * FROM not_exists;SELECT * FROM not_exists", dbName));
+		Assert.assertEquals(2, result.getResults().size());
+		this.influxDB.deleteDatabase(dbName);


should using try...finally to delete tmp db

jiafu1115 · 2016-11-13T14:03:07Z

src/test/java/org/influxdb/InfluxDBTest.java

+	public void testPostQuery() {
+		String dbName = "unittest_" + System.currentTimeMillis();
+		try {
+			this.influxDB.createDatabase(dbName);


move to 123 from 125.

I don't think it's necessary. Droping a non-existent database will not cause any problem.

it is one of best practices instead of if it is necessary for this case. So it is up to you. just one advice for best practice.

jiafu1115

why need this (I notice the encoded=true in all query methods)? if need it, why loss it in write method on 333 line?

jiafu1115 · 2016-11-13T14:45:09Z

src/main/java/org/influxdb/impl/InfluxDBImpl.java

@@ -318,8 +319,9 @@ public QueryResult query(final Query query) {
      call = this.influxDBService.postQuery(this.username,
                                            this.password, query.getDatabase(), query.getCommand());
    } else {
+      String encodedCommand = UrlEscapers.urlFormParameterEscaper().escape(query.getCommand());


why need this (I notice the encoded=true in all query methods)? if need it, why loss it in write method on 333 line?

okhttp-3.4.2 use different policies on encoding query parameter and form field.

For query parameter
HttpUrl.java#L1120

/** Encodes the query parameter using UTF-8 and adds it to this URL's query string. */ public Builder addQueryParameter(String name, String value) { if (name == null) throw new NullPointerException("name == null"); if (encodedQueryNamesAndValues == null) encodedQueryNamesAndValues = new ArrayList<>(); encodedQueryNamesAndValues.add( canonicalize(name, QUERY_COMPONENT_ENCODE_SET, false, false, true, true)); encodedQueryNamesAndValues.add(value != null ? canonicalize(value, QUERY_COMPONENT_ENCODE_SET, false, false, true, true) : null); return this; }

it does not encode some characters, for example ;, but influxdb need ; encode into %3B.

For form field
FormBody.java#L110

public Builder add(String name, String value) { names.add(HttpUrl.canonicalize(name, FORM_ENCODE_SET, false, false, true, true)); values.add(HttpUrl.canonicalize(value, FORM_ENCODE_SET, false, false, true, true)); return this; }

it does encode ; and some other characters.

So why loss encode in line 333? It is query too

my fault, I see, i missed this one. i will fix this

jiafu1115

SHOW+DATABASES? looks strange.....

jiafu1115 · 2016-11-13T22:54:34Z

src/main/java/org/influxdb/impl/InfluxDBImpl.java

@@ -362,7 +362,7 @@ public void deleteDatabase(final String name) {
  @Override
  public List<String> describeDatabases() {
    QueryResult result = execute(this.influxDBService.query(this.username,
-                                                            this.password, "SHOW DATABASES"));
+                                                            this.password, "SHOW+DATABASES"));


SHOW+DATABASES? looks strange.....

a space can be encoded into + or %20

jiafu1115 · 2016-11-14T07:28:39Z

@shanexu I have one idea that if we can move url encode into new method called query.getCommandWithEncoded()? Then we may limit the code change lines and not change every where. For known command, we can does't encode it such as "create database" WDYT?

jiafu1115 · 2016-11-14T07:31:20Z

@shanexu do you have qq? we can talk offline.

jiafu1115 · 2016-11-14T12:14:43Z

LGTM now. you missed the test cases so that coverage decreased

majst01 · 2016-11-14T13:15:41Z

I`m going to merge this now, thanks for all of you for digging into that issue.

shanexu changed the base branch from master to old_master November 12, 2016 23:58

shanexu changed the base branch from old_master to master November 12, 2016 23:58

shanexu force-pushed the master branch from 5f8aa29 to 5685088 Compare November 13, 2016 00:02

shanexu force-pushed the master branch from 5685088 to 297c53f Compare November 13, 2016 00:44

jiafu1115 approved these changes Nov 13, 2016

View reviewed changes

jiafu1115 reviewed Nov 13, 2016

View reviewed changes

shanexu force-pushed the master branch from 17bde0d to 01e17d3 Compare November 13, 2016 14:02

jiafu1115 reviewed Nov 13, 2016

View reviewed changes

fix sending multiple queries bug

6995727

shanexu force-pushed the master branch from ee062ae to 6995727 Compare November 14, 2016 09:44

shanexu changed the title ~~using Field to Post~~ fix send mutiple queries bug Nov 14, 2016

shanexu changed the title ~~fix send mutiple queries bug~~ fix sending mutiple queries with single query bug Nov 14, 2016

add unit test

94bd9e2

majst01 merged commit 970aca8 into influxdata:master Nov 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix sending mutiple queries with single query bug #236

fix sending mutiple queries with single query bug #236

shanexu commented Nov 8, 2016

codecov-io commented Nov 8, 2016 •

edited

Loading

majst01 commented Nov 9, 2016

shanexu commented Nov 9, 2016 •

edited

Loading

majst01 commented Nov 9, 2016

shanexu commented Nov 9, 2016

majst01 commented Nov 12, 2016

majst01 commented Nov 12, 2016

shanexu commented Nov 12, 2016 •

edited

Loading

shanexu commented Nov 13, 2016

jiafu1115 commented Nov 13, 2016 •

edited

Loading

majst01 commented Nov 13, 2016

jiafu1115 left a comment •

edited

Loading

jiafu1115 Nov 13, 2016

jiafu1115 Nov 13, 2016

jiafu1115 Nov 13, 2016

jiafu1115 Nov 13, 2016

shanexu Nov 13, 2016

jiafu1115 Nov 13, 2016

jiafu1115 left a comment

jiafu1115 Nov 13, 2016

shanexu Nov 13, 2016

jiafu1115 Nov 13, 2016

shanexu Nov 13, 2016

jiafu1115 left a comment

jiafu1115 Nov 13, 2016

shanexu Nov 13, 2016 •

edited

Loading

jiafu1115 commented Nov 14, 2016 •

edited

Loading

jiafu1115 commented Nov 14, 2016

jiafu1115 commented Nov 14, 2016

majst01 commented Nov 14, 2016

fix sending mutiple queries with single query bug #236

fix sending mutiple queries with single query bug #236

Conversation

shanexu commented Nov 8, 2016

codecov-io commented Nov 8, 2016 • edited Loading

Current coverage is 72.30% (diff: 63.63%)

majst01 commented Nov 9, 2016

shanexu commented Nov 9, 2016 • edited Loading

majst01 commented Nov 9, 2016

shanexu commented Nov 9, 2016

majst01 commented Nov 12, 2016

majst01 commented Nov 12, 2016

shanexu commented Nov 12, 2016 • edited Loading

shanexu commented Nov 13, 2016

jiafu1115 commented Nov 13, 2016 • edited Loading

majst01 commented Nov 13, 2016

jiafu1115 left a comment • edited Loading

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

jiafu1115 left a 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

jiafu1115 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanexu Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

jiafu1115 commented Nov 14, 2016 • edited Loading

jiafu1115 commented Nov 14, 2016

jiafu1115 commented Nov 14, 2016

majst01 commented Nov 14, 2016

codecov-io commented Nov 8, 2016 •

edited

Loading

shanexu commented Nov 9, 2016 •

edited

Loading

shanexu commented Nov 12, 2016 •

edited

Loading

jiafu1115 commented Nov 13, 2016 •

edited

Loading

jiafu1115 left a comment •

edited

Loading

shanexu Nov 13, 2016 •

edited

Loading

jiafu1115 commented Nov 14, 2016 •

edited

Loading