Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: actionType metadata: delete support #2591 #4186

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES_NEXT_RELEASE
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
- Fix: actionType metadata: delete support (#2591)
- Add: json field in httpCustom and mqttCustom subscriptions (#2560)
- Add: ${service}, ${servicePath} and ${authToken} macros in custom notifications (#4159)
- Fix: conditions.alterationTypes not working properly when conditions.attributes is used in entityUpdate case (#1494, reopened)
Expand Down
3 changes: 2 additions & 1 deletion src/lib/mongoBackend/MongoCommonUpdate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to remove the && !caP->skip part? Could you elaborate on it, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you need to remove the && !caP->skip part? 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll cast and independent PR to check that.

This one: PR #4201

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
/* Note we use cloneCompound=true in the ContextAttribute constructor. This is due to
* cer.entity destructor does release() on the attrs vector */
Expand Down Expand Up @@ -2384,6 +2384,7 @@ static void deleteAttrInNotifyCer
if (caP->name == targetAttr->name)
{
caP->skip = true;
caP->actionType = NGSI_MD_ACTIONTYPE_DELETE;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
# Copyright 2022 Telefonica Investigacion y Desarrollo, S.A.U
#
# This file is part of Orion Context Broker.
#
# Orion Context Broker is free software: you can redistribute it and/or
# modify it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# Orion Context Broker is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero
# General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with Orion Context Broker. If not, see http://www.gnu.org/licenses/.
#
# For those usages not covered by this license please contact with
# iot_support at tid dot es

# VALGRIND_READY - to mark the test ready for valgrindTestSuite.sh

--NAME--
actionType_metadata_delete_support

--SHELL-INIT--
dbInit CB
brokerStart CB
accumulatorStart --pretty-print

--SHELL--

#
# 01. Create entity E1 with attributes A:1, B:2, C:3
# 02. Subscribe to E.* for A, B and C; triggered by B, C
# 03. Delete attribute B
# 04. Dump and reset: see notification with actionType=delete for B
#


echo "01. Create entity E1 with attributes A:1, B:2, C:3"
echo "=================================================="
payload='{
"type": "T",
"id": "E1",
"A": {
"type": "Number",
"value": 1
},
"B": {
"type": "Number",
"value": 2
},
"C": {
"type": "Number",
"value": 3
}
}'
orionCurl --url /v2/entities --payload "$payload"
echo
echo



echo "02. Subscribe to E.* for A, B and C; triggered by B, C"
echo "======================================================"
payload='{
"subject": {
"entities": [
{
"idPattern": "E.*",
"type": "T"
}
],
"condition": {
"attrs": [ "B", "C" ]
}
},
"notification": {
"http": {
"url": "http://localhost:'$LISTENER_PORT'/notify"
},
"attrs": [ "A", "B", "C" ],
"metadata": [ "previousValue", "actionType" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest two separate variants of this test:

  • One with "metadata": [ "previousValue", "actionType" ] (this one)
  • One with "metadata": [ "previousValue" ]

}
}'
orionCurl --url /v2/subscriptions --payload "$payload"
echo
echo

SUB_ID=$(echo "$_responseHeaders" | grep Location | awk -F/ '{ print $4 }' | tr -d "\r\n")



echo "03. Delete attribute B"
echo "======================"
orionCurl --url /v2/entities/E1/attrs/B -X DELETE
echo
echo



echo "04. Dump and reset: see notification with actionType=delete for B"
echo "================================================================="
accumulatorDump
accumulatorReset
echo
echo



--REGEXPECT--
01. Create entity E1 with attributes A:1, B:2, C:3
==================================================
HTTP/1.1 201 Created
Content-Length: 0
Location: /v2/entities/E1?type=T
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)



02. Subscribe to E.* for A, B and C; triggered by B, C
======================================================
HTTP/1.1 201 Created
Content-Length: 0
Location: /v2/subscriptions/REGEX([0-9a-f]{24})
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)



03. Delete attribute B
======================
HTTP/1.1 204 No Content
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)



04. Dump and reset: see notification with actionType=delete for B
=================================================================
POST http://localhost:REGEX(\d+)/notify
Fiware-Servicepath: /
Content-Length: 260
User-Agent: orion/REGEX(\d+\.\d+\.\d+.*)
Ngsiv2-Attrsformat: normalized
Host: localhost:REGEX(\d+)
Accept: application/json
Content-Type: application/json; charset=utf-8
Fiware-Correlator: REGEX([0-9a-f\-]{36}); cbnotif=1

{
"data": [
{
"A": {
"metadata": {},
"type": "Number",
"value": 1
},
"B": {
"metadata": {
"actionType": {
"type": "Text",
"value": "delete"
}
},
"type": "Number",
"value": 2
Comment on lines +168 to +169
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

  1. The previous ones (as this PR currently shows)
  2. Omit the type and value fields (but it should break the structure of attributes, as value is always assumed
  3. 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

},
"C": {
"metadata": {},
"type": "Number",
"value": 3
},
"id": "E1",
"type": "T"
}
],
"subscriptionId": "REGEX([0-9a-f]{24})"
}
=======================================


--TEARDOWN--
brokerStop CB
accumulatorStop $LISTENER_PORT
dbDrop CB