updateContext "replace" and geolocalization

[fgalan]

It is not clear if the implementation of the "replace" functionality at mongoBackend (done in PR #1132) will work right in the case of using location attributes. Four situations needs to be tested:

• Existing location attribute L1, replace including L1
• Existing location attribute L1, replace no including any location attribute
• Existing location attribute L1, replace including location attribute but different (L2)
• Not existing locadtion attribute, replace including location attribute

The right strategy to test this (either unit test for mongoBackend, end2end test harness or a combination of both) will be decided in the moment of addressing this issue.

The best moment to implement this issue is once the new geolocation in NGSIv2 gets implemented (i.e. "geo:point" stuff).

Effort: 3 man day

[arigliano]

Hello, I would like to work on this issue. Can you assign it to me? (due to possible relationship with #3167 )

[fgalan]

Done. Thanks!

[arigliano]

The problem is where building the final append query in case of replace action. As done for the expirationDate feature, the logic has to check if there is a new location, or if the old one must be deleted, and then append it either to set or unset BSONObjBuilders. In order to avoid code complication (too many if), in the updateEntity() function, a good approach could be using a BSONObjBuilder (replaceSet) instead BSON stream form, something like the following:

  if (action == ActionTypeReplace)
  {
    // toSet: { A1: { ... }, A2: { ... } }
    BSONObjBuilder replaceSet;
    int            now = getCurrentTime();

    // This avoids strange behavior like as for the location, as reported in the #1142 issue
    // In order to enable easy append management of fields (e.g. location, dateExpiration),
    // it could be better to use a BSONObjBuilder instead the BSON stream macro below.
//    if (dateExpirationInPayload)
//    {
//      updatedEntity.append("$set", BSON(ENT_ATTRS                   << toSetObj <<
//                                        ENT_ATTRNAMES               << toPushArr <<
//                                        ENT_MODIFICATION_DATE       << now <<
//                                        ENT_EXPIRATION              << currentDateExpiration <<
//                                        ENT_LAST_CORRELATOR         << fiwareCorrelator));
//    }
//    else
//    {
//      updatedEntity.append("$set", BSON(ENT_ATTRS                   << toSetObj <<
//                                        ENT_ATTRNAMES               << toPushArr <<
//                                        ENT_MODIFICATION_DATE       << now <<
//                                        ENT_LAST_CORRELATOR         << fiwareCorrelator));
//      updatedEntity.append("$unset", toUnsetObj);
//    }
//
//    notifyCerP->contextElement.entityId.modDate = now;

    replaceSet.append(ENT_ATTRS, toSetObj);
    replaceSet.append(ENT_ATTRNAMES, toPushArr);
    replaceSet.append(ENT_MODIFICATION_DATE, now);
    replaceSet.append(ENT_LAST_CORRELATOR, fiwareCorrelator);

    if (dateExpirationInPayload)
    {
      replaceSet.append(ENT_EXPIRATION, currentDateExpiration);
    }

    if (newGeoJson.nFields() > 0)
    {
      replaceSet.append(ENT_LOCATION, BSON(ENT_LOCATION_ATTRNAME << locAttr <<
                                    ENT_LOCATION_COORDS   << finalGeoJson));
    }

    updatedEntity.append("$set", replaceSet.obj());

    if (!dateExpirationInPayload || newGeoJson.nFields() == 0)
    {
      updatedEntity.append("$unset", toUnsetObj);
    }
  }

For instance, if (newGeoJson.nFields() > 0) checks indeed if there is a new location in the payload, then to be set.
I have to check all the mentioned 4 cases, it seems switching for and to location/no location with that modification works, but only if in the request there is a no location attribute (L1) with the same name of the old location one (L1). If instead we have a new no location attribute (L2) and there was a L1 location, the "location" root field of L1 in the DB is still there. Could be related to what is written in location.cpp (row 377)?

//
  // FIXME P5 https://github.com/telefonicaid/fiware-orion/issues/1142:
  // note that with the current logic, the name of the attribute meaning location
  // is preserved on a replace operation. By the moment, we can leave this as it is now
  // given that the logic in NGSIv2 for specifying location attributes is gogint to change
  // (the best moment to address this FIXME is probably once NGSIv1 has been deprecated and
  // removed from the code)
  //

As I understand, this case, instead:

• Existing location attribute L1, replace including location attribute but different (L2)
  In this replace action case, should it directly delete L1 and replace location field (both in location root field in the DB document and in its "attrs" array) with L2? That is, it should have a different behavior from Update case, where first you have to delete L1 and then update with L2?
  Currently it does not allow to do this directly, as it returns:

{
    "error": "NoResourcesAvailable",
    "description": "You cannot use more than one geo location attribute when creating an entity [see Orion user manual]"
}

[arigliano]

I've added unit test for 3 cases, except the following one:

• Existing location attribute L1, replace including location attribute but different (L2)
  It should be first discussed/explained, as I wrote in the last part of previous comment.

[fgalan]

Existing location attribute L1, replace including location attribute but different (L2)
In this replace action case, should it directly delete L1 and replace location field (both in location root field in the DB document and in its "attrs" array) with L2? That is, it should have a different behavior from Update case, where first you have to delete L1 and then update with L2?

I will try to ilustrate with an example. Let's assume we have the following situation (S) in DB:

 {
   "_id":
       "id": "E1",
       "type": "T1",
       "servicePath": "/"
   },
   "attrs": {
       "L1": {"value": "-2,4", "type": "geo:point", ...},
       "L1": {"value": "foo", "type": "Text", ...}
   },
   "attrNames": [ "L1", "L2" ],
   "location": {
       "attrName": "L1",
       "coords": {
           "type": "Point",
           "coordinates": [ 4, -2 ]
       }
   }
 }

Then a replace like this is proccessed:

PUT /v2/entities/E1/attrs
{
  "L1": { "value": "bar", "type": "Text" },
  "L2": { "value": "-3, 5", "type": "geo:point" }
}

The resulting situation in DB is:

 {
   "_id":
       "id": "E1",
       "type": "T1",
       "servicePath": "/"
   },
   "attrs": {
       "L1": {"value": "bar", "type": "Text", ...},
       "L2": {"value": "-3, 5", "type": "geo:point", ...}
   },
   "attrNames": [ "L1", "L2" ],
   "location": {
       "attrName": "L2",
       "coords": {
           "type": "Point",
           "coordinates": [ 5, -3 ]
       }
   }
 }

Another case, let's assume again situation S and the following replace request:

PUT /v2/entities/E1/attrs
{
  "L2": { "value": "-3, 5", "type": "geo:point" }
}

the situation in DB after processing it should be:

 {
   "_id":
       "id": "E1",
       "type": "T1",
       "servicePath": "/"
   },
   "attrs": {
       "L2": {"value": "-3, 5", "type": "geo:point", ...}
   },
   "attrNames": [ "L2" ],
   "location": {
       "attrName": "L2",
       "coords": {
           "type": "Point",
           "coordinates": [ 5, -3 ]
       }
   }
 }

Please tell me if this clarifies how it should behave.

From an implementation point of view, I guess that is a matter of adjusting toSet, toUnset, etc. arrays accordingly, but I haven't go too much into details.

[arigliano]

Ok, right. So only for the replace case we should allow replacing a target location attribute (e.g. L2) when there is already a different location attribute? (e.g. L1). Currently we have:

 //
    // Case 1b:
    //   currently we have a loation but the attribute holding it is different from the target attribute -> error
    //
    if (*currentLocAttrName != targetAttr->name)
    {
      *errDetail = "attempt to define a geo location attribute [" + targetAttr->name + "]" +
                   " when another one has been previously defined [" + *currentLocAttrName + "]";

      oe->fill(SccRequestEntityTooLarge,
               "You cannot use more than one geo location attribute when creating an entity [see Orion user manual]",
               "NoResourcesAvailable");

      return false;
    }

So, should this part be refined with a "if v2 replace" check, avoiding to return an error in that case?

[fgalan]

So, should this part be refined with a "if v2 replace" check, avoiding to return an error in that case?

Sounds good (although I haven't analyzed the problem in deep in order to know if there are better options ;)

[fgalan]

In fact, replace can also be used in v1 (POST /v1/updateContext with actionType: REPLACE). However, it is fine if the check is only working for NGSIv2 (NGSIv1 is about to be deprecated so fixing it is not a priority).

[arigliano]

Fixed in 40da980

[fgalan]

Fixed by PR #3283