search

n8n-io / n8n

Free and source-available fair-code licensed workflow automation tool. Easily automate tasks across different services.
https://n8n.io
Other
45.49k stars 6.25k forks source link

Notion Trigger (Beta) triggers doesn't trigger sometimes #6604

Open tonidas opened 1 year ago

tonidas commented 1 year ago

Describe the bug It seems that the Notion Trigger doesn't get triggered in some specific time-based scenarios.

To Reproduce Steps to reproduce the behavior:

  1. Create a Notion Trigger for Page Updated in Database
  2. Update a select field of a Page of the related Database
  3. See that nothing happens, nothing gets triggered. If something happens. wait a minute and go back to step one

Expected behavior That the trigger gets triggered when I change something in a Notion Page

Environment (please complete the following information):

Additional context I've looked at the source code and it looks to be related to the "last minute" query issue at Notion API. (https://github.com/n8n-io/n8n/blob/master/packages/nodes-base/nodes/Notion/NotionTrigger.node.ts) I've looked at the newer release's change log and issues related to Notion but couldn't find something similar.

Joffcom commented 1 year ago

Hey @tonidas

When you say "some specific time based scenarios" do you know what the specific scenario is?

I thought I had a similar issue with the node a while back when we redid part of the logic but I was not able to reproduce it again.

tonidas commented 1 year ago

I was able to reproduce it in a controlled way:

  1. Create a Notion Trigger for "Page Updated in Database"
  2. Update a select field of a Page (let's call it Page "A") of the related Database.
  3. Wait until the Workflow gets triggered
  4. Wait a couple of minutes
  5. Update a select field on the Page A again
  6. Wait endlessly, as this event will be discarded!

Doing some debug, the problem is definitely here: https://github.com/n8n-io/n8n/blob/master/packages/nodes-base/nodes/Notion/NotionTrigger.node.ts

The possibleDuplicates variable is not cleaned up and if you update the same page after some minutes, the code will exclude the entry, thinking that it's a duplicate.

This possibleDuplicates approach was introduced on this PR: https://github.com/n8n-io/n8n/pull/2817

Apart from the fact that possibleDuplicates, I think that there is another issue. This PR makes the pooling ignore changes made in the same minute on the same page.

Lets say that I have a Page A, and this sequence of events:

  1. Event Alpha, 00:01:15, I edit Page A and set its attribute to Xalala
  2. Event Gama, 00:01:30, polling code runs and get the Page A change (Event Alpha), stores Page A id into possibleDuplicates
  3. Event Beta, 00:01:45, I edit Page A and set its attribute to Xelele
  4. Event Zeta, 00:02:30, polling code runs and get the Page A change (Event Beta), and ignores it, as the minute of the events were the same: 00:01

TL;DR I definitely found a bug: possibleDuplicates is not being cleaned after the next execution without updates. But even if that's fixed I believe that there is another bug regarding the possibleDuplicates logic: it's ignoring two events on the same minute.

@knshiro Dear contributor, first I would like to thank you for all your contributions to the community! Secondly, as the mentioned PR was yours, can you kindly help me out on this? If you validate my theories, I could definitely make a PR!

knshiro commented 1 year ago

Hi @tonidas,

It's been a while I worked on this but first thing that needs explaining, the need for this possibleDuplicates comes from the fact that notion timestamp only resolves to minutes and not seconds or milliseconds like usual timestamps. In order to trigger on items that were updated in the same minute the trigger happened without retriggering we kept the list of IDs and only retrigger on new ones. The trade off was that if an item was updated twice in the same minute (one before and one after the trigger), we would miss that update but that was acceptable.

The bug comes from change on an additional commit from @michael-radency that simplified the code but didn't follow the comment that I left above. https://github.com/n8n-io/n8n/pull/2817/commits/bcd32c56f18f3363772a3c8d0e65302ba40a22df#diff-3011b5287d12171cf0929d60657a12416719c8910d5cf08e79dac19b95f7902cR193

In my original PR I only filtered out duplicates on the items updated at the same minute as the last trigger: https://github.com/n8n-io/n8n/commit/45399ae1590ee9dea8881f1a9e27778cfe51412d#diff-3011b5287d12171cf0929d60657a12416719c8910d5cf08e79dac19b95f7902cR184

In the added commit the condition on the same minute was removed.

I hope that helps and thanks for researching that bug @tonidas .

tonidas commented 1 year ago

Hey @knshiro thank you very much for your response.

This Notion behavior on edited time is just bizarre!

Well, I suggest here a fix on the possibleDuplicates and an advisory message at this node's (Notion Trigger) documentation saying that the trigger may ignore events that happened at the same minute.

My use case is just a sync between notion database pages in a specific destination. So, for now I'll stick with a Database Page - Get Many, with the pages edited in the last 10minutes. Processing the same page multiple times for me is not an issue.

Cheers

Joffcom commented 1 year ago

I wonder why it was done that way, I will catch Michael on Monday and ask if he remembers.