Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle status code Uncertain according to the specification #2898

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

larws
Copy link
Contributor

@larws larws commented Dec 10, 2024

Proposed changes

If a method call returns Uncertain as a status code the output arguments values should be set as specified in the specification.

Related Issues

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply. You can also fill these out after creating the PR.

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in 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.

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

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...

When a method state's call method is invoked the output arguments should be written in case the status code is good or uncertain. This behavior would be conform with the current specification.
The result of the Call method in the CustomNodeManager2 class represents the status of the CallMethodResult. It does not correspond to the ServiceResult of the CallResponse, thus returning Good as a general response is incorrect behavior.
@CLAassistant
Copy link

CLAassistant commented Dec 10, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 55.44%. Comparing base (0777bbd) to head (951beb2).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
Stack/Opc.Ua.Core/Types/Utils/ServiceResult.cs 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2898   +/-   ##
=======================================
  Coverage   55.43%   55.44%           
=======================================
  Files         352      352           
  Lines       67602    67605    +3     
  Branches    13849    13851    +2     
=======================================
+ Hits        37477    37483    +6     
  Misses      26021    26021           
+ Partials     4104     4101    -3     

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

@romanett
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mregen mregen requested a review from mrsuciu December 11, 2024 14:07
@mregen
Copy link
Contributor

mregen commented Dec 11, 2024

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.

@larws please sign the contributor agreement

@larws
Copy link
Contributor Author

larws commented Dec 16, 2024

@mregen, @mrsuciu do you happen to know if there is any chance that this fix will end up in the next release? If it won't what would be a realistic release date?

@mrsuciu
Copy link
Contributor

mrsuciu commented Dec 17, 2024

@mregen, @mrsuciu do you happen to know if there is any chance that this fix will end up in the next release? If it won't what would be a realistic release date?

@larws Yes it will end up in the next release which will be most probably sometime at the beginning of next year

@mrsuciu mrsuciu merged commit 5b3f91d into OPCFoundation:master Dec 17, 2024
77 checks passed
@larws larws deleted the bug/status-code-uncertain branch December 17, 2024 13:09
mregen pushed a commit that referenced this pull request Jan 8, 2025
* #2896: Write output arguments for good and uncertain status code

When a method state's call method is invoked the output arguments should be written in case the status code is good or uncertain. This behavior would be conform with the current specification.

* #2896: The service result corresponds the method call result

The result of the Call method in the CustomNodeManager2 class represents the status of the CallMethodResult. It does not correspond to the ServiceResult of the CallResponse, thus returning Good as a general response is incorrect behavior.
mregen added a commit that referenced this pull request Jan 17, 2025
* Some cleanup of the new APIs and Union implementation.

* fix build

* Handle status code Uncertain according to the specification (#2898)

* #2896: Write output arguments for good and uncertain status code

When a method state's call method is invoked the output arguments should be written in case the status code is good or uncertain. This behavior would be conform with the current specification.

* #2896: The service result corresponds the method call result

The result of the Call method in the CustomNodeManager2 class represents the status of the CallMethodResult. It does not correspond to the ServiceResult of the CallResponse, thus returning Good as a general response is incorrect behavior.

* Add SetHiResClockDisabled  & fix Rejected Store Creation in ApplicationConfigurationBuilder (#2909)

Add the method SetHiResClockDisabled to IApplicationConfigurationBuilder
Fix the type of RejectedStore Created by the Builder to: CertificateStoreIdentifier

* revert based on feedback

* move some logic from class code to encoder/decoder

* Fix race condition on Nuget build (#2930)

- nodeset xmlzip is created in obj folders if multiple targets are built in parallel
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.

MethodState Call method does not assign output arguments when status Uncertain is returned
5 participants