Keep each Browse result with its own operation

[kevinherron]

Fixes #2105.

Problem

The server resolves some Browse operations before it browses the rest: an invalid BrowseDirection (Bad_BrowseDirectionInvalid), an unknown ReferenceTypeId (Bad_ReferenceTypeIdInvalid), and a starting Node the AccessController denies Browse on (Good, no references). When one of those came before a browsed operation in the same request, the response had fewer results than the request and the remaining results were shifted. The service result was still Good.

For example, browsing [RootFolder with an unknown ReferenceTypeId, ObjectsFolder]:

	results
Before	[Good [Locations, Server, Aliases, ...]] (one result, ObjectsFolder's references in RootFolder's slot)
After	[Bad_ReferenceTypeIdInvalid [], Good [Locations, Server, Aliases, ...]]

OPC 10000-4 5.9.2.2 requires the results to match the size and order of nodesToBrowse. A client pairing results with its request by position read references for the wrong Node, and the error for the bad operation was lost.

Cause and fix

BrowseHelper.browse browsed the unresolved operations as a filtered list, then wrote result i of that list into slot i of the unfiltered list. The early result in that slot was overwritten, the last browsed operation's slot stayed empty, and the final loop skipped the empty slot.

The fix keeps the filtered list of pending operations and writes each result back to the operation it came from:

-    for (int i = 0; i < nodesToBrowse.size(); i++) {
-      PendingBrowse pb = pending.get(i);
+    for (int i = 0; i < toBrowse.size(); i++) {
+      PendingBrowse pb = toBrowse.get(i);

The final loop now throws IllegalStateException for an operation without a result instead of skipping it, because a skipped slot shifts every later result. After this fix every slot is filled, so this is an invariant check. If it ever fires, the Browse call fails with a service fault instead of returning misaligned results.

Percent Deadband

SubscriptionManager.readTypeDefinitions uses the same helper to find each Node's TypeDefinition for Percent Deadband and pairs results with Nodes by position. Its operations always pass the direction and ReferenceTypeId checks, but a Browse-denied Node shifted the results there too. Creating Percent Deadband items for [Browse-denied TestInt32, AnalogItemType TestAnalogValue] in one request rejected TestAnalogValue with Bad_MonitoredItemFilterUnsupported, because its TypeDefinition was paired with TestInt32. With the fix the pairing holds and TestAnalogValue is accepted.

A Browse-denied AnalogItem still cannot use Percent Deadband, because the server cannot see its TypeDefinition through that Session's Browse permissions. That behavior predates this change and is unchanged.

Tests

BrowseServiceResultOrderTest (integration-tests) runs against a server whose AccessController denies Browse on TestInt32. It covers:

• Each early-resolved case (unknown ReferenceTypeId, invalid BrowseDirection, Browse-denied start Node) placed before one browsed operation, and between two browsed operations. Each operation must get its own status and references at its own index.
• The Percent Deadband pairing described above.

All 7 tests failed before the fix (results missing; TestAnalogValue rejected) and pass after it.

Verification run locally:

• mvn spotless:apply (no changes) and mvn clean compile
• mvn -pl opc-ua-sdk/integration-tests -am test -Dtest='org/eclipse/milo/opcua/sdk/**' -Dsurefire.failIfNoSpecifiedTests=false: SDK Server 1071 tests (3 skipped), Integration Tests 1663 tests (2 skipped), no failures.

1.1.x

main has the same browse() code. The BrowseHelper change applies cleanly there, but the test uses setAccessControllerFactory and createTestServer, which 1.1.x does not have. This PR does not include a backport.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Fixes browse result ordering in the OPC UA server.

This PR appears safe to merge; each Browse result now stays with its original operation.

What we checked:

• Browse results stay in order: toBrowse contains only unresolved operation objects. Each result goes back to its own object, and the server collects results from pending in request order.
• Valid results avoid the exception: ReferenceDescriptionResult has only two possible forms. Both receive an assignment and both are handled by the final loop. No valid result reaches the new exception branch.

Summary

BrowseHelper.browse now keeps each result with the operation that produced it.

• Early errors and Browse-denied results keep their original positions.
• Seven regression cases cover mixed Browse requests and Percent Deadband pairing.
• No actionable issues were found. This review checked source code; it did not run tests.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A["Operations in request order"] --> B["Resolve errors and denied operations"]
  B --> C["Keep unresolved operation objects in toBrowse"]
  C --> D["Browse unresolved operations"]
  D --> E["Assign each result to its operation object"]
  E --> F["Collect all results in original request order"]

Loading

_{Reviews (1) · Last reviewed commit: "Keep each Browse result with its own ope..." · Reviewed by Greptile}