utafrali commented on code in PR #40316:
URL: https://github.com/apache/shardingsphere/pull/40316#discussion_r4336283591
##########
proxy/backend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/backend/firebird/connector/jdbc/statement/FirebirdStatementMemoryStrictlyFetchSizeSetterTest.java:
##########
@@ -48,24 +46,16 @@ class FirebirdStatementMemoryStrictlyFetchSizeSetterTest {
@Test
void assertSetFetchSizeWithDefaultValue() throws SQLException {
Statement statement = mock(Statement.class);
- ContextManager contextManager = mockContextManager(-1);
-
when(ProxyContext.getInstance().getContextManager()).thenReturn(contextManager);
+
when(ProxyContext.getInstance().getContextManager().getMetaDataContexts().getMetaData().getProps().<Integer>getValue(ConfigurationPropertyKey.PROXY_BACKEND_QUERY_FETCH_SIZE)).thenReturn(-1);
Review Comment:
The inlined deep-stub chain
`ProxyContext.getInstance().getContextManager().getMetaDataContexts().getMetaData().getProps().<Integer>getValue(...)`
is repeated verbatim in both test methods and is quite long. The previous
`mockContextManager(int)` helper removed that duplication and read better.
Consider keeping a small helper (e.g. `whenConfiguredFetchSize(int)`) so adding
a third case later does not require pasting the same five-call chain again.
##########
proxy/backend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/backend/firebird/handler/admin/executor/FirebirdShowVariableExecutor.java:
##########
@@ -63,18 +54,7 @@ public final class FirebirdShowVariableExecutor implements
DatabaseAdminQueryExe
public void execute(final ConnectionSession connectionSession, final
ShardingSphereMetaData metaData) {
String name =
Optional.ofNullable(showStatement.getName()).orElse("").toLowerCase(Locale.ROOT);
queryResultMetaData = new
RawQueryResultMetaData(Collections.singletonList(new
RawQueryResultColumnMetaData("", "", name, Types.VARCHAR, "VARCHAR", -1, 0)));
- VariableRowDataGenerator variableRowDataGenerator =
VARIABLE_ROW_DATA_GENERATORS.getOrDefault(name, unused -> new String[]{"", "",
""});
- mergedResult = new LocalDataMergedResult(Collections.singletonList(new
LocalDataQueryResultRow(variableRowDataGenerator.getVariable(connectionSession)[1])));
- }
-
- private interface VariableRowDataGenerator {
-
- /**
- * Get variable.
- *
- * @param connectionSession connection session
- * @return variable
- */
- Object[] getVariable(ConnectionSession connectionSession);
+ // TODO add SHOW statements support
+ mergedResult = new LocalDataMergedResult(Collections.singletonList(new
LocalDataQueryResultRow("server_version".equals(name) ?
ShardingSphereVersion.VERSION : "")));
}
Review Comment:
The inline ternary `"server_version".equals(name) ?
ShardingSphereVersion.VERSION : ""` preserves current behavior, but once a
second variable is added (per the retained `// TODO add SHOW statements
support`) this will immediately regress toward an if/else ladder. Since the
TODO is kept, a short comment noting the map/interface was removed as a
one-entry simplification would make the intent clearer to the next contributor
who implements that TODO.
##########
proxy/backend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/backend/firebird/handler/admin/executor/FirebirdSetVariableAdminExecutorTest.java:
##########
@@ -45,19 +44,14 @@ class FirebirdSetVariableAdminExecutorTest {
@Test
void assertExecute() {
SetStatement setStatement = new SetStatement(databaseType,
Collections.singletonList(new VariableAssignSegment(0, 0, new
VariableSegment(0, 0, "key"), "value")));
- FirebirdSetVariableAdminExecutor executor = new
FirebirdSetVariableAdminExecutor(setStatement);
- ConnectionSession connectionSession = mock(ConnectionSession.class);
- RequiredSessionVariableRecorder requiredSessionVariableRecorder =
mock(RequiredSessionVariableRecorder.class);
- ProxyDatabaseConnectionManager databaseConnectionManager =
mock(ProxyDatabaseConnectionManager.class);
-
when(connectionSession.getRequiredSessionVariableRecorder()).thenReturn(requiredSessionVariableRecorder);
-
when(connectionSession.getDatabaseConnectionManager()).thenReturn(databaseConnectionManager);
+ ConnectionSession connectionSession = mock(ConnectionSession.class,
RETURNS_DEEP_STUBS);
try (MockedStatic<DatabaseTypedSPILoader> databaseTypedSPILoader =
mockStatic(DatabaseTypedSPILoader.class)) {
ReplayedSessionVariableProvider replayedSessionVariableProvider =
mock(ReplayedSessionVariableProvider.class);
when(replayedSessionVariableProvider.isNeedToReplay("key")).thenReturn(true);
databaseTypedSPILoader.when(() ->
DatabaseTypedSPILoader.findService(ReplayedSessionVariableProvider.class,
databaseType)).thenReturn(Optional.of(replayedSessionVariableProvider));
- executor.execute(connectionSession, mock());
- verify(requiredSessionVariableRecorder).setVariable("key",
"value");
- verify(databaseConnectionManager).markSessionVariablesDirty();
+ new
FirebirdSetVariableAdminExecutor(setStatement).execute(connectionSession,
mock());
Review Comment:
Verifying through `connectionSession.getRequiredSessionVariableRecorder()`
and `connectionSession.getDatabaseConnectionManager()` works with
`RETURNS_DEEP_STUBS` because the deep-stub cache returns the same child mock,
but it reads as if a new mock is produced per call. Extracting the two child
mocks into local variables after construction (`RequiredSessionVariableRecorder
recorder = connectionSession.getRequiredSessionVariableRecorder();`) keeps the
deep-stub simplification while making the verifications unambiguous.
--
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]
