feat(ISocketDataConverter): add ISocketDataConverter interface #6420
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 generic ISocketDataConverter interface (with a base class) to unify socket data conversion, refactors IDataPackageAdapter and DataPackageAdapter to use the new converter-based TryConvertTo API, updates the ITcpSocketClientExtensions to accept a converter parameter, and aligns unit tests to the new API. Class diagram for ISocketDataConverter and related classesclassDiagram
class ISocketDataConverter~TEntity~ {
+bool TryConvertTo(ReadOnlyMemory<byte> data, out TEntity? entity)
}
class SocketDataConverterBase~TEntity~ {
<<abstract>>
+bool TryConvertTo(ReadOnlyMemory<byte> data, out TEntity? entity)
}
ISocketDataConverter~TEntity~ <|.. SocketDataConverterBase~TEntity~
class IDataPackageAdapter {
+ValueTask HandlerAsync(ReadOnlyMemory<byte> data, CancellationToken token)
+bool TryConvertTo~TEntity~(ReadOnlyMemory<byte> data, ISocketDataConverter~TEntity~ socketDataConverter, out TEntity? entity)
}
class DataPackageAdapter {
+ValueTask HandlerAsync(ReadOnlyMemory<byte> data, CancellationToken token)
+bool TryConvertTo~TEntity~(ReadOnlyMemory<byte> data, ISocketDataConverter~TEntity~ socketDataConverter, out TEntity? entity)
}
IDataPackageAdapter <|.. DataPackageAdapter
Class diagram for ITcpSocketClientExtensions.SetDataPackageAdapter changesclassDiagram
class ITcpSocketClientExtensions {
+void SetDataPackageAdapter~TEntity~(ITcpSocketClient client, IDataPackageAdapter adapter, ISocketDataConverter~TEntity~ socketDataConverter, Func<TEntity?, Task> callback)
}
class ITcpSocketClient {
+ReceivedCallBack
}
ITcpSocketClientExtensions ..> ITcpSocketClient : uses
ITcpSocketClientExtensions ..> IDataPackageAdapter : uses
ITcpSocketClientExtensions ..> ISocketDataConverter~TEntity~ : uses
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Services/TcpSocket/DataPackageAdapter/DataPackageAdapter.cs:56` </location>
<code_context>
{
- entity = null;
- return false;
+ entity = default;
+ if (socketDataConverter.TryConvertTo(data, out var v))
+ {
+ entity = v;
+ }
+ return entity != null;
}
</code_context>
<issue_to_address>
Returning 'entity != null' may not be correct for value types.
For value types, 'entity != null' will always be true, even if conversion fails. Use the result of TryConvertTo or another method to indicate success for value types.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:679` </location>
<code_context>
- var result = adapter2.TryConvertTo(data, out var t);
- Assert.False(result);
- Assert.Null(t);
+ var result = adapter2.TryConvertTo(data, new MockEntitySocketDataConverter(), out var t);
+ Assert.True(result);
+ Assert.NotNull(t);
+ Assert.Equal([1, 2, 3, 4, 5], entity.Header);
}
</code_context>
<issue_to_address>
Test coverage for failed conversion scenarios is missing.
Please add a test case where TryConvertTo returns false and the entity is null, to ensure failed conversions are properly handled.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var result = adapter2.TryConvertTo(data, new MockEntitySocketDataConverter(), out var t);
Assert.True(result);
Assert.NotNull(t);
Assert.Equal([1, 2, 3, 4, 5], entity.Header);
}
=======
var result = adapter2.TryConvertTo(data, new MockEntitySocketDataConverter(), out var t);
Assert.True(result);
Assert.NotNull(t);
Assert.Equal([1, 2, 3, 4, 5], entity.Header);
}
[Fact]
public void TryConvertTo_FailedConversion_ReturnsFalseAndNullEntity()
{
// Arrange
var adapter = new DataPackageAdapter();
var invalidData = new byte[] { 0xFF, 0xFF }; // Use data that will fail conversion
var converter = new MockEntitySocketDataConverter();
// Act
var result = adapter.TryConvertTo(invalidData, converter, out var entity);
// Assert
Assert.False(result);
Assert.Null(entity);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `test/UnitTest/Services/TcpSocketFactoryTest.cs:714` </location>
<code_context>
await tcs.Task;
- Assert.Null(entity);
+ Assert.NotNull(entity);
}
</code_context>
<issue_to_address>
Test assertion may not match the intended error scenario.
Please clarify whether this test is meant to check for a failed conversion (entity should be null). If not, consider renaming the test or adding a comment to explain the expected behavior. Also, ensure there is a test for the null entity case after a failed conversion.
</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/DataPackageAdapter/DataPackageAdapter.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6420 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 722 722
Lines 31844 31846 +2
Branches 4492 4493 +1
=========================================
+ Hits 31844 31846 +2
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:
|
ArgoZhang
added a commit
that referenced
this pull request
Jul 16, 2025
…interface (#6422) * feat(ISocketDataConverter): add ISocketDataConverter interface (#6420) * feat: 增加数据转换器设计 * refactor: 数据处理器增加数据转化器适配 * test: 增加单元测试 * refactor: 更新判断转换成功逻辑 * test: 更新单元测试 * feat: 增加 SocketDataConverter 标签 * feat: 增加 Parse 转化器 * test: 增加单元测试 * feat: 增加无符号数据类型转换 * test: 增加单元测试 * refactor: 精简代码 * test: 更新单元测试 * feat: 增加防止死锁逻辑 * test: 增加单元测试 * refactor: 重构代码 * feat: 增加转换器 * feat: 增加 GetConverterByType 扩展方法 * test: 更新单元测试 * test: 增加单元测试
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 #6419
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a typed socket data converter interface and integrate it into the data package adapter pipeline, refactoring the conversion methods and updating extensions and tests to use the new approach
New Features:
Enhancements:
Tests: