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

Conversation

Anjali-NEC
Copy link
Contributor

Fix issue #2591

@@ -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.

Comment on lines +168 to +169
"type": "Number",
"value": 2
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

"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" ]

@fgalan
Copy link
Member

fgalan commented Sep 13, 2022

@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.

@Anjali-NEC
Copy link
Contributor Author

@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.

@chandradeep11
Copy link
Contributor

@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.

@fgalan
We apology for putting the issue on hold for long period. Anjali-NEC was working on other high priority issue (i.e. #4149). Currently she is on long holiday due to her personal reasons.
Now we have planned to resume working on this ticket in first week February.

Please let us know which ticket has high priority to you in below PR:
#4186 , #4003 , #4097 and #4090
Based on the priority, we will plan our work on open tickets accordingly

@fgalan
Copy link
Member

fgalan commented Dec 14, 2022

Please let us know which ticket has high priority to you in below PR:
#4186 , #4003 , #4097 and #4090
Based on the priority, we will plan our work on open tickets accordingly

Answered here

@fgalan
Copy link
Member

fgalan commented Dec 19, 2023

After the merging of PR #4332 this PR needs to be upgrades with master branch. Otherwise, functional test will fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants