SQL: forward warning headers to JDBC driver #84499

Luegg · 2022-03-01T13:31:58Z

Follow up on #83943 to ensure that users of the JDBC drivers also can act on the warnings.

The change ensures that warnings in the response headers from the _sql API are available in JdbcResultSet.getWarnings().

elasticsearchmachine · 2022-03-01T13:32:23Z

Hi @Luegg, I've created a changelog YAML for you.

Luegg · 2022-03-01T13:39:56Z

x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java

    }

-    public SqlQueryResponse query(SqlQueryRequest sqlRequest) throws SQLException {
+    public Tuple<SqlQueryResponse, List<String>> query(SqlQueryRequest sqlRequest) throws SQLException {


I'm not too fond of working with tuples everywhere I have to pass the warnings but this was the least intrusive way to get the warnings from the response headers. Since it's an internal client it probably doesn't matter too much but suggestions for solving this differently are welcome.

elasticmachine · 2022-03-02T09:14:37Z

Pinging @elastic/es-ql (Team:QL)

astefan

LGTM. Left two comments.

astefan · 2022-03-03T17:10:15Z

...ugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcWarningsTestCase.java

            assertNull(rs.getWarnings());
        }
    }

+    private static Version WARNING_HANDLING_ADDED_VERSION = Version.V_8_2_0;


Please, move this line at the top of the class.

astefan · 2022-03-03T19:52:56Z

x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java


    }

    private <Response> Response fromContent(
-        ContentType contentType,
+        Function<String, List<String>> headers,


Here I would keep the initial intention of the method (getting the contentType and not the whole list of headers).

bpintea

LGTM

bpintea · 2022-03-04T14:50:27Z

...in/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/JreHttpUrlConnection.java

+            String value = con.getHeaderField(i);
+            while (value != null) {


Just noting that the function would stop iterating on an empty header value? This seems to be legal, though very corner case.

I hope that's never the case but the API does not forbid null headers. Changing the implementation to use URLConnection.getHeaderFields() 👍

costin

Looks good!
The pairing ResultSet#clearWarnings needs to be implemented as well - hence the request for changes.
An alternative to Tuple in 8.x could be a dedicated response class (maybe even a record in 8.x) to avoid the .v1() call or the large Tuple<SqlQueryResponse, List> signature to public ConnectionResponse ... or something along those lines.

Luegg · 2022-03-14T16:58:27Z

@costin good catch regarding ResultSet#clearWarnings. I've added the implementation and also replaced the tuple with a generic wrapper class (records are not available because we're stuck with Java 8 here).

costin

Left some comments around styling, LGTM.

costin · 2022-03-15T11:44:33Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/Cursor.java

+
+    List<String> warnings();
+
+    Cursor withoutWarnings();


I'd prefer to remove the warnings instead of creating a new cursor instance.

costin · 2022-03-15T11:45:52Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/DefaultCursor.java

+
+    @Override
+    public Cursor withoutWarnings() {
+        return new DefaultCursor(client, cursor, columnInfos, rows, meta, Collections.emptyList());


clearWarnings method is easy to map the actual functionality and can simply clear the list with a check to be emptyList-friendly:

if (list.isEmpty() == false) { list.clear() }

List#clear() is not implemented in the list instance used here (it's unmodifiable).

costin · 2022-03-15T11:46:26Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcResultSet.java

@@ -54,7 +54,7 @@
    private final Calendar defaultCalendar;

    private final JdbcStatement statement;
-    private final Cursor cursor;


By using clearWarnings the Cursor is preserved as final.

costin · 2022-03-15T11:46:44Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcResultSet.java

    }

    @Override
    public void clearWarnings() throws SQLException {
        checkOpen();
+        cursor = cursor.withoutWarnings();


cursor.clearWarnings()

costin · 2022-03-15T11:48:58Z

...in/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/JreHttpUrlConnection.java

@@ -183,6 +187,17 @@ public boolean head() throws ClientException {
        }
    }

+    private Function<String, List<String>> getHeaderFields(URLConnection con) {
+        return header -> {
+            for (Map.Entry<String, List<String>> entry : con.getHeaderFields().entrySet()) {


This method returns the first match - does headerFields normalizes keys? For example Content-Type and ConteNT-TYPE have their values under the same key or separate ones?

good catch. Looking at the URLConnection implementation it indeed looks like Content-Type and ConteNT-TYPE header values would end up in two different buckets. While it's unlikely that a server would use two different casing rules I'll have a look at a more stable approach anyway.

Follow up on elastic#83943 to ensure that users of the JDBC drivers also can act on the warnings. The change ensures that warnings in the response headers from the `_sql` API are available in `JdbcResultSet.getWarnings()`.

elasticsearchmachine · 2022-03-15T16:59:57Z

💚 Backport successful

Status	Branch	Result
✅	8.1

* SQL: forward warning headers to JDBC driver (#84499) Follow up on #83943 to ensure that users of the JDBC drivers also can act on the warnings. The change ensures that warnings in the response headers from the `_sql` API are available in `JdbcResultSet.getWarnings()`. * fix version

Luegg added >enhancement :Analytics/SQL SQL querying v8.2.0 v8.1.1 labels Mar 1, 2022

Luegg commented Mar 1, 2022

View reviewed changes

forward warning headers to JDBC driver

990cbcb

Luegg force-pushed the enhance/warningsInJdbc branch from ba95737 to 990cbcb Compare March 1, 2022 13:42

Luegg added 2 commits March 1, 2022 14:43

Update docs/changelog/84499.yaml

056d52c

make headers lookup case-insensitive

4c16d66

Luegg requested review from costin, bpintea, astefan and luigidellaquila March 2, 2022 09:14

Luegg marked this pull request as ready for review March 2, 2022 09:14

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Mar 2, 2022

astefan approved these changes Mar 3, 2022

View reviewed changes

Luegg added 2 commits March 4, 2022 08:57

Merge origin/main into enhance/warningsInJdbc

d3791a3

address review comments

94d85e0

bpintea approved these changes Mar 4, 2022

View reviewed changes

costin requested changes Mar 4, 2022

View reviewed changes

Luegg added 3 commits March 14, 2022 17:05

Merge origin/main into enhance/warningsInJdbc

986ef75

fix ResultSet.clearWarnings

42446b1

replace Tuple with wrapper class

4b90b10

Luegg requested a review from costin March 14, 2022 16:58

safer method for getting headers from URLConnection

321048a

Luegg force-pushed the enhance/warningsInJdbc branch from b55673d to 321048a Compare March 14, 2022 17:04

costin approved these changes Mar 15, 2022

View reviewed changes

make getHeaderFields more robust against inconsistent casing

07238bb

Luegg force-pushed the enhance/warningsInJdbc branch from 7cb982b to 07238bb Compare March 15, 2022 15:04

mutate cursor

a7216d2

Luegg added auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge Automatically create backport pull requests and merge when ready labels Mar 15, 2022

elasticsearchmachine merged commit 7c98c46 into elastic:master Mar 15, 2022

Luegg deleted the enhance/warningsInJdbc branch March 15, 2022 16:59

Luegg mentioned this pull request Mar 15, 2022

[8.1] SQL: forward warning headers to JDBC driver (#84499) #84999

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: forward warning headers to JDBC driver #84499

SQL: forward warning headers to JDBC driver #84499

Luegg commented Mar 1, 2022 •

edited

Loading

elasticsearchmachine commented Mar 1, 2022

Luegg Mar 1, 2022

elasticmachine commented Mar 2, 2022

astefan left a comment

astefan Mar 3, 2022

astefan Mar 3, 2022

bpintea left a comment

bpintea Mar 4, 2022

Luegg Mar 14, 2022

costin left a comment

Luegg commented Mar 14, 2022

costin left a comment

costin Mar 15, 2022

costin Mar 15, 2022

Luegg Mar 15, 2022

costin Mar 15, 2022

costin Mar 15, 2022

costin Mar 15, 2022

Luegg Mar 15, 2022

elasticsearchmachine commented Mar 15, 2022

		String value = con.getHeaderField(i);
		while (value != null) {

SQL: forward warning headers to JDBC driver #84499

SQL: forward warning headers to JDBC driver #84499

Conversation

Luegg commented Mar 1, 2022 • edited Loading

elasticsearchmachine commented Mar 1, 2022

Choose a reason for hiding this comment

elasticmachine commented Mar 2, 2022

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Luegg commented Mar 14, 2022

costin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 15, 2022

💚 Backport successful

Luegg commented Mar 1, 2022 •

edited

Loading