Copilot commented on code in PR #2279:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2279#discussion_r4219012357
##########
extensions/opc/src/OPCCommon.cpp:
##########
@@ -430,6 +430,7 @@ UA_StatusCode
Client::translateBrowsePathsToNodeIdsRequest(const std::string& pa
template<typename T>
UA_StatusCode Client::add_node(const UA_NodeId parent_node_id, const UA_NodeId
target_node_id, const UA_UInt32 ref_type_id, std::string_view browse_name, T
value, UA_NodeId *received_node_id) {
UA_VariableAttributes attr = UA_VariableAttributes_default;
+ attr.accessLevel |= UA_ACCESSLEVELMASK_WRITE;
Review Comment:
`attr.accessLevel` is updated to include WRITE, but `UA_VariableAttributes`
also has `userAccessLevel`, which many OPC UA stacks use to enforce per-session
permissions. If `userAccessLevel` remains read-only, the node may still not be
writable for clients, undermining the intent of this change. Consider also
enabling WRITE on `attr.userAccessLevel` (and ensuring READ is preserved) so
both access control fields allow updates.
##########
extensions/opc/tests/PutOPCProcessorTests.cpp:
##########
@@ -105,6 +105,37 @@ TEST_CASE("Test creating a new node with path node id",
"[putopcprocessor]") {
verifyCreatedNode(expected_node, controller);
}
+TEST_CASE("Test updating a node previously created by the processor",
"[putopcprocessor]") {
+ OpcUaTestServer server;
+ server.start();
+ SingleProcessorTestController
controller{minifi::test::utils::make_processor<processors::PutOPCProcessor>("PutOPCProcessor")};
+ auto put_opc_processor = controller.getProcessor();
+
+ NodeData expected_node{42, server.getNamespaceIndex(), 9999, "everything",
"Simulator/Default/Device1", {}};
+
REQUIRE(put_opc_processor->setProperty(processors::PutOPCProcessor::OPCServerEndPoint.name,
"opc.tcp://127.0.0.1:4840/"));
Review Comment:
This test hard-codes the OPC UA endpoint to port `4840`. Adding another test
that binds/uses the same fixed port increases the likelihood of CI flakes when
tests are executed concurrently (e.g., ctest `-j` or parallel test runners),
due to port collisions. Prefer using a dynamically allocated port from
`OpcUaTestServer` (and wiring that endpoint through `verifyCreatedNode`) or a
per-test unique port to avoid cross-test interference.
##########
extensions/opc/tests/features/opcua.feature:
##########
@@ -94,17 +99,22 @@ Feature: Putting and fetching data to OPC UA server
And an OPC UA server is set up
When all instances start up
- Then in the "fetch-opc-ua-node" container at least one file with the
content "<Value>" is placed in the "/tmp/output" directory in less than 60
seconds
+ # The node did not exist, so this first value proves the node was created.
+ Then in the "fetch-opc-ua-node" container at least one file with the
content "<Initial Value>" is placed in the "/tmp/output" directory in less than
60 seconds
- # Node ids starting from 51000 are pre-defined demo node ids in the test
server application (server_ctt) of the open62541 docker image. There is one
nodeid defined
- # for each type supported by OPC UA. These demo nodes can be used for
testing purposes. "the.answer" is also a pre-defined string id for the same
testing purposes.
- Examples: Topic names and formats to test
- | Node ID Type | Node ID | Value Type | Value |
- | Int | 51034 | String | minifi-test |
- | Int | 51001 | Boolean | True |
- | String | the.answer | Int32 | 54 |
- | Int | 51019 | UInt32 | 123 |
- | Int | 51031 | Double | 66.6 |
+ # Only now, with the node known to exist, write a second value to the same
node. Reading this
+ # one back is only possible if PutOPCProcessor can update a node it
created itself.
+ When a file with the content "<Updated Value>" is placed in "/tmp/input"
in the "update-opc-ua-node" flow
+ Then in the "fetch-opc-ua-node" container at least one file with the
content "<Updated Value>" is placed in the "/tmp/output" directory in less than
60 seconds
+ And the logs of the "update-opc-ua-node" container do not contain the
following message: "Failed to update node" after 1 second
Review Comment:
The log assertion window (`after 1 second`) can miss failures that are
logged later in the 60s wait period (e.g., delayed retries/backpressure),
allowing false positives. To make the scenario robust, align the log-check
window with the potential execution time (e.g., check over the same 60s
interval or after the update assertion completes, using a sufficiently long
window).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
