Conversation
Reviewer's GuideThis PR adds a new OPC DA extension library to BootstrapBlazor, defining core service and subscription interfaces, concrete implementations for OPC DA server operations (connect, read, write, subscriptions), data models and utilities, DI registration, and comprehensive unit tests. Class diagram for new OPC DA extension libraryclassDiagram
class IOpcServer {
+bool IsConnected
+string? ServerName
+bool Connect(string serverName)
+void Disconnect()
+void CancelSubscription(ISubscription subscription)
+ISubscription CreateSubscription(string name, int updateRate = 1000, bool active = true)
+HashSet<OpcReadItem> Read(params HashSet<string> items)
+HashSet<OpcWriteItem> Write(params HashSet<OpcWriteItem> items)
}
class OpcServer {
-Opc.Da.Server? _server
-ConcurrentDictionary<string, HashSet<OpcReadItem>> _valuesCache
+string? ServerName
+bool IsConnected
+bool Connect(string serverName)
+void Disconnect()
+ISubscription CreateSubscription(string name, int updateRate = 1000, bool active = true)
+void CancelSubscription(ISubscription subscription)
+HashSet<OpcReadItem> Read(params HashSet<string> items)
+HashSet<OpcWriteItem> Write(params HashSet<OpcWriteItem> items)
+void Dispose()
}
IOpcServer <|.. OpcServer
class ISubscription {
+bool KeepLastValue
+Action<List<OpcReadItem>>? DataChanged
+Opc.Da.ISubscription GetSubscription()
+void AddItems(IEnumerable<string> items)
}
class OpcSubscription {
+Action<List<OpcReadItem>>? DataChanged
+bool KeepLastValue
+Opc.Da.ISubscription GetSubscription()
+void AddItems(IEnumerable<string> items)
}
ISubscription <|.. OpcSubscription
class IOpcItem {
+string Name
}
class OpcReadItem {
+string Name
+Quality Quality
+DateTime Timestamp
+object? Value
+object? LastValue
}
class OpcWriteItem {
+string Name
+object? Value
+bool Result
}
IOpcItem <|.. OpcReadItem
IOpcItem <|.. OpcWriteItem
class OpcItemEqualityComparer~TItem~ {
+static OpcItemEqualityComparer<TItem> Default
+bool Equals(TItem? x, TItem? y)
+int GetHashCode(TItem item)
}
class Quality {
<<enum>>
Bad
Good
}
Class diagram for service registration extensionclassDiagram
class ServiceCollectionExtensions {
+IServiceCollection AddOpcServer(this IServiceCollection services)
}
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:
- Replace the Read and Write method signatures (using params HashSet<…>) with more conventional params string[] or IEnumerable overloads to simplify the API and caller usage.
- Remove or implement the unused private _valuesCache field in OpcServer to avoid confusion or dead code.
- Wrap OPC COM calls (e.g. _server.Connect, Read, Write) in try/catch blocks to provide clearer error feedback instead of letting COM exceptions bubble up.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replace the Read and Write method signatures (using params HashSet<…>) with more conventional params string[] or IEnumerable<T> overloads to simplify the API and caller usage.
- Remove or implement the unused private _valuesCache field in OpcServer to avoid confusion or dead code.
- Wrap OPC COM calls (e.g. _server.Connect, Read, Write) in try/catch blocks to provide clearer error feedback instead of letting COM exceptions bubble up.
## Individual Comments
### Comment 1
<location> `src/extensions/BootstrapBlazor.OpcDa/OpcServer.cs:104` </location>
<code_context>
+ /// </summary>
+ /// <param name="items"></param>
+ /// <returns></returns>
+ public HashSet<OpcReadItem> Read(params HashSet<string> items)
+ {
+ var server = GetOpcServer();
</code_context>
<issue_to_address>
Using params with HashSet<string> may be confusing and unnecessary.
'params' is generally used with arrays, not collections like HashSet. Consider changing the parameter to 'HashSet<string> items' or 'params string[] items' for clarity and consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// <summary>
/// 读取指定 Item 值方法
/// </summary>
/// <param name="items"></param>
/// <returns></returns>
+ public HashSet<OpcReadItem> Read(params HashSet<string> items)
+ {
+ var server = GetOpcServer();
=======
/// <summary>
/// 读取指定 Item 值方法
/// </summary>
/// <param name="items"></param>
/// <returns></returns>
public HashSet<OpcReadItem> Read(HashSet<string> items)
{
var server = GetOpcServer();
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/extensions/BootstrapBlazor.OpcDa/OpcServer.cs:116` </location>
<code_context>
+ /// </summary>
+ /// <param name="items"></param>
+ /// <returns></returns>
+ public HashSet<OpcWriteItem> Write(params HashSet<OpcWriteItem> items)
+ {
+ var server = GetOpcServer();
</code_context>
<issue_to_address>
The use of 'params' with HashSet<OpcWriteItem> is unconventional.
Consider changing the parameter to 'params OpcWriteItem[] items' or simply 'HashSet<OpcWriteItem> items' for clarity and consistency.
Suggested implementation:
```csharp
/// <param name="items">The OPC items to write.</param>
/// <returns></returns>
public HashSet<OpcWriteItem> Write(params OpcWriteItem[] items)
{
var server = GetOpcServer();
```
```csharp
var results = server.Read([.. items.Select(i => new Item() { ItemName = i })]);
return results.Select(i => new OpcReadItem(i.ItemName, i.Quality.ToQuality(), i.Timestamp, i.Value)).ToHashSet(OpcItemEqualityComparer<OpcReadItem>.Default);
}
```
If the method is called elsewhere in the codebase, those call sites may need to be updated to pass individual `OpcWriteItem` arguments or an array, rather than a `HashSet<OpcWriteItem>`.
</issue_to_address>
### Comment 3
<location> `src/extensions/BootstrapBlazor.OpcDa/OpcSubscription.cs:33` </location>
<code_context>
+ Quality = i.Quality == Opc.Da.Quality.Good ? Quality.Good : Quality.Bad,
+ Timestamp = i.Timestamp
+ };
+ if (KeepLastValue)
+ {
+ var v = _cache.Find(i => i.Name == item.Name);
+ item.LastValue = v.Value;
+ }
</code_context>
<issue_to_address>
Potential NullReferenceException when accessing v.Value if v is not found.
Check if 'v' is not null before accessing 'v.Value' to prevent a NullReferenceException.
</issue_to_address>
### Comment 4
<location> `src/extensions/BootstrapBlazor.OpcDa/OpcItemEqualityComparer.cs:30` </location>
<code_context>
+ /// </summary>
+ /// <param name="item"></param>
+ /// <returns></returns>
+ public int GetHashCode([DisallowNull] TItem item) => item.Name.GetHashCode();
+}
</code_context>
<issue_to_address>
GetHashCode does not handle null Name property.
Add a null check for Name or enforce that Name cannot be null to prevent NullReferenceException.
</issue_to_address>
### Comment 5
<location> `test/UnitTestOpcDa/UnitTest1.cs:14` </location>
<code_context>
+[SupportedOSPlatform("windows")]
+public class UnitTest1
+{
+ [Fact]
+ public void Write_Ok()
+ {
+ var sc = new ServiceCollection();
</code_context>
<issue_to_address>
Missing negative and error case tests for OPC DA server connection and operations.
Please add tests for invalid server names, invalid item names, and operations when not connected to improve error handling coverage.
</issue_to_address>
### Comment 6
<location> `test/UnitTestOpcDa/UnitTest1.cs:30` </location>
<code_context>
+ Assert.Equal(2, values.Count);
+ Assert.All(values, v => Assert.Equal(Quality.Good, v.Quality));
+
+ var results = server.Write([
+ new OpcWriteItem()
+ {
+ Name = "Channel1.Device1.Tag2",
+ Value = 123
+ }
+ ]);
+ Assert.All(results, v => Assert.True(v.Result));
+
+ server.Disconnect();
</code_context>
<issue_to_address>
Test does not verify write failures or partial successes.
Please add a test case where a write operation is expected to fail (e.g., using an invalid tag name) and verify that the Result property is false for failed writes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var results = server.Write([
new OpcWriteItem()
{
Name = "Channel1.Device1.Tag2",
Value = 123
}
]);
Assert.All(results, v => Assert.True(v.Result));
server.Disconnect();
=======
var results = server.Write([
new OpcWriteItem()
{
Name = "Channel1.Device1.Tag2",
Value = 123
}
]);
Assert.All(results, v => Assert.True(v.Result));
// Test write failure with invalid tag name
var failedResults = server.Write([
new OpcWriteItem()
{
Name = "Invalid.Tag.Name",
Value = 456
}
]);
Assert.Single(failedResults);
Assert.False(failedResults[0].Result);
// Test partial success: one valid, one invalid
var mixedResults = server.Write([
new OpcWriteItem()
{
Name = "Channel1.Device1.Tag2",
Value = 789
},
new OpcWriteItem()
{
Name = "Invalid.Tag.Name",
Value = 1011
}
]);
Assert.Equal(2, mixedResults.Count);
Assert.True(mixedResults[0].Result);
Assert.False(mixedResults[1].Result);
server.Disconnect();
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+99
to
+106
| /// <summary> | ||
| /// 读取指定 Item 值方法 | ||
| /// </summary> | ||
| /// <param name="items"></param> | ||
| /// <returns></returns> | ||
| public HashSet<OpcReadItem> Read(params HashSet<string> items) | ||
| { | ||
| var server = GetOpcServer(); |
There was a problem hiding this comment.
suggestion: Using params with HashSet may be confusing and unnecessary.
'params' is generally used with arrays, not collections like HashSet. Consider changing the parameter to 'HashSet items' or 'params string[] items' for clarity and consistency.
Suggested change
| /// <summary> | |
| /// 读取指定 Item 值方法 | |
| /// </summary> | |
| /// <param name="items"></param> | |
| /// <returns></returns> | |
| public HashSet<OpcReadItem> Read(params HashSet<string> items) | |
| { | |
| var server = GetOpcServer(); | |
| /// <summary> | |
| /// 读取指定 Item 值方法 | |
| /// </summary> | |
| /// <param name="items"></param> | |
| /// <returns></returns> | |
| public HashSet<OpcReadItem> Read(HashSet<string> items) | |
| { | |
| var server = GetOpcServer(); |
| /// </summary> | ||
| /// <param name="items"></param> | ||
| /// <returns></returns> | ||
| public HashSet<OpcWriteItem> Write(params HashSet<OpcWriteItem> items) |
There was a problem hiding this comment.
suggestion: The use of 'params' with HashSet is unconventional.
Consider changing the parameter to 'params OpcWriteItem[] items' or simply 'HashSet items' for clarity and consistency.
Suggested implementation:
/// <param name="items">The OPC items to write.</param>
/// <returns></returns>
public HashSet<OpcWriteItem> Write(params OpcWriteItem[] items)
{
var server = GetOpcServer(); var results = server.Read([.. items.Select(i => new Item() { ItemName = i })]);
return results.Select(i => new OpcReadItem(i.ItemName, i.Quality.ToQuality(), i.Timestamp, i.Value)).ToHashSet(OpcItemEqualityComparer<OpcReadItem>.Default);
}If the method is called elsewhere in the codebase, those call sites may need to be updated to pass individual OpcWriteItem arguments or an array, rather than a HashSet<OpcWriteItem>.
Comment on lines
+33
to
+35
| if (KeepLastValue) | ||
| { | ||
| var v = _cache.Find(i => i.Name == item.Name); |
There was a problem hiding this comment.
issue (bug_risk): Potential NullReferenceException when accessing v.Value if v is not found.
Check if 'v' is not null before accessing 'v.Value' to prevent a NullReferenceException.
| /// </summary> | ||
| /// <param name="item"></param> | ||
| /// <returns></returns> | ||
| public int GetHashCode([DisallowNull] TItem item) => item.Name.GetHashCode(); |
There was a problem hiding this comment.
issue: GetHashCode does not handle null Name property.
Add a null check for Name or enforce that Name cannot be null to prevent NullReferenceException.
Comment on lines
+14
to
+15
| [Fact] | ||
| public void Write_Ok() |
There was a problem hiding this comment.
suggestion (testing): Missing negative and error case tests for OPC DA server connection and operations.
Please add tests for invalid server names, invalid item names, and operations when not connected to improve error handling coverage.
Comment on lines
+30
to
+39
| var results = server.Write([ | ||
| new OpcWriteItem() | ||
| { | ||
| Name = "Channel1.Device1.Tag2", | ||
| Value = 123 | ||
| } | ||
| ]); | ||
| Assert.All(results, v => Assert.True(v.Result)); | ||
|
|
||
| server.Disconnect(); |
There was a problem hiding this comment.
suggestion (testing): Test does not verify write failures or partial successes.
Please add a test case where a write operation is expected to fail (e.g., using an invalid tag name) and verify that the Result property is false for failed writes.
Suggested change
| var results = server.Write([ | |
| new OpcWriteItem() | |
| { | |
| Name = "Channel1.Device1.Tag2", | |
| Value = 123 | |
| } | |
| ]); | |
| Assert.All(results, v => Assert.True(v.Result)); | |
| server.Disconnect(); | |
| var results = server.Write([ | |
| new OpcWriteItem() | |
| { | |
| Name = "Channel1.Device1.Tag2", | |
| Value = 123 | |
| } | |
| ]); | |
| Assert.All(results, v => Assert.True(v.Result)); | |
| // Test write failure with invalid tag name | |
| var failedResults = server.Write([ | |
| new OpcWriteItem() | |
| { | |
| Name = "Invalid.Tag.Name", | |
| Value = 456 | |
| } | |
| ]); | |
| Assert.Single(failedResults); | |
| Assert.False(failedResults[0].Result); | |
| // Test partial success: one valid, one invalid | |
| var mixedResults = server.Write([ | |
| new OpcWriteItem() | |
| { | |
| Name = "Channel1.Device1.Tag2", | |
| Value = 789 | |
| }, | |
| new OpcWriteItem() | |
| { | |
| Name = "Invalid.Tag.Name", | |
| Value = 1011 | |
| } | |
| ]); | |
| Assert.Equal(2, mixedResults.Count); | |
| Assert.True(mixedResults[0].Result); | |
| Assert.False(mixedResults[1].Result); | |
| server.Disconnect(); |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 #513
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a new OPC DA extension for BootstrapBlazor that implements core server interaction APIs, data models, and DI integration, accompanied by unit tests for both OPC DA and OPC UA scenarios
New Features:
Build:
Tests: