Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for .NET 10.0 across the solution while improving cross-platform compatibility by extending support to .NET Standard 2.0 and .NET Framework 4.8.1 for several projects. The changes include updating target frameworks, adding conditional compilation for different framework versions, refactoring code for better performance, and introducing a new benchmark project.
Changes:
- Added .NET 10.0 target framework to multiple projects and updated CI/CD workflows
- Extended Redis, Office, Request, RabbitMQ, and WeChat projects to support .NET Standard 2.0 and additional framework versions
- Refactored code to use conditional compilation directives for framework-specific implementations
Reviewed changes
Copilot reviewed 78 out of 78 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/JfYu.sln | Added new benchmark project and additional platform configurations (x64, x86) |
| src/JfYu.WeChat/JfYu.WeChat.csproj | Added net9.0 and net10.0 target frameworks |
| src/JfYu.WeChat/Constant/MiniProgram.cs | Changed class to static |
| src/JfYu.Request/JfYu.Request.csproj | Added net9.0 and net10.0 target frameworks |
| src/JfYu.Request/JfYuHttpClientOptions.cs | Changed default cookie container behavior and added thread-safety warnings |
| src/JfYu.Request/JfYuHttpClient.cs | Reordered constructor parameters |
| src/JfYu.Request/README.md | Updated documentation about cookie container thread safety |
| src/JfYu.Redis/JfYu.Redis.csproj | Added netstandard2.0, net9.0, and net10.0 target frameworks |
| src/JfYu.Redis/Implementation/*.cs | Added conditional compilation for .NET Standard 2.0 compatibility |
| src/JfYu.Redis/Extensions/ArgumentNullExceptionExtension.cs | Added polyfill methods for .NET Standard 2.0 |
| src/JfYu.Office/JfYu.Office.csproj | Added netstandard2.0, net9.0, and net10.0 target frameworks |
| src/JfYu.Office/Excel/Write/JfYuWriterBase.cs | Improved numeric precision formatting for Excel cells |
| src/JfYu.Office/ArgumentNullExceptionExtension.cs | Added polyfill class for .NET Standard 2.0 |
| src/JfYu.Data/JfYu.Data.csproj | Added net9.0 and net10.0 target frameworks |
| src/JfYu.Data/Service/Service.cs | Optimized list operations to use for loops instead of ForEach |
| src/JfYu.RabbitMQ/JfYu.RabbitMQ.csproj | Added net9.0 and net10.0 target frameworks |
| src/JfYu.RabbitMQ/RabbitMQService.cs | Refactored batch sending to use IEnumerable |
| src/JfYu.UnitTests/JfYu.UnitTests.csproj | Added net10.0 target framework and EF6 support for .NET Framework |
| src/JfYu.UnitTests/Models/*.cs | Reorganized faker classes and entity files |
| src/JfYu.UnitTests/Office/Excel/JfYuExcelDbDataReaderWriterTests.cs | Added conditional compilation for EF6 vs EF Core |
| src/JfYu.UnitTests/Redis/*.cs | Removed #if NET8_0_OR_GREATER directives to enable broader framework support |
| src/JfYu.Benchmark/* | Added new benchmark project with insert, update, and select benchmarks |
| src/Directory.Packages.props | Updated package versions and added framework-specific version management |
| .github/workflows/*.yml | Added .NET 10.0 to CI/CD pipelines |
Comments suppressed due to low confidence (1)
src/JfYu.Request/JfYuHttpClient.cs:1
- There are two spaces between 'configuration,' and 'LogFilter'. Consistent spacing should be maintained in parameter lists.
| _database = Client.GetDatabase(redisConfiguration.Value.DbIndex).WithKeyPrefix(redisConfiguration.Value.Prefix); | ||
| _serializer = serializer; | ||
| _lockToken = $"{Environment.MachineName}_{Environment.ProcessId}_{Guid.NewGuid()}"; | ||
| _lockToken = $"{Environment.MachineName}_{Environment.CurrentManagedThreadId}_{Guid.NewGuid()}"; |
There was a problem hiding this comment.
Using Environment.CurrentManagedThreadId in a lock token is problematic because the thread ID can change between lock acquisition and release in async operations. This could prevent proper lock release. Use Environment.ProcessId instead, or consider including a unique identifier that persists across async continuations.
| _lockToken = $"{Environment.MachineName}_{Environment.CurrentManagedThreadId}_{Guid.NewGuid()}"; | |
| _lockToken = $"{Environment.MachineName}_{Environment.ProcessId}_{Guid.NewGuid()}"; |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.InMemory" Version="9.0.13" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore" Version="9.0.13" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Tools" Version="9.0.13" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Proxies" Version="9.0.13" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Design" Version="9.0.13" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.SqlServer" Version="9.0.13" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.13" /> | ||
| <PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" /> | ||
|
|
||
| <!--<PackageVersion Include="Microsoft.EntityFrameworkCore.InMemory" Version="10.0.3" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore" Version="10.0.3" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Tools" Version="10.0.3" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Proxies" Version="10.0.3" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Design" Version="10.0.3" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.SqlServer" Version="10.0.3" /> | ||
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="10.0.3" /> | ||
| <PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" />--> |
There was a problem hiding this comment.
The net10.0 target framework is using Entity Framework Core 9.0.13 packages. If .NET 10.0 exists, there should be corresponding EF Core 10.x packages available. Using an older EF Core version with a newer .NET version may lead to compatibility issues or missing features. Consider updating to EF Core 10.x packages if they exist, or document why older packages are being used.
| <PackageVersion Include="Microsoft.EntityFrameworkCore.InMemory" Version="9.0.13" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore" Version="9.0.13" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Tools" Version="9.0.13" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Proxies" Version="9.0.13" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Design" Version="9.0.13" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.SqlServer" Version="9.0.13" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="9.0.13" /> | |
| <PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" /> | |
| <!--<PackageVersion Include="Microsoft.EntityFrameworkCore.InMemory" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Tools" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Proxies" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Design" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.SqlServer" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="10.0.3" /> | |
| <PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" />--> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.InMemory" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Tools" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Proxies" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Design" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.SqlServer" Version="10.0.3" /> | |
| <PackageVersion Include="Microsoft.EntityFrameworkCore.Sqlite" Version="10.0.3" /> | |
| <PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" /> |
| public interface IService<T, TContext> where T : BaseEntity | ||
| where TContext : DbContext | ||
| { | ||
| /// <summary> | ||
| /// Gets the master database context for write operations. | ||
| /// All Add, Update, and Remove operations use this context. | ||
| /// </summary> | ||
| public TContext Context { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the readonly database context for read operations. | ||
| /// Randomly selects from configured read replicas or falls back to master if none configured. | ||
| /// All query operations use this context for load balancing. | ||
| /// </summary> | ||
| public TContext ReadonlyContext { get; } | ||
|
|
||
| /// <summary> | ||
| /// Adds a single entity asynchronously to the master database. | ||
| /// </summary> | ||
| /// <param name="entity">The entity to add.</param> | ||
| /// <param name="cancellationToken">Cancellation token for the async operation.</param> | ||
| /// <returns>The number of state entries written to the database (typically 1 on success).</returns> | ||
| Task<int> AddAsync(T entity, CancellationToken cancellationToken= default); | ||
| Task<int> AddAsync(T entity, CancellationToken cancellationToken = default); | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
IService<T,TContext> removed the publicly exposed Context / ReadonlyContext properties. Since this is a public interface, this is a breaking change for any downstream consumers that use these properties (including custom derived services/tests). If the goal is to hide contexts, consider keeping the properties (possibly marked obsolete) or introducing a non-breaking alternative (e.g., protected access on the base class plus a separate interface for internal consumers).
| var service = (Service<User, DataContext>)useService; | ||
| var contextField =typeof(Service<User, DataContext>).GetFields(BindingFlags.NonPublic | BindingFlags.Instance).FirstOrDefault(q => q.Name.Contains("_context")); | ||
| var readonlyContextField = typeof(Service<User, DataContext>).GetFields(BindingFlags.NonPublic | BindingFlags.Instance).FirstOrDefault(q => q.Name.Contains("_readonlyContext")); | ||
|
|
||
| var context = contextField!.GetValue(service) as DataContext; | ||
| var readonlyContext = readonlyContextField!.GetValue(service) as DataContext; | ||
|
|
||
| Assert.Contains("127.0.0.1", context!.Database.GetConnectionString()); | ||
| Assert.Contains("127.0.0.1", readonlyContext!.Database.GetConnectionString()); |
There was a problem hiding this comment.
These assertions use reflection to access compiler-generated backing fields for _context / _readonlyContext. This is brittle (field names/shapes can change with compiler changes or refactors) and will make tests harder to maintain. Prefer asserting behavior through the public API, or expose a stable test hook (e.g., internal properties with InternalsVisibleTo, or a derived test-only service that can read protected members).
| if (source.GetType().GetGenericTypeDefinition().Name.StartsWith("Tuple")) | ||
| { | ||
| var newData = (ITuple)source; | ||
| for (int i = 0; i < newData.Length; i++) | ||
| var tupleType = source.GetType(); | ||
| var properties = tupleType.GetProperties(); | ||
| for (int i = 0; i < properties.Length; i++) | ||
| { | ||
| if (newData[i] is IList newItemData) | ||
| if (properties[i].GetValue(source) is IList newItemData) | ||
| { |
There was a problem hiding this comment.
Tuple sheet extraction relies on GetProperties() index order, but reflection property order is not guaranteed. This can lead to writing tuple elements in the wrong order across runtimes/TFMs. To keep deterministic behavior, consider ordering the properties explicitly (e.g., Item1..ItemN), or using ITuple behind #if where available and falling back to ordered reflection only for netstandard2.0.
| using NPOI.SS.Formula.Functions; | ||
| using System.Data; | ||
| using System.Globalization; | ||
| using NPOI.OpenXmlFormats.Spreadsheet; | ||
|
|
There was a problem hiding this comment.
using NPOI.OpenXmlFormats.Spreadsheet; appears unused in this test file, which may produce unnecessary warnings/noise. Consider removing it if it isn't needed.
| /// <param name="logFilter">The log filter.</param> | ||
| /// <param name="logger">The logger.</param> | ||
| public class JfYuHttpClient(IHttpClientFactory factory, JfYuHttpClientConfiguration configuration, CookieContainer? cookieContainer, LogFilter logFilter, ILogger<JfYuHttpClient>? logger = null) : JfYuBaseRequest | ||
| public class JfYuHttpClient(IHttpClientFactory factory, JfYuHttpClientConfiguration configuration, LogFilter logFilter,CookieContainer? cookieContainer = null, ILogger<JfYuHttpClient>? logger = null) : JfYuBaseRequest |
There was a problem hiding this comment.
Making CookieContainer optional in DI is fine, but when it is provided as a singleton shared container, the current JfYuHttpClient implementation still mutates it from multiple scoped instances. The per-instance locking in JfYuHttpClient doesn’t provide cross-instance synchronization, so shared-cookie mode remains subject to race conditions. Consider injecting a shared lock (or avoiding shared mutable CookieContainer entirely) if shared-cookie support is kept.
| /// WARNING: When set to true, the shared CookieContainer is NOT thread-safe. | ||
| /// Concurrent requests may lead to race conditions. Use with caution in multi-threaded environments. | ||
| /// </remarks> | ||
| public bool UseSharedCookieContainer { get; set; } |
There was a problem hiding this comment.
The default value for UseSharedCookieContainer has been changed from true to false. This is a breaking behavioral change that will affect existing consumers who relied on the default behavior of sharing cookies across requests. Applications that depend on shared cookie state will now have isolated cookies per request by default unless they explicitly set UseSharedCookieContainer = true.
While this change improves thread safety by default, it changes the behavior of existing applications. Consider documenting this as a breaking change in release notes and potentially bumping the major version.
| var placeholder = $"{{{replacement.Key}}}"; | ||
| var text = run.Text; | ||
| var texts = text.Split(placeholder); | ||
| var texts = text.Split([placeholder], StringSplitOptions.None); |
There was a problem hiding this comment.
String.Split is being called with a string array parameter [placeholder] to split on a single placeholder value. This approach, while functional, is less efficient than the overload that accepts a single string. The array allocation is unnecessary overhead.
Consider using text.Split(new[] { placeholder }, StringSplitOptions.None) or the ReadOnlySpan-based split methods available in more recent framework versions for better performance.
| var texts = text.Split([placeholder], StringSplitOptions.None); | |
| #if NET8_0_OR_GREATER | |
| var texts = text.Split(placeholder, StringSplitOptions.None); | |
| #else | |
| var texts = text.Split(new[] { placeholder }, StringSplitOptions.None); | |
| #endif |
| /// <param name="callback">Optional callback action to report progress.</param> | ||
| /// <exception cref="InvalidOperationException">Thrown when a title's value cannot be found.</exception> | ||
| protected void Write(IQueryable data, IWorkbook workbook, Type tType, JfYuExcelOptions writeOperation, Dictionary<string, string>? titles, Action<int>? callback = null, bool needAutoCreateSheet = true) | ||
| protected void Write(IQueryable data, IWorkbook workbook, Type tType, JfYuExcelOptions writeOperation, Dictionary<string, string>? titles, Action<int>? callback = null) |
There was a problem hiding this comment.
The unused parameter documentation has been removed while the parameter still exists in the method signature. The needAutoCreateSheet parameter is no longer used in the method body, but it hasn't been removed from the signature. This creates confusion and dead code.
Either restore the parameter's usage if it's needed, or remove it entirely from the method signature. Keeping unused parameters in the signature is a code smell that suggests incomplete refactoring.
| public async Task SendAsync<T>(string exchangeName, T message, string routingKey = "", IDictionary<string, object?>? headers = null, CancellationToken cancellationToken = default) => await SendInternalAsync(exchangeName, routingKey, headers, [message], cancellationToken).ConfigureAwait(false); | ||
|
|
||
| /// <inheritdoc/> | ||
| public async Task SendBatchAsync<T>(string exchangeName, IList<T> messages, string routingKey = "", IDictionary<string, object?>? headers = null, CancellationToken cancellationToken = default) => await SendInternalAsync(exchangeName, routingKey, headers, cancellationToken, messages.ToArray()).ConfigureAwait(false); | ||
| public async Task SendBatchAsync<T>(string exchangeName, IList<T> messages, string routingKey = "", IDictionary<string, object?>? headers = null, CancellationToken cancellationToken = default) => await SendInternalAsync(exchangeName, routingKey, headers, messages, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| private async Task SendInternalAsync<T>(string exchangeName, string routingKey = "", IDictionary<string, object?>? headers = null, CancellationToken cancellationToken = default, params T[] messages) | ||
| private async Task SendInternalAsync<T>(string exchangeName, string routingKey, IDictionary<string, object?>? headers, IEnumerable<T> messages, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
The RabbitMQ SendInternalAsync method signature has been changed to accept IEnumerable<T> instead of params T[], and the parameter order has been modified. This is a breaking change for the private method that could affect binary compatibility if this library is consumed in certain scenarios.
More importantly, the params array was removed but the method is still being called with collection expressions [message] in SendAsync. While this works, it creates an unnecessary array allocation for each single message send. Consider keeping the params overload for single messages or using a more efficient approach.
|
No description provided.