ISSUE-680 # Developer review and coverage check

Author: javiertuya

core/src/main/java/org/jsmart/zerocode/core/db/DbCsvLoader.java

@@ -15,6 +15,9 @@
 
 import com.univocity.parsers.csv.CsvParser;
 
+/**
+ * Data loading in the database from a CSV external source
+ */
 class DbCsvLoader {
 	private static final Logger LOGGER = LoggerFactory.getLogger(DbCsvLoader.class);
 	private Connection conn;
@@ -26,10 +29,10 @@ public DbCsvLoader(Connection conn, CsvParser csvParser) {
 	}
 
 	/**
-	 * Loads rows in csv format (csvLines) into a table in the database
-	 * and returns the total number of rows
+	 * Loads rows in CSV format (csvLines) into a table in the database
+	 * and returns the total number of rows.
 	 */
-	int loadCsv(String table, List<String> csvLines, boolean withHeaders, String nullString) throws SQLException {
+	public int loadCsv(String table, List<String> csvLines, boolean withHeaders, String nullString) throws SQLException {
 		if (csvLines == null || csvLines.isEmpty())
 			return 0;
 
@@ -87,7 +90,7 @@ private List<Object[]> buildParameters(String table, String[] headers, List<Stri
 				LOGGER.info("    row [{}] params: {}", i + 1, Arrays.asList(params).toString());
 			} catch (Exception e) { // Not only SQLException as converter also does parsing
 				String message = String.format("Error matching data type of parameters and table columns at CSV row %d", i + 1);
-				LOGGER.error(message); // do not log the exception because it will be logged by the parent executor (DbCsvLoader)
+				LOGGER.error(message);
 				LOGGER.error("Exception message: {}", e.getMessage());
 				throw new RuntimeException(message, e);
 			}
@@ -123,7 +126,7 @@ private void insertRow(QueryRunner runner, int rowId, String sql, Object[] param
 			runner.update(conn, sql, params);
 		} catch (SQLException e) {
 			String message = String.format("Error inserting data at CSV row %d", rowId + 1);
-			LOGGER.error(message); // do not log the exception because it will be logged by the parent executor (DbCsvLoader)
+			LOGGER.error(message);
 			LOGGER.error("Exception message: {}", e.getMessage());
 			throw new RuntimeException(message, e);
 		}

core/src/main/java/org/jsmart/zerocode/core/db/DbCsvRequest.java

@@ -50,7 +50,7 @@ public String getNullString() {
     }
 
     // Code below is duplicated from org.jsmart.zerocode.core.domain.Parametrized.java and not included in tests.
-    // TODO Consider some refactoring later (to SmartUtils?) and review error message when file not found
+    // TODO Consider some refactoring later and review error message when file not found
 
     private List<String> getCsvSourceFrom(JsonNode csvSourceJsonNode) {
         try {

core/src/main/java/org/jsmart/zerocode/core/db/DbSqlExecutor.java

@@ -25,7 +25,8 @@ public class DbSqlExecutor {
 	public static final String SQL_RESULTS_KEY = "rows";
 	public static final String CSV_RESULTS_KEY = "size";
 
-	@Inject
+	// Optional to log the explanatory error message if the env variables are no defined
+	@Inject(optional = true)
 	@Named("db.driver.url") private String url;
 
 	@Inject(optional = true)
@@ -56,16 +57,17 @@ public Map<String, Object> loadcsv(DbCsvRequest request) {
 			response.put(CSV_RESULTS_KEY, result);
 			return response;
 		} catch (Exception e) {
-			LOGGER.error("Failed to load CSV", e);
-			throw new RuntimeException(e);
+			String message = "Failed to load CSV";
+			LOGGER.error(message, e);
+			throw new RuntimeException(message, e);
 		} finally {
 			closeConnection(conn);
 		}
 	}
 
 	/**
 	 * The EXECUTE operation returns the records retrieved by the SQL specified in the request 
-	 * under the key "rows" (select) or an empty object (insert, update)
+	 * under the key "rows" (select), or an empty object (insert, update)
 	 */
 	public Map<String, Object> EXECUTE(DbSqlRequest request) {
 		return execute(request);
@@ -85,8 +87,9 @@ public Map<String, Object> execute(DbSqlRequest request) {
 			}
 			return response;
 		} catch (SQLException e) {
-			LOGGER.error("Failed to execute SQL", e);
-			throw new RuntimeException(e);
+			String message = "Failed to execute SQL";
+			LOGGER.error(message, e);
+			throw new RuntimeException(message, e);
 		} finally {
 			closeConnection(conn);
 		}
@@ -102,8 +105,10 @@ protected Connection createAndGetConnection() {
 		try {
 			return DriverManager.getConnection(url, user, password);
 		} catch (SQLException e) {
-			LOGGER.error("Failed to create connection", e);
-			throw new RuntimeException(e);
+			String message = "Failed to create connection, Please check the target environment properties "
+					+ "to connect the database (db.driver.url, db.driver.user and db.driver.password)";
+			LOGGER.error(message, e);
+			throw new RuntimeException(message, e);
 		}
 	}
 

core/src/main/java/org/jsmart/zerocode/core/db/DbSqlRequest.java

@@ -12,7 +12,8 @@ public class DbSqlRequest {
 	private final Object[] sqlParams;
 
 	@JsonCreator
-	public DbSqlRequest(@JsonProperty("sqlStatement") String sql,
+	public DbSqlRequest(
+			@JsonProperty("sql") String sql,
 			@JsonProperty("sqlParams") Object[] sqlParams) {
 		this.sql = sql;
 		this.sqlParams = sqlParams;

core/src/main/java/org/jsmart/zerocode/core/db/DbSqlRunner.java

@@ -10,6 +10,9 @@
 import java.util.List;
 import java.util.Map;
 
+/**
+ * Execution of SQL statements against a database
+ */
 class DbSqlRunner {
 	private static final Logger LOGGER = LoggerFactory.getLogger(DbSqlRunner.class);
 	private Connection conn;
@@ -19,14 +22,14 @@ public DbSqlRunner(Connection conn) {
 	}
 
 	/**
-	 * Execute an sql with parameters (optional) and returns a list of maps 
+	 * Executes a SQL statement with parameters (optional) and returns a list of maps 
 	 * with the ResultSet content (select) or null (insert, update)
 	 */
-	List<Map<String, Object>> execute(String sql, Object[] params) throws SQLException {
-		// As there is only one execute operation instead of separate update and query, 
-		// the DbUtils execute method returns a list containing each ResultSet (each is a list of maps):
+	public List<Map<String, Object>> execute(String sql, Object[] params) throws SQLException {
+		// There is only one execute operation instead of separate update and query.
+		// The DbUtils execute method returns a list containing each ResultSet (each is a list of maps):
 		// - Empty (insert and update)
-		// - With one or more ResultSets (select).
+		// - With one or more ResultSets (select): use the first one
 		// - Note that some drivers never return more than one ResultSet (e.g. H2)
 		QueryRunner runner = new QueryRunner();
 		List<List<Map<String, Object>>> result = runner.execute(conn, sql, new MapListHandler(), params);

core/src/main/java/org/jsmart/zerocode/core/db/DbValueConverter.java

@@ -10,12 +10,13 @@
 import java.util.LinkedHashMap;
 import java.util.Map;
 
+import org.apache.commons.lang3.ArrayUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
  * Conversion of string values to be inserted in the database
- * into objects compatible with the java.sql type of the target columns.
+ * into objects compatible with the java sql type of the target columns.
  */
 public class DbValueConverter {
 	private static final Logger LOGGER = LoggerFactory.getLogger(DbSqlExecutor.class);
@@ -58,9 +59,9 @@ private void initializeMetadata() throws SQLException {
 	}
 
 	private String convertToStoredCase(String identifier) throws SQLException {
-		if (databaseMetaData.storesLowerCaseIdentifiers())
+		if (databaseMetaData.storesLowerCaseIdentifiers()) // e.g. Postgres
 			identifier = identifier.toLowerCase();
-		else if (databaseMetaData.storesUpperCaseIdentifiers())
+		else if (databaseMetaData.storesUpperCaseIdentifiers()) // e.g. H2
 			identifier = identifier.toUpperCase();
 		return identifier;
 	}
@@ -71,12 +72,12 @@ private void logInitializeError() {
 	}
 
 	/**
-	 * Given an array of column names and their corresponding values (as strings)
+	 * Given an array of column names and other array with their corresponding values (as strings),
 	 * transforms each value to the compatible data type that allow to be inserted in the database.
-	 * If the column names are missing, uses all columns in the current table as fallback.
+	 * If the column names are missing, uses all columns in the current table as the column names.
 	 */
 	Object[] convertColumnValues(String[] columns, String[] values) {
-		if (columns == null || columns.length == 0) // if no specified, use all columns in the table
+		if (ArrayUtils.isEmpty(columns)) // if no specified, use all columns in the table
 			columns = columnTypes.keySet().toArray(new String[0]);
 
 		Object[] converted = new Object[values.length];
@@ -98,8 +99,8 @@ private Object convertColumnValue(String column, String value) {
 	}
 
 	/**
-	 * Converts the string representation of a data type value into the appropriate simple sql data type.
-	 * If a data type is not handled by this method, returns the input value as fallback.
+	 * Converts the string representation of a data type value into the appropriate simple SQL data type.
+	 * If a data type is not handled by this method (or is string), returns the input string value as fallback.
 	 * 
 	 * See table B-1 in JDBC 4.2 Specification
 	 */
@@ -135,21 +136,26 @@ private Object convertColumnValueFromJavaSqlType(int sqlType, String value) thro
 		case java.sql.Types.TIMESTAMP: return new java.sql.Timestamp(parseDate(value, getTimestampFormats()).getTime());
 		default:
 			return value;
+			// Not including: binary and advanced datatypes (e.g. blob, array...)
 		}
 	}
 
-	// Currently, supported date time formats are a few common ISO-8601 formats
-	// (other common format strings in org.apache.commons.lang3.time.DateFormatUtils)
+	// Supported date time formats are the common ISO-8601 formats
+	// defined in org.apache.commons.lang3.time.DateFormatUtils,
+	// as well as their variants that specify milliseconds.
 	// This may be made user configurable later, via properties and/or embedded in the payload
 
 	private String[] getDateFormats() {
-		return new String[] {"yyyy-MM-dd"};
+		return new String[] { "yyyy-MM-dd" };
 	}
+
 	private String[] getTimeFormats() {
-		return new String[] {"HH:mm:ssZ", "HH:mm:ss.SSSZ"};
+		return new String[] { "HH:mm:ssZ", "HH:mm:ss.SSSZ", "HH:mm:ss", "HH:mm:ss.SSS" };
 	}
+
 	private String[] getTimestampFormats() {
-		return new String[] {"yyyy-MM-dd'T'HH:mm:ssZ", "yyyy-MM-dd'T'HH:mm:ss.SSSZ"};
+		return new String[] { "yyyy-MM-dd'T'HH:mm:ssZ", "yyyy-MM-dd'T'HH:mm:ss.SSSZ", 
+				"yyyy-MM-dd'T'HH:mm:ss", "yyyy-MM-dd'T'HH:mm:ss.SSS" };
 	}
 
 }

core/src/test/java/org/jsmart/zerocode/core/db/DbSqlExecutorScenarioTest.java

@@ -9,6 +9,9 @@
 @RunWith(ZeroCodeUnitRunner.class)
 public class DbSqlExecutorScenarioTest {
 
+	// NOTE: Below tests will fail when run in postgres because this database stores identifiers in lowercase.
+	// to make tests pass, change the keys in the response.rows to lowercase
+	
     @Test
 	@Scenario("integration_test_files/db/db_csv_load_with_headers.json")
 	public void testDbCsvLoadWithHeaders() throws Exception {
@@ -24,4 +27,17 @@ public void testDbCsvLoadWithoutHeaders() throws Exception {
 	public void testDbSqlExecute() throws Exception {
 	}
 
+    // Manual test: error handling.
+    // To facilitate the location of the source of possible errors (e.g. a wrong SQL statement),
+    // exceptions that occur in the DbSqlExecutor should show:
+    // - A log entry with the error message
+    // - The stacktrace of the exception to facilitate locating the source
+    // - The usual chain of errors and stacktraces produced by zerocode when an step fails
+    //
+    // Recommended situations for manual test:
+    // - Target environment variables are no defined
+    // - A syntactically wrong SQL statement in a step
+    // - A header that does not correspond with any column when loading data from CSV
+    // - A value with the wrong data type (e.g. string in a numeric column) when loading data from CSV
+    
 }

core/src/test/java/org/jsmart/zerocode/core/db/DbTestBase.java

@@ -39,7 +39,7 @@ protected void configureTest() {
 	@Named("db.driver.password") protected String password;
 
 	protected Connection conn; // managed connection for each test
-	protected boolean isPostgres = false; // set by each connection, to allow portable assertions (postgres is lowercase)
+	protected boolean isPostgres = false; // set by each connection, to allow portable assertions
 
 	@Before
 	public void setUp() throws SQLException {
@@ -61,6 +61,8 @@ protected List<Map<String, Object>> execute(String sql, Object[] params) throws
 		return runner.execute(sql, params);
 	}
 
+	// Table and columns in all tests are uppercase because H2 stores uppercase by default.
+	// But postgres stores lowercase, so some expected strings need case conversion
 	protected String convertDbCase(String value) {
 		return isPostgres ? value.toLowerCase() : value;
 	}

core/src/test/java/org/jsmart/zerocode/core/db/DbValueConverterTest.java

@@ -57,8 +57,9 @@ public void convertDateAndTimeValues() throws SQLException {
 	}
 
 	@Test
-	public void convertWithLowerAndUppercase() throws SQLException {
-		// Test date types to ensure that is the converter who makes the conversion, not the driver
+	public void convertWithMixedCaseColumnName() throws SQLException {
+		// Uses a date type to ensure that is the converter who tries making the conversion, not the driver
+		// (neither H2 nor Postgres drivers do conversion of dates)
 		List<Map<String, Object>> rows = doTestConversion("", "ITable", "VDATE DATE",
 				new String[] { "VDate" }, 
 				new String[] { "2024-09-04" },
@@ -74,6 +75,16 @@ public void whenNoColumnsSpecified_ThenAllTableColumns_AreIncluded() throws SQLE
 				"[{VINT=101, VCHAR=astring}]");
 	}
 
+	@Test
+	public void whenNoColumnsSpecified_AndColumnCountMismatch_ThenConversionFails() throws SQLException {
+		assertThrows(SQLException.class, () -> {
+			doTestConversion("", "FMTABLE", "VINT BIGINT", 
+				null, 
+				new String[] { "101", "999" },
+				"[{VINT=101}]");
+		});
+	}
+
 	@Test
 	public void whenColumnNotFound_ThenConversionFails() throws SQLException {
 		assertThrows(SQLException.class, () -> {
@@ -84,8 +95,20 @@ public void whenColumnNotFound_ThenConversionFails() throws SQLException {
 		});
 	}
 
-	// Failures due to problems with metadata:
-	// - These tests will pass because the H2 driver converts numeric values
+	@Test
+	public void whenValueHasWrongFormat_ThenConversionFails() throws SQLException {
+		assertThrows(SQLException.class, () -> {
+			doTestConversion("", "FVTABLE", "VTS TIMESTAMP", 
+					new String[] { "VTS" },
+					new String[] { "notadate" }, 
+					"[{VTS=notadate}]");
+		});
+	}
+	
+	// Failures due to problems with metadata.
+	// Simulates failures getting metadata so that the conversion is left to
+	// be done by the driver.
+	// - Below tests will pass because the H2 driver converts numeric values
 	// - but fail if driver is changed to Postgres (does not convert numeric), skipped
 
 	@Test

core/src/test/resources/db_test.properties

@@ -7,9 +7,12 @@ db.driver.url=jdbc:h2:./target/test_db_sql_executor
 # db.driver.password=
 
 # To run the tests with postgres:
-# - run container: docker run --name zerocode-postgres -p:5432:5432 -e POSTGRES_PASSWORD=mypassword -d postgres
+# - Spin up the DB container (this was tested with the below versions #680, latest version is the recommended)
+#     docker run --name zerocode-postgres -p:5432:5432 -e POSTGRES_PASSWORD=mypassword -d postgres:17.0
+#     docker run --name zerocode-postgres -p:5432:5432 -e POSTGRES_PASSWORD=mypassword -d postgres:12.0
+#     docker run --name zerocode-postgres -p:5432:5432 -e POSTGRES_PASSWORD=mypassword -d postgres:9.0
 # - add the driver dependency to the pom.xml: https://central.sonatype.com/artifact/org.postgresql/postgresql
-# - and uncomment these properties
+# - and uncomment the properties below:
 # db.driver.url=jdbc:postgresql://localhost:5432/postgres
 # db.driver.user=postgres
 # db.driver.password=mypassword