From 7f40607dd3a139dadfbe7a433ab0d0b35f35975b Mon Sep 17 00:00:00 2001 From: florijan Date: Wed, 30 Aug 2017 09:38:16 +0200 Subject: [PATCH] TypedValue test and implementation improvements Summary: Some TypedValue arithmetic ops threw exceptions for the wrong reasons, the op applicability type checks were wrong. Also the tests for that behavior were wrong. These are the fixes. Reviewers: mislav.bradac, buda Reviewed By: mislav.bradac Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D724 --- src/query/typed_value.cpp | 45 ++++--- tests/unit/typed_value.cpp | 238 ++++++++++++++++++++----------------- 2 files changed, 155 insertions(+), 128 deletions(-) diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp index 5eee0c57b..4df502a5c 100644 --- a/src/query/typed_value.cpp +++ b/src/query/typed_value.cpp @@ -451,7 +451,19 @@ double ToDouble(const TypedValue &value) { } TypedValue operator<(const TypedValue &a, const TypedValue &b) { - if (a.type() == TypedValue::Type::Bool || b.type() == TypedValue::Type::Bool) + auto is_legal = [](TypedValue::Type type) { + switch (type) { + case TypedValue::Type::Null: + case TypedValue::Type::Int: + case TypedValue::Type::Double: + case TypedValue::Type::String: + return true; + default: + return false; + } + }; + + if (!is_legal(a.type()) || !is_legal(b.type())) throw TypedValueException("Invalid 'less' operand types({} + {})", a.type(), b.type()); @@ -619,16 +631,18 @@ TypedValue operator+(const TypedValue &a) { */ inline void EnsureArithmeticallyOk(const TypedValue &a, const TypedValue &b, bool string_ok, const std::string &op_name) { - if (a.type() == TypedValue::Type::Bool || b.type() == TypedValue::Type::Bool) + auto is_legal = [string_ok](const TypedValue &value) { + return value.IsNumeric() || + (string_ok && value.type() == TypedValue::Type::String); + }; + + // Note that List and Null can also be valid in arithmetic ops. They are not + // checked here because they are handled before this check is performed in + // arithmetic op implementations. + + if (!is_legal(a) || !is_legal(b)) throw TypedValueException("Invalid {} operand types {}, {}", op_name, a.type(), b.type()); - - if (string_ok) return; - - if (a.type() == TypedValue::Type::String || - b.type() == TypedValue::Type::String) - throw TypedValueException("Invalid subtraction operands types {}, {}", - a.type(), b.type()); } TypedValue operator+(const TypedValue &a, const TypedValue &b) { @@ -651,7 +665,6 @@ TypedValue operator+(const TypedValue &a, const TypedValue &b) { } EnsureArithmeticallyOk(a, b, true, "addition"); - // no more Bool nor Null, summing works on anything from here onward if (a.type() == TypedValue::Type::String || b.type() == TypedValue::Type::String) @@ -667,9 +680,8 @@ TypedValue operator+(const TypedValue &a, const TypedValue &b) { } TypedValue operator-(const TypedValue &a, const TypedValue &b) { - EnsureArithmeticallyOk(a, b, false, "subtraction"); - if (a.IsNull() || b.IsNull()) return TypedValue::Null; + EnsureArithmeticallyOk(a, b, false, "subtraction"); // at this point we only have int and double if (a.type() == TypedValue::Type::Double || @@ -681,9 +693,8 @@ TypedValue operator-(const TypedValue &a, const TypedValue &b) { } TypedValue operator/(const TypedValue &a, const TypedValue &b) { - EnsureArithmeticallyOk(a, b, false, "division"); - if (a.IsNull() || b.IsNull()) return TypedValue::Null; + EnsureArithmeticallyOk(a, b, false, "division"); // at this point we only have int and double if (a.type() == TypedValue::Type::Double || @@ -697,9 +708,8 @@ TypedValue operator/(const TypedValue &a, const TypedValue &b) { } TypedValue operator*(const TypedValue &a, const TypedValue &b) { - EnsureArithmeticallyOk(a, b, false, "multiplication"); - if (a.IsNull() || b.IsNull()) return TypedValue::Null; + EnsureArithmeticallyOk(a, b, false, "multiplication"); // at this point we only have int and double if (a.type() == TypedValue::Type::Double || @@ -711,9 +721,8 @@ TypedValue operator*(const TypedValue &a, const TypedValue &b) { } TypedValue operator%(const TypedValue &a, const TypedValue &b) { - EnsureArithmeticallyOk(a, b, false, "modulo"); - if (a.IsNull() || b.IsNull()) return TypedValue::Null; + EnsureArithmeticallyOk(a, b, false, "modulo"); // at this point we only have int and double if (a.type() == TypedValue::Type::Double || diff --git a/tests/unit/typed_value.cpp b/tests/unit/typed_value.cpp index 313f2a0c8..b6ed58be0 100644 --- a/tests/unit/typed_value.cpp +++ b/tests/unit/typed_value.cpp @@ -3,26 +3,44 @@ // Created by Florijan Stamenkovic on 24.01.17.. // #include +#include +#include #include +#include "database/dbms.hpp" #include "gtest/gtest.h" #include "query/typed_value.hpp" -namespace { - using query::TypedValue; using query::TypedValueException; -std::vector MakePropsAllTypes() { - return { - true, - "something", - 42, - 0.5, - TypedValue::Null, - std::vector{true, "something", 42, 0.5, TypedValue::Null}}; -} -} +class AllTypesFixture : public testing::Test { + protected: + std::vector values_; + Dbms dbms_; + std::unique_ptr dba_ = dbms_.active(); + + void SetUp() override { + values_.emplace_back(TypedValue::Null); + values_.emplace_back(true); + values_.emplace_back(42); + values_.emplace_back(3.14); + values_.emplace_back("something"); + values_.emplace_back( + std::vector{true, "something", 42, 0.5, TypedValue::Null}); + values_.emplace_back( + std::map{{"a", true}, + {"b", "something"}, + {"c", 42}, + {"d", 0.5}, + {"e", TypedValue::Null}}); + auto vertex = dba_->InsertVertex(); + values_.emplace_back(vertex); + values_.emplace_back( + dba_->InsertEdge(vertex, vertex, dba_->EdgeType("et"))); + values_.emplace_back(query::Path{}); + } +}; void EXPECT_PROP_FALSE(const TypedValue &a) { EXPECT_TRUE(a.type() == TypedValue::Type::Bool && !a.Value()); @@ -145,22 +163,30 @@ TEST(TypedValue, Hash) { hash(TypedValue(std::map{{"a", 1}}))); } -TEST(TypedValue, Less) { - // not_bool_type < bool -> exception - auto props = MakePropsAllTypes(); - for (int i = 0; i < (int)props.size(); ++i) { - if (props.at(i).type() == TypedValue::Type::Bool) continue; - // the comparison should raise an exception - // cast to (void) so the compiler does not complain about unused comparison - // result - EXPECT_THROW((void)(props.at(i) < TypedValue(true)), TypedValueException); +TEST_F(AllTypesFixture, Less) { + // 'Less' is legal only between numerics, Null and strings. + auto is_string_compatible = [](const TypedValue &v) { + return v.IsNull() || v.type() == TypedValue::Type::String; + }; + auto is_numeric_compatible = [](const TypedValue &v) { + return v.IsNull() || v.IsNumeric(); + }; + for (TypedValue &a : values_) { + for (TypedValue &b : values_) { + if (is_numeric_compatible(a) && is_numeric_compatible(b)) continue; + if (is_string_compatible(a) && is_string_compatible(b)) continue; + // Comparison should raise an exception. Cast to (void) so the compiler + // does not complain about unused comparison result. + EXPECT_THROW((void)(a < b), TypedValueException); + } } - // not_bool_type < Null = Null - props = MakePropsAllTypes(); - for (int i = 0; i < (int)props.size(); ++i) { - if (props.at(i).type() == TypedValue::Type::Bool) continue; - EXPECT_PROP_ISNULL(props.at(i) < TypedValue::Null); + // legal_type < Null = Null + for (TypedValue &value : values_) { + if (!(value.IsNumeric() || value.type() == TypedValue::Type::String)) + continue; + EXPECT_PROP_ISNULL(value < TypedValue::Null); + EXPECT_PROP_ISNULL(TypedValue::Null < value); } // int tests @@ -213,48 +239,58 @@ TEST(TypedValue, UnaryPlus) { EXPECT_THROW(+TypedValue("something"), TypedValueException); } -/** - * Performs a series of tests on properties of all types. The tests - * evaluate how arithmetic operators behave w.r.t. exception throwing - * in case of invalid operands and null handling. - * - * @param string_ok Indicates if or not the operation tested works - * with String values (does not throw). - * @param op The operation lambda. Takes two values and resturns - * the results. - */ -void ExpectArithmeticThrowsAndNull( - bool string_list_ok, - std::function op) { - // arithmetic ops that raise - auto props = MakePropsAllTypes(); - for (int i = 0; i < (int)props.size(); ++i) { - if (!string_list_ok || props.at(i).type() == TypedValue::Type::String) { - EXPECT_THROW(op(TypedValue(true), props.at(i)), TypedValueException); - EXPECT_THROW(op(props.at(i), TypedValue(true)), TypedValueException); +class TypedValueArithmeticTest : public AllTypesFixture { + protected: + /** + * Performs a series of tests on properties of all types. The tests + * evaluate how arithmetic operators behave w.r.t. exception throwing + * in case of invalid operands and null handling. + * + * @param string_list_ok Indicates if or not the operation tested works + * with String and List values (does not throw). + * @param op The operation lambda. Takes two values and resturns + * the results. + */ + void ExpectArithmeticThrowsAndNull( + bool string_list_ok, + std::function op) { + // If one operand is always valid, the other can be of any type. + auto always_valid = [string_list_ok](const TypedValue &value) { + return value.IsNull() || + (string_list_ok && value.type() == TypedValue::Type::List); + }; + + // If we don't have an always_valid operand, they both must be plain valid. + auto valid = [string_list_ok](const TypedValue &value) { + switch (value.type()) { + case TypedValue::Type::Int: + case TypedValue::Type::Double: + return true; + case TypedValue::Type::String: + return string_list_ok; + default: + return false; + } + }; + + for (const TypedValue &a : values_) { + for (const TypedValue &b : values_) { + if (always_valid(a) || always_valid(b)) continue; + if (valid(a) && valid(b)) continue; + EXPECT_THROW(op(a, b), TypedValueException); + EXPECT_THROW(op(b, a), TypedValueException); + } } - if (!string_list_ok) { - EXPECT_THROW(op(TypedValue("some"), props.at(i)), TypedValueException); - EXPECT_THROW(op(props.at(i), TypedValue("some")), TypedValueException); - EXPECT_THROW(op(TypedValue("[1]"), props.at(i)), TypedValueException); - EXPECT_THROW(op(props.at(i), TypedValue("[]")), TypedValueException); + + // null resulting ops + for (const TypedValue &value : values_) { + EXPECT_PROP_ISNULL(op(value, TypedValue::Null)); + EXPECT_PROP_ISNULL(op(TypedValue::Null, value)); } } +}; - // null resulting ops - props = MakePropsAllTypes(); - for (int i = 0; i < (int)props.size(); ++i) { - if (props.at(i).type() == TypedValue::Type::Bool) continue; - if (!string_list_ok && (props.at(i).type() == TypedValue::Type::String || - props.at(i).type() == TypedValue::Type::List)) - continue; - - EXPECT_PROP_ISNULL(op(props.at(i), TypedValue::Null)); - EXPECT_PROP_ISNULL(op(TypedValue::Null, props.at(i))); - } -} - -TEST(TypedValue, Sum) { +TEST_F(TypedValueArithmeticTest, Sum) { ExpectArithmeticThrowsAndNull( true, [](const TypedValue &a, const TypedValue &b) { return a + b; }); @@ -283,7 +319,7 @@ TEST(TypedValue, Sum) { (TypedValue(in) + TypedValue(in)).Value>(), out3); } -TEST(TypedValue, Difference) { +TEST_F(TypedValueArithmeticTest, Difference) { ExpectArithmeticThrowsAndNull( false, [](const TypedValue &a, const TypedValue &b) { return a - b; }); @@ -296,7 +332,7 @@ TEST(TypedValue, Difference) { EXPECT_FLOAT_EQ((TypedValue(2.5) - TypedValue(2)).Value(), 0.5); } -TEST(TypedValue, Divison) { +TEST_F(TypedValueArithmeticTest, Divison) { ExpectArithmeticThrowsAndNull( false, [](const TypedValue &a, const TypedValue &b) { return a / b; }); EXPECT_THROW(TypedValue(1) / TypedValue(0), TypedValueException); @@ -311,7 +347,7 @@ TEST(TypedValue, Divison) { EXPECT_FLOAT_EQ((TypedValue(10.0) / TypedValue(4)).Value(), 2.5); } -TEST(TypedValue, Multiplication) { +TEST_F(TypedValueArithmeticTest, Multiplication) { ExpectArithmeticThrowsAndNull( false, [](const TypedValue &a, const TypedValue &b) { return a * b; }); @@ -322,7 +358,7 @@ TEST(TypedValue, Multiplication) { EXPECT_FLOAT_EQ((TypedValue(10.2) * TypedValue(4)).Value(), 10.2 * 4); } -TEST(TypedValue, Modulo) { +TEST_F(TypedValueArithmeticTest, Modulo) { ExpectArithmeticThrowsAndNull( false, [](const TypedValue &a, const TypedValue &b) { return a % b; }); EXPECT_THROW(TypedValue(1) % TypedValue(0), TypedValueException); @@ -337,43 +373,32 @@ TEST(TypedValue, Modulo) { EXPECT_FLOAT_EQ((TypedValue(10.0) % TypedValue(4)).Value(), 2.0); } -TEST(TypedValue, TypeIncompatibility) { - auto props = MakePropsAllTypes(); +class TypedValueLogicTest : public AllTypesFixture { + protected: + /** + * Logical operations (logical and, or) are only legal on bools + * and nulls. This function ensures that the given + * logical operation throws exceptions when either operand + * is not bool or null. + * + * @param op The logical operation to test. + */ + void TestLogicalThrows( + std::function op) { + for (const auto &p1 : values_) { + for (const auto &p2 : values_) { + // skip situations when both p1 and p2 are either bool or null + auto p1_ok = p1.type() == TypedValue::Type::Bool || p1.IsNull(); + auto p2_ok = p2.type() == TypedValue::Type::Bool || p2.IsNull(); + if (p1_ok && p2_ok) continue; - // iterate over all the props, plus one, what will return - // the Null property, which must be incompatible with all - // the others - for (int i = 0; i < (int)props.size(); ++i) - for (int j = 0; j < (int)props.size(); ++j) - EXPECT_EQ(props.at(i).type() == props.at(j).type(), i == j); -} - -/** - * Logical operations (logical and, or) are only legal on bools - * and nulls. This function ensures that the given - * logical operation throws exceptions when either operand - * is not bool or null. - * - * @param op The logical operation to test. - */ -void TestLogicalThrows( - std::function op) { - auto props = MakePropsAllTypes(); - for (int i = 0; i < (int)props.size(); ++i) { - auto p1 = props.at(i); - for (int j = 0; j < (int)props.size(); ++j) { - auto p2 = props.at(j); - // skip situations when both p1 and p2 are either bool or null - auto p1_ok = p1.type() == TypedValue::Type::Bool || p1.IsNull(); - auto p2_ok = p2.type() == TypedValue::Type::Bool || p2.IsNull(); - if (p1_ok && p2_ok) continue; - - EXPECT_THROW(op(p1, p2), TypedValueException); + EXPECT_THROW(op(p1, p2), TypedValueException); + } } } -} +}; -TEST(TypedValue, LogicalAnd) { +TEST_F(TypedValueLogicTest, LogicalAnd) { TestLogicalThrows( [](const TypedValue &p1, const TypedValue &p2) { return p1 && p2; }); @@ -383,7 +408,7 @@ TEST(TypedValue, LogicalAnd) { EXPECT_PROP_EQ(TypedValue(false) && TypedValue(true), TypedValue(false)); } -TEST(TypedValue, LogicalOr) { +TEST_F(TypedValueLogicTest, LogicalOr) { TestLogicalThrows( [](const TypedValue &p1, const TypedValue &p2) { return p1 || p2; }); @@ -393,7 +418,7 @@ TEST(TypedValue, LogicalOr) { EXPECT_PROP_EQ(TypedValue(false) || TypedValue(true), TypedValue(true)); } -TEST(TypedValue, LogicalXor) { +TEST_F(TypedValueLogicTest, LogicalXor) { TestLogicalThrows( [](const TypedValue &p1, const TypedValue &p2) { return p1 ^ p2; }); @@ -403,10 +428,3 @@ TEST(TypedValue, LogicalXor) { EXPECT_PROP_EQ(TypedValue(true) ^ TypedValue(false), TypedValue(true)); EXPECT_PROP_EQ(TypedValue(false) ^ TypedValue(false), TypedValue(false)); } - -TEST(TypedValue, ImplicitStringConversion) { - std::vector v; - auto pv = PropertyValue("Mirko"); - v.push_back(PropertyValue("Mirko")); - ASSERT_EQ(v[0].Value(), "Mirko"); -}