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

Support Dynamic Complex Types #2146

Merged
merged 11 commits into from
Jul 16, 2023

Conversation

MarkusHorstmann
Copy link
Contributor

Proposed changes

Complex Type support assumes that every OPC type is represented by exactly one .Net type, which in turn knows encoding ids, XML names etc.
These changes enable use of a single type that implements encoding/decoding based on OPC type information at runtime.

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.

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #2146 (0f1085a) into master (5f54e08) will increase coverage by 0.01%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master    #2146      +/-   ##
==========================================
+ Coverage   57.90%   57.92%   +0.01%     
==========================================
  Files         324      324              
  Lines       61826    61833       +7     
==========================================
+ Hits        35801    35816      +15     
+ Misses      26025    26017       -8     
Impacted Files Coverage Δ
...ck/Opc.Ua.Core/Types/Encoders/EncodeableFactory.cs 70.67% <40.00%> (-1.20%) ⬇️
Stack/Opc.Ua.Core/Types/Encoders/XmlDecoder.cs 76.65% <100.00%> (+0.08%) ⬆️
Stack/Opc.Ua.Core/Types/Encoders/XmlEncoder.cs 76.89% <100.00%> (+0.16%) ⬆️

... and 7 files with indirect coverage changes

mregen added a commit that referenced this pull request May 19, 2023
…2155)

Cherry pick bugfix from #2146:
ReadExtensionObject returns partial JSON for some types.
It appears that this is due to the JSON being read before the JsonTextWriter is closed.

see also: 2a4ba56

Co-authored-by: Markus Horstmann <markushorstmann@hotmail.com>
@mregen
Copy link
Contributor

mregen commented Jun 1, 2023

Hi @MarkusHorstmann , please elaborate how this extension can be used, or even better provide a test case / sample. I already cherry picked your bug fix, thanks!

@MarkusHorstmann
Copy link
Contributor Author

Hi @MarkusHorstmann , please elaborate how this extension can be used, or even better provide a test case / sample. I already cherry picked your bug fix, thanks!

I have added test cases using a dynamic encodeable with simple type information (just a list of string-valued fields). For a realistic use case feel free to look at the CESMII NodeSet utilities. It is not meant to be a sample, though.

@mregen mregen self-assigned this Jun 3, 2023
@mregen mregen added this to the 1.4.372 feature update milestone Jun 3, 2023
@mregen mregen added enhancement API or feature enhancement complex Complex Types labels Jun 3, 2023
@mregen mregen requested a review from mrsuciu June 14, 2023 09:49
@mregen
Copy link
Contributor

mregen commented Jun 14, 2023

Hi @MarkusHorstmann, looks pretty good, any idea why the tests are failing?

@mregen
Copy link
Contributor

mregen commented Jun 14, 2023

Check failure on line 1 in Opc.Ua.Core.Tests.Types.Encoders.JsonEncoderTests.Test_ExtensionObjectWithDynamicEncodeable

@azure-pipelinesazure-pipelines
/ OPCFoundation.UA-.NETStandard
Opc.Ua.Core.Tests.Types.Encoders.JsonEncoderTests.Test_ExtensionObjectWithDynamicEncodeable
System.InvalidOperationException : Collection was modified; enumeration operation may not execute.

Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

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

please fix the test cases

@MarkusHorstmann
Copy link
Contributor Author

please fix the test cases

The new tests should now run fine. There are two other tests failing (ReadOnDiscoveryChannel and AppVeyor build) that don't seem to be related to my changes, and I've seen them fail in other branches as well. Please let me know if you think otherwise.

@MarkusHorstmann
Copy link
Contributor Author

Oops, thought I was closing the review comment, not the PR. Reopened it.

Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

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

Hi @MarkusHorstmann, we discussed the changes today but some questions came up. We need to follow up in a seperate thread.

/// </summary>
/// <returns>The xml qualified name for the type instance</returns>
XmlQualifiedName GetXmlName(IServiceMessageContext context);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It is unclear why the IServiceMessageContext is needed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A IComplexTypeInstance implementation needs the Encodable Factory and the Namespace Table from the context. The reason is that the implementation may need to make additional nested types in a structure available via the Factory. Otherwise, all known types would have to be pre-registered, which largely defeats the purpose of a truly dynamic (= meta-data-driven) type implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mregen Please let me know if you need any additional information or maybe want to discuss on a call?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @MarkusHorstmann , we discussed the PR yesterday in the user group and it is accepted, just taking a final check then its ready to go. Thanks!

@gabrielgriesser
Copy link

Hello, I'm interested about this feature. I have to create Structure Type (so ExtensionObject) XML from my autoDiscovery code.

I am able to auto discover my StructureDefinition :

`
foreach (var result in browseResults)
{
if (result.NodeClass != NodeClass.DataType)
continue;

        var varNodeId = NodeId.Parse(result.NodeId.ToString());

        if (Session.ReadNode(varNodeId) is DataTypeNode dataTypeNode && (dataTypeNode.DataTypeDefinition != null))
            if (result.DisplayName.Text.Contains("ST_"))
            {
                Debug.WriteLine("     DatTypeNode = {0}, NodeClass = {1}, NodeID = {2}, NodeFields = {3}", dataTypeNode.DisplayName.Text, dataTypeNode.NodeClass, dataTypeNode,
                    dataTypeNode.DataTypeDefinition);
                var sd = dataTypeNode.DataTypeDefinition.Body as StructureDefinition;

                // TODO Create my own XML from sd structure
            }


        BrowseRecursive(browser, varNodeId);
    }

`

But now, how can I dynamically create an XML file and how to use it to decode my ExtensionObject when I read my variable using Session.ReadValue() ?

@MarkusHorstmann
Copy link
Contributor Author

MarkusHorstmann commented Jun 27, 2023

Hello, I'm interested about this feature. I have to create Structure Type (so ExtensionObject) XML from my autoDiscovery code.

I am able to auto discover my StructureDefinition :

` foreach (var result in browseResults) { if (result.NodeClass != NodeClass.DataType) continue;

        var varNodeId = NodeId.Parse(result.NodeId.ToString());

        if (Session.ReadNode(varNodeId) is DataTypeNode dataTypeNode && (dataTypeNode.DataTypeDefinition != null))
            if (result.DisplayName.Text.Contains("ST_"))
            {
                Debug.WriteLine("     DatTypeNode = {0}, NodeClass = {1}, NodeID = {2}, NodeFields = {3}", dataTypeNode.DisplayName.Text, dataTypeNode.NodeClass, dataTypeNode,
                    dataTypeNode.DataTypeDefinition);
                var sd = dataTypeNode.DataTypeDefinition.Body as StructureDefinition;

                // TODO Create my own XML from sd structure
            }


        BrowseRecursive(browser, varNodeId);
    }

`

But now, how can I dynamically create an XML file and how to use it to decode my ExtensionObject when I read my variable using Session.ReadValue() ?

Hi, @gabrielgriesser! This Pull Request only provides the necessary hooks in the Core to make it possible implement dynamic types. I have implemented a dynamic type that I need in the context of a NodeSet editing tool, and it uses the editing tool's type system not the OPC SDK's type definition classes. The code is publically available here (https://github.com/cesmii/CESMII-NodeSet-Utilities/blob/main/src/CESMII.OpcUa.NodeSetModel.Factory.Opc/DynamicComplexType.cs). It should be relatively straightforward to change it to read the type information from a DataTypeDefinition instead of the NodeModel's DataTypeModel.

I also implemented a test case that implements a rudimentary dynamic type that handles only string values. This could be another starting point.

/// </summary>
/// <returns>The xml qualified name for the type instance</returns>
XmlQualifiedName GetXmlName(IServiceMessageContext context);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @MarkusHorstmann , we discussed the PR yesterday in the user group and it is accepted, just taking a final check then its ready to go. Thanks!

@mregen
Copy link
Contributor

mregen commented Jul 14, 2023

@MarkusHorstmann please merge master into your PR, the ci VMs had been updated recently so the failing build will not pass otherwise and i can't merge.

@mregen mregen merged commit 2738a29 into OPCFoundation:master Jul 16, 2023
TimJoehnk pushed a commit to TimJoehnk/UA-.NETStandard that referenced this pull request Aug 4, 2023
Complex Type support assumes that every OPC type is represented by exactly one .Net type, which in turn knows encoding ids, XML names etc.
These changes enable use of a single type that implements encoding/decoding based on OPC type information at runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complex Complex Types enhancement API or feature enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JsonDecoder.ReadExtensionObject returns no or partial Json
3 participants