Merge pull request #4444 from telefonicaid/hardening/4211_notify_upon…

…_actual_change_in_json_value

FIX correctly identify changes in JSON attribute values/metadata in subscription triggering logic

CHANGES_NEXT_RELEASE

@@ -1,5 +1,6 @@
 - Add: servicePath field to builtin attributes (#2877)
 - Add: notification.mqtt.retain and notification.mqttCustom.retain flag for MQTT retain in notifications (#4388)
+- Fix: correctly detect JSON attribute and metadata value changes in subscription triggering logic (#4211, #4434, #643)
 - Fix: DateTime and geo:json types were not supported in custom notifications using ngsi patching (#4435)
 - Fix: logDeprecate not working correctly (`geo:json` wrongly considered as deprecated)
 - Fix: improve error traces (#4387)

src/lib/mongoBackend/MongoCommonUpdate.cpp

@@ -136,7 +136,7 @@ static bool hasMetadata(std::string name, std::string type, ContextAttribute* ca
 *
 * equalMetadataValues -
 */
-static bool equalMetadataValues(const orion::BSONObj& md1, const orion::BSONObj& md2)
+static bool equalMetadataItems(const orion::BSONObj& md1, const orion::BSONObj& md2)
 {
   bool md1TypeExist = md1.hasField(ENT_ATTRS_MD_TYPE);
   bool md2TypeExist = md2.hasField(ENT_ATTRS_MD_TYPE);
@@ -147,58 +147,23 @@ static bool equalMetadataValues(const orion::BSONObj& md1, const orion::BSONObj&
     return false;
   }
 
-  // If type exists in both metadata elments, check if they are the same
+  // If type exists in both metadata elements, check if they are the same
   if (md1TypeExist && md2TypeExist)
   {
-    if (getFieldF(md1, ENT_ATTRS_MD_TYPE).type() != getFieldF(md2, ENT_ATTRS_MD_TYPE).type())
+    if ((getFieldF(md1, ENT_ATTRS_MD_TYPE).type() != orion::String))
     {
+      LM_E(("Runtime Error (unallowed JSON type for metadata NGSI type: %d)", getFieldF(md1, ENT_ATTRS_MD_TYPE).type()));
       return false;
     }
-    switch (getFieldF(md1, ENT_ATTRS_MD_TYPE).type())
+    if ((getFieldF(md2, ENT_ATTRS_MD_TYPE).type() != orion::String))
     {
-      /* FIXME #643 P6: metadata array/object are now supported, but we haven't
-         implemented yet the logic to compare compounds between them
-      case Object:
-        ...
-        break;
-
-       case Array:
-        ...
-        break;
-      */
-
-    case orion::NumberDouble:
-      if (getNumberFieldF(md1, ENT_ATTRS_MD_TYPE) != getNumberFieldF(md2, ENT_ATTRS_MD_TYPE))
-      {
-        return false;
-      }
-      break;
-
-    case orion::Bool:
-      if (getBoolFieldF(md1, ENT_ATTRS_MD_TYPE) != getBoolFieldF(md2, ENT_ATTRS_MD_TYPE))
-      {
-        return false;
-      }
-      break;
-
-    case orion::String:
-      if (getStringFieldF(md1, ENT_ATTRS_MD_TYPE) != getStringFieldF(md2, ENT_ATTRS_MD_TYPE))
-      {
-        return false;
-      }
-      break;
-
-    case orion::jstNULL:
-      if (!getFieldF(md2, ENT_ATTRS_MD_TYPE).isNull())
-      {
-        return false;
-      }
-      break;
+      LM_E(("Runtime Error (unallowed JSON type for metadata NGSI type: %d)", getFieldF(md2, ENT_ATTRS_MD_TYPE).type()));
+      return false;
+    }
 
-    default:
-      LM_E(("Runtime Error (unknown JSON type for metadata NGSI type: %d)", getFieldF(md1, ENT_ATTRS_MD_TYPE).type()));
+    if (getStringFieldF(md1, ENT_ATTRS_MD_TYPE) != getStringFieldF(md2, ENT_ATTRS_MD_TYPE))
+    {
       return false;
-      break;
     }
   }
 
@@ -210,15 +175,11 @@ static bool equalMetadataValues(const orion::BSONObj& md1, const orion::BSONObj&
 
   switch (getFieldF(md1, ENT_ATTRS_MD_VALUE).type())
   {
-    /* FIXME not yet
     case orion::Object:
-      ...
-      break;
+      return getObjectFieldF(md1, ENT_ATTRS_MD_VALUE).equal(getObjectFieldF(md2, ENT_ATTRS_MD_VALUE));
 
     case orion::Array:
-      ...
-      break;
-    */
+      return getArrayFieldF(md1, ENT_ATTRS_MD_VALUE).equal(getArrayFieldF(md2, ENT_ATTRS_MD_VALUE));
 
     case orion::NumberDouble:
       return getNumberFieldF(md1, ENT_ATTRS_MD_VALUE) == getNumberFieldF(md2, ENT_ATTRS_MD_VALUE);
@@ -266,7 +227,7 @@ static bool equalMetadata(const orion::BSONObj& md1, const orion::BSONObj& md2)
     orion::BSONObj md1Item = getObjectFieldF(md1, currentMd);
     orion::BSONObj md2Item = getObjectFieldF(md2, currentMd);
 
-    if (!equalMetadataValues(md1Item, md2Item))
+    if (!equalMetadataItems(md1Item, md2Item))
     {
       return false;
     }
@@ -281,7 +242,7 @@ static bool equalMetadata(const orion::BSONObj& md1, const orion::BSONObj& md2)
 *
 * changedAttr -
 */
-static bool attrValueChanges(const orion::BSONObj& attr, ContextAttribute* caP, const bool& forcedUpdate, ApiVersion apiVersion)
+static bool attrValueChanges(const orion::BSONObj& attr, ContextAttribute* caP)
 {
   /* Not finding the attribute field at MongoDB is considered as an implicit "" */
   if (!attr.hasField(ENT_ATTRS_VALUE))
@@ -304,13 +265,13 @@ static bool attrValueChanges(const orion::BSONObj& attr, ContextAttribute* caP,
     return true;
 
   case orion::NumberDouble:
-    return caP->valueType != orion::ValueTypeNumber || caP->numberValue != getNumberFieldF(attr, ENT_ATTRS_VALUE) || forcedUpdate;
+    return caP->valueType != orion::ValueTypeNumber || caP->numberValue != getNumberFieldF(attr, ENT_ATTRS_VALUE);
 
   case orion::Bool:
-    return caP->valueType != orion::ValueTypeBoolean || caP->boolValue != getBoolFieldF(attr, ENT_ATTRS_VALUE) || forcedUpdate;
+    return caP->valueType != orion::ValueTypeBoolean || caP->boolValue != getBoolFieldF(attr, ENT_ATTRS_VALUE);
 
   case orion::String:
-    return caP->valueType != orion::ValueTypeString || caP->stringValue != getStringFieldF(attr, ENT_ATTRS_VALUE) || forcedUpdate;
+    return caP->valueType != orion::ValueTypeString || caP->stringValue != getStringFieldF(attr, ENT_ATTRS_VALUE);
 
   case orion::jstNULL:
     return caP->valueType != orion::ValueTypeNull;
@@ -529,48 +490,50 @@ static ChangeType mergeAttrInfo
   /* Was it an actual update? */
   ChangeType changeType = NO_CHANGE;
 
+  /* We consider there is a change in the value if one or more of the following are true:
+   *
+   * 1) forcedUpdate is enabled
+   * 2) the value of the attribute changed (see attrValueChanges or CompoundValueNode::equal() for details)
+   * 3) the type of the attribute changed (in this case, !attr.hasField(ENT_ATTRS_TYPE) is needed, as attribute
+   *    type is optional according to NGSI and the attribute may not have that field in the BSON)
+   *
+   * In addition, we consider there is change in the metadata if:
+   *
+   * 3) the metadata changed (this is done checking if the size of the original and final metadata vectors is
+   *    different and, if they are of the same size, checking if the vectors are not equal)
+   */
+  bool valueChanged;
+  bool typeChanged;
+  bool mdChanged;
   if (caP->compoundValueP == NULL)
   {
-    /* In the case of simple value, we consider there is a change in the value if one or more of the following are true:
-     *
-     * 1) the value of the attribute changed (see attrValueChanges for details)
-     * 2) the type of the attribute changed (in this case, !attr.hasField(ENT_ATTRS_TYPE) is needed, as attribute
-     *    type is optional according to NGSI and the attribute may not have that field in the BSON)
-     *
-     * In addition, we consider there is change in the metadata if:
-     *
-     * 3) the metadata changed (this is done checking if the size of the original and final metadata vectors is
-     *    different and, if they are of the same size, checking if the vectors are not equal)
-     */
-    bool valueChanged = attrValueChanges(attr, caP, forcedUpdate, apiVersion) ||
-                      ((!caP->type.empty()) && (!attr.hasField(ENT_ATTRS_TYPE) || getStringFieldF(attr, ENT_ATTRS_TYPE) != caP->type) );
-    bool mdChanged = (mdNew.nFields() != mdSize || !equalMetadata(md, mdNew));
-
-    if (valueChanged && !mdChanged)
-    {
-      changeType = CHANGE_ONLY_VALUE;
-    }
-    else if (!valueChanged && mdChanged)
-    {
-      changeType = CHANGE_ONLY_MD;
-    }
-    else if (valueChanged && mdChanged)
-    {
-      changeType = CHANGE_VALUE_AND_MD;
-    }
-    else  // !valueChanged && !mdChanged
-    {
-      changeType = NO_CHANGE;
-    }
+    valueChanged = forcedUpdate || attrValueChanges(attr, caP);
   }
   else
   {
-    // FIXME #643 P6: in the case of compound value, it's more difficult to know if an attribute
-    // has really changed its value (many levels have to be traversed). Until we can develop the
-    // matching logic, we consider CHANGE_VALUE_AND_MD always.
-    //
+    valueChanged = forcedUpdate || !caP->compoundValueP->equal(getFieldF(attr, ENT_ATTRS_VALUE));
+  }
+  typeChanged = ((!caP->type.empty()) && (!attr.hasField(ENT_ATTRS_TYPE) || getStringFieldF(attr, ENT_ATTRS_TYPE) != caP->type));
+  mdChanged = (mdNew.nFields() != mdSize || !equalMetadata(md, mdNew));
+
+  valueChanged = valueChanged || typeChanged;
+
+  if (valueChanged && !mdChanged)
+  {
+    changeType = CHANGE_ONLY_VALUE;
+  }
+  else if (!valueChanged && mdChanged)
+  {
+    changeType = CHANGE_ONLY_MD;
+  }
+  else if (valueChanged && mdChanged)
+  {
     changeType = CHANGE_VALUE_AND_MD;
   }
+  else  // !valueChanged && !mdChanged
+  {
+    changeType = NO_CHANGE;
+  }
 
   /* 5. Add modification date (actual change only if actual update) */
   if (changeType)

src/lib/mongoDriver/BSONArray.cpp

@@ -87,6 +87,17 @@ std::string BSONArray::toString(void) const
 
 
 
+/* ****************************************************************************
+*
+* BSONArray::equal -
+*/
+bool BSONArray::equal(const BSONArray& ba)
+{
+  return (bson_compare(this->b, ba.b) == 0);
+}
+
+
+
 /* ****************************************************************************
 *
 * BSONArray::operator= -

src/lib/mongoDriver/BSONArray.h

@@ -46,6 +46,7 @@ class BSONArray
   BSONArray(const BSONArray& _ba);
   int nFields(void) const;
   std::string toString(void) const;
+  bool equal(const BSONArray& ba);
   BSONArray& operator= (const BSONArray& rhs);
 
   // methods to be used only by mongoDriver/ code (with references to low-level driver code)

src/lib/mongoDriver/BSONObj.cpp

@@ -192,6 +192,17 @@ void BSONObj::toElementsVector(std::vector<BSONElement>* v)
 
 
 
+/* ****************************************************************************
+*
+* BSONObj::equal -
+*/
+bool BSONObj::equal(const BSONObj& bo)
+{
+  return (bson_compare(this->b, bo.b) == 0);
+}
+
+
+
 /* ****************************************************************************
 *
 * BSONObj::operator= -

src/lib/mongoDriver/BSONObj.h

@@ -59,6 +59,7 @@ class BSONObj
   bool isEmpty(void) const;
   void toStringMap(std::map<std::string, std::string>* m);
   void toElementsVector(std::vector<BSONElement>* v);
+  bool equal(const BSONObj& bo);
   BSONObj& operator= (const BSONObj& rhs);
 
   // methods to be used only by mongoDriver/ code (with references to low-level driver code)

src/lib/parse/CompoundValueNode.cpp

@@ -33,6 +33,7 @@
 #include "common/JsonHelper.h"
 #include "common/macroSubstitute.h"
 #include "alarmMgr/alarmMgr.h"
+#include "mongoDriver/safeMongo.h"
 #include "parse/forbiddenChars.h"
 
 #include "orionTypes/OrionValueType.h"
@@ -522,6 +523,90 @@ std::string CompoundValueNode::check(const std::string& path)
 
 
 
+/* ****************************************************************************
+*
+* CompoundValueNode::equal
+*
+*/
+bool CompoundValueNode::equal(const orion::BSONElement& be)
+{
+  // Note objects cannot be declared inside switch block
+  std::vector<orion::BSONElement> ba;
+  orion::BSONObj bo;
+
+  switch (valueType)
+  {
+  case orion::ValueTypeString:
+    return (be.type() == orion::String) && (stringValue == be.String());
+
+  case orion::ValueTypeNumber:
+    // FIXME P2: according to regression tests, this seems to work with all number types (int32/int64/double)
+    // However, let's keep an eye on this in the case some day it fails...
+    return (be.type() == orion::NumberDouble && numberValue == be.Number());
+
+  case orion::ValueTypeBoolean:
+    return (be.type() == orion::Bool) && (boolValue == be.Bool());
+
+  case orion::ValueTypeNull:
+    return (be.type() == orion::jstNULL);
+
+  case orion::ValueTypeVector:
+    // nodeP must be a vector
+    if (be.type() != orion::Array)
+    {
+      return false;
+    }
+    ba = be.Array();
+    // nodeP must have the same number of elements
+    if (childV.size() != ba.size())
+    {
+      return false;
+    }
+    for (unsigned int ix = 0; ix < childV.size(); ix++)
+    {
+      if (!(childV[ix]->equal(ba[ix])))
+      {
+        return false;
+      }
+    }
+    return true;
+
+  case orion::ValueTypeObject:
+    // nodeP must be a object
+    if (be.type() != orion::Object)
+    {
+      return false;
+    }
+    bo = be.embeddedObject();
+    if ((int) childV.size() != bo.nFields())
+    {
+      return false;
+    }
+    for (unsigned int ix = 0; ix < childV.size(); ix++)
+    {
+      if (!bo.hasField(childV[ix]->name))
+      {
+        return false;
+      }
+      if (!(childV[ix]->equal(getFieldF(bo, childV[ix]->name))))
+      {
+        return false;
+      }
+    }
+    return true;
+
+  case orion::ValueTypeNotGiven:
+    LM_E(("Runtime Error (value type not given (%s))", name.c_str()));
+    return false;
+
+  default:
+    LM_E(("Runtime Error (value type unknown (%s))", name.c_str()));
+    return false;
+  }
+}
+
+
+
 /* ****************************************************************************
 *
 * CompoundValueNode:toJson

src/lib/parse/CompoundValueNode.h

@@ -33,6 +33,8 @@
 
 #include "orionTypes/OrionValueType.h"
 
+#include "mongoDriver/BSONElement.h"
+
 
 namespace orion
 {
@@ -112,6 +114,7 @@ class CompoundValueNode
   CompoundValueNode*  add(const orion::ValueType _type, const std::string& _name, double _value);
   CompoundValueNode*  add(const orion::ValueType _type, const std::string& _name, bool _value);
   std::string         check(const std::string& path);
+  bool                equal(const orion::BSONElement& be);
   std::string         finish(void);
 
   std::string         toJson(std::map<std::string, std::string>* replacementsP = NULL);