feat(SocketDataConverterCollections): add global converter configuration function #6449
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR introduces a global socket data converter configuration mechanism, refactors the core conversion infrastructure to resolve converters both via type attributes and from a central DI-backed collection, renames existing converter attributes for clarity, and adds corresponding unit tests to cover the new flows. Sequence diagram for resolving a socket data converter (attribute and global collection)sequenceDiagram
participant Client as ITcpSocketClient
participant Adapter as IDataPackageAdapter
participant ServiceProvider
participant Converters as SocketDataConverterCollections
participant Callback as User Callback
Client->>Adapter: SetDataPackageAdapter<TEntity>(callback)
Adapter->>Client: Check for SocketDataTypeConverterAttribute on TEntity
alt Attribute present
Adapter->>Attribute: Activator.CreateInstance(Type)
Adapter->>Callback: Use attribute-specified converter
else Attribute not present
Adapter->>Client: GetSocketDataConverter<TEntity>()
Client->>ServiceProvider: GetRequiredService<IOptions<SocketDataConverterCollections>>()
ServiceProvider->>Converters: TryGetTypeConverter<TEntity>()
alt Converter found
Adapter->>Callback: Use global converter
else Converter not found
Adapter->>Callback: callback(default)
end
end
Class diagram for new and updated socket data converter infrastructureclassDiagram
class SocketDataConverterCollections {
+void AddOrUpdateTypeConverter<TEntity>(ISocketDataConverter<TEntity> converter)
+void AddOrUpdatePropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, SocketDataPropertyConverterAttribute attribute)
+bool TryGetTypeConverter<TEntity>(out ISocketDataConverter<TEntity> converter)
+bool TryGetPropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, out SocketDataPropertyConverterAttribute converterAttribute)
+bool TryGetPropertyConverter<TEntity>(MemberInfo memberInfo, out SocketDataPropertyConverterAttribute converterAttribute)
}
class ISocketDataConverter {
}
class ISocketDataConverter~TEntity~ {
+bool TryConvertTo(ReadOnlyMemory<byte> data, out TEntity? entity)
}
ISocketDataConverter~TEntity~ --|> ISocketDataConverter
class SocketDataConverter~TEntity~ {
+SocketDataConverter()
+SocketDataConverter(SocketDataConverterCollections converters)
#bool Parse(ReadOnlyMemory<byte> data, TEntity entity)
-SocketDataPropertyConverterAttribute? GetPropertyConverterAttribute(PropertyInfo propertyInfo)
}
SocketDataConverter~TEntity~ --|> ISocketDataConverter~TEntity~
class SocketDataTypeConverterAttribute {
+Type? Type
}
class SocketDataPropertyConverterAttribute {
+Type? Type
+Type? ConverterType
+object[]? ConverterParameters
+int Offset
+int Length
+string? EncodingName
}
Class diagram for DefaultTcpSocketClient and DI integrationclassDiagram
class DefaultTcpSocketClient {
+IServiceProvider? ServiceProvider
+object? GetService(Type serviceType)
}
DefaultTcpSocketClient --|> IServiceProvider
DefaultTcpSocketClient --|> ITcpSocketClient
Class diagram for attribute renaming and usageclassDiagram
class SocketDataTypeConverterAttribute {
+Type? Type
}
class SocketDataPropertyConverterAttribute {
+Type? Type
+Type? ConverterType
+object[]? ConverterParameters
+int Offset
+int Length
+string? EncodingName
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- The parameterless constructor on SocketDataConverter always creates a new SocketDataConverterCollections instead of using the DI-configured global collection, so converters instantiated via Activator.CreateInstance won't pick up any user-configured converters—consider resolving the converter from IServiceProvider or requiring a constructor injection of the shared collections.
- There are inconsistent attribute names in the samples (e.g.
SocketTypeDataConverterAttributevsSocketDataTypeConverterAttribute); please review and unify the attribute naming across all documentation snippets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The parameterless constructor on SocketDataConverter always creates a new SocketDataConverterCollections instead of using the DI-configured global collection, so converters instantiated via Activator.CreateInstance won't pick up any user-configured converters—consider resolving the converter from IServiceProvider or requiring a constructor injection of the shared collections.
- There are inconsistent attribute names in the samples (e.g. `SocketTypeDataConverterAttribute` vs `SocketDataTypeConverterAttribute`); please review and unify the attribute naming across all documentation snippets.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs:37` </location>
<code_context>
+ /// <typeparam name="TEntity"></typeparam>
+ /// <param name="propertyExpression"></param>
+ /// <param name="attribute"></param>
+ public void AddOrUpdatePropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, SocketDataPropertyConverterAttribute attribute)
+ {
+ if (propertyExpression.Body is MemberExpression memberExpression)
+ {
+ if(attribute.Type == null)
</code_context>
<issue_to_address>
Add support for UnaryExpression in AddOrUpdatePropertyConverter.
Add handling for UnaryExpression in addition to MemberExpression to support cases like value type property access.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
public void AddOrUpdatePropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, SocketDataPropertyConverterAttribute attribute)
{
if (propertyExpression.Body is MemberExpression memberExpression)
{
if(attribute.Type == null)
{
attribute.Type = memberExpression.Type;
}
_propertyConverters.AddOrUpdate(memberExpression.Member, m => attribute, (m, v) => attribute);
}
}
=======
public void AddOrUpdatePropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, SocketDataPropertyConverterAttribute attribute)
{
MemberExpression? memberExpression = null;
if (propertyExpression.Body is MemberExpression directMember)
{
memberExpression = directMember;
}
else if (propertyExpression.Body is UnaryExpression unary && unary.Operand is MemberExpression unaryMember)
{
memberExpression = unaryMember;
}
if (memberExpression != null)
{
if (attribute.Type == null)
{
attribute.Type = memberExpression.Type;
}
_propertyConverters.AddOrUpdate(memberExpression.Member, m => attribute, (m, v) => attribute);
}
else
{
throw new ArgumentException("The property expression is not a valid member expression.", nameof(propertyExpression));
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs:41` </location>
<code_context>
+ {
+ if (propertyExpression.Body is MemberExpression memberExpression)
+ {
+ if(attribute.Type == null)
+ {
+ attribute.Type = memberExpression.Type;
</code_context>
<issue_to_address>
Potential type mismatch when setting attribute.Type from memberExpression.Type.
memberExpression.Type may not always reflect the property's actual type, especially with nullable or boxed types. Use property metadata to ensure accuracy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (propertyExpression.Body is MemberExpression memberExpression)
{
if(attribute.Type == null)
{
attribute.Type = memberExpression.Type;
}
_propertyConverters.AddOrUpdate(memberExpression.Member, m => attribute, (m, v) => attribute);
}
=======
if (propertyExpression.Body is MemberExpression memberExpression)
{
if(attribute.Type == null)
{
// Use reflection to get the property's actual type
var propertyInfo = memberExpression.Member as System.Reflection.PropertyInfo;
if (propertyInfo != null)
{
attribute.Type = propertyInfo.PropertyType;
}
else
{
attribute.Type = memberExpression.Type;
}
}
_propertyConverters.AddOrUpdate(memberExpression.Member, m => attribute, (m, v) => attribute);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs:69` </location>
<code_context>
+ /// 获得指定数据类型属性转换器方法
+ /// </summary>
+ /// <typeparam name="TEntity"></typeparam>
+ public bool TryGetPropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, [NotNullWhen(true)] out SocketDataPropertyConverterAttribute? converterAttribute)
+ {
+ converterAttribute = null;
+ var ret = false;
+ if (propertyExpression.Body is MemberExpression memberExpression && TryGetPropertyConverter<TEntity>(memberExpression.Member, out var v))
+ {
+ converterAttribute = v;
</code_context>
<issue_to_address>
TryGetPropertyConverter should handle UnaryExpression in propertyExpression.
Like AddOrUpdatePropertyConverter, this method should also handle cases where the expression body is a UnaryExpression, especially for value types, to ensure all property converters are recognized.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
public bool TryGetPropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, [NotNullWhen(true)] out SocketDataPropertyConverterAttribute? converterAttribute)
{
converterAttribute = null;
var ret = false;
if (propertyExpression.Body is MemberExpression memberExpression && TryGetPropertyConverter<TEntity>(memberExpression.Member, out var v))
{
converterAttribute = v;
ret = true;
}
return ret;
}
=======
public bool TryGetPropertyConverter<TEntity>(Expression<Func<TEntity, object?>> propertyExpression, [NotNullWhen(true)] out SocketDataPropertyConverterAttribute? converterAttribute)
{
converterAttribute = null;
var ret = false;
MemberExpression? memberExpression = null;
if (propertyExpression.Body is MemberExpression directMember)
{
memberExpression = directMember;
}
else if (propertyExpression.Body is UnaryExpression unary && unary.Operand is MemberExpression unaryMember)
{
memberExpression = unaryMember;
}
if (memberExpression != null && TryGetPropertyConverter<TEntity>(memberExpression.Member, out var v))
{
converterAttribute = v;
ret = true;
}
return ret;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/BootstrapBlazor/Extensions/ITcpSocketClientExtensions.cs:153` </location>
<code_context>
+ converter = client.GetSocketDataConverter<TEntity>();
+ }
+
+ if (converter == null)
+ {
+ // 设置正常回调
adapter.ReceivedCallBack = async buffer => await callback(default);
}
+ else
+ {
+ // 设置转化器
</code_context>
<issue_to_address>
Consider logging or diagnostics when no converter is found.
Logging a warning or adding diagnostics here would aid troubleshooting when no converter is found.
Suggested implementation:
```csharp
if (converter == null)
{
// 日志警告：未找到数据转换器
// 假设有一个名为 Logger 的静态日志实例可用
Logger?.LogWarning("No ISocketDataConverter<{EntityType}> found for {EntityType}. Using default callback.", typeof(TEntity).Name, typeof(TEntity).Name);
// 设置正常回调
adapter.ReceivedCallBack = async buffer => await callback(default);
}
```
- If you do not have a `Logger` instance available, you should inject or otherwise provide an `ILogger` (or your diagnostics mechanism) to this context.
- Replace `Logger?.LogWarning` with your actual logging or diagnostics call as appropriate for your project.
</issue_to_address>
### Comment 5
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:813` </location>
<code_context>
+
+ // 等待接收数据处理完成
+ await tcs.Task;
+ Assert.NotNull(entity);
+ Assert.Equal([1, 2, 3, 4, 5], entity.Header);
+ Assert.Equal([3, 4], entity.Body);
+
+ server.Stop();
</code_context>
<issue_to_address>
Consider asserting the full state of the entity, including uninitialized properties.
Additionally, check that any other properties remain at their default values to confirm the converter doesn't set unintended fields.
Suggested implementation:
```csharp
// 等待接收数据处理完成
await tcs.Task;
Assert.NotNull(entity);
Assert.Equal([1, 2, 3, 4, 5], entity.Header);
Assert.Equal([3, 4], entity.Body);
// Assert the full state of the entity, including uninitialized properties
// (Assuming the entity type is MyEntity, replace with actual type if different)
Assert.Null(entity.SomeOtherProperty); // Example: check a string property is null
Assert.Equal(0, entity.SomeIntProperty); // Example: check an int property is default
Assert.False(entity.SomeBoolProperty); // Example: check a bool property is default
// Add more assertions for all properties as appropriate
server.Stop();
```
- Replace `SomeOtherProperty`, `SomeIntProperty`, and `SomeBoolProperty` with the actual property names and types of your entity.
- Add assertions for all properties of the entity to ensure their state is as expected (either set by the converter or at their default values).
</issue_to_address>
### Comment 6
<location> `src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverter.cs:55` </location>
<code_context>
var ret = false;
if (entity != null)
{
+ var unuseProperties = new List<PropertyInfo>(32);
+
+ // 通过 SocketDataPropertyConverterAttribute 特性获取属性转换器
</code_context>
<issue_to_address>
Consider removing the unused property list and consolidating converter lookup into a single helper method for clarity.
```csharp
// Remove the unused `unuseProperties` list and inline dual‐lookup logic.
// Consolidate attribute resolution into one helper, then simplify Parse:
protected virtual bool Parse(ReadOnlyMemory<byte> data, TEntity entity)
{
if (entity == null) return false;
// collect only properties with a converter (either from attribute or from `converters`)
var props = typeof(TEntity)
.GetProperties(BindingFlags.Public | BindingFlags.Instance)
.Where(p => p.CanWrite)
.Select(p => (p, attr: ResolveConverter(p)))
.Where(x => x.attr is not null);
foreach (var (prop, attr) in props)
{
prop.SetValue(entity, attr!.ConvertTo(data));
}
return true;
}
private SocketDataPropertyConverterAttribute? ResolveConverter(PropertyInfo p) =>
p.GetCustomAttribute<SocketDataPropertyConverterAttribute>(inherit: false)
?? (converters.TryGetPropertyConverter<TEntity>(p, out var conv) ? conv : null);
```
• Drops the unneeded `new List<PropertyInfo>(32)`
• Centralizes converter lookup into `ResolveConverter`
• Maintains identical behavior with a single, clear attribute path.
</issue_to_address>
### Comment 7
<location> `src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs:85` </location>
<code_context>
+ /// 获得指定数据类型属性转换器方法
+ /// </summary>
+ /// <typeparam name="TEntity"></typeparam>
+ public bool TryGetPropertyConverter<TEntity>(MemberInfo memberInfo, [NotNullWhen(true)] out SocketDataPropertyConverterAttribute? converterAttribute)
+ {
+ converterAttribute = null;
</code_context>
<issue_to_address>
Consider refactoring the two TryGetPropertyConverter overloads into a single core MemberInfo-based method with a thin Expression wrapper to reduce duplication.
Here’s one way to collapse the two `TryGetPropertyConverter` overloads into a single “core” MemberInfo lookup plus a tiny Expression‐wrapper. This removes the redundant generic overload and simplifies the boolean plumbing:
```csharp
public class SocketDataConverterCollections
{
// …
// 1) Core lookup: no TEntity generic, just MemberInfo
public bool TryGetPropertyConverter(MemberInfo member, [NotNullWhen(true)] out SocketDataPropertyConverterAttribute? attr)
=> _propertyConverters.TryGetValue(member, out attr);
// 2) Expression‐wrapper: thin shell over the MemberInfo API
public bool TryGetPropertyConverter<TEntity>(
Expression<Func<TEntity, object?>> expr,
[NotNullWhen(true)] out SocketDataPropertyConverterAttribute? attr)
{
if (expr.Body is not MemberExpression m)
{
attr = null;
return false;
}
// set default Type if not already set
if (_propertyConverters.TryGetValue(m.Member, out var existing) && existing.Type == null)
{
existing.Type = m.Type;
}
return TryGetPropertyConverter(m.Member, out attr);
}
// 3) You can remove the old generic MemberInfo overload entirely:
// public bool TryGetPropertyConverter<TEntity>(MemberInfo memberInfo, …) { … }
// … rest of class …
}
```
Key wins:
- Only one “core” lookup method; all callers (Expression or direct MemberInfo) go through it.
- No more duplicate generics on MemberInfo.
- Boolean return is a single expression, no local `ret` required.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverter.cs
Show resolved
Hide resolved
src/BootstrapBlazor/Services/TcpSocket/DataConverter/SocketDataConverterCollections.cs
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6449 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 749 +1
Lines 32266 32359 +93
Branches 4561 4571 +10
=========================================
+ Hits 32266 32359 +93
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #6448
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a global socket data converter configuration mechanism and integrate it with DI to streamline type and property converter registration and resolution.
New Features:
Enhancements:
Documentation:
Tests: