restconf: better error message for possible missing action node

There is a possible data race in sysrepo when calling actions. First,
we check if the action node is present and then we executed the action.
There is a possibility that between those two sysrepo calls the action
node can be created or deleted.
Fortunately, sysrepo raises and error when the action node is not
present when performing the rpc call. Unfortunately, it is the same
error as if the input data tree for the rpc was invalid.

I was thinking about removing the check for the action node existence
and rely solely on the sysrepo's exception. This would indeed remove
the data race but the disadvantage is, that the error message is now
very broad (in case of actions) because we can't differentiate between
invalid input data and missing action node.

I don't think this would bring a better behaviour from rousette.
There are two possible scenarios with the current behaviour:

1) The data node gets created between the check and the rpc call.
From the client's point of view the node does not exist in the moment
the request is created and I *think* it is ok to reject it.

2) The data node gets deleted between the check and the rpc call.
From the client's point of view the node was existing and it is expected
to return something. However, sysrepo (and rousette) reports the
validation failure error which is (at least) confusing. Therefore I have
edited the error message to report this possibility as well.

Bug: #2
Change-Id: Iae1a70160a8c5d32ed041ef90c4b67e18cd964c1

src/restconf/Server.cpp

@@ -163,22 +163,30 @@ void processActionOrRPC(std::shared_ptr<RequestContext> requestCtx)
 {
     requestCtx->sess.switchDatastore(sysrepo::Datastore::Operational);
     auto ctx = requestCtx->sess.getContext();
+    bool isAction = false;
 
     try {
         auto rpcSchemaNode = ctx.findPath(requestCtx->restconfRequest.path);
         if (!requestCtx->dataFormat.request && static_cast<bool>(rpcSchemaNode.asActionRpc().input().child())) {
             throw ErrorResponse(400, "protocol", "invalid-value", "Content-type header missing.");
         }
 
-        // validate if action node's parent is present
-        if (rpcSchemaNode.nodeType() == libyang::NodeType::Action) {
-            // FIXME: This is race-prone: we check for existing action data node but before we send the RPC the node may be gone
+        isAction = rpcSchemaNode.nodeType() == libyang::NodeType::Action;
+
+        // check if action node's parent is present
+        if (isAction) {
+            /*
+             * This is race-prone:
+             *  - The data node exists but might get deleted right after this check: Sysrepo throws an error when this happens.
+             *  - The data node does not exist but might get created right after this check: The node was not there when the request was issues so it should not be a problem
+             */
             auto [pathToParent, pathSegment] = asLibyangPathSplit(ctx, requestCtx->req.uri().path);
             if (!requestCtx->sess.getData(pathToParent)) {
                 throw ErrorResponse(400, "application", "operation-failed", "Action data node '" + requestCtx->restconfRequest.path + "' does not exist.");
             }
         }
 
+
         auto [parent, rpcNode] = ctx.newPath2(requestCtx->restconfRequest.path);
 
         if (!requestCtx->payload.empty()) {
@@ -219,9 +227,10 @@ void processActionOrRPC(std::shared_ptr<RequestContext> requestCtx)
             /*
              * FIXME: This happens on invalid input data (e.g., missing mandatory nodes) or missing action data node.
              * The former (invalid input data) should probably be validated by libyang's parseOp but it only parses. Is there better way? At least somehow extract logs?
-             * We check on the missing action data node, but it is racy.
+             * We can check if the action node exists before sending the RPC but that is racy because two sysrepo operations must be done (query + rpc) and operational DS cannot be locked.
              */
-            rejectWithError(requestCtx->sess.getContext(), requestCtx->dataFormat.response, requestCtx->req, requestCtx->res, 400, "application", "operation-failed", "Input data validation failed");
+            rejectWithError(requestCtx->sess.getContext(), requestCtx->dataFormat.response, requestCtx->req, requestCtx->res, 400, "application", "operation-failed",
+                    "Validation failed. Invalid input data"s + (isAction ? " or the action node is not present" : "") + ".");
         } else {
             rejectWithError(requestCtx->sess.getContext(), requestCtx->dataFormat.response, requestCtx->req, requestCtx->res, 500, "application", "operation-failed", "Internal server error due to sysrepo exception: "s + e.what());
         }

tests/restconf-rpc.cpp

@@ -217,7 +217,7 @@ TEST_CASE("invoking actions and rpcs")
       {
         "error-type": "application",
         "error-tag": "operation-failed",
-        "error-message": "Input data validation failed"
+        "error-message": "Validation failed. Invalid input data."
       }
     ]
   }
@@ -233,7 +233,7 @@ TEST_CASE("invoking actions and rpcs")
       {
         "error-type": "application",
         "error-tag": "operation-failed",
-        "error-message": "Input data validation failed"
+        "error-message": "Validation failed. Invalid input data."
       }
     ]
   }