Add validation rule for precision field (hyperledger-iroha#1370)

* Add validation rule for the precision field. * Add cases for the precision field in field validator test. * Update docs - clarify the valid precision range * Remove NOP validator for the precision field Signed-off-by: Igor Egorov <[email protected]>
itorK · May 25, 2018 · 34c09ed · 34c09ed
1 parent df66fa3
commit 34c09ed
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 6 deletions.
diff --git a/docs/source/api/commands.rst b/docs/source/api/commands.rst
@@ -36,6 +36,11 @@ Schema
         uint32 precision = 2;
     }
 
+.. note::
+    Please note that due to a known issue you would not get any exception if you pass invalid precision value.
+    Valid range is: 0 <= precision <= 255
+
+
 Structure
 ^^^^^^^^^
 
@@ -224,6 +229,10 @@ Schema
         uint32 precision = 3;
     }
 
+.. note::
+    Please note that due to a known issue you would not get any exception if you pass invalid precision value.
+    Valid range is: 0 <= precision <= 255
+
 Structure
 ^^^^^^^^^
 
@@ -233,7 +242,7 @@ Structure
 
     "Asset name", "domain-unique name for asset", "`[a-z_0-9]{1,32}`", "soracoin"
     "Domain ID", "target domain to make relation with", "RFC1035 [#f1]_, RFC1123 [#f2]_", "japan"
-    "Precision", "number of digits after comma/dot", "0 <= precision <= uint32 max", "2"
+    "Precision", "number of digits after comma/dot", "0 <= precision <= 255", "2"
 
 Validation
 ^^^^^^^^^^
@@ -566,6 +575,10 @@ Schema
        uint32 precision = 2;
     }
 
+.. note::
+    Please note that due to a known issue you would not get any exception if you pass invalid precision value.
+    Valid range is: 0 <= precision <= 255
+
 Structure
 ^^^^^^^^^
 
@@ -628,4 +641,4 @@ Validation
 4. Source account can transfer money, and destination account can receive money (their roles have these permissions)
 
 .. [#f1] https://www.ietf.org/rfc/rfc1035.txt
-.. [#f2] https://www.ietf.org/rfc/rfc1123.txt
+.. [#f2] https://www.ietf.org/rfc/rfc1123.txt
diff --git a/docs/source/api/queries.rst b/docs/source/api/queries.rst
@@ -337,6 +337,10 @@ Response Schema
         uint32 precision = 3;
     }
 
+.. note::
+    Please note that due to a known issue you would not get any exception if you pass invalid precision value.
+    Valid range is: 0 <= precision <= 255
+
 Response Structure
 ^^^^^^^^^^^^^^^^^^
 
@@ -346,7 +350,7 @@ Response Structure
 
     "Asset ID", "identifier of asset used for checking the balance", "<asset_name>#<domain_id>", "jpy"
     "Domain ID", "domain related to this asset", "RFC1035 [#f1]_, RFC1123 [#f2]_", "japan"
-    "Precision", "number of digits after comma", "0 < precision < 256", "2"
+    "Precision", "number of digits after comma", "0 <= precision <= 255", "2"
 
 Get Roles
 ^^^^^^^^^

diff --git a/shared_model/validators/field_validator.cpp b/shared_model/validators/field_validator.cpp
@@ -16,8 +16,11 @@
  */
 
 #include "validators/field_validator.hpp"
+
 #include <boost/algorithm/string_regex.hpp>
 #include <boost/format.hpp>
+#include <limits>
+
 #include "cryptography/crypto_provider/crypto_verifier.hpp"
 #include "permissions.hpp"
 
@@ -208,7 +211,20 @@ namespace shared_model {
     void FieldValidator::validatePrecision(
         ReasonsGroupType &reason,
         const interface::types::PrecisionType &precision) const {
-      // define precision constraints
+      /* The following validation is pointless since PrecisionType is already
+       * uint8_t, but it is going to be changed and the validation will become
+       * meaningful.
+       */
+      interface::types::PrecisionType min = std::numeric_limits<uint8_t>::min();
+      interface::types::PrecisionType max = std::numeric_limits<uint8_t>::max();
+      if (precision < min or precision > max) {
+        auto message =
+            (boost::format(
+                 "Precision value (%d) is out of allowed range [%d; %d]")
+             % precision % min % max)
+                .str();
+        reason.second.push_back(std::move(message));
+      }
     }
 
     void FieldValidator::validatePermission(

diff --git a/test/module/shared_model/validators/field_validator_test.cpp b/test/module/shared_model/validators/field_validator_test.cpp
@@ -130,13 +130,17 @@ class FieldValidatorTest : public ValidatorsTest {
                                           &FieldValidatorTest::permission,
                                           permission_test_cases));
 
+    field_validators.insert(makeValidator("precision",
+                                          &FieldValidator::validatePrecision,
+                                          &FieldValidatorTest::precision,
+                                          precision_test_cases));
+
     // TODO: add validation to all fields
     for (const auto &field : {"value",
                               "signature",
                               "commands",
                               "quorum",
-                              "tx_hashes",
-                              "precision"}) {
+                              "tx_hashes"}) {
       field_validators.insert(makeNullValidator(field));
     }
   }
@@ -541,6 +545,31 @@ class FieldValidatorTest : public ValidatorsTest {
   }
   std::vector<FieldTestCase> permission_test_cases = permissionTestCases();
 
+  std::vector<FieldTestCase> precision_test_cases{
+      makeValidCase(&FieldValidatorTest::precision, 0),
+      makeValidCase(&FieldValidatorTest::precision, 1),
+      makeValidCase(&FieldValidatorTest::precision, 255),
+
+      // The following cases are written because the type of PrecisionType is
+      // going to be changed.
+
+      // The case is disabled till PrecisionType will become a signed type,
+      // now it is unsigned char.
+      //      makeTestCase("negative precision",
+      //                   &FieldValidatorTest::precision,
+      //                   -3,
+      //                   false,
+      //                   "negative precision"),
+
+      // Disabled, because PrecisionType is 1 byte type. The case should be
+      // enabled when PrecisionType will be 2 bytes int or more.
+      //      makeTestCase("precision value is more than max",
+      //                   &FieldValidatorTest::precision,
+      //                   256,
+      //                   false,
+      //                   "more than max")
+  };
+
   /**************************************************************************/
 
   /// Create no-operation validator