Copilot commented on code in PR #37560:
URL: https://github.com/apache/shardingsphere/pull/37560#discussion_r2649611905
##########
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/DatabaseProxyConnectorFactoryTest.java:
##########
@@ -114,4 +139,10 @@ private ShardingSphereDatabase mockDatabase() {
when(result.getProtocolType()).thenReturn(databaseType);
return result;
}
+
+ private static Stream<Arguments> driverTypeProvider() {
+ return Stream.of(
+ Arguments.of(true, Collections.emptyList(),
JDBCDriverType.PREPARED_STATEMENT),
+ Arguments.of(false, Collections.singletonList(1),
JDBCDriverType.PREPARED_STATEMENT));
+ }
Review Comment:
The test parameters in the driverTypeProvider method are testing two
different scenarios (preferPreparedStatement = true with empty parameters, and
preferPreparedStatement = false with non-empty parameters), but both expect the
same result (JDBCDriverType.PREPARED_STATEMENT). This suggests that the test is
not actually verifying the driver type selection logic effectively. Consider
adding a test case that expects JDBCDriverType.STATEMENT to validate that the
logic correctly differentiates between the two driver types.
##########
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/ral/queryable/export/ExportDatabaseConfigurationExecutorTest.java:
##########
@@ -82,28 +98,64 @@ private Map<String, StorageUnit> createStorageUnits() {
.collect(Collectors.toMap(Entry::getKey, entry ->
DataSourcePoolPropertiesCreator.create(entry.getValue()), (oldValue,
currentValue) -> oldValue, LinkedHashMap::new));
Map<String, StorageUnit> result = new LinkedHashMap<>(propsMap.size(),
1F);
for (Entry<String, DataSourcePoolProperties> entry :
propsMap.entrySet()) {
- StorageUnit storageUnit = mock(StorageUnit.class,
RETURNS_DEEP_STUBS);
+ StorageUnit storageUnit = mock(StorageUnit.class);
when(storageUnit.getDataSourcePoolProperties()).thenReturn(entry.getValue());
result.put(entry.getKey(), storageUnit);
}
return result;
}
+ private ShardingRuleConfiguration createShardingRuleConfiguration() {
+ ShardingRuleConfiguration result = new ShardingRuleConfiguration();
+ result.getTables().add(createTableRuleConfiguration());
+ result.setDefaultDatabaseShardingStrategy(new
StandardShardingStrategyConfiguration("order_id", "ds_inline"));
+ result.setDefaultTableShardingStrategy(new
NoneShardingStrategyConfiguration());
+ result.getKeyGenerators().put("snowflake", new
AlgorithmConfiguration("SNOWFLAKE", new Properties()));
+ result.getShardingAlgorithms().put("ds_inline", new
AlgorithmConfiguration("INLINE", PropertiesBuilder.build(new
Property("algorithm-expression", "ds_${order_id % 2}"))));
+ return result;
+ }
+
+ private ShardingTableRuleConfiguration createTableRuleConfiguration() {
+ ShardingTableRuleConfiguration result = new
ShardingTableRuleConfiguration("t_order", "ds_${0..1}.t_order_${0..1}");
+ result.setKeyGenerateStrategy(new
KeyGenerateStrategyConfiguration("order_id", "snowflake"));
+ return result;
+ }
+
+ @SneakyThrows({IOException.class, URISyntaxException.class})
+ private String loadExpectedRow() {
+ URL url =
Objects.requireNonNull(ExportDatabaseConfigurationExecutorTest.class.getResource("/expected/export-database-configuration.yaml"));
+ return Files.readAllLines(Paths.get(url.toURI())).stream().filter(each
-> !each.startsWith("#") &&
!each.trim().isEmpty()).collect(Collectors.joining(System.lineSeparator()))
+ + System.lineSeparator();
+ }
+
@Test
void assertExecuteWithEmptyDatabase() {
ShardingSphereDatabase database = mock(ShardingSphereDatabase.class,
RETURNS_DEEP_STUBS);
when(database.getName()).thenReturn("empty_db");
when(database.getResourceMetaData().getStorageUnits()).thenReturn(Collections.emptyMap());
when(database.getRuleMetaData().getConfigurations()).thenReturn(Collections.emptyList());
- ExportDatabaseConfigurationStatement sqlStatement = new
ExportDatabaseConfigurationStatement(null, new FromDatabaseSegment(0, new
DatabaseSegment(0, 0, new IdentifierValue("empty_db"))));
- ExportDatabaseConfigurationExecutor executor = new
ExportDatabaseConfigurationExecutor();
executor.setDatabase(database);
- Collection<LocalDataQueryResultRow> actual =
executor.getRows(sqlStatement, mock(ContextManager.class));
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(
+ new ExportDatabaseConfigurationStatement(null, new
FromDatabaseSegment(0, new DatabaseSegment(0, 0, new
IdentifierValue("empty_db")))), mock(ContextManager.class));
assertThat(actual.size(), is(1));
LocalDataQueryResultRow row = actual.iterator().next();
assertThat(row.getCell(1), is("databaseName: empty_db" +
System.lineSeparator()));
}
+ @Test
+ void assertExecuteWithFilePath() throws IOException {
+ ShardingSphereDatabase database = mock(ShardingSphereDatabase.class,
RETURNS_DEEP_STUBS);
+ when(database.getName()).thenReturn("file_db");
+
when(database.getResourceMetaData().getStorageUnits()).thenReturn(Collections.emptyMap());
+
when(database.getRuleMetaData().getConfigurations()).thenReturn(Collections.emptyList());
+ Path tempFile = tempDir.resolve("export-db-config.yaml");
+ executor.setDatabase(database);
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(new
ExportDatabaseConfigurationStatement(tempFile.toString(),
mock(FromDatabaseSegment.class)), mock(ContextManager.class));
+ LocalDataQueryResultRow row = actual.iterator().next();
+ assertThat(row.getCell(1), is(String.format("Successfully exported to:
'%s'", tempFile)));
+ assertThat(new String(Files.readAllBytes(tempFile)), is("databaseName:
file_db" + System.lineSeparator()));
Review Comment:
The file read operation at line 156 doesn't specify a character encoding.
Consider using Files.readAllBytes(tempFile) with explicit charset conversion
like 'new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8)' to
ensure consistent behavior across different platforms.
##########
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/ral/queryable/export/ExportMetaDataExecutorTest.java:
##########
@@ -129,12 +144,26 @@ private ShardingSphereDatabase
mockEmptyShardingSphereDatabase() {
@Test
void assertExecute() {
ContextManager contextManager = mockContextManager();
- Collection<LocalDataQueryResultRow> actual = new
ExportMetaDataExecutor().getRows(new ExportMetaDataStatement(null),
contextManager);
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(new
ExportMetaDataStatement(null), contextManager);
assertThat(actual.size(), is(1));
LocalDataQueryResultRow row = actual.iterator().next();
assertMetaData(row.getCell(3),
Base64.encodeBase64String(EXPECTED_NOT_EMPTY_METADATA_VALUE.getBytes()));
}
+ @Test
+ void assertExecuteWithFilePath() throws IOException {
+ ContextManager contextManager = mockContextManager();
+
when(contextManager.getComputeNodeInstanceContext().getInstance().getMetaData().getId()).thenReturn("file_instance");
+ Path tempFile = tempDir.resolve("export-metadata.json");
+ ExportMetaDataStatement sqlStatement = new
ExportMetaDataStatement(tempFile.toString());
+ Collection<LocalDataQueryResultRow> actual =
executor.getRows(sqlStatement, contextManager);
+ LocalDataQueryResultRow row = actual.iterator().next();
+ assertThat(row.getCell(1), is("file_instance"));
+ assertThat(row.getCell(3), is(String.format("Successfully exported
to：'%s'", tempFile)));
+ String fileContent = new String(Files.readAllBytes(tempFile));
+ assertMetaData(Base64.encodeBase64String(fileContent.getBytes()),
Base64.encodeBase64String(EXPECTED_NOT_EMPTY_METADATA_VALUE.getBytes()));
Review Comment:
The file read operation at line 163 and encoding at line 164 don't specify a
character encoding. This could lead to platform-dependent behavior. Consider
using StandardCharsets.UTF_8 explicitly for both Files.readAllBytes and
String.getBytes() to ensure consistent behavior across different platforms.
--
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]
