Copilot commented on code in PR #17702:
URL:
https://github.com/apache/dolphinscheduler/pull/17702#discussion_r2563431666
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sql/src/test/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTaskTest.java:
##########
@@ -197,4 +205,228 @@ void testVarPoolSetting() {
Assertions.assertEquals("1", varPoolParam.getValue());
Assertions.assertEquals(Direct.OUT, varPoolParam.getDirect());
}
+
+ @Test
+ void
testGenerateEmptyRow_WithNonNullResultSet_ReturnsEmptyValuesForAllColumns()
throws Exception {
+ // Arrange
+ ResultSet mockResultSet = mock(ResultSet.class);
+ ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
+
+ when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
+ when(mockMetaData.getColumnCount()).thenReturn(2);
+ when(mockMetaData.getColumnLabel(1)).thenReturn("id");
+ when(mockMetaData.getColumnLabel(2)).thenReturn("name");
+
+ Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
+ method.setAccessible(true);
+
+ // Act
+ ArrayNode result = (ArrayNode) method.invoke(sqlTask, mockResultSet);
+
+ // Assert
+ Assertions.assertNotNull(result);
+ Assertions.assertEquals(1, result.size());
+
+ ObjectNode row = (ObjectNode) result.get(0);
+ Assertions.assertEquals("", row.get("id").asText());
+ Assertions.assertEquals("", row.get("name").asText());
+ }
+
+ @Test
+ void testGenerateEmptyRow_WithNullResultSet_ReturnsErrorObject() throws
Exception {
+ // Arrange
+ Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
+ method.setAccessible(true);
+
+ // Act
+ ArrayNode result = (ArrayNode) method.invoke(sqlTask, (ResultSet)
null);
+
+ // Assert
+ Assertions.assertNotNull(result);
+ Assertions.assertEquals(1, result.size());
+
+ ObjectNode row = (ObjectNode) result.get(0);
+ Assertions.assertTrue(row.has("error"));
+ Assertions.assertEquals("resultSet is null",
row.get("error").asText());
+ }
Review Comment:
Missing test coverage for `generateEmptyRow` with duplicate column labels.
While the main `resultProcess` method has comprehensive tests for duplicate
columns, the `generateEmptyRow` method (which is called when the ResultSet is
empty) lacks similar test cases. This could lead to inconsistent behavior when
handling empty results with duplicate column names.
Consider adding a test case like:
```java
@Test
void testGenerateEmptyRow_WithDuplicateColumns_DeduplicatesLabels() throws
Exception {
ResultSet mockResultSet = mock(ResultSet.class);
ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
when(mockMetaData.getColumnCount()).thenReturn(3);
when(mockMetaData.getColumnLabel(1)).thenReturn("id");
when(mockMetaData.getColumnLabel(2)).thenReturn("id"); // duplicate
when(mockMetaData.getColumnLabel(3)).thenReturn("name");
Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
method.setAccessible(true);
ArrayNode result = (ArrayNode) method.invoke(sqlTask, mockResultSet);
Assertions.assertNotNull(result);
Assertions.assertEquals(1, result.size());
ObjectNode row = (ObjectNode) result.get(0);
Assertions.assertTrue(row.has("id"));
Assertions.assertTrue(row.has("id_2")); // deduplicated
Assertions.assertTrue(row.has("name"));
}
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sql/src/test/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTaskTest.java:
##########
@@ -197,4 +205,228 @@ void testVarPoolSetting() {
Assertions.assertEquals("1", varPoolParam.getValue());
Assertions.assertEquals(Direct.OUT, varPoolParam.getDirect());
}
+
+ @Test
+ void
testGenerateEmptyRow_WithNonNullResultSet_ReturnsEmptyValuesForAllColumns()
throws Exception {
+ // Arrange
+ ResultSet mockResultSet = mock(ResultSet.class);
+ ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
+
+ when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
+ when(mockMetaData.getColumnCount()).thenReturn(2);
+ when(mockMetaData.getColumnLabel(1)).thenReturn("id");
+ when(mockMetaData.getColumnLabel(2)).thenReturn("name");
+
+ Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
+ method.setAccessible(true);
+
+ // Act
+ ArrayNode result = (ArrayNode) method.invoke(sqlTask, mockResultSet);
+
+ // Assert
+ Assertions.assertNotNull(result);
+ Assertions.assertEquals(1, result.size());
+
+ ObjectNode row = (ObjectNode) result.get(0);
+ Assertions.assertEquals("", row.get("id").asText());
+ Assertions.assertEquals("", row.get("name").asText());
+ }
+
+ @Test
+ void testGenerateEmptyRow_WithNullResultSet_ReturnsErrorObject() throws
Exception {
+ // Arrange
+ Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
+ method.setAccessible(true);
+
+ // Act
+ ArrayNode result = (ArrayNode) method.invoke(sqlTask, (ResultSet)
null);
+
+ // Assert
+ Assertions.assertNotNull(result);
+ Assertions.assertEquals(1, result.size());
+
+ ObjectNode row = (ObjectNode) result.get(0);
+ Assertions.assertTrue(row.has("error"));
+ Assertions.assertEquals("resultSet is null",
row.get("error").asText());
+ }
+
+ @Test
+ void testResultProcess_NullResultSet_ReturnsEmptyResult() throws Exception
{
+ Method resultProcessMethod =
SqlTask.class.getDeclaredMethod("resultProcess", ResultSet.class);
+ resultProcessMethod.setAccessible(true);
+
+ // Mock a null ResultSet
+ String result = (String) resultProcessMethod.invoke(sqlTask,
(ResultSet) null);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.equalsIgnoreCase("[{\"error\":\"resultSet
is null\"}]"));
+ }
+
+ @Test
+ void testResultProcess_EmptyResultSet_ReturnsEmptyResult() throws
Exception {
+ // Mock a non-null ResultSet that contains no data rows
+ ResultSet mockResultSet = mock(ResultSet.class);
+ ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
+
+ when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
+ when(mockMetaData.getColumnCount()).thenReturn(2);
+ when(mockMetaData.getColumnLabel(1)).thenReturn("id");
+ when(mockMetaData.getColumnLabel(2)).thenReturn("name");
+ when(mockResultSet.next()).thenReturn(false); // no rows available
+
+ Method resultProcessMethod =
SqlTask.class.getDeclaredMethod("resultProcess", ResultSet.class);
+ resultProcessMethod.setAccessible(true);
+
+ String result = (String) resultProcessMethod.invoke(sqlTask,
mockResultSet);
+
+ Assertions.assertNotNull(result);
+ // Verify the result contains empty string values for all columns and
is a valid JSON array
+ Assertions.assertTrue(result.contains("\"id\":\"\""));
+ Assertions.assertTrue(result.contains("\"name\":\"\""));
+ Assertions.assertTrue(result.startsWith("[{"));
+ Assertions.assertTrue(result.endsWith("}]"));
+ }
+
+ @Test
+ void testResultProcess_ColumnLabelIsNull_UsesGenericName() throws
Exception {
+ // Mock ResultSet with a null column label
+ ResultSet mockRs = mock(ResultSet.class);
+ ResultSetMetaData mockMd = mock(ResultSetMetaData.class);
+
+ when(mockRs.getMetaData()).thenReturn(mockMd);
+ when(mockMd.getColumnCount()).thenReturn(1);
+ when(mockMd.getColumnLabel(1)).thenReturn(null); // null label
+ when(mockRs.next()).thenReturn(true, false);
+ when(mockRs.getObject(1)).thenReturn("value1");
+
+ Method method = SqlTask.class.getDeclaredMethod("resultProcess",
ResultSet.class);
+ method.setAccessible(true);
+
+ String result = (String) method.invoke(sqlTask, mockRs);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.contains("\"col_1\":\"value1\""));
+ }
+
+ @Test
+ void testResultProcess_ColumnLabelIsEmpty_UsesGenericName() throws
Exception {
+ // Mock ResultSet with an empty column label
+ ResultSet mockRs = mock(ResultSet.class);
+ ResultSetMetaData mockMd = mock(ResultSetMetaData.class);
+
+ when(mockRs.getMetaData()).thenReturn(mockMd);
+ when(mockMd.getColumnCount()).thenReturn(1);
+ when(mockMd.getColumnLabel(1)).thenReturn(""); // empty label
+ when(mockRs.next()).thenReturn(true, false);
+ when(mockRs.getObject(1)).thenReturn("value1");
+
+ Method method = SqlTask.class.getDeclaredMethod("resultProcess",
ResultSet.class);
+ method.setAccessible(true);
+
+ String result = (String) method.invoke(sqlTask, mockRs);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.contains("\"col_1\":\"value1\""));
+ }
+
+ @Test
+ void testResultProcess_SingleRowWithDuplicateColumns_GeneratesUniqueKeys()
throws Exception {
+ ResultSet mockResultSet = mock(ResultSet.class);
+ ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
+
+ when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
+ when(mockMetaData.getColumnCount()).thenReturn(3);
+ when(mockMetaData.getColumnLabel(1)).thenReturn("name");
+ when(mockMetaData.getColumnLabel(2)).thenReturn("name"); // duplicate
+ when(mockMetaData.getColumnLabel(3)).thenReturn("age");
+
+ when(mockResultSet.next())
+ .thenReturn(true) // first row
+ .thenReturn(false); // no more rows
+
+ when(mockResultSet.getObject(1)).thenReturn("Alice");
+ when(mockResultSet.getObject(2)).thenReturn("Bob");
+ when(mockResultSet.getObject(3)).thenReturn(30);
+
+ Method resultProcessMethod =
SqlTask.class.getDeclaredMethod("resultProcess", ResultSet.class);
+ resultProcessMethod.setAccessible(true);
+
+ String result = (String) resultProcessMethod.invoke(sqlTask,
mockResultSet);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.contains("\"name\":\"Alice\""));
+ Assertions.assertTrue(result.contains("\"name_2\":\"Bob\"")); //
deduplicated
+ Assertions.assertTrue(result.contains("\"age\":30"));
+ Assertions.assertTrue(result.startsWith("[{"));
+ Assertions.assertTrue(result.endsWith("}]"));
+ }
+
+ @Test
+ void testResultProcess_DuplicateColumnLabels_AreDeduplicated() throws
Exception {
+ // Mock ResultSet with three columns all named "id"
+ ResultSet mockRs = mock(ResultSet.class);
+ ResultSetMetaData mockMd = mock(ResultSetMetaData.class);
+
+ when(mockRs.getMetaData()).thenReturn(mockMd);
+ when(mockMd.getColumnCount()).thenReturn(3);
+ when(mockMd.getColumnLabel(1)).thenReturn("id");
+ when(mockMd.getColumnLabel(2)).thenReturn("id"); // duplicate
+ when(mockMd.getColumnLabel(3)).thenReturn("id"); // duplicate again
+
+ when(mockRs.next()).thenReturn(true, false);
+ when(mockRs.getObject(1)).thenReturn(101);
+ when(mockRs.getObject(2)).thenReturn(102);
+ when(mockRs.getObject(3)).thenReturn(103);
+
+ Method method = SqlTask.class.getDeclaredMethod("resultProcess",
ResultSet.class);
+ method.setAccessible(true);
+
+ String result = (String) method.invoke(sqlTask, mockRs);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.contains("\"id\":101"));
+ Assertions.assertTrue(result.contains("\"id_2\":102"));
+ Assertions.assertTrue(result.contains("\"id_3\":103"));
+ }
+
+ @Test
+ void
testResultProcess_ColumnNames_IdIdId2Id2Id3_HandlesSuffixCollisionCorrectly()
throws Exception {
Review Comment:
[nitpick] The test name
`testResultProcess_ColumnNames_IdIdId2Id2Id3_HandlesSuffixCollisionCorrectly`
is quite long and could be simplified. Consider renaming to
`testResultProcess_HandlesSuffixCollisionWithDuplicateColumns` for better
readability while maintaining clarity about what is being tested.
```suggestion
void testResultProcess_HandlesSuffixCollisionWithDuplicateColumns()
throws Exception {
```
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-sql/src/test/java/org/apache/dolphinscheduler/plugin/task/sql/SqlTaskTest.java:
##########
@@ -197,4 +205,228 @@ void testVarPoolSetting() {
Assertions.assertEquals("1", varPoolParam.getValue());
Assertions.assertEquals(Direct.OUT, varPoolParam.getDirect());
}
+
+ @Test
+ void
testGenerateEmptyRow_WithNonNullResultSet_ReturnsEmptyValuesForAllColumns()
throws Exception {
+ // Arrange
+ ResultSet mockResultSet = mock(ResultSet.class);
+ ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
+
+ when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
+ when(mockMetaData.getColumnCount()).thenReturn(2);
+ when(mockMetaData.getColumnLabel(1)).thenReturn("id");
+ when(mockMetaData.getColumnLabel(2)).thenReturn("name");
+
+ Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
+ method.setAccessible(true);
+
+ // Act
+ ArrayNode result = (ArrayNode) method.invoke(sqlTask, mockResultSet);
+
+ // Assert
+ Assertions.assertNotNull(result);
+ Assertions.assertEquals(1, result.size());
+
+ ObjectNode row = (ObjectNode) result.get(0);
+ Assertions.assertEquals("", row.get("id").asText());
+ Assertions.assertEquals("", row.get("name").asText());
+ }
+
+ @Test
+ void testGenerateEmptyRow_WithNullResultSet_ReturnsErrorObject() throws
Exception {
+ // Arrange
+ Method method = SqlTask.class.getDeclaredMethod("generateEmptyRow",
ResultSet.class);
+ method.setAccessible(true);
+
+ // Act
+ ArrayNode result = (ArrayNode) method.invoke(sqlTask, (ResultSet)
null);
+
+ // Assert
+ Assertions.assertNotNull(result);
+ Assertions.assertEquals(1, result.size());
+
+ ObjectNode row = (ObjectNode) result.get(0);
+ Assertions.assertTrue(row.has("error"));
+ Assertions.assertEquals("resultSet is null",
row.get("error").asText());
+ }
+
+ @Test
+ void testResultProcess_NullResultSet_ReturnsEmptyResult() throws Exception
{
+ Method resultProcessMethod =
SqlTask.class.getDeclaredMethod("resultProcess", ResultSet.class);
+ resultProcessMethod.setAccessible(true);
+
+ // Mock a null ResultSet
+ String result = (String) resultProcessMethod.invoke(sqlTask,
(ResultSet) null);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.equalsIgnoreCase("[{\"error\":\"resultSet
is null\"}]"));
+ }
+
+ @Test
+ void testResultProcess_EmptyResultSet_ReturnsEmptyResult() throws
Exception {
+ // Mock a non-null ResultSet that contains no data rows
+ ResultSet mockResultSet = mock(ResultSet.class);
+ ResultSetMetaData mockMetaData = mock(ResultSetMetaData.class);
+
+ when(mockResultSet.getMetaData()).thenReturn(mockMetaData);
+ when(mockMetaData.getColumnCount()).thenReturn(2);
+ when(mockMetaData.getColumnLabel(1)).thenReturn("id");
+ when(mockMetaData.getColumnLabel(2)).thenReturn("name");
+ when(mockResultSet.next()).thenReturn(false); // no rows available
+
+ Method resultProcessMethod =
SqlTask.class.getDeclaredMethod("resultProcess", ResultSet.class);
+ resultProcessMethod.setAccessible(true);
+
+ String result = (String) resultProcessMethod.invoke(sqlTask,
mockResultSet);
+
+ Assertions.assertNotNull(result);
+ // Verify the result contains empty string values for all columns and
is a valid JSON array
+ Assertions.assertTrue(result.contains("\"id\":\"\""));
+ Assertions.assertTrue(result.contains("\"name\":\"\""));
+ Assertions.assertTrue(result.startsWith("[{"));
+ Assertions.assertTrue(result.endsWith("}]"));
+ }
+
+ @Test
+ void testResultProcess_ColumnLabelIsNull_UsesGenericName() throws
Exception {
+ // Mock ResultSet with a null column label
+ ResultSet mockRs = mock(ResultSet.class);
+ ResultSetMetaData mockMd = mock(ResultSetMetaData.class);
+
+ when(mockRs.getMetaData()).thenReturn(mockMd);
+ when(mockMd.getColumnCount()).thenReturn(1);
+ when(mockMd.getColumnLabel(1)).thenReturn(null); // null label
+ when(mockRs.next()).thenReturn(true, false);
+ when(mockRs.getObject(1)).thenReturn("value1");
+
+ Method method = SqlTask.class.getDeclaredMethod("resultProcess",
ResultSet.class);
+ method.setAccessible(true);
+
+ String result = (String) method.invoke(sqlTask, mockRs);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.contains("\"col_1\":\"value1\""));
+ }
+
+ @Test
+ void testResultProcess_ColumnLabelIsEmpty_UsesGenericName() throws
Exception {
+ // Mock ResultSet with an empty column label
+ ResultSet mockRs = mock(ResultSet.class);
+ ResultSetMetaData mockMd = mock(ResultSetMetaData.class);
+
+ when(mockRs.getMetaData()).thenReturn(mockMd);
+ when(mockMd.getColumnCount()).thenReturn(1);
+ when(mockMd.getColumnLabel(1)).thenReturn(""); // empty label
+ when(mockRs.next()).thenReturn(true, false);
+ when(mockRs.getObject(1)).thenReturn("value1");
+
+ Method method = SqlTask.class.getDeclaredMethod("resultProcess",
ResultSet.class);
+ method.setAccessible(true);
+
+ String result = (String) method.invoke(sqlTask, mockRs);
+
+ Assertions.assertNotNull(result);
+ Assertions.assertTrue(result.contains("\"col_1\":\"value1\""));
+ }
Review Comment:
Missing test coverage for multiple null or empty column labels. While there
are tests for single null/empty columns that verify the fallback to "col_1",
there's no test to verify that multiple null/empty columns get unique names
(col_1, col_2, col_3, etc.).
Consider adding a test case:
```java
@Test
void
testResultProcess_MultipleNullColumnLabels_GeneratesUniqueGenericNames() throws
Exception {
ResultSet mockRs = mock(ResultSet.class);
ResultSetMetaData mockMd = mock(ResultSetMetaData.class);
when(mockRs.getMetaData()).thenReturn(mockMd);
when(mockMd.getColumnCount()).thenReturn(3);
when(mockMd.getColumnLabel(1)).thenReturn(null);
when(mockMd.getColumnLabel(2)).thenReturn("");
when(mockMd.getColumnLabel(3)).thenReturn(null);
when(mockRs.next()).thenReturn(true, false);
when(mockRs.getObject(1)).thenReturn("val1");
when(mockRs.getObject(2)).thenReturn("val2");
when(mockRs.getObject(3)).thenReturn("val3");
Method method = SqlTask.class.getDeclaredMethod("resultProcess",
ResultSet.class);
method.setAccessible(true);
String result = (String) method.invoke(sqlTask, mockRs);
Assertions.assertNotNull(result);
Assertions.assertTrue(result.contains("\"col_1\":\"val1\""));
Assertions.assertTrue(result.contains("\"col_2\":\"val2\""));
Assertions.assertTrue(result.contains("\"col_3\":\"val3\""));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
