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

Avoid triggering notification if metadata changes but not the attribute value itself #3727

Closed
fgalan opened this issue Nov 18, 2020 · 15 comments
Labels
Milestone

Comments

@fgalan
Copy link
Member

fgalan commented Nov 18, 2020

Currently, the metadata of an attribute is considered part of its value. Thus, a subscription using a given attribute as a triggering condition will trigger notifications in cases in which the attribute value doesn't change but metadata does. This is precluding some use cases.

Thus, in order to increase the flexibility of the subscription, this case should be allowed. The simpler approach seems to use a global general setting in the subscription condition (i.e. "metadataAreNotPartOfValue": false :) so notifications are sent only if the value of the attribute itself changes.

Although maybe is a too "coarse-grained" solution and other approaches are possible...

@fgalan fgalan added the backlog label Nov 18, 2020
@Anjali-NEC
Copy link
Contributor

Hi @fgalan Sir,

Please confirm this issue need to be fixed? if yes then I will try to analyze and start working on it.

@kzangeli
Copy link
Member

"notifyOnMetadataChange" might be a better name.
And, seems like a good idea, if you ask me :)

@fgalan
Copy link
Member Author

fgalan commented Sep 16, 2021

Please confirm this issue need to be fixed? if yes then I will try to analyze and start working on it.

Yes, as far as I understand, this issue is still valid. Thanks for your willingness to work on it!

With regards to the name of the field, notifyOnMetadataChange sounds better than my original proposal, although by the moment the actual name it's not important (we can change it easily during the PR process with a search-and-replace on the code if we find a better one).

@Anjali-NEC
Copy link
Contributor

@fgalan @kzangeli,

As per my understanding we need to add an optional bool field notifyOnMetadataChange in the subscription payload. when "notifyOnMetadataChange": false no notifications are sent if the value of metadata changes, notifications are sent only if the value of the attribute itself changes. similarly in case of "notifyOnMetadataChange": true notifications are sent in both the cases (i.e changes in value of attribute or changes in metadata)

Please confirm my understanding and assign this issue to me.
Thanks

@kzangeli
Copy link
Member

kzangeli commented Oct 1, 2021

Sounds good to me.

What should be the default value for "notifyOnMetadataChange" ?
Preferably what's done right now, which I guess is to not notify (@fgalan knows) ?

@fgalan
Copy link
Member Author

fgalan commented Oct 4, 2021

@Anjali-NEC analysis sounds also good to me.

Regarding the default for notifyOnMetadataChange I agree it should be the one that match with current behaviour to avoid backward compatibility problems.

@Anjali-NEC
Copy link
Contributor

@fgalan sir,

I have added bool field notifyOnMetadataChange in subscription payload, Now I want to use this field where the metadata updated. As per my analysis the code for metadata update is written in src/lib/mongoBackend/MongoCommonUpdate.cpp in mergeAttrInfo function L676 to L679 https://github.com/telefonicaid/fiware-orion/blob/master/src/lib/mongoBackend/MongoCommonUpdate.cpp#L676 I want to add logic in this function like this :

      if (notifyOnMetadataChange == false)
      {
         actualUpdate = (attrValueChanges(attr, caP, forcedUpdate, apiVersion) ||
                        ((!caP->type.empty()) &&
                         (!attr.hasField(ENT_ATTRS_TYPE) || getStringFieldF(attr, ENT_ATTRS_TYPE) != caP->type)));
      }
      else
      {
         actualUpdate = (attrValueChanges(attr, caP, forcedUpdate, apiVersion) ||
                        ((!caP->type.empty()) &&
                         (!attr.hasField(ENT_ATTRS_TYPE) || getStringFieldF(attr, ENT_ATTRS_TYPE) != caP->type)) ||
                           mdNew.nFields() != mdSize || !equalMetadata(md, mdNew));
      }

But I have no idea how to get the value of notifyOnMetadataChange from database and use in if/else condition in this function. Please confirm my approach and suggest the right place for applying this condition.
Thanks

@fgalan
Copy link
Member Author

fgalan commented Oct 19, 2021

In my opinion, in that part of the code you could detect if the entity has changed its attributes or only has changed metadata. Maybe you could transform actualUpdate in an enum with three values: None, onlyMetadata, value.

Next, you should manage to pass back that information and use it to filter out subscriptions that doesn't match the criteria. You can do it in one of these two points:

  • At the moment in which DB/cache is searched for subscriptions, i.e. addTriggeredSubscriptions() logic
  • At the moment in which triggered subscriptions are proccessed, i.e. processSubscriptions() logic

This section in the development manual may be useful to get an idea of the overall update entity program flow.

I hope this comment helps you in the implementation of this feature.

@Anjali-NEC
Copy link
Contributor

Hi @fgalan sir,
Currently I am working on another issues So, I am putting this issue on hold. I will work on this issue in future and will try to analyzing this issue.

@chandradeep11
Copy link
Contributor

chandradeep11 commented Dec 6, 2022

@fgalan
We apology for putting the issue on hold for long period. @Anjali-NEC was working on other high priority issue. Currently she is on holiday due to her personal reasons.
Now we have planned to resume working on this ticket in end of February.

@chetan-NEC
Copy link
Contributor

Hi @fgalan, I'm working on it, and soon I will upload a diff.

@fgalan
Copy link
Member Author

fgalan commented Jan 30, 2023

Hi @fgalan, I'm working on it, and soon I will upload a diff.

Great! Thanks for your contribution! :)

@chetan-NEC
Copy link
Contributor

Hi @fgalan, It's a strange thought, in my opinion. When I only change an entity's metadata, the attribute value is set to "null," and the value type is set to "None." as I've seen in master branch code. Do you think this is correct?
Could you please provide your suggestions?

Below is a detailed explanation.

Step 1: Entity Creation

curl localhost:1026/v2/entities -s -S --header 'Content-Type: application/json' -d @- <<EOF
{
  "id": "Room1",
  "type": "Room",
  "temperature": {
    "value": 26.5,
    "type": "Float",
    "metadata": {
      "accuracy": {
        "value": 0.8,
        "type": "Float"
      }
    }
  },
  "pressure": {
    "value": 720,
    "type": "Integer",
	"metadata": {
      "accuracy": {
        "value": 0.9,
        "type": "Float"
      }
    }
  }
}
EOF

Step 2: Update Entity

curl localhost:1026/v2/entities/Room1/attrs -s -S -H 'Content-Type: application/json' -X PATCH -d @- <<EOF
{
  "temperature": {
    "metadata": {
            "accuracy": {
                "type": "Float",
                "value": 9.77
            }
       }
    }
}
EOF

Step 3: The updated Room1 entity output is shown below
curl localhost:1026/v2/entities/Room1 -s -S -H 'Accept: application/json' | python -mjson.tool

{
    "id": "Room1",
    "type": "Room",
    "temperature": {
        "type": "None",
        "value": null,
        "metadata": {
            "accuracy": {
                "type": "Float",
                "value": 9.77
            }
        }
    },
	"pressure": {
        "type": "Integer",
        "value": 720,
        "metadata": {
            "accuracy": {
                "type": "Float",
                "value": 0.9
            }
        }
    }
}

@fgalan
Copy link
Member Author

fgalan commented Feb 2, 2023

With regards to step 2:

curl localhost:1026/v2/entities/Room1/attrs -s -S -H 'Content-Type: application/json' -X PATCH -d @- <<EOF
{
  "temperature": {
    "metadata": {
            "accuracy": {
                "type": "Float",
                "value": 9.77
            }
       }
    }
}
EOF

Take into account what it said in this section of the documentation about Partial Representation (i.e. when some of the elements in the request is missing) https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/orion-api.md#partial-representations:

Attribute/metadata value may be omitted in requests, meaning that the attribute/metadata has null value.

That means an implicit "value": null in your request.

In addition, it is said:

Attribute/metadata type may be omitted in requests. When omitted in attribute/metadata creation or in update operations, a default is used for the type depending on the value:

  • ...
  • If value is null, then None is used.

That means an implicit "type": "None" in your request.

Thus, step 2 request is equivalent to:

curl localhost:1026/v2/entities/Room1/attrs -s -S -H 'Content-Type: application/json' -X PATCH -d @- <<EOF
{
  "temperature": {
    "value": null,
    "type": "None",
    "metadata": {
            "accuracy": {
                "type": "Float",
                "value": 9.77
            }
       }
    }
}
EOF

Which is consistent with result in step 3.

Note that at the present moment, NGSIv2 API doesn't include operations to manage metadata (i.e. adding a metadata without touching the attribute value). There is an issue about it (#2567) that is open to contributions.

@fgalan
Copy link
Member Author

fgalan commented Feb 10, 2023

PR #4274 #4300 #4310

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