telefonicaid / fiware-orion

Context Broker and CEF building block for context data management, providing NGSI interfaces.
https://fiware-orion.rtfd.io/
GNU Affero General Public License v3.0
212 stars 265 forks source link

Notifications sent on identical data when JSON attribute values are used #4211

Closed WillFreitag closed 7 months ago

WillFreitag commented 2 years ago

Bug description Orion sends a notifier, even if no attribute is changed.

How to reproduce it Steps to reproduce the behavior (as an example)

  1. On a blank System, create a subscription like:
    {
        "description": "My Subscription",
        "subject": {
            "entities": [
                {
                    "idPattern": ".*",
                    "type": "Vehicle",
                }
            ]
            ,
            "condition":{
                "attrs":["location"]
            }
        },
        "notification": {
            "http": {
                "url": "http://quantumleap/v2/notify"
            },
            "attrs": [
                "location"
            ],
            "onlyChangedAttrs": true
        }
    }
  2. Send entities to Orion like:

    {
       "id": "entity_id:1",
       "type": "Vehicle",
       "name": {
           "type": "Text",
           "value": "Something"
       },
       "location": {
           "type": "geo:json",
           "value": { 
               "type": "Point",
               "coordinates": [10.0, 10.0]
    
           }
       }
    }
  3. The notification is sent each and every time, even if the location is not changed.
  4. This ends up in a database (Postgres behind QuantumLeap) having duplicate data (no changes in location).

Expected behavior The notification is only fired on attribute change.

Additional information

fgalan commented 2 years ago

Thank you for your issue report! Pretty clear and straightforward :)

We have recently solved a problem that sounds similar to the one you describe.

- Fix: conditions.alterationTypes not working properly when conditions.attributes is used in entityUpdate case (#1494, reopened)

However, that fix has not been yet released (it is planned for Orion 3.8.0, which is the next version)

Could you test with telefonicaiot/fiware-orion:latest docker image and tell us if it works, please?

WillFreitag commented 2 years ago

Great! I will test (tomorrow) and give feedback!

Thanks!

WillFreitag commented 2 years ago

All the same :-( Using telefonicaiot/fiware-orion:latest (Orion-Version "3.7.0-next") has the same result. I had the idea, that location or doubles in general might be a problem (you know best, comparing doubles is not as easy as "val1 == val2" when it comes to check the condition for the notifier), so I changed the payload sending a simple string - did not help. So the issue is still there.

WillFreitag commented 2 years ago

For completeness, this is, what I'm currently trying: Notification:

{
    "description": "MG",
    "subject": {
        "entities": [
            {
                "idPattern": ".*",
                "type": "Vehicle",
            }
        ]
        ,
        "condition":{
            "attrs":["test_attribute"]
        }
    },
    "notification": {
        "http": {
            "url": "http://quantumleap:8668/v2/notify"
        },
        "attrs": [
            "test_attribute"
        ],
        "onlyChangedAttrs": true
    }

And the payload, sent via "PUT" after a first "POST":

{
    "test_attribute": {
        "type": "Text",
        "value": "test"
    }
}

I will try some more things (e.g. using POST with upsert, switch from normailzed to keyValues) in the next days...

kzangeli commented 2 years ago

Comparing floating point values is a difficult task in a program. CPU's are kind of made for integers ...

I ran into the same problem with Orion-LD and my idea of solving it (haven't implementing it yet) is to use a range for floating point comparisons and not an exact comparison.

Instead of comparing with EXACT numbers:

   (P1.x == 0.123), 

I'd do an AND:

  (P1.x >= 0.1229999) AND (P1.x <= 0.123001).

Then the question arrives, WHO exactly defines the precision? Well, in the case of queries, it'd be a URI param (?precision=4 (decimals)), and for subscriptions, we'd add a field expressing the same.

fgalan commented 2 years ago

All the same :-( Using telefonicaiot/fiware-orion:latest (Orion-Version "3.7.0-next") has the same result. I had the idea, that location or doubles in general might be a problem (you know best, comparing doubles is not as easy as "val1 == val2" when it comes to check the condition for the notifier), so I changed the payload sending a simple string - did not help. So the issue is still there.

Just to be sure... could you do a GET /version in Orion service port and post the result, pls?

WillFreitag commented 2 years ago

@fgalan The output is:

{
"orion" : {
  "version" : "3.7.0-next",
  "uptime" : "0 d, 2 h, 18 m, 47 s",
  "git_hash" : "90d592b1293645f43cb12af49fe128c3e7aedb7a",
  "compile_time" : "Thu Sep 1 12:17:37 UTC 2022",
  "compiled_by" : "root",
  "compiled_in" : "d30c3c23186e",
  "release_date" : "Thu Sep 1 12:17:37 UTC 2022",
  "machine" : "x86_64",
  "doc" : "https://fiware-orion.rtfd.io/",
  "libversions": {
     "boost": "1_74",
     "libcurl": "libcurl/7.74.0 OpenSSL/1.1.1n zlib/1.2.11 brotli/1.0.9 libidn2/2.3.0 libpsl/0.21.0 (+libidn2/2.3.0) libssh2/1.9.0 nghttp2/1.43.0 librtmp/2.3",
     "libmosquitto": "2.0.12",
     "libmicrohttpd": "0.9.70",
     "openssl": "1.1",
     "rapidjson": "1.1.0",
     "mongoc": "1.17.4",
     "bson": "1.17.4"
  }
}
}

@kzangeli You're right and the "solution" (in C++) is EPSILON. But as I mentioned before...the problem here is not related to that thingy...

WillFreitag commented 2 years ago

What I did before (I'm using a Node-RED flow), is sending the above payload to Orion Context Broker using the verb PUT (id and type in URL and not in payload) and on a 404 (entity not found - cannot update), I added id and type to the payload and sent it via POST. Next time, the same id can be updated via PUT. Now, I switched over to POST always with options=upsert and the subscription works as expected (only fired, if data has changed) as long as the payload contains only "simple" types (strings, numbers...integer or double)! As soon as I send location data, the notification is sent always...weird! To be more precise: works (notification only sent when value changes):

{
    "test_attribute": {
        "type": "Number",
        "value": 10.1
    }
}

does not work (always notifying - even on identical values):

{
    "test_attribute": {
        "type": "geo:json",
        "value": { 
            "type": "Point",
            "coordinates": [10.2, 10.0]

        }
    }
}
fgalan commented 2 years ago

Maybe the problem is related which the usage of JSON values in attributes. By the above report, it seems it is not related with numbers exclusively.

Could you test the following case and report the result, please?

Case 1:

{
    "test_attribute": {
        "type": "noMatter",
        "value": { 
            "x": "a",
            "y": "b"
        }
    }
}

Case 2:

{
    "test_attribute": {
        "type": "noMatter",
        "value":  [ "x", "y" ]
    }
}
WillFreitag commented 2 years ago

Here we go: OKAY means: Subscription works as expected, only sent on changed data. FAIL means: Subscription is firing always, even on identical data.


OKAY:

let simple = {
    "test_attribute": {
        "type": "Number",
        "value": 10.1
    },
};

FAIL:

let geo = {
    "test_attribute": {
        "type": "geo:json",
        "value": { 
            "type": "Point",
            "coordinates": [10.2, 10.0]

        }
    },
};

FAIL:

let object_ = {
    "test_attribute": {
        "type": "noMatter",
        "value": { 
            "x": "a",
            "y": "b"
        }
    },
};

FAIL:

let array_ = {
    "test_attribute": {
        "type": "noMatter",
        "value":  [ "x", "y" ]
    },
};

Conclusion: You're totally right. This as nothing to do with "geo:json" or doubles or so, but with "complex" types (structures).

kzangeli commented 2 years ago

Not so sure Orion supports "check of value change for compound value". It's ... a bit complex. Fermin - easy to check! (last time I was there, it wasn't implemented for compounds)

I implemented it myself for Orion-LD - by rendering the old vs rendering the new value, then just a simple strcmp() ... That works just fine as long as the order of the fields doesn't change ... A bit useless to tell you the truth ...

To do it well, you'd need to order all fields of the compound values (old and new - recursively), and after that you can compare rendered strings.

As I said, a bit complex

That said, float comparisons, if not done with a range (GE AND LE) will give problems as well, as I stated earlier. Don't dismiss that.

WillFreitag commented 2 years ago

Great "idea"! About that ordering-thing: If you parse both JSONs into an object and flush those objects into JSON afterwards...should'nt both JSONs have the same order?

kzangeli commented 2 years ago

If the parser you use supports ordering of fields, then yes, of course. Orion uses rapidjson - I'm not a fan and far from an expert ... :)

You still might have a problem with Arrays. Can't really order the values inside an array, as the broker doesn't know what the user wants, how it semantically understands the order of the values in an array.

Anyway, best effort, right?

I mean:

[  "a", "b" ]

is not necessarily the same as:

[ "b", "a" ]
WillFreitag commented 2 years ago

If Orion uses std::array there is "no problem" comparing two arrays for equality...

std::array<string, 2> as1 {"a", "b"};
std::array<string, 2> as2 {"b", "a"};

cout << "Arrays are equal? " << as1 == as2;
kzangeli commented 2 years ago

Had it been that simple ... It might be an object containing an array, containing an object, containing an array ... Any depth.

Either you compare the old tree with the new tree (recursively looking up members from tree 1 in tree 2, and then mark every field as "compared", afterwards make sure all fields were compared in both trees), or you sort and render and strcmp. This is far from trivial, there's no easy fix. That's why it wasn't implemented "back in the day", Guess we had bigger fish to fry (I believe this may date back to 2016 or so).

It is possible to implement, though, I did it myself. It's just a bit of work ...

Array will always be a problem though. For some implementations [ 1,2 ] === [ 2, 1 ], for other implementations that might be an important difference (e.g. "I have one sister and two brothers" vs "I have two sisters and one brother"). The broker is agnostic to this type of semantics.

We'd need an option somewhere for the comparison, sometyhing expression "pay attention to array order"

fgalan commented 2 years ago

The solution to this issue would be to implement "deep compare" in attribute value JSON structures. Orion uses an internal way of representing JSON (CompoundValue) so some recursive function should be implemented to do so. The feedback in the previous comments can be useful to that end.

I'm changing the title to describe more precisely the issue.

(I think this is not the first time we find this and even there is an old issue about this. However, I haven't been able to find it among the open issues).

kzangeli commented 2 years ago

The solution to this issue would be to implement "deep compare"

Yes, correct. That's much nicer than "sort/render/strcmp"

WillFreitag commented 2 years ago

Just to have an answer to my client...Do you think, this will be implemented in 3.8.0? ...and further: is there a release date planned yet? And again (already stated this in another issue): I'd really love to contribute but I don't find the time at all :-(

fgalan commented 2 years ago

Just to have an answer to my client...Do you think, this will be implemented in 3.8.0? ...and further: is there a release date planned yet?

It would depend if (and when) some brave developer volunteers to implement it :) By the moment it is [not] ours immediate priorities. You can always use a side not-JSON attribute that changes at the same time than the JSON one (typically, a "TimeInstant" attribute of type DateTime with the timestamp of the change).

And again (already stated this in another issue): I'd really love to contribute but I don't find the time at all :-(

Maybe you can ask budget to your client so you can allocate resources to contribute with it. I understand your client could be an interested party in getting this functionality done ;)

WillFreitag commented 2 years ago

Okay. My current workaround is, that no objects/arrays are used at all (e.g. Geo:JSON is now split into two doubles lat/lng). Naturally, we are leaving Smart Data Model with this but it's a workaround only - okay with our customer.

About the contribution: That project is about evaluating the FIWARE stack -the customer wants to know, if it fits his needs - so no money for development here :-( ...

And as a reminder for the fix: Keep in mind, that the condition acts differently, depending on how the Orion Context Broker is used. PUT/POST or POST with upsert. (See my comment above)

fgalan commented 2 years ago

Okay. My current workaround is, that no objects/arrays are used at all (e.g. Geo:JSON is now split into two doubles lat/lng). Naturally, we are leaving Smart Data Model with this but it's a workaround only - okay with our customer.

Note that if you split latitude and longitude into two different attributes instead of using GeoJSON, you will not be able to benefic from the geo-query features in Orion.

About the contribution: That project is about evaluating the FIWARE stack -the customer wants to know, if it fits his needs - so no money for development here :-( ...

Pitty :(

And as a reminder for the fix: Keep in mind, that the condition acts differently, depending on how the Orion Context Broker is used. PUT/POST or POST with upsert. (See my comment above)

Your comment will be taken into account when this issue gets addressed.

Thanks for all your valuable feedback!

JeffersonLupinacci commented 1 year ago

Hello everyone, any news about this problem with GEO:JSON? We are having a lot of problems with the new version of orion:3.7.0 and the coordinates.

Would it perhaps be possible to use checks from the turf library to validate that the geo:json's are different?

https://github.com/Turfjs/turf/tree/master/packages/turf-boolean-equal

https://libgeos.org/ - Symmetric Difference

Br, Jefferson.

fgalan commented 1 year ago

Hello everyone, any news about this problem with GEO:JSON? We are having a lot of problems with the new version of orion:3.7.0 and the coordinates.

Based on the previous analysis (see all the discussion above) it seems is not a problem with geo:json in particular, but with any JSON used as attribute value.

As I told before:

It would depend if (and when) some brave developer volunteers to implement it :)

Maybe you would like to implement the fix (given that is seems you have a strong use case needing it)? I'd be more than happy to review your pull request and provide feedback and guidance on it :)

Finally, you say "a lot or problems with the new version of orion:3.7.0 and the coordinates". Two comments:

  1. At the present moment, 3.7.0 is not "new version" :). I'd suggest to use the latests version available at the present moment (Orion 3.10.1)
  2. Could you detail the exact problems do you have with the coordinates, please? I'd like to know if all them are related with this issue or if there is something else.

Thanks for your feedback!

JeffersonLupinacci commented 1 year ago

Whenever I run the request to the informed endpoint, the entity is received by the subscription, regardless of having changes or not.

curl 'http://orion:1026/v2/op/update' -H 'Content-Type: application/json' -H 'Accept: application/json' -H 'fiware-service: orion' -H 'fiware-servicepath: /test' -d '{
    "actionType": "UPDATE",
    "entities": [
        {
            "id": "orion.0001",
            "type": "orion",
            "position": {
                "value": {
                    "type": "Point",
                    "coordinates": [
                        -77.02503667,
                        -12.131508679
                    ]
                },
                "type": "geo:json"
            },
            "typology": {
                "type": "string",
                "value": "String"
            },

        }
    ]
}'

In order not to receive notifications constantly, we are using coordinates in the geo:point format and this way we don't have the problem of receiving them all the time.

{
       "position": {
            "type": "geo:point",
            "value": "38.742375,-9.128965",
            "metadata": {}
        },
}

Sometimes we have much more complex coordinates like MultiPoint, MultiPolygon or MultiLineString

As our integrations with external services are designed to receive data via NIFI, we have not implemented a logic to validate field by field before sending it to the broker and in many cases the data does not change.

fgalan commented 10 months ago

Related issue https://github.com/telefonicaid/fiware-orion/issues/4434 (similar problem in metadata)

fgalan commented 10 months ago

PR https://github.com/telefonicaid/fiware-orion/pull/4444

fgalan commented 10 months ago

(I think this is not the first time we find this and even there is an old issue about this. However, I haven't been able to find it among the open issues).

It was #643 :)

fgalan commented 10 months ago

A fix for this problem has been provided (in master branch by the time being). The version of Orion available in telefonicaiot/fiware-orion:latest at dockerhub includes this fix.

@WillFreitag @JeffersonLupinacci it would be great if you could test with that version an provide feedback (either if the issue is solver or not), please. Thanks!

fgalan commented 7 months ago

Orion 3.11.0 is about to be released. Thus, we are closing this issue.

Of course it could be reopened if @WillFreitag and/or @JeffersonLupinacci feedback comes in the future.