[feature] implement scheduled message unpublishing via DELETE request to /{topic}/{messageId}

[GamerGirlandCo]

Test cases are included.
Related: #303
closes #954

[ElectricTea]

+1 for unpublishing scheduled messages. My usecase is rescheduling reminders and due date notifications.

[binwiederhier]

This is awesome. I will 100% get this in for the next release.

I already rebased this on top of the main branch locally, but I have a question.

Given the latest feature (https://docs.ntfy.sh/publish/#updating-deleting-notifications) already implements the DELETE endpoint, my thinking was this:

• If the message is not yet published, delete it from the cache (your code)
• If it is already published, emit a message_delete event (logic in main)

Analogously, publishing to /topic/message-id should update the message in the cache if it's not published yet (assuming it has a Delay), or otherwise treat it as a new message?! I'm a bit fuzzy on what should happen here.

@wunter8 Thoughts?

[binwiederhier]

I'm very excited about implementing updates on unpublished messages, because that can serve as a dead man's switch

[wunter8]

I think what you described, binwiederhier, is the same as what I originally considered here: #303 (comment)

Copying some thoughts I had previously:

I think we should only keep 1 scheduled message for a particular sequence_id at a time. So a first message can be scheduled with sequence_id: reminder. Then, if we want to update that (push the reminder into the future, change the reminder message, etc.), we can POST a new message with sequence_id: reminder and whatever fields we want (message, delay, attachments, etc.). The previous revision would be removed from the DB (I'm thinking deleted entirely), and the new revision would be added/scheduled to be sent in the future.

I think deleting entirely might be better than modifying it's timestamp field or published field because if we change the DB to show the message was published and don't send anything to the client (since it wasn't actually sent), if the client polls the server for all messages in the cache for a topic, they'd get a bunch of scheduled revisions that were never supposed to be sent.

If we delete the revisions, using the subscriber API, you could still check for scheduled messages on a particular topic, but you wouldn't be able to see a history of revisions for the scheduled message, only the most recent.

However, I'm now considering the scenario when you send (assuming all these have the same sequence_id) message A then immediately send message B with a 10 minute delay. Then there are two messages in the cache for that sequence_id: A which has been published and B which hasn't been published. If you then send DELETE /topic/sequence_id, what should happen?

Option A: the scheduled message is deleted/unscheduled
Option B: the published message A is deleted on client devices
Option C: both A and B
Option D: we add a parameter to the DELETE request that lets the user choose between options A, B, and C??

I think clearing a message doesn't have the same problem because you can't clear something that hasn't sent yet. So I think just DELETE has these extra considerations

[wunter8]

One way to distinguish between deleting an existing message and deleting a scheduled message could be to require setting a delay when deleting a scheduled message (even though the delete will happen immediately)

So

DELETE /topic/sequence_id

would delete an existing/already sent message with sequence_id

And

DELETE /topic/sequence_id
Delay: 10 min

would delete a scheduled/delayed message with sequence_id

[binwiederhier]

Your comment is great. It's very complicated. My brain is not working right because I didn't sleep. I will re-read later/tmr and respond.

I think we somehow need to restrict sequences with scheduled messages, so that there can never be more than one scheduled message in a sequence in the DB.

This is what I was trying to come up with:

DELETE /mytopic/myseq
  m := s.cache.MessageBySID(myseq)
  if m.Published {
    return {sequence_id:myseq,event:message_delete,...}
  } else {
    s.cache.DeleteBySID(myseq) // Does this hard-delete all messages in the seq?
    return m
  }
 
POST /mytopic/myseq -- update message 
  m := s.cache.MessageBySID(myseq)
  if !m.Published {
    s.cache.DeleteBySID(myseq) // Does this hard-delete all messages in the seq?
  }
  return {id:abc...,sequence_id:myseq,event:message,...}

[binwiederhier]

@wunter8 I tried re-reading your comment and even with "awake-brain" I still have trouble understanding it.

So let me try to analyze it one paragraph at a time:

I think we should only keep 1 scheduled message for a particular sequence_id at a time. So a first message can be scheduled with sequence_id: reminder. Then, if we want to update that (push the reminder into the future, change the reminder message, etc.), we can POST a new message with sequence_id: reminder and whatever fields we want (message, delay, attachments, etc.). The previous revision would be removed from the DB (I'm thinking deleted entirely), and the new revision would be added/scheduled to be sent in the future.

Yes! So basically, if you try to update or delete a scheduled message with a sequence ID, we must look it up and delete the previous versions of it:

DELETE /mytopic/myseq
  m := s.cache.MessageBySID(myseq)
  if !m.Published {
    s.cache.DeleteBySID(myseq) // Does this hard-delete all messages in the seq?
  }

POST /mytopic/myseq -- update message 
  m := s.cache.MessageBySID(myseq)
  if !m.Published {
    s.cache.DeleteBySID(myseq) // Does this hard-delete all messages in the seq?
    return {id:newIDabc...,sequence_id:myseq,event:message,...}
  }

I think deleting entirely might be better than modifying it's timestamp field or published field because if we change the DB to show the message was published and don't send anything to the client (since it wasn't actually sent), if the client polls the server for all messages in the cache for a topic, they'd get a bunch of scheduled revisions that were never supposed to be sent.

Agreed. We must ensure that only one scheduled message per sequence ID exists in the database.

If we delete the revisions, using the subscriber API, you could still check for scheduled messages on a particular topic, but you wouldn't be able to see a history of revisions for the scheduled message, only the most recent.

So if we do /mytopic/json?poll=1&scheduled=1, the previously scheduled message would disappear. That's what you meant, right? If so, yes, that is correct, because we'll delete the previously scheduled message from the DB.

However, I'm now considering the scenario when you send (assuming all these have the same sequence_id) message A then immediately send message B with a 10 minute delay. Then there are two messages in the cache for that sequence_id: A which has been published and B which hasn't been published. If you then send DELETE /topic/sequence_id, what should happen?

Option 1: the scheduled message is deleted/unscheduled
Option 2: the published message A is deleted on client devices
Option 3: both 1 and 2
Option 4: we add a parameter to the DELETE request that lets the user choose between options 1, 2, and 3??

(I renamed the options to 1..4 to avoid confusion with message A + B.)

This is the scenario you described:

curl -d created ntfy.sh/mytopic/myseq1  ---> (a)
curl -d updated -H "Delay: 30s" ntfy.sh/mytopic/myseq1 --> (b)
curl -X DELETE ntfy.sh/mytopic/myseq1  ---> What to do??

Honestly, I don't know. This is clearly ambigious. Maybe unpublish is a separate API endpoint, like POST /mytopic/seq1/cancel? Or we also make the message_delete event a different endpoint??!

I think clearing a message doesn't have the same problem because you can't clear something that hasn't sent yet. So I think just DELETE has these extra considerations

👍

[wunter8]

Yes, you understood it all correctly!

Did you see my second message with a possible way of resolving the ambiguity?

[binwiederhier]

Did you see my second message with a possible way of resolving the ambiguity?

I did! And I hate it, haha.

I just had a realization regarding the 4 options. DELETE ntfy.sh/mytopic/myseq1 deletes a sequence, not a single message, so we should do both: hard delete all scheduled messages in the db AND send out a message_delete event. Everything else would lead to confusion I think and would be perceived as a bug.

How about this:

DELETE /mytopic/myseq
  s.cache.DeleteScheduledBySID(myseq)
  m := {sequence_id:myseq,event:message_delete,...}
  m.cache.AddMessage(m)
  return m
 
POST /mytopic/myseq -- update message 
  s.cache.DeleteScheduledBySID(myseq)
  m := {id:abc...,sequence_id:myseq,event:message,...}
  m.cache.AddMessage(m)
  return m

[wunter8]

Deleting a sent message feels like it should be different from deleting a scheduled message. But I can also understand your perspective since we're dealing with whole "sequence_ids" and not individual messages

[binwiederhier]

This works wonderfully. 100% written by Cursor: #1556

I'd like to implement updating and deleting of unpublished (scheduled) messages. Conceptually, I'd like to fit this into the concept of a "sequence id".

• When updating an (scheduled) message, I'd like to delete the existing scheduled message with the same sequence id from the database, and add a new message with the same sequence id. This logic goes into the existing handlePublish() endpoint handler.

• When deleting a (scheduled) message, i'd like to delete the existing scheduled message with the same sequence id (like above) from the database, and emit a message_delete event (like it happens already today). This should go in the handleDelete handler.

Pseudo code:

DELETE /mytopic/myseq
s.cache.DeleteScheduledBySID(myseq)
m := {sequence_id:myseq,event:message_delete,...}
m.cache.AddMessage(m)
return m

POST /mytopic/myseq -- update message
s.cache.DeleteScheduledBySID(myseq)
m := {id:abc...,sequence_id:myseq,event:message,...}
m.cache.AddMessage(m)
return m

I read the code and it all looks sound. I need to update the attachment deletion for unpublished messages, then I'll merge it and deploy it to staging.

[binwiederhier]

Superseded by #1556

Thanks @GamerGirlandCo for implementing this. Due to the complexity of this (with the other "update and delete notifications" code since added), I can't merge this. I gave you credit in the release notes!