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

Add webhook support #1626

Merged
merged 5 commits into from
Nov 22, 2021
Merged

Conversation

Lyncredible
Copy link
Contributor

@Lyncredible Lyncredible commented Sep 10, 2021

Summary

This change adds webhook support to enable real-time syncing of remote updates. It takes advantage of the webhooks functionality provided by Microsoft. This fixes #1620.

How does it work

  1. The onedrive process stands up a local web server to receive incoming notifications (to be sent by Microsoft)
  2. The onedrive process registers the public-facing url of the web server with Microsoft, via the POST /subscriptions endpoint.
  3. When there is an remote update, Microsoft sends a notification request to the pre-registered url.
  4. The onedrive process receives the notification request, and kicks off a new delta sync loop to fetch updates.

Code changes

  1. onedrive.d contains the bulk of logic to manage the local web server and to register/renew subscriptions with Microsoft. It is also responsible for processing incoming notification requests. When such requests are received, it sends signals to the main sync loop.
  2. main.d contains the logic to process incoming signals and execute delta sync to fetch updates.
  3. The embeded web server is made possible via cgi.d from adamdruppe/arsd
  4. New config options are added to config.d and also documented.

Testing

Unfortunately I could only test this manually with a very straightforward setup: personal OneDrive account with no shared folders. More testing is welcome!

What is next

Currently the public-facing url has to be statically configured. This should work well for most users either with static public IP or with dynamic DNS services. We could consider adding UPnP support to allow dynamic configuration via automatic port forwarding.

docs/USAGE.md Outdated Show resolved Hide resolved
@@ -83,19 +89,19 @@ After installing the application you must authorize the application with your On

You will be asked to open a specific URL by using your web browser where you will have to login into your Microsoft Account and give the application the permission to access your files. After giving permission to the application, you will be redirected to a blank page. Copy the URI of the blank page into the application.
```text
[user@hostname ~]$ onedrive
[user@hostname ~]$ onedrive
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My editor also removed a bunch of extra whitespaces. It is better to review this PR with the Hide whitespace changes option turned on (or by adding ?w=1 to the PR url)

@abraunegg
Copy link
Owner

@Lyncredible
Thanks for the PR, will review further when I have time.

One question I do have, and it is not immediately visible, is the license for cgi.d. If you can format your readme.md in the new arsd folder as per https://github.com/abraunegg/onedrive/tree/master/src/notifications so that it can be easilly seen that cgi.d is cgi.d copyright 2008-2021, Adam D. Ruppe. Provided under the Boost Software License. This will help folk who rely on the licensing side of things to be squeaky clean to enable the package to be including in distributions.

Regarding the '*' -> '-' changes, if you can put those back please given that is the standard I have been using.

Re the whitespace removal at the end of various lines - leave that one - that is on the whiteboard at the moment post #1494 being implemented, as there is a lot of extra whitespace that has crept in over the years.

I would also add, under the main readme.md (given the removal of 'While local changes are uploaded right away ... ' text), add under 'Features' Supports webhooks to subscribe to remote change notifications - or whatever else is suitably valid.

In terms of when this PR will be merged into 'master', most likely after #1494 is merged - so potentially you will need to think about a few updates based on those changes coming down the pipeline. I would PR merge this before that change, however, I am too far along the journey to stop, merge, backtrack then revisit all the language change code.

This will give also folk the opportunity to test this PR to really bake / shake this out in it's current format as well.

@Lyncredible
Copy link
Contributor Author

One question I do have, and it is not immediately visible, is the license for cgi.d. If you can format your readme.md in the new arsd folder as per https://github.com/abraunegg/onedrive/tree/master/src/notifications so that it can be easilly seen that cgi.d is cgi.d copyright 2008-2021, Adam D. Ruppe. Provided under the Boost Software License. This will help folk who rely on the licensing side of things to be squeaky clean to enable the package to be including in distributions.

That is a great suggestion. I have updated the readme file accordingly.

Regarding the '*' -> '-' changes, if you can put those back please given that is the standard I have been using.

Alright, I have changed it back.

Re the whitespace removal at the end of various lines - leave that one - that is on the whiteboard at the moment post #1494 being implemented, as there is a lot of extra whitespace that has crept in over the years.

Cool. I will wait and see what to do after #1494 is merged.

I would also add, under the main readme.md (given the removal of 'While local changes are uploaded right away ... ' text), add under 'Features' Supports webhooks to subscribe to remote change notifications - or whatever else is suitably valid.

I have already added a line Real-Time syncing of remote updates under Features. Is that okay?

In terms of when this PR will be merged into 'master', most likely after #1494 is merged - so potentially you will need to think about a few updates based on those changes coming down the pipeline. I would PR merge this before that change, however, I am too far along the journey to stop, merge, backtrack then revisit all the language change code.

This will give also folk the opportunity to test this PR to really bake / shake this out in it's current format as well.

Sounds good to me!

@abraunegg abraunegg merged commit bfeeae9 into abraunegg:master Nov 22, 2021
@abraunegg
Copy link
Owner

@Lyncredible
Whilst this is merged into Master - we now have this depreciation warning:

src/arsd/cgi.d(3570): Deprecation: function `onedrive.OneDriveWebhook.serve.serveEmbeddedHttp!((cgi) => handle(cgi, parentTid), Cgi, 5000000L).serveEmbeddedHttp` function requires a dual-context, which is deprecated
src/onedrive.d(126):        instantiated from here: `serveEmbeddedHttp!((cgi) => handle(cgi, parentTid), Cgi, 5000000L)`

Please Fix

abraunegg added a commit that referenced this pull request Nov 23, 2021
Revert PR #1626 due to deprecation function that needs to be resolved before PR can be merged again
@abraunegg abraunegg mentioned this pull request Nov 23, 2021
@abraunegg
Copy link
Owner

@Lyncredible
This PR, whilst it was merged, has been reverted via #1716

abraunegg added a commit that referenced this pull request Nov 23, 2021
Revert PR #1626 due to deprecation function that needs to be resolved before PR can be merged again
@Lyncredible Lyncredible mentioned this pull request Nov 23, 2021
@Lyncredible
Copy link
Contributor Author

Lyncredible commented Nov 23, 2021

@abraunegg Thanks for spotting the issue. I have created PR #1718 to address the deprecation warning. The fix can be seen in commit 5b89455.

In short, we need OneDriveWebhook.handle to be a static function, otherwise we would hit the member function "requires a dual-context, which is deprecated" warning. To workaround the problem, I changed OneDriveWebhook class to be a singleton class, even thought it does not have to be one.

The problem is deemed a bug and should be fixed in the compilers eventually. The singleton stuff could be undone when it is fixed. The root cause is described in the links below.

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Webhook integration for real-time remote OneDrive updates
2 participants