[CALCITE-2219] Check if resource is closed, and throws #33

laurentgo · 2018-03-19T00:58:10Z

According to JDBC spec, most methods from Connection, Statement,
PreparedStatement and ResultSet should throw an exception if the
resource is closed.

risdenk

Changes look reasonable to me. Only a few minor comments.

risdenk · 2018-03-19T02:09:05Z

core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java

-    throw HELPER.unsupported();
+    checkOpen();
+    // Always return an empty map since custom types is not supported
+    return new HashMap<>();


Would it be more correct to return SQLFeatureNotSupportedException? I don't think it matters either way but below is from the javadoc for Connection.

SQLFeatureNotSupportedException - if the JDBC driver does not support this method

yes, probably more correct. I think I got the truncated version of the javadoc on this one. I will revert.

risdenk · 2018-03-19T02:11:55Z

core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java

@@ -392,14 +420,17 @@ public void setClientInfo(Properties properties)
  }

  public String getClientInfo(String name) throws SQLException {
-    throw HELPER.unsupported();
+    checkOpen();
+    return null;


This could be return this.getClientInfo().getProperty(name) now that getClientInfo() returns a Properties object. This would remove the need for the added checkOpen() and explicit return null.

risdenk · 2018-03-19T02:14:41Z

core/src/main/java/org/apache/calcite/avatica/AvaticaStatement.java

  }

  public void setMaxFieldSize(int max) throws SQLException {
-    throw connection.HELPER.unsupported();
+    throw AvaticaConnection.HELPER.unsupported();


Would it make sense to make this be a noop instead of returning unsupported now that getMaxFieldSize() is set to always return 0.

not if the value is set to something different from 0, if not supported

Ok that makes sense. Thanks!

joshelser · 2018-03-21T16:40:37Z

@laurentgo any chance I could request that you write a test case for one of this scenarios? Maybe the one in which lead you to find that this bug existed?
Otherwise, looks good to me too.

laurentgo · 2018-03-21T17:46:13Z

Not really triggered by a bug, but more by the extra code in Drill and Dremio JDBC drivers to implements the JDBC spec. Let me see if can reuse the same strategy as the Drill code (helper class to iterate over methods and check that the methods throws the correct exception).

There are also lots of methods where unsupported is thrown instead of a sqlexception for closed. I wonder if the correct thing to do would be to check first for closed, and then throw unsupported, and if it's fine for now.

joshelser · 2018-03-21T18:02:48Z

Not really triggered by a bug, but more by the extra code in Drill and Dremio JDBC drivers to implements the JDBC spec. Let me see if can reuse the same strategy as the Drill code (helper class to iterate over methods and check that the methods throws the correct exception).

Ok. I definitely didn't want to suggest that you try to cover every single one of these methods you fixed up :). I like to see at least one test added when we fix straighforward bugs like this, even if it is known that we're only catching one case. Appreciate you looking into it!

risdenk · 2018-04-28T21:00:36Z

@laurentgo - Any plans to add a test or few for this?

laurentgo · 2018-04-28T21:08:37Z

yes, still my plan, just need to find some time to wrap it up.

laurentgo · 2018-06-07T22:21:26Z

@risdenk @joshelser any comment regarding the unit tests?

risdenk · 2018-06-07T22:22:32Z

Whoops definitely missed there was a commit added to this. Not sure it sends an email :( I can take a look.

risdenk

Changes look good. Only a few minor spelling items. Testing framework for closed is cool.

Also checked in combination with PR #56 to make sure there weren't going to be new checkstyle issues.

One thing that could be improved is using Lambdas with JDK 8 instead of full methods. Only found this since IDE suggested it.

risdenk · 2018-06-07T22:46:26Z

core/src/test/java/org/apache/calcite/avatica/AvaticaClosedCheckHelper.java

+import static org.junit.Assert.fail;
+
+/**
+ * Helper class for tests checkign close behavior


sp nit: checking

risdenk · 2018-06-07T22:47:52Z

core/src/test/java/org/apache/calcite/avatica/AvaticaClosedConnectionTest.java

+
+  private static final Predicate<? super Method> METHOD_FILTER = new Predicate<Method>() {
+    public boolean test(Method m) {
+      // Skip methods not overriden by Avatica


sp nit: overridden

risdenk · 2018-06-07T22:48:22Z

core/src/test/java/org/apache/calcite/avatica/AvaticaClosedPreparedStatementTest.java

+public class AvaticaClosedPreparedStatementTest {
+  private static final Predicate<? super Method> METHOD_FILTER = new Predicate<Method>() {
+    public boolean test(Method m) {
+      // Skip methods not overriden by Avatica


sp nit: overridden

risdenk · 2018-06-07T22:48:28Z

core/src/test/java/org/apache/calcite/avatica/AvaticaClosedResultSetTest.java

+public class AvaticaClosedResultSetTest {
+  private static final Predicate<? super Method> METHOD_FILTER = new Predicate<Method>() {
+    public boolean test(Method m) {
+      // Skip methods not overriden by Avatica


sp nit: overridden

laurentgo · 2018-06-07T23:12:00Z

If I merge the checkstyle patch first, I might be able to revert to my lambda implementation :)

Fix warnings about accessing AvaticaConnection.HELPER static field by using a class instance.

According to JDBC spec, most methods from Connection, Statement, PreparedStatement and ResultSet should throw an exception if the resource is closed.

F21 · 2018-06-13T23:03:57Z

Friendly ping @laurentgo

risdenk reviewed Mar 19, 2018

View reviewed changes

risdenk approved these changes Mar 19, 2018

View reviewed changes

laurentgo force-pushed the laurentgo/CALCITE-2219 branch from 9e92484 to ada6c42 Compare March 19, 2018 02:54

laurentgo force-pushed the laurentgo/CALCITE-2219 branch 3 times, most recently from f191edd to 7a92285 Compare June 5, 2018 14:32

risdenk approved these changes Jun 7, 2018

View reviewed changes

laurentgo force-pushed the laurentgo/CALCITE-2219 branch 2 times, most recently from c0ec5f6 to 47bac3d Compare June 9, 2018 01:01

laurentgo added 2 commits June 9, 2018 11:43

Fix static field access warnings

da3f03b

Fix warnings about accessing AvaticaConnection.HELPER static field by using a class instance.

[CALCITE-2219] Check if resource is closed, and throws

20707cc

According to JDBC spec, most methods from Connection, Statement, PreparedStatement and ResultSet should throw an exception if the resource is closed.

laurentgo force-pushed the laurentgo/CALCITE-2219 branch from 47bac3d to 20707cc Compare June 9, 2018 18:44

asfgit closed this in 2ebfd80 Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-2219] Check if resource is closed, and throws #33

[CALCITE-2219] Check if resource is closed, and throws #33

laurentgo commented Mar 19, 2018

risdenk left a comment

risdenk Mar 19, 2018

laurentgo Mar 19, 2018

risdenk Mar 19, 2018

risdenk Mar 19, 2018

laurentgo Mar 19, 2018

risdenk Mar 19, 2018

joshelser commented Mar 21, 2018

laurentgo commented Mar 21, 2018

joshelser commented Mar 21, 2018

risdenk commented Apr 28, 2018

laurentgo commented Apr 28, 2018

laurentgo commented Jun 7, 2018

risdenk commented Jun 7, 2018

risdenk left a comment

risdenk Jun 7, 2018

risdenk Jun 7, 2018

risdenk Jun 7, 2018

risdenk Jun 7, 2018

laurentgo commented Jun 7, 2018

F21 commented Jun 13, 2018

[CALCITE-2219] Check if resource is closed, and throws #33

[CALCITE-2219] Check if resource is closed, and throws #33

Conversation

laurentgo commented Mar 19, 2018

risdenk 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

joshelser commented Mar 21, 2018

laurentgo commented Mar 21, 2018

joshelser commented Mar 21, 2018

risdenk commented Apr 28, 2018

laurentgo commented Apr 28, 2018

laurentgo commented Jun 7, 2018

risdenk commented Jun 7, 2018

risdenk 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

laurentgo commented Jun 7, 2018

F21 commented Jun 13, 2018