geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From nre...@apache.org
Subject [geode] branch feature/GEODE-3781 updated: Refactor long methods and add types
Date Fri, 17 Nov 2017 18:08:50 GMT
This is an automated email from the ASF dual-hosted git repository.

nreich pushed a commit to branch feature/GEODE-3781
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/feature/GEODE-3781 by this push:
     new 7fd92d5  Refactor long methods and add types
7fd92d5 is described below

commit 7fd92d55896b8abc4f24afe09e667893b0df3be3
Author: Nick Reich <nreich@pivotal.io>
AuthorDate: Fri Nov 17 10:08:15 2017 -0800

    Refactor long methods and add types
---
 .../jdbc/internal/ConnectionManager.java           | 158 ++++++++++++---------
 .../geode/connectors/jdbc/internal/SqlHandler.java |  54 +++----
 2 files changed, 115 insertions(+), 97 deletions(-)

diff --git a/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/ConnectionManager.java
b/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/ConnectionManager.java
index 4f4a520..462c51c 100644
--- a/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/ConnectionManager.java
+++ b/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/ConnectionManager.java
@@ -57,6 +57,50 @@ class ConnectionManager {
     return getNewConnection(config);
   }
 
+  <K> List<ColumnValue> getColumnToValueList(ConnectionConfiguration config,
+                                         RegionMapping regionMapping, K key, PdxInstance
value, Operation operation) {
+    String keyColumnName = getKeyColumnName(config, regionMapping.getTableName());
+    ColumnValue keyColumnValue = new ColumnValue(true, keyColumnName, key);
+
+    if (operation.isDestroy() || operation.isGet()) {
+      return Collections.singletonList(keyColumnValue);
+    }
+
+    List<ColumnValue> result = createColumnValueList(regionMapping, value, keyColumnName);
+    result.add(keyColumnValue);
+    return result;
+  }
+
+  void close() {
+    connectionMap.values().forEach(this::close);
+  }
+
+  String getKeyColumnName(ConnectionConfiguration connectionConfig, String tableName) {
+    return tableToPrimaryKeyMap.computeIfAbsent(tableName, k -> computeKeyColumnName(connectionConfig,
k));
+  }
+
+  ConnectionConfiguration getConnectionConfig(String connectionConfigName) {
+    return configService.getConnectionConfig(connectionConfigName);
+  }
+
+  PreparedStatement getPreparedStatement(Connection connection, List<ColumnValue> columnList,
+                                         String tableName, Operation operation, int pdxTypeId)
{
+    PreparedStatementCache statementCache = preparedStatementCache.get();
+
+    if (statementCache == null) {
+      statementCache = new PreparedStatementCache();
+      preparedStatementCache.set(statementCache);
+    }
+
+    return statementCache.getPreparedStatement(connection, columnList, tableName, operation,
+        pdxTypeId);
+  }
+
+  // package protected for testing purposes only
+  Connection getSQLConnection(ConnectionConfiguration config) throws SQLException {
+    return DriverManager.getConnection(config.getUrl(), config.getUser(), config.getPassword());
+  }
+
   private synchronized Connection getNewConnection(ConnectionConfiguration config) {
     Connection connection;
     try {
@@ -69,83 +113,72 @@ class ConnectionManager {
     return connection;
   }
 
-  // package protected for testing purposes only
-  Connection getSQLConnection(ConnectionConfiguration config) throws SQLException {
-    return DriverManager.getConnection(config.getUrl(), config.getUser(), config.getPassword());
-  }
-
-  List<ColumnValue> getColumnToValueList(ConnectionConfiguration config,
-      RegionMapping regionMapping, Object key, PdxInstance value, Operation operation) {
-    String keyColumnName = getKeyColumnName(config, regionMapping.getTableName());
-    ColumnValue keyCV = new ColumnValue(true, keyColumnName, key);
-    if (operation.isDestroy() || operation.isGet()) {
-      return Collections.singletonList(keyCV);
-    }
-
-    List<String> fieldNames = value.getFieldNames();
-    List<ColumnValue> result = new ArrayList<>(fieldNames.size() + 1);
-    for (String fieldName : fieldNames) {
+  private List<ColumnValue> createColumnValueList(RegionMapping regionMapping, PdxInstance
value,
+                                     String keyColumnName) {
+    List<ColumnValue> result = new ArrayList<>();
+    for (String fieldName : value.getFieldNames()) {
       String columnName = regionMapping.getColumnNameForField(fieldName);
       if (columnName.equalsIgnoreCase(keyColumnName)) {
         continue;
       }
-      Object columnValue = value.getField(fieldName);
-      ColumnValue cv = new ColumnValue(false, columnName, columnValue);
-      result.add(cv);
+      ColumnValue columnValue = new ColumnValue(false, columnName, value.getField(fieldName));
+      result.add(columnValue);
     }
-    result.add(keyCV);
     return result;
   }
 
-  String getKeyColumnName(ConnectionConfiguration connectionConfig, String tableName) {
-    return tableToPrimaryKeyMap.computeIfAbsent(tableName, k -> {
-      return computeKeyColumnName(connectionConfig, k);
-    });
-  }
-
   private String computeKeyColumnName(ConnectionConfiguration connectionConfig, String tableName)
{
     // TODO: check config for key column
-    String key;
+    String key = null;
     try {
       Connection connection = getConnection(connectionConfig);
       DatabaseMetaData metaData = connection.getMetaData();
       ResultSet tables = metaData.getTables(null, null, "%", null);
-      String realTableName = null;
-      while (tables.next()) {
-        String name = tables.getString("TABLE_NAME");
-        if (name.equalsIgnoreCase(tableName)) {
-          if (realTableName != null) {
-            throw new IllegalStateException("Duplicate tables that match region name");
-          }
-          realTableName = name;
-        }
-      }
-      if (realTableName == null) {
-        throw new IllegalStateException("no table was found that matches " + tableName);
-      }
-      ResultSet primaryKeys = metaData.getPrimaryKeys(null, null, realTableName);
-      if (!primaryKeys.next()) {
-        throw new IllegalStateException(
-            "The table " + tableName + " does not have a primary key column.");
-      }
-      key = primaryKeys.getString("COLUMN_NAME");
-      if (primaryKeys.next()) {
-        throw new IllegalStateException(
-            "The table " + tableName + " has more than one primary key column.");
-      }
+
+      String realTableName = getTableNameFromMetaData(tableName, tables);
+      key = getPrimaryKeyColumnNameFromMetaData(realTableName, metaData);
+
     } catch (SQLException e) {
-      key = null;
       handleSQLException(e);
     }
     return key;
   }
 
-  private void handleSQLException(SQLException e) {
-    throw new IllegalStateException("NYI: handleSQLException", e);
+  private String getTableNameFromMetaData(String tableName, ResultSet tables) throws SQLException
{
+    String realTableName = null;
+    while (tables.next()) {
+      String name = tables.getString("TABLE_NAME");
+      if (name.equalsIgnoreCase(tableName)) {
+        if (realTableName != null) {
+          throw new IllegalStateException("Duplicate tables that match region name");
+        }
+        realTableName = name;
+      }
+    }
+
+    if (realTableName == null) {
+      throw new IllegalStateException("no table was found that matches " + tableName);
+    }
+    return realTableName;
   }
 
-  void close() {
-    connectionMap.values().forEach(connection -> close(connection));
+  private String getPrimaryKeyColumnNameFromMetaData(String tableName, DatabaseMetaData metaData)
+      throws SQLException {
+    ResultSet primaryKeys = metaData.getPrimaryKeys(null, null, tableName);
+    if (!primaryKeys.next()) {
+      throw new IllegalStateException(
+          "The table " + tableName + " does not have a primary key column.");
+    }
+    String key = primaryKeys.getString("COLUMN_NAME");
+    if (primaryKeys.next()) {
+      throw new IllegalStateException(
+          "The table " + tableName + " has more than one primary key column.");
+    }
+    return key;
+  }
+
+  private void handleSQLException(SQLException e) {
+    throw new IllegalStateException("NYI: handleSQLException", e);
   }
 
   private void close(Connection connection) {
@@ -156,19 +189,4 @@ class ConnectionManager {
       }
     }
   }
-
-  ConnectionConfiguration getConnectionConfig(String connectionConfigName) {
-    return configService.getConnectionConfig(connectionConfigName);
-  }
-
-  PreparedStatement getPreparedStatement(Connection connection, List<ColumnValue> columnList,
-      String tableName, Operation operation, int pdxTypeId) {
-    PreparedStatementCache statementCache = preparedStatementCache.get();
-    if (statementCache == null) {
-      statementCache = new PreparedStatementCache();
-      preparedStatementCache.set(statementCache);
-    }
-    return statementCache.getPreparedStatement(connection, columnList, tableName, operation,
-        pdxTypeId);
-  }
 }
diff --git a/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/SqlHandler.java
b/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/SqlHandler.java
index 13c25de..c61a9db 100644
--- a/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/SqlHandler.java
+++ b/geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/SqlHandler.java
@@ -38,7 +38,7 @@ public class SqlHandler {
     manager.close();
   }
 
-  public PdxInstance read(Region region, Object key) {
+  public <K, V> PdxInstance read(Region<K, V> region, K key) {
     if (key == null) {
       throw new IllegalArgumentException("Key for query cannot be null");
     }
@@ -57,7 +57,7 @@ public class SqlHandler {
     return executeReadStatement(statement, columnList, factory, regionMapping, keyColumnName);
   }
 
-  private PdxInstanceFactory getPdxInstanceFactory(Region region, RegionMapping regionMapping)
{
+  private <K, V> PdxInstanceFactory getPdxInstanceFactory(Region<K, V> region,
RegionMapping regionMapping) {
     InternalCache cache = (InternalCache) region.getRegionService();
     String valueClassName = regionMapping.getPdxClassName();
     PdxInstanceFactory factory;
@@ -75,11 +75,7 @@ public class SqlHandler {
     PdxInstance pdxInstance = null;
     synchronized (statement) {
       try {
-        int idx = 0;
-        for (ColumnValue columnValue : columnList) {
-          idx++;
-          statement.setObject(idx, columnValue.getValue());
-        }
+        setValuesInStatement(statement, columnList);
         ResultSet resultSet = statement.executeQuery();
         if (resultSet.next()) {
 
@@ -109,11 +105,20 @@ public class SqlHandler {
     return pdxInstance;
   }
 
+  private void setValuesInStatement(PreparedStatement statement, List<ColumnValue>
columnList)
+      throws SQLException {
+    int index = 0;
+    for (ColumnValue columnValue : columnList) {
+      index++;
+      statement.setObject(index, columnValue.getValue());
+    }
+  }
+
   private String mapColumnNameToFieldName(String columnName) {
     return columnName.toLowerCase();
   }
 
-  public void write(Region region, Operation operation, Object key, PdxInstance value) {
+  public <K, V> void write(Region<K, V> region, Operation operation, K key, PdxInstance
value) {
     if (value == null && operation != Operation.DESTROY) {
       throw new IllegalArgumentException("PdxInstance cannot be null for non-destroy operations");
     }
@@ -124,53 +129,48 @@ public class SqlHandler {
     List<ColumnValue> columnList =
         manager.getColumnToValueList(connectionConfig, regionMapping, key, value, operation);
 
-    int pdxTypeId = 0;
-    if (value != null) {
-      pdxTypeId = ((PdxInstanceImpl) value).getPdxType().getTypeId();
-    }
+    int pdxTypeId = value == null ? 0 : ((PdxInstanceImpl) value).getPdxType().getTypeId();
     PreparedStatement statement = manager.getPreparedStatement(
         manager.getConnection(connectionConfig), columnList, tableName, operation, pdxTypeId);
     int updateCount = executeWriteStatement(statement, columnList, operation, false);
+
+    // Destroy action not guaranteed to modify any database rows
     if (operation.isDestroy()) {
-      // TODO: should we check updateCount here? Probably not. It is possible we have nothing
in the
-      // table to destroy.
       return;
     }
+
     if (updateCount <= 0) {
-      Operation upsertOp;
-      if (operation.isUpdate()) {
-        upsertOp = Operation.CREATE;
-      } else {
-        upsertOp = Operation.UPDATE;
-      }
+      Operation upsertOp = getOppositeOperation(operation);
       PreparedStatement upsertStatement = manager.getPreparedStatement(
           manager.getConnection(connectionConfig), columnList, tableName, upsertOp, pdxTypeId);
       updateCount = executeWriteStatement(upsertStatement, columnList, upsertOp, true);
     }
+
     if (updateCount != 1) {
       throw new IllegalStateException("Unexpected updateCount " + updateCount);
     }
   }
 
+  private Operation getOppositeOperation(Operation operation) {
+    return operation.isUpdate() ? Operation.CREATE : Operation.UPDATE;
+  }
+
   private int executeWriteStatement(PreparedStatement statement, List<ColumnValue>
columnList,
       Operation operation, boolean handleException) {
+    int updateCount = 0;
     synchronized (statement) {
       try {
-        int idx = 0;
-        for (ColumnValue columnValue : columnList) {
-          idx++;
-          statement.setObject(idx, columnValue.getValue());
-        }
-        return statement.executeUpdate();
+        setValuesInStatement(statement, columnList);
+        updateCount = statement.executeUpdate();
       } catch (SQLException e) {
         if (handleException || operation.isDestroy()) {
           handleSQLException(e);
         }
-        return 0;
       } finally {
         clearStatementParameters(statement);
       }
     }
+    return updateCount;
   }
 
   private void clearStatementParameters(PreparedStatement statement) {

-- 
To stop receiving notification emails like this one, please contact
['"commits@geode.apache.org" <commits@geode.apache.org>'].

Mime
View raw message