Copilot commented on code in PR #43:
URL:
https://github.com/apache/iotdb-client-csharp/pull/43#discussion_r2681541528
##########
src/Apache.IoTDB/SessionPool.Builder.cs:
##########
@@ -131,9 +146,9 @@ public SessionPool Build()
// if nodeUrls is not empty, use nodeUrls to create session pool
if (_nodeUrls.Count > 0)
{
- return new SessionPool(_nodeUrls, _username, _password,
_fetchSize, _zoneId, _poolSize, _enableRpcCompression, _connectionTimeoutInMs,
_sqlDialect, _database);
+ return new SessionPool(_nodeUrls, _username, _password,
_fetchSize, _zoneId, _poolSize, _enableRpcCompression, _connectionTimeoutInMs,
_useSsl, _certificatePath ,_sqlDialect, _database);
}
- return new SessionPool(_host, _port, _username, _password,
_fetchSize, _zoneId, _poolSize, _enableRpcCompression, _connectionTimeoutInMs,
_sqlDialect, _database);
+ return new SessionPool(_host, _port, _username, _password,
_fetchSize, _zoneId, _poolSize, _enableRpcCompression, _connectionTimeoutInMs,
_useSsl, _certificatePath, _sqlDialect, _database);
Review Comment:
There are two extra spaces before _sqlDialect. While this doesn't affect
functionality, it's inconsistent with the coding style used elsewhere.
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -423,12 +428,14 @@ public async Task<string> GetTimeZone()
}
}
- private async Task<Client> CreateAndOpen(string host, int port, bool
enableRpcCompression, int timeout, string sqlDialect, string database,
CancellationToken cancellationToken = default)
+ private async Task<Client> CreateAndOpen(string host, int port, bool
enableRpcCompression, int timeout, bool useSsl, string cert, string sqlDialect,
string database, CancellationToken cancellationToken = default)
Review Comment:
The parameter name 'cert' is ambiguous. Consider renaming it to
'certificatePath' to be consistent with the field name and improve code clarity.
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -423,12 +428,14 @@ public async Task<string> GetTimeZone()
}
}
- private async Task<Client> CreateAndOpen(string host, int port, bool
enableRpcCompression, int timeout, string sqlDialect, string database,
CancellationToken cancellationToken = default)
+ private async Task<Client> CreateAndOpen(string host, int port, bool
enableRpcCompression, int timeout, bool useSsl, string cert, string sqlDialect,
string database, CancellationToken cancellationToken = default)
{
- var tcpClient = new TcpClient(host, port);
- tcpClient.SendTimeout = timeout;
- tcpClient.ReceiveTimeout = timeout;
- var transport = new TFramedTransport(new
TSocketTransport(tcpClient, null));
+
+ TTransport socket = useSsl ?
+ new TTlsSocketTransport(host, port, null, timeout, new
X509Certificate2(File.ReadAllBytes(cert))) :
+ new TSocketTransport(host, port, null, timeout);
Review Comment:
The new SSL functionality lacks test coverage. Since the repository has test
infrastructure in place, consider adding tests that verify SSL connections work
correctly when SSL is enabled, and that appropriate error handling occurs when
certificate paths are invalid or SSL configuration is incorrect.
##########
src/Apache.IoTDB/SessionPool.Builder.cs:
##########
@@ -131,9 +146,9 @@ public SessionPool Build()
// if nodeUrls is not empty, use nodeUrls to create session pool
if (_nodeUrls.Count > 0)
{
- return new SessionPool(_nodeUrls, _username, _password,
_fetchSize, _zoneId, _poolSize, _enableRpcCompression, _connectionTimeoutInMs,
_sqlDialect, _database);
+ return new SessionPool(_nodeUrls, _username, _password,
_fetchSize, _zoneId, _poolSize, _enableRpcCompression, _connectionTimeoutInMs,
_useSsl, _certificatePath ,_sqlDialect, _database);
Review Comment:
There's an extra space before _sqlDialect. While this doesn't affect
functionality, it's inconsistent with the coding style used elsewhere.
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -126,11 +129,11 @@ public SessionPool(List<string> nodeUrls, string
username, string password, int
{
}
public SessionPool(List<string> nodeUrls, string username, string
password, int fetchSize, string zoneId, int poolSize, bool
enableRpcCompression, int timeout)
- : this(nodeUrls, username, password, fetchSize,
zoneId, poolSize, enableRpcCompression, timeout,
IoTDBConstant.TREE_SQL_DIALECT, "")
+ : this(nodeUrls, username, password, fetchSize,
zoneId, poolSize, enableRpcCompression, timeout, false, null,
IoTDBConstant.TREE_SQL_DIALECT, "")
Review Comment:
There's an extra space after the comma following _certificatePath. While
this doesn't affect functionality, it's inconsistent with the coding style used
in the same line and elsewhere.
```suggestion
: this(nodeUrls, username, password, fetchSize,
zoneId, poolSize, enableRpcCompression, timeout, false, null,
IoTDBConstant.TREE_SQL_DIALECT, "")
```
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -423,12 +428,14 @@ public async Task<string> GetTimeZone()
}
}
- private async Task<Client> CreateAndOpen(string host, int port, bool
enableRpcCompression, int timeout, string sqlDialect, string database,
CancellationToken cancellationToken = default)
+ private async Task<Client> CreateAndOpen(string host, int port, bool
enableRpcCompression, int timeout, bool useSsl, string cert, string sqlDialect,
string database, CancellationToken cancellationToken = default)
{
- var tcpClient = new TcpClient(host, port);
- tcpClient.SendTimeout = timeout;
- tcpClient.ReceiveTimeout = timeout;
- var transport = new TFramedTransport(new
TSocketTransport(tcpClient, null));
+
+ TTransport socket = useSsl ?
+ new TTlsSocketTransport(host, port, null, timeout, new
X509Certificate2(File.ReadAllBytes(cert))) :
+ new TSocketTransport(host, port, null, timeout);
+
Review Comment:
The certificate file is loaded synchronously using File.ReadAllBytes without
any validation. This could cause runtime exceptions if the certificate path is
invalid, the file doesn't exist, is not accessible, or contains invalid
certificate data. Consider validating that the certificate path is provided
when SSL is enabled and that the file exists before attempting to load it.
```suggestion
TTransport socket;
if (useSsl)
{
if (string.IsNullOrWhiteSpace(cert))
{
throw new ArgumentException("Certificate path must be
provided when SSL is enabled.", nameof(cert));
}
if (!File.Exists(cert))
{
throw new FileNotFoundException($"Certificate file not
found at path '{cert}'.", cert);
}
X509Certificate2 certificate;
try
{
var certificateBytes = File.ReadAllBytes(cert);
certificate = new X509Certificate2(certificateBytes);
}
catch (Exception ex)
{
throw new InvalidOperationException($"Failed to load SSL
certificate from path '{cert}'.", ex);
}
socket = new TTlsSocketTransport(host, port, null, timeout,
certificate);
}
else
{
socket = new TSocketTransport(host, port, null, timeout);
}
```
--
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]
