Skip to content

Add TryParse methods to NodeId and ExpandedNodeId#3302

Merged
marcschier merged 5 commits intomasterfrom
copilot/add-tryparse-methods
Nov 1, 2025
Merged

Add TryParse methods to NodeId and ExpandedNodeId#3302
marcschier merged 5 commits intomasterfrom
copilot/add-tryparse-methods

Conversation

Copy link
Contributor

Copilot AI commented Oct 30, 2025

Adding TryParse methods to NodeId and ExpandedNodeId classes

Plan

  • Add TryParse(string text, out NodeId value) to NodeId class
  • Add TryParse(string text, out ExpandedNodeId value) to ExpandedNodeId class
  • Add TryParse(IServiceMessageContext, string text, out NodeId value, NodeIdParsingOptions) to NodeId class
  • Add TryParse(IServiceMessageContext, string text, out ExpandedNodeId value, NodeIdParsingOptions) to ExpandedNodeId class
  • Create comprehensive unit tests for all TryParse methods covering all identifier types (numeric, string, guid, opaque)
  • Test edge cases: null, empty strings, invalid formats, namespace handling
  • Run build and tests to validate implementation
  • Code review and security scanning
  • Refactor so Parse calls TryParse (not vice versa) to avoid try-catch for control flow
  • Refactor ExpandedNodeId InternalTryParse to avoid creating object in try-catch

Summary

Completed all requested changes. TryParse is the core implementation with parsing logic that returns bool without throwing. Parse methods call TryParse internally and throw appropriate exceptions when parsing fails. This eliminates try-catch for control flow in both NodeId and ExpandedNodeId.

The ExpandedNodeId.InternalTryParse now directly parses the components (server index, namespace URI, and inner NodeId) without creating an ExpandedNodeId object in a try-catch block.

All 1643 BuiltInType tests pass successfully.

Original prompt

This section details on the original issue you should resolve

<issue_title>Add TryParse to ExpandedNodeId and NodeId</issue_title>
<issue_description>### Type of issue

  • Bug
  • Enhancement
  • Compliance
  • Question
  • Help wanted

Current Behavior

Currently ExpandedNodeId and NodeId, just support the Parse method, that return a NodeId. Even if NodeId.Null would be possible the parse method throws Exceptions e.g. when expanded node id contains namespace that isn't in Namespace table.

Expected Behavior

Implement TryParse, that return true if it could be parsed and otherwise false.

Steps To Reproduce

No response

Environment

- OS:
- Environment:
- Runtime:
- Nuget Version:
- Component:
- Server:
- Client:

Anything else?

No response</issue_description>

<agent_instructions>Add a bool TryParse(out NodeId/ExpandedNodeId value) method to the class that has the Parse methods today. Add unit tests that validate all inputs.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI and others added 2 commits October 30, 2025 14:40
…tests

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
…e consistency

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Copilot AI changed the title [WIP] Add TryParse method to ExpandedNodeId and NodeId Add TryParse methods to NodeId and ExpandedNodeId Oct 30, 2025
Copilot AI requested a review from marcschier October 30, 2025 14:48
@marcschier
Copy link
Collaborator

@copilot Update the change to not use Try/Catch to return true or false. Instead, change the current Parse to use TryParse and still attain current behavior.

Changed architecture so TryParse contains the parsing logic without throwing exceptions.
Parse methods now call TryParse and throw appropriate exceptions on failure.
This avoids using try-catch for control flow as requested.

All 1643 BuiltInType tests pass.

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 30, 2025

@copilot Update the change to not use Try/Catch to return true or false. Instead, change the current Parse to use TryParse and still attain current behavior.

Updated the implementation per your request. TryParse is now the core implementation containing the parsing logic without throwing exceptions. Parse methods call TryParse and throw appropriate exceptions (ArgumentException or ServiceResultException) when parsing fails.

Changes in commit 081af02:

  • Created InternalTryParse methods that return bool with out parameters for both NodeId and ExpandedNodeId
  • Parse methods now call InternalTryParse and throw exceptions based on the error type
  • TryParse methods call InternalTryParse directly without any exception handling
  • All 1643 BuiltInType tests pass

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 67.00000% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.57%. Comparing base (820f6a2) to head (38c62d6).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
Stack/Opc.Ua.Core/Types/BuiltIn/ExpandedNodeId.cs 66.38% 31 Missing and 9 partials ⚠️
Stack/Opc.Ua.Core/Types/BuiltIn/NodeId.cs 67.90% 23 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3302      +/-   ##
==========================================
+ Coverage   57.86%   58.57%   +0.71%     
==========================================
  Files         367      367              
  Lines       79969    82447    +2478     
  Branches    13875    14507     +632     
==========================================
+ Hits        46274    48295    +2021     
- Misses      29490    29840     +350     
- Partials     4205     4312     +107     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

errorMessage = null;
value = Null;

try
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please also rework this try/catch to use a TryParse model instead of creating the ExpandedNodeId with the text to parse and catching any errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the ExpandedNodeId parsing in commit 38c62d6. The static InternalTryParse method now directly parses the server index, namespace URI, and inner NodeId components without creating an ExpandedNodeId object in a try-catch block.

Also updated the instance InternalParse method to use a new InternalTryParseInstance helper that implements the TryParse pattern. InternalParse now calls this helper and throws appropriate exceptions (ArgumentException or ServiceResultException) based on the error type.

All 1643 BuiltInType tests pass.

…try-catch

Reworked the static InternalTryParse method to directly parse ExpandedNodeId
components without creating an ExpandedNodeId object and catching exceptions.

Also refactored the instance InternalParse method to use a new InternalTryParseInstance
helper that implements TryParse pattern, with InternalParse calling it and throwing
appropriate exceptions (ArgumentException or ServiceResultException) on failure.

All 1643 BuiltInType tests pass.

Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
@marcschier marcschier marked this pull request as ready for review October 31, 2025 14:21
@marcschier marcschier merged commit 7626d82 into master Nov 1, 2025
179 of 194 checks passed
@romanett romanett deleted the copilot/add-tryparse-methods branch November 7, 2025 15:09
@romanett romanett mentioned this pull request Feb 10, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add TryParse to ExpandedNodeId and NodeId

4 participants