ECC-1620: Better error message for bad units

src/grib_accessor_class_g2step_range.cc

@@ -187,45 +187,17 @@ static int unpack_string(grib_accessor* a, char* val, size_t* len)
     return GRIB_SUCCESS;
 }
 
-
-//static int pack_string_old(grib_accessor* a, const char* val, size_t* len)
-//{
-//    grib_accessor_g2step_range* self = (grib_accessor_g2step_range*)a;
-//    grib_handle* h                   = grib_handle_of_accessor(a);
-//    int ret = 0;
-
-//    std::vector<Step> steps = parse_range(val);
-//    if (steps.size() == 0)
-//        return GRIB_INVALID_ARGUMENT;
-
-//    Step step_0 = steps[0];
-//    Step step_1;
-//    if (steps.size() > 1) {
-//        std::tie(step_0, step_1) = find_common_units(steps[0].optimize_unit(), steps[1].optimize_unit());
-//        if ((ret = grib_set_long_internal(h, "stepUnits", step_0.unit().to_long())))
-//            return ret;
-//    }
-
-//    if ((ret = grib_set_long_internal(h, self->start_step, step_0.value<long>())))
-//        return ret;
-
-//    if ((self->end_step != NULL) && (steps.size() > 1)) {
-//        if ((ret = grib_set_long_internal(h, self->end_step, step_1.value<long>())))
-//            return ret;
-//    }
-//    return GRIB_SUCCESS;
-//}
-
-
-static int pack_string_new(grib_accessor* a, const char* val, size_t* len)
+static int pack_string(grib_accessor* a, const char* val, size_t* len)
 {
     grib_accessor_g2step_range* self = (grib_accessor_g2step_range*)a;
     grib_handle* h                   = grib_handle_of_accessor(a);
     int ret = 0;
 
     std::vector<Step> steps = parse_range(val);
-    if (steps.size() == 0)
+    if (steps.size() == 0) {
+        grib_context_log(a->context, GRIB_LOG_ERROR, "Could not parse step range: %s", val);
         return GRIB_INVALID_ARGUMENT;
+    }
 
     Step step_0 = steps[0];
     Step step_1;
@@ -247,11 +219,6 @@ static int pack_string_new(grib_accessor* a, const char* val, size_t* len)
     return GRIB_SUCCESS;
 }
 
-static int pack_string(grib_accessor* a, const char* val, size_t* len)
-{
-    return pack_string_new(a, val, len);
-}
-
 static int value_count(grib_accessor* a, long* count)
 {
     *count = 1;

src/grib_accessor_class_optimal_step_units.cc

@@ -148,6 +148,21 @@ static long staticForceStepUnits = Unit{Unit::Value::MISSING}.value<long>();
 static int pack_long(grib_accessor* a, const long* val, size_t* len)
 {
     grib_handle* h                   = grib_handle_of_accessor(a);
+    try {
+        if (!Unit::is_visible(Unit{*val}))
+            throw std::runtime_error("Unit masked");
+    }
+    catch (std::exception& e) {
+        auto visible_units = Unit::list_visible_units();
+        std::string visible_units_str;
+        for (auto& u : visible_units)
+            visible_units_str += Unit{u}.value<std::string>() + ",";
+        visible_units_str.pop_back();
+
+        std::string msg = std::string{"Invalid unit: "} + std::to_string(*val) + " (" + e.what() + ")" + ". Available units are: " + visible_units_str;
+        grib_context_log(a->context, GRIB_LOG_ERROR, msg.c_str());
+        return GRIB_INVALID_ARGUMENT;
+    }
 
     int ret;
     staticStepUnits = *val;
@@ -192,10 +207,21 @@ static int pack_string(grib_accessor* a, const char* val, size_t* len)
 {
     try {
         long unit = Unit{val}.value<long>();
+
+        if (!Unit::is_visible(Unit{val}))
+            throw std::runtime_error("Unit masked");
+
         pack_long(a, &unit, len);
     }
     catch (std::exception& e) {
-        grib_context_log(a->context, GRIB_LOG_ERROR, "Invalid unit: %s", val);
+        auto visible_units = Unit::list_visible_units();
+        std::string visible_units_str;
+        for (auto& u : visible_units)
+            visible_units_str += Unit{u}.value<std::string>() + ",";
+        visible_units_str.pop_back();
+
+        std::string msg = "Invalid unit: " + std::string(val) + " (" + e.what() + ")" + ". Available units are: " + visible_units_str;
+        grib_context_log(a->context, GRIB_LOG_ERROR, msg.c_str());
         return GRIB_INVALID_ARGUMENT;
     }
     return GRIB_SUCCESS;

src/step.cc

@@ -114,11 +114,11 @@ std::pair<Step, Step> find_common_units(const Step& startStep, const Step& endSt
         b.recalculateValue();
     }
     else {
-        auto it = std::find_if(Unit::unit_order_.begin(), Unit::unit_order_.end(), [&](const auto& e) {
+        auto it = std::find_if(Unit::publicly_visible_units_.begin(), Unit::publicly_visible_units_.end(), [&](const auto& e) {
             return e == a.unit().value<Unit::Value>() || e == b.unit().value<Unit::Value>();
         });
 
-        assert(it != Unit::unit_order_.end());
+        assert(it != Unit::publicly_visible_units_.end());
 
         a.set_unit(*it);
         b.set_unit(*it);
@@ -165,7 +165,7 @@ Step& Step::optimize_unit()
     unit_ = internal_unit_;
     Seconds<long> seconds = to_seconds<long>(internal_value_, internal_unit_);
 
-    for (auto it = Unit::unit_order_.rbegin(); it != Unit::unit_order_.rend(); ++it) {
+    for (auto it = Unit::publicly_visible_units_.rbegin(); it != Unit::publicly_visible_units_.rend(); ++it) {
         long multiplier = Unit::get_converter().unit_to_duration(*it);
         if (seconds.count() % multiplier == 0) {
             internal_value_ = seconds.count() / multiplier;
@@ -178,14 +178,6 @@ Step& Step::optimize_unit()
     return *this;
 }
 
-//Step Step::copy() const {
-//    Step ret{};
-//    ret.internal_value_ = internal_value_;
-//    ret.internal_unit_ = internal_unit_;
-//    ret.unit_ = unit_;
-//    return ret;
-//}
-
 template <>
 std::string Step::value<std::string>(const std::string& format) const {
     constexpr int max_size = 128;

src/step_unit.cc

@@ -12,11 +12,10 @@
 
 Unit::Map Unit::map_{};
 
-std::vector<Unit::Value> Unit::unit_order_ = {
+std::vector<Unit::Value> Unit::publicly_visible_units_ = {
     Unit::Value::SECOND,
     Unit::Value::MINUTE,
     Unit::Value::HOUR,
-    //Unit::Value::DAY,
 };
 
 std::vector<Unit::Value> Unit::complete_unit_order_ = {

src/step_unit.h

@@ -16,6 +16,7 @@
 #include <stdexcept>
 #include <array>
 #include <unordered_map>
+#include <algorithm>
 
 template <typename T> using Minutes = std::chrono::duration<T, std::ratio<60>>;
 template <typename T> using Hours = std::chrono::duration<T, std::ratio<3600>>;
@@ -60,9 +61,24 @@ public:
     };
 
     Unit() : internal_value_(Value::HOUR) {}
+
     explicit Unit(Value unit_value) : internal_value_(unit_value) {}
-    explicit Unit(const std::string& unit_value) {internal_value_ = map_.name_to_unit(unit_value);}
-    explicit Unit(long unit_value) {internal_value_ = map_.long_to_unit(unit_value);}
+
+    explicit Unit(const std::string& unit_value) {
+        try {
+            internal_value_ = map_.name_to_unit(unit_value);
+        } catch (std::exception& e) {
+            throw std::runtime_error(std::string{"Unit not found"});
+        }
+    }
+
+    explicit Unit(long unit_value) {
+        try {
+            internal_value_ = map_.long_to_unit(unit_value);
+        } catch (std::exception& e) {
+            throw std::runtime_error(std::string{"Unit not found"});
+        }
+    }
 
     bool operator>(const Unit& other) const {return map_.unit_to_duration(internal_value_) > map_.unit_to_duration(other.internal_value_);}
     bool operator==(const Value value) const {return map_.unit_to_duration(internal_value_) == map_.unit_to_duration(value);}
@@ -75,8 +91,20 @@ public:
         return *this;
     }
 
+    static bool is_visible(Unit unit) {
+        return std::find(publicly_visible_units_.begin(), publicly_visible_units_.end(), unit.internal_value_) != publicly_visible_units_.end();
+    }
+
+    static std::vector<Unit> list_visible_units() {
+        std::vector<Unit> result;
+        for (const auto& value : publicly_visible_units_) {
+            result.push_back(Unit(value));
+        }
+        return result;
+    }
+
     template <typename T> T value() const;
-    static std::vector<Value> unit_order_;
+    static std::vector<Value> publicly_visible_units_;
     static std::vector<Value> complete_unit_order_;
 
 private: