Fix: actionType metadata: delete support #2591 #4186
Conversation
| @@ -1832,7 +1832,7 @@ static bool processOnChangeConditionForUpdateContext | |||
| { | |||
| for (unsigned int jx = 0; jx < attrL.size(); jx++) | |||
| { | |||
| if (caP->name == attrL[jx] && !caP->skip) | |||
| if (caP->name == attrL[jx]) | |||
There was a problem hiding this comment.
Why did you need to remove the && !caP->skip part? Could you elaborate on it, please?
There was a problem hiding this comment.
Why did you need to remove the
&& !caP->skippart? Could you elaborate on it, please?
As per my understanding this caP->skip field is used to mark deleted attributes that must not be included in the notification and this condition is used to add the attributes in the notification. So, I removed !caP->skip so that the deleted attributes included in the notification.
There was a problem hiding this comment.
Regardless of this new functionality, my doubt is if && !caP->skip is really needed. Maybe it was needed in the past but some refactor in the code make it unneeded.
I'll cast and independent PR to check that.
There was a problem hiding this comment.
I'll cast and independent PR to check that.
This one: PR #4201
There was a problem hiding this comment.
PR #4201 went ok, thus validating this change.
However, I'll investigate a little bit, to understand why this was there but now it is not needed :)
There was a problem hiding this comment.
Looking into this into more detail (see #4201 (comment)) I'm afraid that the modification is not valid.
This PR doesn't detect the problem due to in the moment the PR was created master didn't have the proper coverage in tests. Now that these test has been included (in PR #4204), if you upgrade Anjali-NEC:issue2591 with master, we will be see some tests failing.
| "type": "Number", | ||
| "value": 2 |
There was a problem hiding this comment.
Currently, default behaviour is that deleted attribute doesn't appear in notifications (see delete_attribute_in_empty_condition.test and delete_attribute_in_condition.test test cases).
It could make sense to include deleted attributes in notifications when actionType is used (otherwise the delete case for actionType metadata is pointless). However, in this case, which should be used as type and value here?
- The previous ones (as this PR currently shows)
- Omit the
typeandvaluefields (but it should break the structure of attributes, asvalueis always assumed - Use null value, i.e.
"type": "None"and"value": null
I think the most semantically correct is number 3.
Another interesting case should be using actionType and previousValue in combination. In this case, previousValue should show the value previous to the deletion. Something like this:
"B": {
"metadata": {
"actionType": {
"type": "Text",
"value": "delete"
},
"previousValue": {
"type": "Number",
"value": 2
}
},
"type": "None",
"value": null
| "url": "http://localhost:'$LISTENER_PORT'/notify" | ||
| }, | ||
| "attrs": [ "A", "B", "C" ], | ||
| "metadata": [ "previousValue", "actionType" ] |
There was a problem hiding this comment.
I'd suggest two separate variants of this test:
- One with
"metadata": [ "previousValue", "actionType" ](this one) - One with
"metadata": [ "previousValue" ]
|
@Anjali-NEC could you tell us the status on this PR? Did you get my last feedback (around two weeks ago)? It would be great to know it, given in the next FIWARE TSC Orion is going to be discussed. |
I am putting this PR on hold currently I am busy in working other issue. |
@fgalan Please let us know which ticket has high priority to you in below PR: |
|
After the merging of PR #4332 this PR needs to be upgrades with |
Fix issue #2591