QPID-6717: Change Selector beaviour and tests to accord with consensus semantics

- It seems to be the consensus amongst JMS Selector implementations that
  the NOT IN expression should return false if none of the types match

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1701301 13f79535-47bb-0310-9956-ffa450edef68

4 files changed, 69 insertions, 12 deletions

diff --git a/qpid/cpp/src/qpid/broker/SelectorExpression.cpp b/qpid/cpp/src/qpid/broker/SelectorExpression.cpp
index 497ec2f6f5..1f2161471f 100644
--- a/qpid/cpp/src/qpid/broker/SelectorExpression.cpp
+++ b/qpid/cpp/src/qpid/broker/SelectorExpression.cpp
@@ -420,6 +420,49 @@ public:
     }
 };
 
+class NotInExpression : public BoolExpression {
+    boost::scoped_ptr<Expression> e;
+    boost::ptr_vector<Expression> l;
+
+public:
+    NotInExpression(Expression* e_, boost::ptr_vector<Expression>& l_) :
+        e(e_)
+    {
+        l.swap(l_);
+    }
+
+    void repr(ostream& os) const {
+        os << *e << " NOT IN (";
+        for (std::size_t i = 0; i<l.size(); ++i){
+            os << l[i] << (i<l.size()-1 ? ", " : ")");
+        }
+    }
+
+    BoolOrNone eval_bool(const SelectorEnv& env) const {
+        Value ve(e->eval(env));
+        if (unknown(ve)) return BN_UNKNOWN;
+        BoolOrNone r = BN_TRUE;
+        for (std::size_t i = 0; i<l.size(); ++i){
+            Value li(l[i].eval(env));
+            if (unknown(li)) {
+                r = BN_UNKNOWN;
+                continue;
+            }
+            // Check if types are incompatible. If nothing further in the list
+            // matches or is unknown and we had a type incompatibility then
+            // result still false.
+            if (r!=BN_UNKNOWN &&
+                !sameType(ve,li) && !(numeric(ve) && numeric(li))) {
+                r = BN_FALSE;
+                continue;
+            }
+
+            if (ve==li) return BN_FALSE;
+        }
+        return r;
+    }
+};
+
 // Arithmetic Expression types
 
 class ArithmeticExpression : public Expression {
@@ -775,7 +818,7 @@ Expression* andExpression(Tokeniser& tokeniser)
     return e.release();
 }
 
-BoolExpression* specialComparisons(Tokeniser& tokeniser, std::auto_ptr<Expression> e1) {
+BoolExpression* specialComparisons(Tokeniser& tokeniser, std::auto_ptr<Expression> e1, bool negated = false) {
     switch (tokeniser.nextToken().type) {
     case T_LIKE: {
         const Token t = tokeniser.nextToken();
@@ -784,6 +827,7 @@ BoolExpression* specialComparisons(Tokeniser& tokeniser, std::auto_ptr<Expressio
             return 0;
         }
         // Check for "ESCAPE"
+        std::auto_ptr<BoolExpression> l;
         if ( tokeniser.nextToken().type==T_ESCAPE ) {
             const Token e = tokeniser.nextToken();
             if ( e.type!=T_STRING ) {
@@ -796,11 +840,12 @@ BoolExpression* specialComparisons(Tokeniser& tokeniser, std::auto_ptr<Expressio
             if (e.val=="%" || e.val=="_") {
                 throwParseError(tokeniser, "'%' and '_' are not allowed as ESCAPE characters");
             }
-            return new LikeExpression(e1.release(), t.val, e.val);
+            l.reset(new LikeExpression(e1.release(), t.val, e.val));
         } else {
             tokeniser.returnTokens();
-            return new LikeExpression(e1.release(), t.val);
+            l.reset(new LikeExpression(e1.release(), t.val));
         }
+        return negated ? new UnaryBooleanExpression(&notOp, l.release()) : l.release();
     }
     case T_BETWEEN: {
         std::auto_ptr<Expression> lower(addExpression(tokeniser));
@@ -811,7 +856,8 @@ BoolExpression* specialComparisons(Tokeniser& tokeniser, std::auto_ptr<Expressio
         }
         std::auto_ptr<Expression> upper(addExpression(tokeniser));
         if ( !upper.get() ) return 0;
-        return new BetweenExpression(e1.release(), lower.release(), upper.release());
+        std::auto_ptr<BoolExpression> b(new BetweenExpression(e1.release(), lower.release(), upper.release()));
+        return negated ? new UnaryBooleanExpression(&notOp, b.release()) : b.release();
     }
     case T_IN: {
         if ( tokeniser.nextToken().type!=T_LPAREN ) {
@@ -829,7 +875,8 @@ BoolExpression* specialComparisons(Tokeniser& tokeniser, std::auto_ptr<Expressio
             error = "missing ',' or ')' after IN";
             return 0;
         }
-        return new InExpression(e1.release(), list);
+        if (negated) return new NotInExpression(e1.release(), list);
+        else return new InExpression(e1.release(), list);
     }
     default:
         error = "expected LIKE, IN or BETWEEN";
@@ -865,9 +912,7 @@ Expression* comparisonExpression(Tokeniser& tokeniser)
                 return 0;
         }
     case T_NOT: {
-        std::auto_ptr<BoolExpression> e(specialComparisons(tokeniser, e1));
-        if (!e.get()) return 0;
-        return new UnaryBooleanExpression(&notOp, e.release());
+        return specialComparisons(tokeniser, e1, true);
     }
     case T_BETWEEN:
     case T_LIKE:
diff --git a/qpid/cpp/src/qpid/broker/SelectorValue.cpp b/qpid/cpp/src/qpid/broker/SelectorValue.cpp
index 0855fa91d0..f2e50db5d7 100644
--- a/qpid/cpp/src/qpid/broker/SelectorValue.cpp
+++ b/qpid/cpp/src/qpid/broker/SelectorValue.cpp
@@ -85,7 +85,7 @@ NumericPairBase* promoteNumeric(const Value& v1, const Value& v2)
 {
     if (!numeric(v1) || !numeric(v2)) return 0;
 
-    if (v1.type != v2.type) {
+    if (!sameType(v1,v2)) {
         switch (v1.type) {
         case Value::T_INEXACT: return new NumericPair<double>(v1.x, v2.i);
         case Value::T_EXACT: return new NumericPair<double>(v1.i, v2.x);
@@ -109,7 +109,7 @@ bool operator==(const Value& v1, const Value& v2)
     boost::scoped_ptr<NumericPairBase> nbp(promoteNumeric(v1, v2));
     if (nbp) return nbp->eq();
 
-    if (v1.type != v2.type) return false;
+    if (!sameType(v1,v2)) return false;
     switch (v1.type) {
     case Value::T_BOOL: return v1.b == v2.b;
     case Value::T_STRING: return *v1.s == *v2.s;
@@ -123,7 +123,7 @@ bool operator!=(const Value& v1, const Value& v2)
     boost::scoped_ptr<NumericPairBase> nbp(promoteNumeric(v1, v2));
     if (nbp) return nbp->ne();
 
-    if (v1.type != v2.type) return false;
+    if (!sameType(v1,v2)) return false;
     switch (v1.type) {
     case Value::T_BOOL: return v1.b != v2.b;
     case Value::T_STRING: return *v1.s != *v2.s;
diff --git a/qpid/cpp/src/qpid/broker/SelectorValue.h b/qpid/cpp/src/qpid/broker/SelectorValue.h
index 3a731fd000..aa2197b225 100644
--- a/qpid/cpp/src/qpid/broker/SelectorValue.h
+++ b/qpid/cpp/src/qpid/broker/SelectorValue.h
@@ -101,6 +101,10 @@ inline bool numeric(const Value& v) {
     return v.type == Value::T_EXACT || v.type == Value::T_INEXACT;
 }
 
+inline bool sameType(const Value& v1, const Value& v2) {
+    return v1.type == v2.type;
+}
+
 std::ostream& operator<<(std::ostream& os, const Value& v);
 
 bool operator==(const Value&, const Value&);
diff --git a/qpid/cpp/src/tests/Selector.cpp b/qpid/cpp/src/tests/Selector.cpp
index d512476f8b..6995420f17 100644
--- a/qpid/cpp/src/tests/Selector.cpp
+++ b/qpid/cpp/src/tests/Selector.cpp
@@ -450,11 +450,19 @@ QPID_AUTO_TEST_CASE(comparisonEval)
     BOOST_CHECK(qb::Selector("20 >= 19.0 and 20 > 19").eval(env));
     BOOST_CHECK(qb::Selector("42 <= 42.0 and 37.0 >= 37").eval(env));
     BOOST_CHECK(qb::Selector("(A IN ('hello', 'there', 1 , true, (1-17))) IS NULL").eval(env));
+    BOOST_CHECK(qb::Selector("(-16 IN ('hello', A, 'there', true)) IS NULL").eval(env));
+    BOOST_CHECK(qb::Selector("(-16 NOT IN ('hello', 'there', A, true)) IS NULL").eval(env));
+    BOOST_CHECK(qb::Selector("(-16 IN ('hello', 'there', true)) IS NOT NULL").eval(env));
+    BOOST_CHECK(!qb::Selector("-16 IN ('hello', 'there', true)").eval(env));
+    BOOST_CHECK(qb::Selector("(-16 NOT IN ('hello', 'there', true)) IS NOT NULL").eval(env));
+    BOOST_CHECK(!qb::Selector("-16 NOT IN ('hello', 'there', true)").eval(env));
+    BOOST_CHECK(qb::Selector("(-16 NOT IN ('hello', 'there', A, 1 , true)) IS NULL").eval(env));
     BOOST_CHECK(qb::Selector("'hello' IN ('hello', 'there', 1 , true, (1-17))").eval(env));
     BOOST_CHECK(qb::Selector("TRUE IN ('hello', 'there', 1 , true, (1-17))").eval(env));
     BOOST_CHECK(qb::Selector("-16 IN ('hello', 'there', 1 , true, (1-17))").eval(env));
+    BOOST_CHECK(!qb::Selector("-16 NOT IN ('hello', 'there', 1 , true, (1-17))").eval(env));
     BOOST_CHECK(!qb::Selector("1 IN ('hello', 'there', 'polly')").eval(env));
-    BOOST_CHECK(qb::Selector("1 NOT IN ('hello', 'there', 'polly')").eval(env));
+    BOOST_CHECK(!qb::Selector("1 NOT IN ('hello', 'there', 'polly')").eval(env));
     BOOST_CHECK(!qb::Selector("'hell' IN ('hello', 'there', 1 , true, (1-17))").eval(env));
     BOOST_CHECK(qb::Selector("('hell' IN ('hello', 'there', 1 , true, (1-17), A)) IS NULL").eval(env));
     BOOST_CHECK(qb::Selector("('hell' NOT IN ('hello', 'there', 1 , true, (1-17), A)) IS NULL").eval(env));