Unify ContextElement class into Entity class #3301

fgalan · 2018-09-21T11:51:14Z

This PR is related with issue #1298, going another step forward in render logic unification and simplification work.

The main purpose of this PR is to unify ContextElement and Entity classes (WI1). There was a lot of duplicated functionality on them. ContextElement class (the old NGSIv1 class) has been removed and now Entity (born in the times of NGSIv2) is used in all cases. Some pieces of old logic of ContextElement are still needed and have been moved to Entity.

(WI = Work Item)

In the refactor process, some other modification were needed:

WI2: Change the filter-modifying-attributes-vector-during-render strategy (old Entity::filterAttributes() function) for attributes and metadata introduced by PR NGSIv2 rendering improvements (simplification, unification and performance) #3285. The aim was to simplify render parametrization but at the end it was a bad idea that actually added a bug. The problem occurred during subscription processing on entity update/creation, as the ContextElementResponse used is shared among all the notifications to send. Thus, if the Entity::filterAttributes() method removed/changed some attribute/metadata during the 1st notification, that change could affect to the next notifications. In other words, the filter used in one subscription was not isolated and may affect other subscriptions triggered by the same entity update/creation. Logic has been simplified (old Entity::filterAttributes() was pretty complex, especially regarding dynamic memory management), now implemented with the help of Entity::filterAndOrderAttrs() and filterAndOrderMetadata() methods. Test cases have been added to cases/3285 to raise and fix the problem.
- Note the fix for this bug is not mentioned in CNR as it was introduced during the current devel cycle (1.15.0-next).
WI3: As part of the filtering refactor due to the former, now the approach is to add all the potential builtins attributes and metadata at entity composition time (either for response in a GET operation or for being notified during an entity update or initial notification in subscription create/update). This simplifies the filtering logic at render stage as we can just remove attributes/metadata we do not want. Previously, we have an awful mix of attribute/metadata removal and builtin attribute/metadata addition that was complex to understand and change. Builtin attrs/metadata addition is done in NGSIv2 (always) and in NGSIv1 notifications (as we cannot distinguish between subscriptions created using NGSIv2 with legacy format -where builtin must be supported- and subscriptions created using NGSIv1 -where buitin doesn’t need to be supported).
WI4: Because of the former, some of the NGSIv1 renders (in particular, the set involved in notification rendering) have need a modification to include the attributes/metadata filters (and blacklist bool) as parameters. A change that extends for all the renders invocation hierarchy, but needed in order to keep backward compatibility.
WI5: Another consequence of the former is that when a user-defined attribute/metadata has same name than a built-in one (not recommended, but it may happen) we can have a more consistent behavior, the same for all cases (before that, we only pay attention to dateCreated/dateModified the behaviour of the rest was undefined). So the user-defined attribute shadows the built-in always, which is the desired behavior to keep backward compatibility (for users that have been using these names before the built-in were invented). Documentation regarding this has been improved in the associated .md and some .test have been fixed.
WI6: During the work on this PR I found that CB was generating JSON output with duplicated keys. I have upgraded accumulator-server.py and the functional test engine in order to raise error in this case (mjson.tool is not able to do that, so I implemented testJson.py) and fixed the problems to make make ft happy again.
WI7: Some changes in functions signature, to remove unused parameters (many renders still use apiVersion when they really don’t need it) and fix style (some conts& and pointers adjustment or in/out data)

Appart from the new case/3285 files, some changes were done in .test files. Please find justifications in inline comments in the files.

In order to make the review easier, I have split the PR in two commits:

A first one with the code, doc and functional test changes (around +2,700 -1,900 lines)
A second one with unit test adjusts (around +3,800 -3,280 lines)

Maybe per-review commit is better this case. Thus, reviewer can focus on the first one and mostly ignore the second.

…h several derived changes

fgalan · 2018-09-21T12:01:57Z

test/functionalTest/cases/0000_log_operation/log_rest.test

@@ -47,7 +47,7 @@ echo


 echo "=== 4. POST trace level"
-orionCurl --url "/log/traceLevel/101-102,105" -X "POST" --include 2> /dev/null
+orionCurl --url "/log/traceLevel/101-102,105" -X "POST" 2> /dev/null


I had problems with this file when I introduced testJson.py checker... not sure of the cause, but removing the --include curl parameter (as in other orionCurl statement in this same .test) solved it.

fgalan · 2018-09-21T12:02:49Z

test/functionalTest/cases/0876_attribute_dates/attribute_dates.test

@@ -183,7 +183,7 @@ Date: REGEX(.*)
 02. GET E1/A1, see dateCreated == dateModified
 ==============================================
 HTTP/1.1 200 OK
-Content-Length: 176


The change in the lenght is due this test previously include duplicated keys, now removed. It seems that mjson.tool doesn't detect that and print one of the keys (I think the last one it gets in the input) in the pretty printed output.

fgalan · 2018-09-21T12:03:09Z

test/functionalTest/cases/0876_attribute_dates/get_attr_dates_with_metadata.test

@@ -891,7 +891,7 @@ Date: REGEX(.*)
 05a. GET /v2/entities/E1/attrs/A?metadata=dateCreated
 =====================================================
 HTTP/1.1 200 OK


The change in the lenght is due this test previously include duplicated keys, now removed. It seems that mjson.tool doesn't detect that and print one of the keys (I think the last one it gets in the input) in the pretty printed output.

fgalan · 2018-09-21T12:03:17Z

test/functionalTest/cases/0876_attribute_dates/notify_attr_dates_with_metadata.test

@@ -546,7 +546,7 @@ Date: REGEX(.*)
 =======================================================================
 POST http://localhost:REGEX(\d+)/notify
 Fiware-Servicepath: /
-Content-Length: 260


The change in the lenght is due this test previously include duplicated keys, now removed. It seems that mjson.tool doesn't detect that and print one of the keys (I think the last one it gets in the input) in the pretty printed output.

fgalan · 2018-09-21T12:04:01Z

test/functionalTest/cases/2507_csub_metadata_field/metadata_filter.test

@@ -458,7 +458,7 @@ Fiware-Correlator: REGEX([0-9a-f\-]{36})
 =======================================
 POST http://localhost:REGEX(\d+)/notify
 Fiware-Servicepath: /
-Content-Length: 806


The change in the lenght is due this test previously include duplicated keys, now removed. It seems that mjson.tool doesn't detect that and print one of the keys (I think the last one it gets in the input) in the pretty printed output.

fgalan · 2018-09-21T12:05:01Z

.../functionalTest/cases/2507_csub_metadata_field/special_metadata_in_notifications_ngsiv1.test

@@ -97,7 +97,7 @@ payload='{
    },
    "attrs": [ "A", "B", "C" ],
    "attrsFormat": "legacy",
-    "metadata": [ "previousValue", "actionType" ]


With the changes in the NGSIv1 renders (WI4) now the order of the attributes in metadata matters also for NGSIv1. I see this as a kind of improvement.

fgalan · 2018-09-21T12:06:51Z

test/functionalTest/cases/0876_attribute_dates/attrs_dates_overridden_by_user.test

@@ -34,12 +34,12 @@ brokerStart CB
 # NGSIv2 implementation notes). Maybe in the future could change.
 #
 # 01. Create entity with user provided dateCreated and dateModified metadata in a far away future
-# 02. GET /v2/entities/E1 and don't get dateModified or dateCreated metadata [should get user defined dates]


Diff is pretty noisy in this file. I'd suggest to have a look to the final file and ensure it correct regarding what is told in section "User attributes or metadata matching builtin name" of ngsiv2_implementation_notes.md file.

fgalan · 2018-09-21T12:07:00Z

test/functionalTest/cases/0876_attribute_dates/attrs_dates_overridden_by_user_subs.test

@@ -33,26 +33,26 @@ accumulatorStart --pretty-print
 #
 # (Test focuses on dateModified, but dateCreated is expected to work in a similar way)
 #
-# E1 notifications in steps 1 to 9 should not happen (they have been marked with [*]), as the


Diff is pretty noisy in this file. I'd suggest to have a look to the final file and ensure it correct regarding what is told in section "User attributes or metadata matching builtin name" of ngsiv2_implementation_notes.md file.

fgalan · 2018-09-21T12:07:17Z

test/functionalTest/cases/0876_entity_dates/entity_dates_overridden_by_user.test

@@ -38,8 +38,8 @@ brokerStart CB
 # 03. GET /v2/entities/E1?attrs=dateModified,dateCreated and get user defined dates
 # 04. GET /v2/entities?q=dateCreated>2049-01-01 and get nothing [should be E1]
 # 05. GET /v2/entities?q=dateModified>2049-01-01 and get nothing [should be E1]
-# 06. GET /v2/entities?q=dateCreated<2049-01-01 and get E1 [should get nothing]


Diff is pretty noisy in this file. I'd suggest to have a look to the final file and ensure it correct regarding what is told in section "User attributes or metadata matching builtin name" of ngsiv2_implementation_notes.md file.

fgalan · 2018-09-21T12:07:24Z

test/functionalTest/cases/0876_entity_dates/entity_dates_overridden_by_user_subs.test

@@ -33,27 +33,27 @@ accumulatorStart --pretty-print
 #
 # (Test focuses on dateModified, but dateCreated is expected to work in a similar way)
 #
-# E1 notifications in steps 1 to 9 should not happen (they has been marked with [*]), as the


Diff is pretty noisy in this file. I'd suggest to have a look to the final file and ensure it correct regarding what is told in section "User attributes or metadata matching builtin name" of ngsiv2_implementation_notes.md file.

fgalan · 2018-09-21T12:08:17Z

CC: @fisuda in the case you want to sync the changes in doc in this PR in the Japanese translation.

The PR is large :) but the documentation part is small. Just to have a look to the couple of .md files modified in this PR.

fisuda · 2018-09-21T12:41:51Z

Hi @fgalan
Noted with thanks.

fgalan · 2018-09-21T19:43:36Z

Summary of the valgrind pass on this branch:

11 functional tests failed (not a leak, just a func-test failure):
   015: get_entity_dates_with_options.test (exit code 1)
   025: https.test (exit code 1)
   301: direct_https_notifications_no_accept_selfsigned.test (exit code 1)
   335: query_local_query_not_found.test (exit code 1)
   383: attribute_dates.test (exit code 1)
   384: attrs_dates_overridden_by_user_subs.test (exit code 1)
   386: filter_attr_datecreated_sub.test (exit code 1)
   393: filter_datecreated_sub.test (exit code 1)
   558: mq_as_uri_param_for_metadata.test (exit code 1)
   633: subCacheThrottlingAndExpiration.test (exit code 1)
   824: 2064_attrs_blacklist_complex.test (exit code 1)
---------------------------------------

Total test time: 16252.19 seconds (270 minutes)
Great, all valgrind tests ran without any memory leakage

fisuda · 2018-09-23T08:40:54Z

There are four typos.

in ngsiv2_implementation_notes.md
- Firs of all -> First
- In fact, the NGSIv2 specifiation forbids -> specification
- Howerver, if you are forced to have -> However
in transient_entities.md
- and subscritions -> subscriptions

fgalan · 2018-09-24T17:18:23Z

@fisuda thanks for the feeback! Typo should have been fixed in new commit 87d9870. Please have a look and provide LGTM to the PR if you find it aceptable. Thx!

fisuda · 2018-09-24T21:39:11Z

Thanks for fixing the typos.
LGTM

kzangeli

Mostly typos and stuff. A few suggestions

kzangeli · 2018-09-26T17:04:14Z

doc/manuals/user/ngsiv2_implementation_notes.md

+(The content of this section applies to all builtins except `dateExpires` attribute. Check the document
+[on transient entities](transient_entities.md) for specific information about `dateExpires`).
+
+First of all: **you are strongly encouraged to not use attributes or metadata with same name of a NGSIv2


with same name of a NGSIv2 builtin one

First, an NGSI.

Then, either :

with the same name as an NGSIv2 builtin

OR

with the name of an NGSIv2 builtin

Fixed in 58d9889

kzangeli · 2018-09-26T17:09:37Z

src/lib/apiTypesV2/Entity.cpp

-* dateCreatedOption and dateModifiedOption are due to deprecated ways of requesting
-* date in response. If used, the date is added at the end.
+*/
+void Entity::addAllExceptShadowed(std::vector<ContextAttribute*>*  orderedAttrs)


Perhaps add 'Attribute' to the method name. "All" doesn't say much:

Entity::addAllAttributesExceptShadowed or
Entity::addAllButShadowedAttributes

Fixed in 58d9889

kzangeli · 2018-09-26T17:10:51Z

src/lib/apiTypesV2/Entity.cpp

-
-      for (unsigned int ix = 0; ix < attrsFilter.size(); ix++)
+      // Filter, blacklist. The order is the one in the entity, after removing attributes.
+      // In blacklis case shadowed attributes (dateCreated, etc) are never included


Typo: blacklist

Fixed in 58d9889

kzangeli · 2018-09-26T17:12:02Z

src/lib/apiTypesV2/Entity.cpp

+    }
+    else
+    {
+      // Filter, no blacklist. Processing will depend either '*' is in the attrsFilter or not


depend either => depend on whether

Fixed in 58d9889

kzangeli · 2018-09-26T17:19:03Z

src/lib/convenience/RegisterProviderRequest.cpp


  out += startTag();
-  out += metadataVector.render(commaAfterMetadataVector);
+  // out += metadataVector.render(commaAfterMetadataVector); //FIXME #1298 remove context registration metadata


These comments left by mistake or pending something in issue #1298 ?

Something pending in issue #1298. To be removed in a future PR, coming soon.

NTC

kzangeli · 2018-09-26T17:45:00Z

src/lib/orionTypes/EntityType.cpp

  if (typeNameBefore && asJsonOut)
  {
    out += valueTag("name", type, true);
-    out += contextAttributeVector.render(asJsonObject, EntityTypes, true, true, true);
+    out += contextAttributeVector.render(asJsonObject, EntityTypes, contextAttributeVector.vec, emptyMdV, true, true, true);


Do you send a member (vec) of the contextAttributeVector to a method (render) of contextAttributeVector?
I guess you have a good reason ... Otherwise, doesn't make much sense.

That parameter is a std::vector<ContextAttribute*> that defines the attributes to be rendered and in which order. In this particular case (due to the lack of filter, I guess), the attributes to be rendered and their order much with the one in the vector itself. That's why contextAttributeVector.vec is used. It may seems a bit werid, but it's ok.

NTC

kzangeli · 2018-09-26T17:47:40Z

src/lib/serviceRoutines/getIndividualContextEntity.cpp

-  TIMED_RENDER(answer = response.render(asJsonObject, IndividualContextEntity));
+
+  // No attribute or metadata filter in this case, an empty vector is used to fulfil method signature
+  std::vector<std::string> emptyV;


Sooo many of these ...
Had you used a pointer instead, you could save all those creations of variables. Just send NULL to the function.

Just a suggestion

I though in that alternative... but style mandates to use const& for read-only parameters like this so NULL cannot be used.

There are alternatives to fix this (i.e. change style guidelines to take into account this kind of cases) or use a variant of the function will less parameters (which invokes in sequence the full-fledge version, so the "problem" is only in one place).

I have opened an issue for that: #3314

NCT

kzangeli · 2018-09-26T17:51:36Z

src/lib/serviceRoutines/postQueryContext.cpp

@@ -382,7 +382,7 @@ std::string postQueryContext
  for (unsigned int ix = 0 ; ix < qcrsP->contextElementResponseVector.size(); ++ix)
  {
    ContextElementResponse* cerP  = qcrsP->contextElementResponseVector[ix];
-    EntityId*               eP    = &cerP->contextElement.entityId;
+    EntityId en(cerP->entity.id, cerP->entity.type, cerP->entity.isPattern);


Not sure I understand this change ...
Instead of just pointing to the already existing entity, you create a new copy?
And only to send its address to functions later, as far as I can see.
Why not just point to the entity?

Ah wait, you might be changing type from Entity to EntityId ...

It may be related with some other changes in function signatures and so on, but it works (otherwise compilation or test would not pass).

NTC

kzangeli · 2018-09-26T17:52:49Z

src/lib/serviceRoutines/postQueryContext.cpp

@@ -431,16 +431,16 @@ std::string postQueryContext
          // If we find a suitable existing contextElementResponse, we put it there,
          // otherwise, we have to create a new contextElementResponse.
          //
-          ContextElementResponse* contextElementResponseP = localQcrsP->contextElementResponseVector.lookup(eP);
+          ContextElementResponse* contextElementResponseP = localQcrsP->contextElementResponseVector.lookup(&cerP->entity);


This is a little strange ...
All changes are from 'eP' to '&en', except this one ... here you replace eP with &cerP->entity ...
I have no clue, might be OK, it just looks a bit strange

lookup() signature has changed, from EntityId* (eP was an EntityId*) to Entity* (we are passing the memory address of cerP->entity). It's ok, code compiles and tests are passing.

NTC

kzangeli · 2018-09-26T17:56:48Z

src/lib/serviceRoutinesV2/serviceRoutinesCommon.h

+
+/* ****************************************************************************
+*
+* semRender -


semRender? :)

Fixed in 58d9889

fgalan · 2018-09-27T09:09:22Z

All the comment addressed and fixes included. Ready to merge.

If after reviewing the way comments have been addressed something extra needs to be done, it will be done in a next PR (this PR is somehow blocked two others, so we are doing this way this time).

AlvaroVega

LGTM

…class (JP) Unify ContextElement class into Entity class (#3301)

kzangeli · 2018-09-27T16:50:31Z

LGTM

fgalan mentioned this pull request Sep 21, 2018

JsonHelper usage in all NGSIv2 renders (second step: optimization and performance) #1298

Open

fgalan added 2 commits September 21, 2018 13:56

FIX ContextElement class removed in favour of Entity class, along wit…

d6acf7b

…h several derived changes

FIX utest aligned with latests changes

a5dcf0f

fgalan force-pushed the hardening/1298_unify_entity_clases branch from a4567d0 to a5dcf0f Compare September 21, 2018 11:57

fgalan commented Sep 21, 2018

View reviewed changes

FIX typos raised in review

87d9870

kzangeli suggested changes Sep 26, 2018

View reviewed changes

FIX issues raised during code review

58d9889

fgalan mentioned this pull request Sep 27, 2018

Empty vector as parameters (should be simplified) #3314

Open

AlvaroVega approved these changes Sep 27, 2018

View reviewed changes

AlvaroVega merged commit 0d65607 into master Sep 27, 2018

AlvaroVega deleted the hardening/1298_unify_entity_clases branch September 27, 2018 09:14

This was referenced Sep 27, 2018

Functional test for #3182 issue #3315

Merged

dateModified/dateCreated not included in initial notifications #3182

Closed

fgalan added a commit that referenced this pull request Sep 27, 2018

Merge pull request #3319 from fisuda/update/doc_unity_contextelement_…

3a5087b

…class (JP) Unify ContextElement class into Entity class (#3301)

Unify ContextElement class into Entity class #3301

Unify ContextElement class into Entity class #3301

Conversation

fgalan commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Sep 21, 2018

fisuda commented Sep 21, 2018

fgalan commented Sep 21, 2018

fisuda commented Sep 23, 2018

fgalan commented Sep 24, 2018

fisuda commented Sep 24, 2018

kzangeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Sep 27, 2018

AlvaroVega left a comment

Choose a reason for hiding this comment

kzangeli commented Sep 27, 2018