Copilot commented on code in PR #6631: URL: https://github.com/apache/ignite-3/pull/6631#discussion_r2366945631
########## modules/platforms/dotnet/Apache.Ignite/Sql/IgniteDbTransaction.cs: ########## @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +namespace Apache.Ignite.Sql; + +using System.Data; +using System.Data.Common; +using System.Threading; +using System.Threading.Tasks; +using Transactions; + +/// <summary> +/// Ignite database transaction. +/// </summary> +public sealed class IgniteDbTransaction : DbTransaction +{ + /// <summary> + /// Initializes a new instance of the <see cref="IgniteDbTransaction"/> class. + /// </summary> + /// <param name="tx">Underlying Ignite transaction.</param> + /// <param name="isolationLevel">Isolation level.</param> + /// <param name="connection">Connection.</param> + public IgniteDbTransaction(ITransaction tx, IsolationLevel isolationLevel, DbConnection connection) + { + IgniteTransaction = tx; + IsolationLevel = isolationLevel; + DbConnection = connection; + } + + /// <inheritdoc /> + public override IsolationLevel IsolationLevel { get; } + + /// <inheritdoc /> + public override bool SupportsSavepoints => false; + + /// <summary> + /// Gets the underlying Ignite transaction. + /// </summary> + public ITransaction IgniteTransaction { get; } + + /// <inheritdoc /> + protected override DbConnection DbConnection { get; } + + /// <inheritdoc /> + public override void Commit() => CommitAsync(CancellationToken.None).GetAwaiter().GetResult(); + + /// <inheritdoc /> + public override void Rollback() => RollbackAsync(null!, CancellationToken.None).GetAwaiter().GetResult(); Review Comment: Passing `null!` as the savepoint name parameter to `RollbackAsync` is incorrect. The method signature expects a `string savepointName`, but since savepoints are not supported (as indicated by `SupportsSavepoints => false`), this should pass an empty string or null without the null-forgiving operator. ```suggestion public override void Rollback() => RollbackAsync(null, CancellationToken.None).GetAwaiter().GetResult(); ``` ########## modules/platforms/dotnet/Apache.Ignite/Sql/IgniteDbConnectionStringBuilder.cs: ########## @@ -0,0 +1,189 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +namespace Apache.Ignite.Sql; + +using System; +using System.Collections.Generic; +using System.Data.Common; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; + +/// <summary> +/// Ignite connection string builder. +/// </summary> +[SuppressMessage("Design", "CA1010:Generic interface should also be implemented", Justification = "Reviewed.")] +public sealed class IgniteDbConnectionStringBuilder : DbConnectionStringBuilder +{ + /// <summary> + /// Gets the character used to separate multiple endpoints in the connection string. + /// </summary> + public const char EndpointSeparator = ','; + + private static readonly IReadOnlySet<string> KnownKeys = new HashSet<string>(StringComparer.OrdinalIgnoreCase) + { + nameof(Endpoints), + nameof(SocketTimeout), + nameof(OperationTimeout), + nameof(HeartbeatInterval), + nameof(ReconnectInterval), + nameof(SslEnabled), + nameof(Username), + nameof(Password) + }; + + /// <summary> + /// Initializes a new instance of the <see cref="IgniteDbConnectionStringBuilder"/> class. + /// </summary> + public IgniteDbConnectionStringBuilder() + { + // No-op. + } + + /// <summary> + /// Initializes a new instance of the <see cref="IgniteDbConnectionStringBuilder"/> class. + /// </summary> + /// <param name="connectionString">Connection string.</param> + public IgniteDbConnectionStringBuilder(string connectionString) + { + ConnectionString = connectionString; + } + + /// <summary> + /// Gets or sets the Ignite endpoints. + /// Multiple endpoints can be specified separated by comma, e.g. "localhost:10800,localhost:10801". + /// If the port is not specified, the default port 10800 is used. + /// </summary> + [SuppressMessage("Usage", "CA2227:Collection properties should be read only", Justification = "Reviewed.")] + public IList<string> Endpoints + { + get => this[nameof(Endpoints)] is string endpoints + ? endpoints.Split(EndpointSeparator, StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + : []; + set => this[nameof(Endpoints)] = string.Join(EndpointSeparator, value); + } + + /// <summary> + /// Gets or sets the socket timeout. See <see cref="IgniteClientConfiguration.SocketTimeout"/> for more details. + /// </summary> + public TimeSpan SocketTimeout + { + get => GetString(nameof(SocketTimeout)) is { } s + ? TimeSpan.Parse(s, CultureInfo.InvariantCulture) + : IgniteClientConfiguration.DefaultSocketTimeout; + set => this[nameof(SocketTimeout)] = value.ToString(); + } + + /// <summary> + /// Gets or sets the socket timeout. See <see cref="IgniteClientConfiguration.OperationTimeout"/> for more details. + /// </summary> + public TimeSpan OperationTimeout + { + get => GetString(nameof(OperationTimeout)) is { } s + ? TimeSpan.Parse(s, CultureInfo.InvariantCulture) + : IgniteClientConfiguration.DefaultOperationTimeout; + set => this[nameof(OperationTimeout)] = value.ToString(); + } + + /// <summary> + /// Gets or sets the heartbeat interval. See <see cref="IgniteClientConfiguration.HeartbeatInterval"/> for more details. + /// </summary> + public TimeSpan HeartbeatInterval + { + get => GetString(nameof(HeartbeatInterval)) is { } s + ? TimeSpan.Parse(s, CultureInfo.InvariantCulture) + : IgniteClientConfiguration.DefaultHeartbeatInterval; + set => this[nameof(HeartbeatInterval)] = value.ToString(); + } + + /// <summary> + /// Gets or sets the reconnect interval. See <see cref="IgniteClientConfiguration.ReconnectInterval"/> for more details. + /// </summary> + public TimeSpan ReconnectInterval + { + get => GetString(nameof(ReconnectInterval)) is { } s + ? TimeSpan.Parse(s, CultureInfo.InvariantCulture) + : IgniteClientConfiguration.DefaultReconnectInterval; + set => this[nameof(ReconnectInterval)] = value.ToString(); + } + + /// <summary> + /// Gets or sets a value indicating whether SSL is enabled. + /// </summary> + public bool SslEnabled + { + get => GetString(nameof(SslEnabled)) is { } s && bool.Parse(s); + set => this[nameof(SslEnabled)] = value.ToString(); + } + + /// <summary> + /// Gets or sets the username for authentication. + /// </summary> + public string? Username + { + get => GetString(nameof(Username)); + set => this[nameof(Username)] = value; + } + + /// <summary> + /// Gets or sets the username for authentication. Review Comment: The XML documentation comment incorrectly states 'username' when the property is for 'password'. Should be 'Gets or sets the password for authentication.' ```suggestion /// Gets or sets the password for authentication. ``` ########## modules/platforms/dotnet/Apache.Ignite/Sql/IgniteDbCommand.cs: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +namespace Apache.Ignite.Sql; + +using System; +using System.Data; +using System.Data.Common; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Threading; +using System.Threading.Tasks; +using NodaTime; +using Table; +using Transactions; + +/// <summary> +/// Ignite database command. +/// </summary> +public sealed class IgniteDbCommand : DbCommand +{ + private readonly CancellationTokenSource _cancellationTokenSource = new(); + + private IgniteDbParameterCollection? _parameters; + + private string _commandText = string.Empty; + + /// <inheritdoc /> + [AllowNull] + public override string CommandText + { + get => _commandText; + set => _commandText = value ?? string.Empty; + } + + /// <inheritdoc /> + public override int CommandTimeout { get; set; } + + /// <summary> + /// Gets or sets the page size (number of rows fetched at a time by the underlying Ignite data reader). + /// </summary> + public int PageSize { get; set; } = SqlStatement.DefaultPageSize; + + /// <inheritdoc /> + public override CommandType CommandType { get; set; } = CommandType.Text; + + /// <inheritdoc /> + public override UpdateRowSource UpdatedRowSource { get; set; } + + /// <inheritdoc /> + public override bool DesignTimeVisible { get; set; } + + /// <summary> + /// Gets or sets the transaction within which the command executes. + /// </summary> + public IgniteDbTransaction? IgniteDbTransaction + { + get => (IgniteDbTransaction?)Transaction; + set => Transaction = value; + } + + /// <inheritdoc /> + protected override DbConnection? DbConnection { get; set; } + + /// <inheritdoc /> + protected override DbParameterCollection DbParameterCollection => + _parameters ??= new IgniteDbParameterCollection(); + + /// <inheritdoc /> + protected override DbTransaction? DbTransaction { get; set; } + + /// <inheritdoc /> + public override void Cancel() => _cancellationTokenSource.Cancel(); + + /// <inheritdoc /> + public override int ExecuteNonQuery() => + ExecuteNonQueryAsync(CancellationToken.None).GetAwaiter().GetResult(); + + /// <inheritdoc /> + [SuppressMessage("Reliability", "CA2007:Consider calling ConfigureAwait on the awaited task", Justification = "False positive")] + public override async Task<int> ExecuteNonQueryAsync(CancellationToken cancellationToken) + { + var args = GetArgs(); + var statement = GetStatement(); + + using CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, _cancellationTokenSource.Token); + + try + { + await using IResultSet<object> resultSet = await GetSql().ExecuteAsync<object>( + transaction: GetIgniteTx(), + statement, + linkedCts.Token, + args).ConfigureAwait(false); + + Debug.Assert(!resultSet.HasRowSet, "!resultSet.HasRowSet"); + + if (resultSet.AffectedRows < 0) + { + return resultSet.WasApplied ? 1 : 0; + } + + return (int)resultSet.AffectedRows; + } + catch (SqlException sqlEx) + { + throw new IgniteDbException(sqlEx.Message, sqlEx); + } + } + + /// <inheritdoc /> + [SuppressMessage("Reliability", "CA2007:Consider calling ConfigureAwait on the awaited task", Justification = "False positive")] + public override async Task<object?> ExecuteScalarAsync(CancellationToken cancellationToken) + { + var args = GetArgs(); + var statement = GetStatement(); + + using CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource( + cancellationToken, _cancellationTokenSource.Token); + + try + { + await using IResultSet<IIgniteTuple> resultSet = await GetSql().ExecuteAsync( + transaction: GetIgniteTx(), + statement, + linkedCts.Token, + args).ConfigureAwait(false); + + await foreach (var row in resultSet) + { + // Return the first result. + return row[0]; + } + } + catch (SqlException sqlEx) + { + throw new IgniteDbException(sqlEx.Message, sqlEx); + } + + throw new IgniteDbException("Query returned no results: " + statement); + } + + /// <inheritdoc /> + public override object? ExecuteScalar() => ExecuteScalarAsync().GetAwaiter().GetResult(); + + /// <inheritdoc /> + public override void Prepare() => throw new NotSupportedException("Prepare is not supported."); + + /// <inheritdoc /> + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + + if (disposing) + { + _cancellationTokenSource.Dispose(); + } + + base.Dispose(disposing); Review Comment: The `base.Dispose(disposing)` method is called twice - once at line 167 and again at line 174. The second call should be removed as it's redundant and could cause issues. ```suggestion ``` -- 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]
