UPDATED Correct error response for /app/log/public/historical/range w…

…hen given a future to_seqno (#6402)

Co-authored-by: Eddy Ashton <[email protected]>

samples/apps/logging/js/src/logging.js

@@ -92,7 +92,12 @@ function get_historical_range_impl(request, isPrivate, nextLinkPrefix) {
   if (from_seqno !== undefined) {
     from_seqno = parseInt(from_seqno);
     if (isNaN(from_seqno)) {
-      throw new Error("from_seqno is not an integer");
+      return {
+        statusCode: 400,
+        body: {
+          error: { code: "InvalidInput", msg: "from_seqno is not an integer" },
+        },
+      };
     }
   } else {
     // If no from_seqno is specified, defaults to very first transaction
@@ -121,7 +126,12 @@ function get_historical_range_impl(request, isPrivate, nextLinkPrefix) {
 
   // Range must be in order
   if (to_seqno < from_seqno) {
-    throw new Error("to_seqno must be >= from_seqno");
+    return {
+      statusCode: 400,
+      body: {
+        error: { code: "InvalidInput", msg: "to_seqno must be >= from_seqno" },
+      },
+    };
   }
 
   // End of range must be committed
@@ -132,7 +142,12 @@ function get_historical_range_impl(request, isPrivate, nextLinkPrefix) {
     isCommitted = txStatus === "Committed";
   }
   if (!isCommitted) {
-    throw new Error("End of range must be committed");
+    return {
+      statusCode: 400,
+      body: {
+        error: { code: "InvalidInput", msg: "End of range must be committed" },
+      },
+    };
   }
 
   const max_seqno_per_page = 2000;

samples/apps/logging/logging.cpp

@@ -360,6 +360,10 @@ namespace loggingapp
           return status;
         }
       }
+      else if (result == ccf::ApiResult::NotFound)
+      {
+        return ccf::TxStatus::Unknown;
+      }
 
       return std::nullopt;
     }
@@ -1472,25 +1476,21 @@ namespace loggingapp
 
         // End of range must be committed
         const auto tx_status = get_tx_status(to_seqno);
-        if (!tx_status.has_value())
-        {
-          ctx.rpc_ctx->set_error(
-            HTTP_STATUS_INTERNAL_SERVER_ERROR,
-            ccf::errors::InternalError,
-            "Unable to retrieve Tx status");
-          return;
-        }
-
-        if (tx_status.value() != ccf::TxStatus::Committed)
+        if (
+          !tx_status.has_value() ||
+          tx_status.value() != ccf::TxStatus::Committed)
         {
+          const auto tx_status_msg = tx_status.has_value() ?
+            tx_status_to_str(tx_status.value()) :
+            "not found";
           ctx.rpc_ctx->set_error(
             HTTP_STATUS_BAD_REQUEST,
             ccf::errors::InvalidInput,
             fmt::format(
               "Only committed transactions can be queried. Transaction at "
               "seqno {} is {}",
               to_seqno,
-              ccf::tx_status_to_str(tx_status.value())));
+              tx_status_msg));
           return;
         }
 

tests/e2e_logging.py

@@ -947,6 +947,40 @@ def id_for(i):
 
         infra.commit.wait_for_commit(c, seqno=last_seqno, view=view, timeout=3)
 
+        LOG.info("Checking error responses")
+        # Reversed range is illegal
+        r = c.get(
+            f"/app/log/public/historical/range?from_seqno={last_seqno}&to_seqno=1&id={id_a}"
+        )
+        assert r.status_code == http.HTTPStatus.BAD_REQUEST
+        assert r.body.json()["error"]["code"] == "InvalidInput"
+        r = c.get(
+            f"/app/log/public/historical/range?from_seqno={last_seqno}&to_seqno={last_seqno-1}&id={id_a}"
+        )
+        assert r.status_code == http.HTTPStatus.BAD_REQUEST
+        assert r.body.json()["error"]["code"] == "InvalidInput"
+
+        # Asking for future seqnos gives a clear error
+        # - First find latest valid seqno
+        r = c.get("/node/commit")
+        assert r.status_code == http.HTTPStatus.OK
+        last_valid_seqno = TxID.from_str(r.body.json()["transaction_id"]).seqno
+
+        # - Try a very invalid seqno
+        r = c.get(
+            f"/app/log/public/historical/range?to_seqno={last_valid_seqno*2}&id={id_a}"
+        )
+        assert r.status_code == http.HTTPStatus.BAD_REQUEST
+        assert r.body.json()["error"]["code"] == "InvalidInput"
+
+        # - Try the first invalid seqno
+        r = c.get(
+            f"/app/log/public/historical/range?to_seqno={last_valid_seqno+1}&id={id_a}"
+        )
+        assert r.status_code == http.HTTPStatus.BAD_REQUEST
+        assert r.body.json()["error"]["code"] == "InvalidInput"
+
+        LOG.info("Verifying historical ranges")
         entries_a, _ = network.txs.verify_range_for_idx(id_a, node=primary)
         entries_b, _ = network.txs.verify_range_for_idx(id_b, node=primary)
         entries_c, _ = network.txs.verify_range_for_idx(id_c, node=primary)