Merge pull request #1375 from FIWARE/issue/1374

Fixed issues #1368 and #1374

CHANGES_NEXT_RELEASE

@@ -1,4 +1,5 @@
 Fixed issues:
-#1368 - bug in subscription patch
-#280  - error handling in registration creation - making sure exclusive registrations specify entity id and attributes
-#280  - error handling in registration updated  - making sure exclusive registrations specify entity id and attributes
+#1368 - bug in subscription patch regarding modification of cached fields 'description', 'q', and 'entities'
+#1374 - similar to #1368
+#280  - error handling in registration creation - making sure exclusive registrations specify mandatory fields entity-id and attributes
+#280  - error handling in registration updates  - making sure exclusive registrations specify mandatory fields entity-id and attributes

src/lib/cache/subCache.cpp

@@ -591,6 +591,24 @@ void subCacheItemDestroy(CachedSubscription* cSubP)
     cSubP->url = NULL;
   }
 
+  if (cSubP->protocolString != NULL)
+  {
+    free(cSubP->protocolString);
+    cSubP->protocolString = NULL;
+  }
+
+  if (cSubP->ip != NULL)
+  {
+    free(cSubP->ip);
+    cSubP->ip = NULL;
+  }
+
+  if (cSubP->rest != NULL)
+  {
+    free(cSubP->rest);
+    cSubP->rest = NULL;
+  }
+
   if (cSubP->tenant != NULL)
   {
     free(cSubP->tenant);
@@ -863,6 +881,10 @@ bool subCacheItemInsert
   cSubP->next                  = NULL;
   cSubP->count                 = 0;
   cSubP->status                = status;
+  cSubP->url                   = NULL;
+  cSubP->ip                    = NULL;
+  cSubP->protocolString        = NULL;
+  cSubP->rest                  = NULL;
 
   if ((cSubP->expirationTime > 0) && (cSubP->expirationTime < orionldState.requestTime))
   {
@@ -968,7 +990,18 @@ bool subCacheItemInsert
   //
   cSubP->url = strdup(httpInfo.url.c_str());
   urlParse(cSubP->url, &cSubP->protocolString, &cSubP->ip, &cSubP->port, &cSubP->rest);
-  cSubP->protocol = protocolFromString(cSubP->protocolString);
+
+  if (cSubP->protocolString != NULL)
+  {
+    cSubP->protocol       = protocolFromString(cSubP->protocolString);
+    cSubP->protocolString = strdup(cSubP->protocolString);
+  }
+
+  if (cSubP->ip != NULL)
+    cSubP->ip = strdup(cSubP->ip);
+
+  if (cSubP->rest != NULL)
+    cSubP->rest = strdup(cSubP->rest);
 
   //
   // String filters
@@ -1163,9 +1196,9 @@ int subCacheItemRemove(CachedSubscription* cSubP)
 #if 0
 // -----------------------------------------------------------------------------
 //
-// debugSubCache -
+// subCacheDebug -
 //
-void debugSubCache(const char* prefix, const char* title)
+void subCacheDebug(const char* prefix, const char* title)
 {
   CachedSubscription* subP = subCache.head;
 
@@ -1185,6 +1218,7 @@ void debugSubCache(const char* prefix, const char* title)
 #endif
 
 
+
 /* ****************************************************************************
 *
 * subCacheRefresh -
@@ -1196,7 +1230,7 @@ void debugSubCache(const char* prefix, const char* title)
 */
 void subCacheRefresh(void)
 {
-  // debugSubCache("KZ", "------------- BEFORE REFRESH ------------------------");
+  // subCacheDebug("KZ", "------------- BEFORE REFRESH ------------------------");
 
   // Empty the cache
   subCacheDestroy();
@@ -1221,7 +1255,7 @@ void subCacheRefresh(void)
 
   ++subCache.noOfRefreshes;
 
-  // debugSubCache("KZ", "------------- AFTER REFRESH ------------------------");
+  // subCacheDebug("KZ", "------------- AFTER REFRESH ------------------------");
 }
 
 

src/lib/mongoBackend/mongoSubCache.cpp

@@ -303,7 +303,18 @@ int mongoSubCacheItemInsert(const char* tenant, const BSONObj& sub)
   // IP, port and rest
   cSubP->url = strdup(cSubP->httpInfo.url.c_str());
   urlParse(cSubP->url, &cSubP->protocolString, &cSubP->ip, &cSubP->port, &cSubP->rest);
-  cSubP->protocol = protocolFromString(cSubP->protocolString);
+
+  if (cSubP->protocolString != NULL)
+  {
+    cSubP->protocol = protocolFromString(cSubP->protocolString);
+    cSubP->protocolString = strdup(cSubP->protocolString);
+  }
+
+  if (cSubP->rest != NULL)
+    cSubP->rest = strdup(cSubP->rest);
+
+  if (cSubP->ip != NULL)
+    cSubP->ip = strdup(cSubP->ip);
 
   // q
   cSubP->qText = sub.hasField("ldQ")? strdup(getStringFieldF(&sub, "ldQ")) : NULL;

src/lib/orionld/common/subCacheApiSubscriptionInsert.cpp

@@ -120,7 +120,11 @@ CachedSubscription* subCacheApiSubscriptionInsert(KjNode* apiSubscriptionP, QNod
     cSubP->lastFailure = lastFailureP->value.f;
 
   if (descriptionP != NULL)
+  {
+    if (cSubP->description != NULL)
+      free(cSubP->description);
     cSubP->description = strdup(descriptionP->value.s);
+  }
 
   if (qP != NULL)
     cSubP->expression.q = qP->value.s;
@@ -129,7 +133,11 @@ CachedSubscription* subCacheApiSubscriptionInsert(KjNode* apiSubscriptionP, QNod
     cSubP->expression.mq = mqP->value.s;
 
   if (ldqP != NULL)
+  {
+    if (cSubP->qText != NULL)
+      free(cSubP->qText);
     cSubP->qText = strdup(ldqP->value.s);
+  }
 
   if (isActiveP != NULL)
   {
@@ -273,6 +281,9 @@ CachedSubscription* subCacheApiSubscriptionInsert(KjNode* apiSubscriptionP, QNod
             cSubP->ip,
             cSubP->port,
             cSubP->rest));
+        cSubP->protocolString = strdup(cSubP->protocolString);
+        cSubP->ip             = strdup(cSubP->ip);
+        cSubP->rest           = strdup(cSubP->rest);
       }
 
       if (cSubP->protocol == MQTT)

src/lib/orionld/dbModel/dbModelFromApiSubscription.cpp

@@ -247,7 +247,7 @@ bool dbModelFromApiSubscription(KjNode* apiSubscriptionP, bool patch)
     {
       fragmentP->name    = (char*) "expiration";
       fragmentP->type    = KjFloat;
-      fragmentP->value.f = parse8601Time(fragmentP->value.s);  // FIXME: Already done in pcheckSubscription() ...
+      fragmentP->value.f = parse8601Time(fragmentP->value.s);
     }
     else if (strcmp(fragmentP->name, "throttling") == 0)
       throttlingP = fragmentP;

src/lib/orionld/dbModel/dbModelToApiSubscription.cpp

@@ -350,8 +350,10 @@ KjNode* dbModelToApiSubscription(KjNode* dbSubP, const char* tenant, bool forSub
     KjNode* v2qP  = kjLookup(dbExpressionP, "q");
     KjNode* v2mqP = kjLookup(dbExpressionP, "mq");
 
-    kjChildRemove(dbExpressionP, v2qP);
-    kjChildRemove(dbExpressionP, v2mqP);
+    if (v2qP)
+      kjChildRemove(dbExpressionP, v2qP);
+    if (v2mqP)
+      kjChildRemove(dbExpressionP, v2mqP);
 
     if (orionldState.apiVersion != NGSI_LD_V1)  // FIXME: When taking from DB at startup, this won't work ...
     {

src/lib/orionld/mongoc/mongocSubscriptionLookup.cpp

@@ -34,6 +34,7 @@ extern "C"
 #include "orionld/common/orionldState.h"                         // orionldState
 #include "orionld/mongoc/mongocConnectionGet.h"                  // mongocConnectionGet
 #include "orionld/mongoc/mongocKjTreeFromBson.h"                 // mongocKjTreeFromBson
+#include "orionld/mongoc/mongocWriteLog.h"                       // MONGOC_WLOG - FIXME: change name to mongocLog.h
 #include "orionld/mongoc/mongocSubscriptionLookup.h"             // Own interface
 
 
@@ -67,6 +68,7 @@ KjNode* mongocSubscriptionLookup(const char* subscriptionId)
   // Run the query
   //
   // semTake(&mongoSubscriptionsSem);
+  MONGOC_RLOG("Lookup Subscription", orionldState.tenantP->mongoDbName, "subscriptions", &mongoFilter, LmtMongoc);
   if ((mongoCursorP = mongoc_collection_find_with_opts(orionldState.mongoc.subscriptionsP, &mongoFilter, NULL, NULL)) == NULL)
   {
     LM_E(("Internal Error (mongoc_collection_find_with_opts ERROR)"));

src/lib/orionld/mongoc/mongocSubscriptionReplace.cpp

@@ -64,11 +64,6 @@ bool mongocSubscriptionReplace(const char* subscriptionId, KjNode* dbSubscriptio
   LM_T(LmtMongoc, ("Creating the _id selector for subscription id '%s'", subscriptionId));
   bson_append_utf8(&selector, "_id", 3, subscriptionId, -1);
 
-  // The "_id" field cannot be present in the payload - mongodb complains if it is
-  KjNode* _idP = kjLookup(dbSubscriptionP, "_id");
-  if (_idP != NULL)
-    kjChildRemove(dbSubscriptionP, _idP);
-
   mongocKjTreeToBson(dbSubscriptionP, &replacement);
 
   MONGOC_WLOG("Updating Subscription", orionldState.tenantP->mongoDbName, "subscriptions", &selector, &replacement, LmtMongoc);

src/lib/orionld/mongoc/mongocWriteLog.cpp

@@ -69,14 +69,18 @@ void mongocWriteLog
   if (selectorP != NULL)
   {
     char* selector = bson_as_json(selectorP, NULL);
+
     snprintf(line, sizeof(line), "  * Selector:              '%s'", selector);
     lmOut(line, 'T', fileNameOnly, lineNo, functionName, traceLevel, NULL);
     bson_free(selector);
   }
 
-  char* request = bson_as_json(requestP, NULL);
+  if (requestP != NULL)
+  {
+    char* request = bson_as_json(requestP, NULL);
 
-  snprintf(line, sizeof(line), "  * Request:               '%s'", request);
-  lmOut(line,     'T', fileNameOnly, lineNo, functionName, traceLevel, NULL);
-  bson_free(request);
+    snprintf(line, sizeof(line), "  * Request:               '%s'", request);
+    lmOut(line,     'T', fileNameOnly, lineNo, functionName, traceLevel, NULL);
+    bson_free(request);
+  }
 }

src/lib/orionld/mongoc/mongocWriteLog.h

@@ -44,6 +44,19 @@ do
 
 
 
+// -----------------------------------------------------------------------------
+//
+// MONGOC_RLOG -
+//
+#define MONGOC_RLOG(msg, dbName, collectionName, selectorP, traceLevel)                                             \
+do                                                                                                                  \
+{                                                                                                                   \
+  if (LM_MASK(LogLevelDebug) && lmOk('T', traceLevel) == LmsOk)                                                     \
+    mongocWriteLog(msg, dbName, collectionName, selectorP, NULL, __FILE__, __LINE__, __FUNCTION__, traceLevel);     \
+} while (0)
+
+
+
 // -----------------------------------------------------------------------------
 //
 // mongocWriteLog -

src/lib/orionld/payloadCheck/CMakeLists.txt

@@ -61,7 +61,6 @@ SET (SOURCES
      pcheckQuery.cpp
      pcheckReceiverInfo.cpp
      pcheckRegistration.cpp
-     pcheckSubscription.cpp
      pcheckTimeInterval.cpp
      pCheckQuery.cpp
      pCheckRegistrationMode.cpp

src/lib/orionld/payloadCheck/pCheckSubscription.cpp

@@ -97,12 +97,14 @@ extern "C"
 bool pCheckSubscription
 (
   KjNode*   subP,
+  bool      isCreate,          // true if POST, false if PATCH
+  char*     subscriptionId,    // non-NULL if PATCH
   KjNode*   idP,
   KjNode*   typeP,
   KjNode**  endpointP,
   KjNode**  qNodeP,
   QNode**   qTreeP,
-  char**    qTextP,
+  char**    qRenderedForDbP,
   bool*     qValidForV2P,
   bool*     qIsMqP,
   KjNode**  uriPP,
@@ -130,13 +132,23 @@ bool pCheckSubscription
   {
     PCHECK_STRING(idP, 0, NULL, SubscriptionIdPath, 400);
     PCHECK_URI(idP->value.s, true, 0, NULL, SubscriptionIdPath, 400);
+
+    if (subscriptionId != NULL)
+    {
+      if (strcmp(idP->value.s, subscriptionId) != 0)
+      {
+        orionldError(OrionldBadRequestData, "The Subscription ID cannot be modified", "id", 400);
+        return false;
+      }
+    }
   }
-  else if (typeP != NULL)
+
+  if (typeP != NULL)
   {
     PCHECK_STRING(typeP, 0, NULL, SubscriptionTypePath, 400);
     if (strcmp(typeP->value.s, "Subscription") != 0)
     {
-      orionldError(OrionldBadRequestData, "Invalid value for Subscription TYPE", typeP->value.s, 400);
+      orionldError(OrionldBadRequestData, "Invalid value for Subscription Type", typeP->value.s, 400);
       return false;
     }
   }
@@ -155,8 +167,8 @@ bool pCheckSubscription
     }
     else if (strcmp(subItemP->name, "description") == 0)
     {
-      PCHECK_STRING(subItemP, 0, NULL, SubscriptionNamePath, 400);
-      PCHECK_DUPLICATE(descriptionP,  subItemP, 0, NULL, SubscriptionNamePath, 400);
+      PCHECK_STRING(subItemP, 0, NULL, SubscriptionDescriptionPath, 400);
+      PCHECK_DUPLICATE(descriptionP,  subItemP, 0, NULL, SubscriptionDescriptionPath, 400);
     }
     else if (strcmp(subItemP->name, "entities") == 0)
     {
@@ -187,7 +199,7 @@ bool pCheckSubscription
       PCHECK_DUPLICATE(qP, subItemP, 0, NULL, SubscriptionQPath, 400);
       PCHECK_STRING(qP, 0, NULL, SubscriptionQPath, 400);
 
-      *qTreeP = qBuild(qP->value.s, qTextP, qValidForV2P, qIsMqP, true);  // 5th parameter: qToDbModel == true
+      *qTreeP = qBuild(qP->value.s, qRenderedForDbP, qValidForV2P, qIsMqP, true);  // 5th parameter: qToDbModel == true
       *qNodeP = qP;
 
       if (*qTreeP == NULL)
@@ -218,7 +230,7 @@ bool pCheckSubscription
     {
       PCHECK_OBJECT(subItemP, 0, NULL, SubscriptionNotificationPath, 400);
       PCHECK_DUPLICATE(notificationP,  subItemP, 0, NULL, SubscriptionNotificationPath, 400);
-      if (pCheckNotification(notificationP, false, uriPP, notifierInfoPP, mqttChangeP) == false)
+      if (pCheckNotification(notificationP, isCreate == false, uriPP, notifierInfoPP, mqttChangeP) == false)
         return false;
     }
     else if ((strcmp(subItemP->name, "expiresAt") == 0) || (strcmp(subItemP->name, "expires") == 0))
@@ -231,6 +243,20 @@ bool pCheckSubscription
     {
       PCHECK_DUPLICATE(throttlingP, subItemP, 0, NULL, SubscriptionThrottlingPath, 400);
       PCHECK_NUMBER(throttlingP, 0, NULL, SubscriptionThrottlingPath, 400);
+
+      //
+      // Can't be negative
+      //
+      if ((throttlingP->type == KjInt) && (throttlingP->value.i < 0))
+      {
+        orionldError(OrionldBadRequestData, "Negative Number not allowed in this position", SubscriptionThrottlingPath, 400);
+        return false;
+      }
+      else if ((throttlingP->type == KjFloat) && (throttlingP->value.f < 0))
+      {
+        orionldError(OrionldBadRequestData, "Negative Number not allowed in this position", SubscriptionThrottlingPath, 400);
+        return false;
+      }
     }
     else if (strcmp(subItemP->name, "lang") == 0)
     {
@@ -260,21 +286,24 @@ bool pCheckSubscription
   }
 
   // Make sure all mandatory fields are present
-  if (typeP == NULL)
-  {
-    orionldError(OrionldBadRequestData, "Mandatory field missing", SubscriptionTypePath, 400);
-    return false;
-  }
-  else if (notificationP == NULL)
+  if (isCreate == true)
   {
-    orionldError(OrionldBadRequestData, "Mandatory field missing", SubscriptionNotificationPath, 400);
-    return false;
-  }
+    if (typeP == NULL)
+    {
+      orionldError(OrionldBadRequestData, "Mandatory field missing", SubscriptionTypePath, 400);
+      return false;
+    }
+    else if (notificationP == NULL)
+    {
+      orionldError(OrionldBadRequestData, "Mandatory field missing", SubscriptionNotificationPath, 400);
+      return false;
+    }
 
-  if ((entitiesP == NULL) && (watchedAttributesP == NULL))
-  {
-    orionldError(OrionldBadRequestData, "Mandatory field missing", "At least one of 'entities' and 'watchedAttributes' must be present" , 400);
-    return false;
+    if ((entitiesP == NULL) && (watchedAttributesP == NULL))
+    {
+      orionldError(OrionldBadRequestData, "Mandatory field missing", "At least one of 'entities' and 'watchedAttributes' must be present" , 400);
+      return false;
+    }
   }
 
   return true;

src/lib/orionld/payloadCheck/pCheckSubscription.h

@@ -39,6 +39,8 @@ extern "C"
 extern bool pCheckSubscription
 (
   KjNode*   subP,
+  bool      isCreate,          // true if POST, false if PATCH
+  char*     subscriptionId,    // non-NULL if PATCH
   KjNode*   idP,
   KjNode*   typeP,
   KjNode**  endpointP,