-
Notifications
You must be signed in to change notification settings - Fork 265
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
3efed0a
c539255
8d8174d
7a65459
973365f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest two separate variants of this test:
|
||
} | ||
}' | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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:
|
||
}, | ||
"C": { | ||
"metadata": {}, | ||
"type": "Number", | ||
"value": 3 | ||
}, | ||
"id": "E1", | ||
"type": "T" | ||
} | ||
], | ||
"subscriptionId": "REGEX([0-9a-f]{24})" | ||
} | ||
======================================= | ||
|
||
|
||
--TEARDOWN-- | ||
brokerStop CB | ||
accumulatorStop $LISTENER_PORT | ||
dbDrop CB |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one: PR #4201
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.