Fix/extension object preserve encoding typeid

[gudeljuluri]

See #3771

Proposed changes

When decoders (BinaryDecoder, JsonDecoder, XmlDecoder) successfully decode an ExtensionObject body into a typed IEncodeable, they were constructing the result with new ExtensionObject(encodeable), which sets ExtensionObject.TypeId = encodeable.TypeId — the DataType NodeId (e.g., i=887 for EUInformation).

Per OPC 10000-6 v1.05.07 §5.2.2.15 (Table 25 – ExtensionObject Binary DataEncoding), the TypeId field is:
"The identifier for the DataTypeEncoding node in the Server's AddressSpace."

This is the encoding NodeId (e.g., i=889), not the DataType NodeId. The decoders correctly read this value from the wire but then overwrote it when wrapping the decoded body.

This caused issues for consumers relying on ExtensionObject.TypeId (e.g., for type registration or lookup), and could cause BadTypeMismatch errors when writing back a previously read ExtensionObject through a code path that doesn't use the encoder's override logic.

Fix

Changed 4 call sites across 3 decoders to use new ExtensionObject(extension.TypeId, encodeable) instead of new ExtensionObject(encodeable), preserving the wire encoding NodeId:
• BinaryDecoder.ReadExtensionObject — binary body path
• BinaryDecoder.ReadExtensionObject — XML-in-binary body path
• JsonDecoder.TryGetExtensionObjectFromElement — JSON body path
• XmlDecoder.ReadExtensionObjectBody — XML body path

Note: PubSubJsonDecoder already correctly used new ExtensionObject(typeId, encodeable).

Why this is safe

The Binary and XML encoders already ignore ExtensionObject.TypeId for decoded bodies — they derive the wire TypeId from IEncodeable.BinaryEncodingId / IEncodeable.XmlEncodingId respectively. The IEncodeable body still carries all three ids (TypeId, BinaryEncodingId, XmlEncodingId), so no information is lost.

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

This issue was previously identified in PR #952 (April 2020), but that fix went the wrong direction — it explicitly overwrote ExtensionObject.TypeId with encodeable.TypeId (the DataType id). That was reverted in PRs #979 and #980 (May 2020), but the correct fix (preserving the wire encoding id) was never applied.

[CLAassistant]

All committers have signed the CLA.

[romanett]

@gudeljuluri can you write some Tests that verifiy the correct behaviour being present with your fix?

[marcschier]

@gudeljuluri Could you open an issue with above info and link?

[marcschier]

@gudeljuluri can you write some Tests that verifiy the correct behaviour being present with your fix?

Probably as easy as finding an existing test and checking the typeid points to binary or xmlencoding identifier. That would be all that is needed IMO.

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.04%. Comparing base (296a904) to head (851f96c).

Files with missing lines	Patch %	Lines
Stack/Opc.Ua.Types/Encoders/BinaryDecoder.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3761      +/-   ##
==========================================
+ Coverage   71.98%   72.04%   +0.06%     
==========================================
  Files         677      677              
  Lines      130754   130752       -2     
  Branches    22260    22261       +1     
==========================================
+ Hits        94123    94204      +81     
+ Misses      29963    29885      -78     
+ Partials     6668     6663       -5     

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

[gudeljuluri]

For the XMLDecoder, I used the existing test ReadExtensionObjectBodyKnownTypeReturnsEncodeable.

For the BinaryDecoder, I created a new IEncodable implementation with different type IDs (TestEncodeableWithDifferentTypeIds) and added a new test, ReadExtensionObjectPreservesEncodingTypeId. I’m not sure whether this aligns perfectly with your testing methodology, so feel free to adjust it as needed.

I also created a new issue 3771

[romanett]

@gudeljuluri thank you this seems good👍 can you merge with master so we can get CI to pass

[romanett]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[romanett]

@gudeljuluri can you please fix the now failing Tests?

[marcschier]

Lots of test failures. @gudeljuluri could you look into those? You can see those individual tests better on the azure devops pipeline, tests tab.

[gudeljuluri]

The tests are failing mainly because the expected results were created using ExtensionObject(IEncodeable), which also copies body.TypeId into ExtensionObject.TypeId. I assume this constructor was originally intended for scenarios where the encoding type is unknown.

My fix resolves the issue on the decoder side, but this constructor is used in many places, and I’m not sure how to handle it without causing unintended side effects. I also can’t fully assess how any change to it would impact the rest of the codebase.

I’ve committed one last change that will fix some tests where the encoding type is accessible, but overall I think I should abandon this PR. Sorry for the inconvenience.

[romanett]

@gudeljuluri thank you for the heads up, we will discuss in the working group how to proceed, and close the PR If needed.

[marcschier]

I think it would be best to change the Equals implementation of ExtensionObject type to test any combination of incoming TypeId against the TypeId and both binary and xml EncodingId of the inner IEncodeable.

The tests should them pass, it is not breaking and factually / spec correct.

[marcschier]

Once #3777 is in, we can merge from master and get this one in.

[marcschier]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[marcschier]

Final issue despite flaky tests:

TestExtensionObjectWithDynamicEncodeable

Error message
Assert.That(encodedJson, Is.EqualTo(expectedJson))
Expected string length 71 but was 78. Strings differ at index 33.
Expected: "{"Test":{"UaTypeId":"s=test_dyn2_typeid","Foo":"bar_1","Foo2"..."
But was: "{"Test":{"UaTypeId":"s=test_dyn2_xmlencodingid","Foo":"bar_1"..."
--------------------------------------------^

at Opc.Ua.Core.Tests.Types.Encoders.JsonEncoderTests.TestExtensionObjectWithDynamicEncodeable() in /_/Tests/Opc.Ua.Core.Tests/Types/Encoders/JsonEncoderTests.cs:line 1544

[gudeljuluri]

This failure is expected given the decoder changes. The test encodes to XML, decodes from XML (which now correctly preserves the XmlEncodingId in ExtensionObject.TypeId), then re-encodes to JSON. The JsonEncoder was using value.TypeId directly, which after the XML decode now contains the XML encoding ID instead of the DataType NodeId.

Per OPC UA Part 6 §5.4.2.16, the UaTypeId field in JSON must be the NodeId of a DataType Node. Fixed the JsonEncoder.WriteExtensionObject to use encodeable.TypeId (the DataType NodeId) when the body is IEncodeable, falling back to value.TypeId only for raw bodies. This is consistent with how BinaryEncoder already uses encodeable. BinaryEncodingId and XmlEncoder uses encodeable.XmlEncodingId — each encoder resolves the correct TypeId from the body itself, independent of what's stored in ExtensionObject.TypeId.

[marcschier]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).