caronc / apprise-api

A lightweight REST framework that wraps the Apprise Notification Library
https://hub.docker.com/r/caronc/apprise
MIT License
638 stars 56 forks source link

Bug with tags or I'm not understanding how it should work #103

Closed lcx closed 1 year ago

lcx commented 1 year ago

:beetle: Describe the bug As I've read in the documentation, sending a request with tags combined by comma and/or space should do a AND matching of the tags. So if I send a POST like this

curl -X POST -d 'tag=devops,notify&body=test message&title=Fooba' http://notify.foo.com/notify/blaa

should send the message only to those URLs that have the Tag devops AND notify, but it still sends the message to all urls.

:bulb: Screenshots and Logs Here is my configuration for this key

version: 1
tag: panic
urls:
  - pover://foo@bar:
    - tag: devops, notify
  - pover://foo@bar?priority=1:
    - tag: devops, high
  - pover://foo@bar?priority=2:
    - tag: cris, emergency

if I check the configuration via API, it kind of makes sense why it's sending it to all urls

curl http://notify.foo.com/json/urls/

gives me this result

{
  "tags": [
    "notify",
    "panic",
    "devops",
    "high",
    "cris",
    "emergency"
  ],
  "urls": [
    {
      "url": "pover://foo@bar//?priority=normal&format=text&overflow=upstream&rto=4.0&cto=4.0&verify=yes",
      "tags": [
        "panic",
        "devops",
        "notify"
      ]
    },
    {
      "url": "pover://6pover://foo@bar//?priority=high&format=text&overflow=upstream&rto=4.0&cto=4.0&verify=yes",
      "tags": [
        "panic",
        "devops",
        "high"
      ]
    },
    {
      "url": "pover://pover://foo@bar//?priority=emergency&expire=3600&retry=900&format=text&overflow=upstream&rto=4.0&cto=4.0&verify=yes",
      "tags": [
        "panic",
        "cris",
        "emergency"
      ]
    }
  ]
}

so from what I am seeing the global tags contain all URL tags which now makes sense why it's sending 3 messages instead of just the expected one, but hen again, if I just send to devops, it does only send to the two url with the devops tag so I'm confused. What am I doing wrong?

:computer: Your System Details: running on docker, with the docker:latest as of 18. Feb. 2023

caronc commented 1 year ago

I need to fix the CI in another branch, but i think you pointed out nicely in your observation that i only support OR. This has been fixed in the latest PR. Would love if you could test it out for me!

lcx commented 1 year ago

Sure, I'll try to spin up this branch in Docker but not sure how quick I'll get to it ... actually ... it's got a compose file, everything seems to be there. Let me try it right now :)

lcx commented 1 year ago

oh yes ... 🙌 first one with tag=notify,cris is interpreted as OR which sends to all three devices. Second one with tag=notify cris is interpreted as AND which sends just the one device matching. Aweeesome!

CleanShot_2023-02-24_at_10 07 32
caronc commented 1 year ago

I'll try to figure out why all 3 are being notified with your or statement. Something is still off here.

caronc commented 1 year ago

Hmm, strangely i can't reproduce this issue you got.

When i used the current code and pushed notify,cris, (OR effectiveness) it only notified the 1st and 3rd entry (as expected).

When i do notify cris (AND effectiveness) it doesn't notify anything.

Both are expected results and differ from what you shared above..

Something is amiss here...

lcx commented 1 year ago

oh sorry, my bad. The test I used does not match the test data I posted in the initial issue. Can't check the branch on that server since it's running in docker via caprover and would be hard to set up a test environment. I started it locally on my machine and did a try. The OR is correct since I added the cris to every notification. so, let me retry this with my initial data and it works as expected:

✅ here we have devops OR notify 'tag=devops,notify

CleanShot_2023-02-26_at_13 47 33

✅ here we have devops AND notify 'tag=devops,notify

CleanShot_2023-02-26_at_13 48 22

✅ and here is just cris

CleanShot_2023-02-26_at_13 49 07

again, really really sorry, was in the middle of something and didn't think to just use the same setup as in the reported issue.

caronc commented 1 year ago

The code has been merged; closing this off :rocket: