Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3211 +/- ##
==========================================
+ Coverage 57.42% 57.47% +0.04%
==========================================
Files 355 355
Lines 78540 78540
Branches 13855 13855
==========================================
+ Hits 45104 45139 +35
+ Misses 29182 29149 -33
+ Partials 4254 4252 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in ExtendedNodeId JSON encoding where the JSON structure was not properly closed when ServerUri was present. It also modernizes test code by updating string formatting from concatenation and interpolation to raw string literals.
- Fixes incorrect JSON encoding for ExtendedNodeId with ServerUri by moving
PopStructure()call outside the conditional block - Updates all test string literals to use C# 11+ raw string literal syntax ("""...""") for better readability
- Adds comprehensive test case for ExtendedNodeId with ServerUri to verify the fix
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Stack/Opc.Ua.Core/Types/Encoders/JsonEncoder.cs | Fixes JSON structure closure bug by moving PopStructure() call outside ServerUri conditional |
| Tests/Opc.Ua.Core.Tests/Types/Encoders/JsonEncoderTests.cs | Updates test strings to raw literals and adds test case for ExtendedNodeId with ServerUri |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hermotech
left a comment
hermotech
left a comment
There was a problem hiding this comment.
This fixes the issue the same way I tested in parallel.
Thank you!
|
Thanks for the quick action on the bug report and providing a fix. Much appreciated! We unfortunately have a blocker that prevents us from updating to the latest version of the packages. The blocker can be explained as follows: Currently we serialize alarm event fields using serializer.Serialize() and not the JsonEncoder. We cannot easily and quickly get customers to change to a new schema (result of regular system serialization with newer versions) We need the JsonEncoder fix here as a patch for the 1.5.374.* release so that we can introduce a new feature that will use JsonEncoder with the fix here, but still maintain the same json schema we achieve for the existing feature where JsonEncoder is not yet used. Would it be possible to cherry pick the fix for JsonEncoder here to the 374 branch so that a patch can be released? |
Proposed changes
Describe the changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...