Dynamic Complex Type System for OPC UA Server applications#3818
Dynamic Complex Type System for OPC UA Server applications#3818salihgoncu wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3818 +/- ##
==========================================
+ Coverage 70.91% 71.00% +0.09%
==========================================
Files 750 751 +1
Lines 140301 140314 +13
Branches 23668 23670 +2
==========================================
+ Hits 99490 99633 +143
+ Misses 32694 32590 -104
+ Partials 8117 8091 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| </ItemGroup> | ||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\Libraries\Opc.Ua.Client\Opc.Ua.Client.csproj" /> | ||
| <ProjectReference Include="..\..\Libraries\Opc.Ua.Client.ComplexTypes\Opc.Ua.Client.ComplexTypes.csproj" /> |
There was a problem hiding this comment.
Unnecessary, Opc.Ua.Client.ComplexTypes project has its own test project (because legacy). Opc.Ua.ComplexTypes.csproj should already be brought in through Opc.Ua.CLient.csproj.
| @@ -41,6 +41,8 @@ | |||
|
|
|||
| using Opc.Ua.Core.TestFramework; | |||
|
|
|||
There was a problem hiding this comment.
Lots of empty lines above new usings, please remove (run roslynator fixer)
|
|
||
| using Opc.Ua.Client; | ||
| using Opc.Ua.ComplexTypes; | ||
| using Opc.Ua.Client.ComplexTypes; |
There was a problem hiding this comment.
This is a breaking change. While I am not against it - I would prefer one namespace. Can we move the types in Opc.Ua.Client.ComplexTypes to Opc.Ua.Client, so you bring in Opc.Ua.ComplexTypes and Opc.Ua.Client and thats it? Same on server side?
There was a problem hiding this comment.
On the other hand, Opc.Ua.Client.ComplexTypes can collapse into Opc.Ua.Client and Opc.Ua.Server.ComplexTypes likewise into Opc.Ua.Server but I think the common stuff should stay in Opc.Ua.ComplexTypes. We cannot have a reference to Opc.Ua.Client from the Server implementations. It would be awkward and unintuitive.
There was a problem hiding this comment.
Yes, when we talk about namespace, I think Opc.Ua.ComplexTypes would be referencing all common pieces, Opc.Ua.Client the client side, Opc.Ua.Server the server stuff. And Opc.Ua.Client.ComplexTypes should reference the old reflection emit stuff.
There was a problem hiding this comment.
I do not see the need for Opc.Ua.Server.ComplexTypes, can you move the code under the Opc.Ua.Server project, and into a ComplexTypes folder?
There was a problem hiding this comment.
See previous comment.
There was a problem hiding this comment.
Please move those 3 files you moved here back to Opc.Ua.Client. The reason there is Opc.Ua.Client.ComplexTypes is strictly for back compat with 1.5 and to segregate the legacy reflection based type builders. These old reflection emit type builders should not pollute the 1.6 native AOT paths. This also extends to the pieces you moved to Opc.Ua.ComplexTypes, move the reflection.emit bits back to this project too (Opc.Ua.Client.ComplexTypes).
OR - if you absolutely need the reflection emit path on the server side, create a Opc.Ua.ComplexTypes.Reflection package that essentially replaces Opc.Ua.Client.ComplexTypes (today). We can then add a metadata nuget in nuget.spec to wrap it so it keeps its name.
There was a problem hiding this comment.
Once you moved the reflection emit code back out, link this package from Opc.Ua.Server and Opc.Ua.Client, so comes "for free" when bringing either one in.
Also - I would suggest a new Opc.Ua.ComplexTypes.Tests project that covers the code in this new project in particular (but not by pulling in Opc.Ua.Client, or server, just the remaining code.
|
@salihgoncu can you update the docs also? Both migrationguide and the complex types docs. And ideally with rational regarding the changes you are proposing.. |
|
Created the PR to be able to run the CI first and foremost. - I know there are things to fix and move around still and PR is marked as draft for this reason. Will resolve the issues that did not manifest locally and then resolve the comments. |
|
Key feedback: Project structure needs some work due to the historical component of it all: The reflection.emit code should ideally remain where it is (because legacy), the client code goes into Opc.Ua.Client, the server code into Opc.Ua.Server. The common code into new Opc.Ua.ComplexType nuget. Optionally - if reflection emit should be in server (ideally not, as it has a lot of bugs and leaks memory), then a new Opc.Ua.ComplexType.Reflection / or .Legacy project with a metapackage to point the old Opc.Ua.Client.ComplexType project to it (which should then be removed). Again, ideally not. Test wise - same - keep specific tests in the related test projects, e.g. there should be dedicated tests to test the new Opc.Ua.ComplexType project in Opc.Ua.ComplexType.Tests project. These should be unit tests for all files in the Opc.Ua.ComplexType project and can contain integration tests too. Server or client unit tests related to ComplexType code that is only in server/client projects should be int he server/client test projects. |
|
Ok, as discussed, Opc.Ua.ComplexTypes.Reflection.csproj should contain the old reflection.emit code currently in Opc.Ua.Client.ComplexTypes. Put both server and client code in there, do not worry about splitting. because it would only depend on Opc.Ua.ComplexTypes nuget, not on server/client. Then add a nuspec to generate the old Opc.Ua.Client.ComplexTypes by putting the Opc.Ua.ComplexTypes and .Reflection nugets into that "meta package". |
| <ProjectReference Include="..\..\Libraries\Opc.Ua.Client\Opc.Ua.Client.csproj" /> | ||
| <ProjectReference Include="..\..\Libraries\Opc.Ua.Client.ComplexTypes\Opc.Ua.Client.ComplexTypes.csproj" /> | ||
| <ProjectReference Include="..\..\Libraries\Opc.Ua.ComplexTypes\Opc.Ua.ComplexTypes.csproj" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
We should pull this in via Opc.Ua.Client.csproj, not seperate.
2 participants
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
We split the monolithic complex-types stack into a layered three-project architecture and added a server-side counterpart. The original Opc.Ua.Client.ComplexTypes package owned the Reflection.Emit type builders, the runtime base types (BaseComplexType, OptionalFieldsComplexType, UnionComplexType, attributes), and the DI surface, while the main Opc.Ua.Client library carried the interfaces (IComplexType{Resolver,Factory,Builder,FieldBuilder}), the AOT-friendly Default* builders, DataDictionary, DataTypeDefinitionExtension, and the exception types — meaning the complex-types domain was fragmented across two assemblies with no clean reuse boundary.
We extracted everything that isn't client-orchestration into a new Opc.Ua.ComplexTypes library under the Opc.Ua.ComplexTypes namespace, leaving Opc.Ua.Client.ComplexTypes as a thin client-side package that now contains only the session-bound orchestration (ComplexTypeSystem, NodeCacheResolver, ComplexTypeSystemFactory, ComplexTypesExtensions, and OpcUaComplexTypesBuilderExtensions), and we removed the complex-types code from Opc.Ua.Client entirely. On top of that foundation we created a new Opc.Ua.Server.ComplexTypes library with a working minimal implementation — ServerComplexTypeSystem (with RegisterEnumeration / RegisterStructure / multi-pass Flush for resolving fields whose DataTypes arrive out of order), a DI-injected ServerComplexTypeSystemFactory, and an IOpcUaServerBuilder.AddComplexTypes() hosting extension — so hosted servers can finally register custom Structure/Enumeration DataTypes on their IEncodeableFactory without each application reinventing that plumbing. As a result, Opc.Ua.Core → Opc.Ua.ComplexTypes → Opc.Ua.{Client,Server}.ComplexTypes is now a clean dependency chain where the heavy lifting (Emit, base types, schema parsing, exceptions) lives in one shared library, Opc.Ua.Client is leaner and no longer carries logic it doesn't own, server authors get a first-class story for custom complex types instead of being forced to depend on a client package.