8000 Use namespace index if possible when encoding ExpandedNodeId by kevinherron · Pull Request #1451 · eclipse-milo/milo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use namespace index if possible when encoding ExpandedNodeId #1451

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 29, 2025
Merged

Use namespace index if possible when encoding ExpandedNodeId #1451

merged 1 commit into from
Apr 29, 2025

Conversation

kevinherron
Copy link
Contributor

Using the namespace index is more compact than encoding a namespace URI.

It's also a workaround for bugs observed in the CTT and UaExpert, where the Client ignores the explicitly specified namespace URI in the ExpandedNodeId and just uses the namespace index, which is always 0 when the URI is encoded.

fixes #1450

@kevinherron
Use namespace index if possible when encoding ExpandedNodeId
bf28a1f 
Using the namespace index is more compact than encoding a namespace URI.

It's also a workaround for bugs observed in the CTT and UaExpert, where the Client ignores the explicitly specified namespace URI in the ExpandedNodeId and just uses the namespace index, which is always 0 when the URI is encoded.

fixes #1450
@kevinherron kevinherron added this to the 1.0.0-M2 milestone Apr 29, 2025
@johannwesely
Copy link

Hello, I tried the fix an it works well for me no more errors in CTT Adress Space Base. Thank you very much.

@kevinherron kevinherron merged commit bf5d4e3 into 1.0 Apr 29, 2025
6 checks passed
@kevinherron kevinherron mentioned this pull request Apr 29, 2025
Closed
kevinherron added a commit that referenced this pull request Apr 29, 2025
@kevinherron
Revert "Use namespace index if possible when encoding ExpandedNodeId (#…
b81832f 
…1451)"

This reverts commit bf5d4e3.
kevinherron added a commit that referenced this pull request Apr 29, 2025
@kevinherron
Revert "Use namespace index if possible when encoding ExpandedNodeId (#…
872d2da 
…1451)"

This reverts commit bf5d4e3.
@johannwesely
Copy link

Hi Kevin,
i updatet to 1.0.0-M2 and realized that this change has been reverted. So I have the Type Cache Error for ObjectTypes again . Why did you revert this ?

Greetings Johann

@kevinherron
Copy link
Contributor Author
kevinherron commented May 12, 2025 •
edited
Loading

I reverted the band-aid fix and replaced it with a fix for the actual root cause: 4a27357

When I tested it with your namespace and the CTT it was still fixed... I'll check again this week I suppose.

Make sure that you aren't pulling in a mix of M1 and M2 artifacts. The uanodeset artifact still points to M1 artifacts.

@kevinherron
Copy link
Contributor Author
kevinherron commented May 12, 2025 •
edited
Loading

Make sure that you aren't pulling in a mix of M1 and M2 artifacts. The uanodeset artifact still points to M1 artifacts.

Actually, make sure the version of uanodeset you are using is 0.3-M2, which does point at Milo 1.0.0-M2. Other versions will give you a mismatch.

@johannwesely
Copy link

Hi,
you are right with ctt the errors are gone but i still see them with UAExpert #1428. Your band-aid fix also worked for UaExpert so i got confused.

@johannwesely
Copy link

But i sometimes running the testscripts (not always?) get this error in CTT and i think it has to do with the custom object types as well. Address Space Base shows no errors.
Screenshot 2025-05-13 105449

@kevinherron
Copy link
Contributor Author

@johannwesely it seems I had never properly published uanodeset 0.3-M2. I just re-ran that release and it should be available soon. Make sure that you are using this version, and that you don't have a mix of M1 and M2 artifacts on your class path after you have updated.

Your nodeset file is working in both the CTT and UaExpert without errors for me.

@johannwesely
Copy link

Hello,
I use uanodeset 0.3-M2 and Milo 1.0.0.-M2 and made sure to not use old dependencies, but still have the same issue. How did you load the Nodeset i sent you for testing. Because there are multiple namespaces inside the .xml file i use and in your example server there is only one ?
I Also have to do something like this in my DataTypeNamespace

		@Override
		protected boolean filterNamespace(String namespaceUri) {
			return !namespaceUri.equals("http://opcfoundation.org/UA/");
	
		}

otherwise it wont load namespaces at all. Can you explain me what is the diffrence between the quickfix (wich works fine ) and the M2 release for my understanding? Greetings Johann

@kevinherron
Copy link
Contributor Author

Can you explain me what is the diffrence between the quickfix (wich works fine ) and the M2 release for my understanding?

The quick fix PR hooked into the binary encoder and replaced every outgoing ExpandedNodeId that had a URI in it with one that used an index instead. The actual fix located the place (browsing) where ExpandedNodeId with a URI was being returned and fixed it, because it turns out there's a specific passage in the spec that states ExpandedNodeId in the browse results must use an index.

I think the issue you may be having now is that the type definition in the ReferenceDescription is not getting the same treatment, but it seems UaExpert expects it.

@kevinherron
Copy link
Contributor Author

I Also have to do something like this in my DataTypeNamespace

	@Override
	protected boolean filterNamespace(String namespaceUri) {
		return !namespaceUri.equals("http://opcfoundation.org/UA/");

	}

Are you also overriding getNodeSetInputStreams and returning a List of all the NodeSet XML files?

@johannwesely
Copy link

yes it looks like this

	@Override
	protected List<InputStream> getNodeSetInputStreams() {
		URL xmlFilePath = ResourceUtils.getResource(DataTypeNamespace.class, "/" + TOX_UA_NODE_SET_XML, path);
		try {
			InputStream inputStream = xmlFilePath.openStream();
			return List.of(inputStream);
		} catch (IOException e) {
			loggerService.getLogger(this).error("Failed to import opc ua node set:  {}", xmlFilePath, e);
		}
		return List.of();
	}

@kevinherron
Copy link
Contributor Author
kevinherron commented May 20, 2025 8000

Ok I think this is okay...

It is a little strange that a single Namespace instance actually manages the Nodes for all these namespaces, because a Namespace explicitly indicates its namespace index and URI, but it seems to be okay 🤷

@kevinherron
Copy link
Contributor Author

@johannwesely I need to write up an issue and talk with some OPC peeps, but a patch for Milo that I think fixes your issue:

Subject: [PATCH] Convert type definitions to relative ExpandedNodeId
---
Index: opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/helpers/BrowseHelper.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/helpers/BrowseHelper.java b/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/helpers/BrowseHelper.java
--- a/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/helpers/BrowseHelper.java	(revision e921de7336c92c127b4bb1398b07d66af8832740)
+++ b/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/helpers/BrowseHelper.java	(date 1747745278911)
@@ -369,7 +369,8 @@
     var typeDefinitions = new ArrayList<ExpandedNodeId>();
 
     for (int i = 0; i < nodeIds.size(); i++) {
-      NodeId nodeId = nodeIds.get(i).toNodeId(server.getNamespaceTable()).orElse(NodeId.NULL_VALUE);
+      NamespaceTable namespaceTable = server.getNamespaceTable();
+      NodeId nodeId = nodeIds.get(i).toNodeId(namespaceTable).orElse(NodeId.NULL_VALUE);
 
       BrowseAttributes attributes = browseAttributes.get(i);
       NodeClass nodeClass = attributes.nodeClass;
@@ -380,7 +381,17 @@
         switch (attributes.nodeClass) {
           case Object, Variable -> {
             try {
-              typeDefinitions.add(browseTypeDefinition(server, nodeId));
+              ExpandedNodeId typeDefinitionId = browseTypeDefinition(server, nodeId);
+              if (typeDefinitionId.isLocal()) {
+                if (typeDefinitionId.isAbsolute()) {
+                  typeDefinitionId = typeDefinitionId.relative(namespaceTable).orElseThrow();
+                }
+              } else {
+                if (typeDefinitionId.isRelative()) {
+                  typeDefinitionId = typeDefinitionId.absolute(namespaceTable).orElseThrow();
+                }
+              }
+              typeDefinitions.add(typeDefinitionId);
             } catch (UaException e) {
               LoggerFactory.getLogger(BrowseHelper.class)
                   .error("Error browsing TypeDefinition for nodeId={}", nodeId, e);

It basically just applies the same treatment to the type definition returned in a ReferenceDescription as the target NodeId gets.

@johannwesely
Copy link

I apllied your patch and it works. Thank you . So will this be in the release also?

@kevinherron
Copy link
Contributor Author

1.0.0 was released yesterday, so this will have to wait a bit for the 1.0.1 patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0-M2
Development

Successfully merging this pull request may close these issues.

Use namespace index if possible when encoding ExpandedNodeId
2 participants
@kevinherron @johannwesely
0