gianm commented on code in PR #19231:
URL: https://github.com/apache/druid/pull/19231#discussion_r3270978385
##########
sql/src/main/java/org/apache/druid/sql/PreparedStatement.java:
##########
@@ -31,14 +31,25 @@
public class PreparedStatement extends AbstractStatement
{
private final SqlQueryPlus originalRequest;
+ private final String remoteAddress;
public PreparedStatement(
final SqlToolbox lifecycleToolbox,
final SqlQueryPlus queryPlus
)
{
- super(lifecycleToolbox, queryPlus, null);
+ this(lifecycleToolbox, queryPlus, null);
+ }
+
+ public PreparedStatement(
Review Comment:
nit, but I'd mark `remoteAddress` as `@Nullable` and remove the 2-arg
constructor. Generally it's nicer not to have a lot of different constructors.
##########
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java:
##########
@@ -1950,4 +1950,91 @@ private static Map<String, Object> row(final
Pair<String, ?>... entries)
}
return m;
}
+
+ /**
+ * Test that remote address is properly captured and logged for JDBC Avatica
connections.
+ * This verifies the fix for issue #19230 which ensures that the client's
remote address
+ * is tracked through the entire SQL execution lifecycle.
+ */
+ @Test
+ public void testRemoteAddressInLogs() throws SQLException
+ {
+ testRequestLogger.clear();
+
+ try (Statement stmt = client.createStatement()) {
+ stmt.executeQuery("SELECT COUNT(*) AS cnt FROM druid.foo");
+ }
+
+ Assert.assertEquals(1, testRequestLogger.getSqlQueryLogs().size());
+ RequestLogLine logLine = testRequestLogger.getSqlQueryLogs().get(0);
+
+ String remoteAddress = logLine.getRemoteAddr();
+ Assert.assertNotNull("Remote address should not be null", remoteAddress);
+
+ Assert.assertTrue(
+ "Remote address should be a valid IP or localhost",
+ remoteAddress.contains("localhost") ||
+ remoteAddress.contains("127.0.0.1") ||
+ remoteAddress.contains("0:0:0:0:0:0:0:1") ||
+ (remoteAddress.contains(".") && remoteAddress.length() >= 7)
Review Comment:
It would be better to make this look specifically for IP addresses, for
example by using a regex. It doesn't have to be super strict.
`^\d+\.\d+\.\d+\.\d+$` is fine. The idea is just to make sure that extraneous
stuff isn't included in the `remoteAddress`.
##########
sql/src/main/java/org/apache/druid/sql/SqlStatementFactory.java:
##########
@@ -51,11 +51,21 @@ public HttpStatement httpStatement(
public DirectStatement directStatement(final SqlQueryPlus sqlRequest)
{
- return new DirectStatement(lifecycleToolbox, sqlRequest);
+ return new DirectStatement(lifecycleToolbox, sqlRequest, null);
+ }
+
+ public DirectStatement directStatement(final SqlQueryPlus sqlRequest, final
String remoteAddress)
Review Comment:
nit, but I'd mark `remoteAddress` as `@Nullable` and remove the 1-arg form
of `directStatement` and `preparedStatement`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
