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 an endpoint to check connectivity to WOPI server #9202

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

meven
Copy link
Contributor

@meven meven commented Jun 3, 2024

The endpoint is accessible at /hosting/wopiAccessCheck

You send it a POST with input json:
{
"callbackUrl: ""
}

For instance:
curl -ki https://localhost:9980/hosting/wopiAccessCheck --header "Content-Type: application/json" -d '{"callbackUrl":"https://cool.local"}'

Returns json such as:
{
"status": 0,
"details": "OK"
}
With status and details giving hints to what went wrong if it did.

The callbackUrl needs just to be an HTTPS endpoint, it does not need to be the wopi service but can be only a healthcheck url for instance.

Change-Id: Ibdef0bbf0d2fd98e3d293a06dfcfc79478d618e5

cc @juliushaertl

Superseeds #4353

@meven meven added the 24.04 label Jun 3, 2024
@meven meven self-assigned this Jun 3, 2024
@meven meven requested review from mmeeks and Ashod June 3, 2024 08:06
@meven meven marked this pull request as draft June 4, 2024 13:17
@meven meven force-pushed the meven/diagnose-connection branch from 9d0faa2 to 49b4465 Compare June 4, 2024 13:43
@meven meven removed the request for review from mmeeks June 5, 2024 09:10
@meven meven force-pushed the meven/diagnose-connection branch 3 times, most recently from 85b67fb to 80b6af1 Compare June 5, 2024 09:22
Copy link
Contributor

@mmeeks mmeeks left a comment

Choose a reason for hiding this comment

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

Good first cut - glad it works; needs re-working =) Thanks!

wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Fixed Show fixed Hide fixed
wsd/ClientRequestDispatcher.cpp Fixed Show fixed Hide fixed
wsd/ClientRequestDispatcher.cpp Fixed Show fixed Hide fixed
wsd/ClientRequestDispatcher.cpp Fixed Show fixed Hide fixed
@meven
Copy link
Contributor Author

meven commented Nov 13, 2024

The code is now ported to use async requests, but lacks detailed error reporting, cool internal API does not surface hostname errors, connection errors.

@meven meven force-pushed the meven/diagnose-connection branch 4 times, most recently from 7f0b0ae to b313981 Compare November 13, 2024 11:53
@meven meven force-pushed the meven/diagnose-connection branch 2 times, most recently from fb9350a to 5c125b6 Compare November 25, 2024 15:02
@meven meven force-pushed the meven/diagnose-connection branch from 5c125b6 to eed41e5 Compare November 26, 2024 11:39
wsd/ClientRequestDispatcher.cpp Fixed Show fixed Hide fixed
wsd/ClientRequestDispatcher.cpp Fixed Show fixed Hide fixed
net/HttpRequest.hpp Outdated Show resolved Hide resolved
@meven meven force-pushed the meven/diagnose-connection branch from 9d90ebb to e616a8b Compare December 16, 2024 08:25
@meven meven force-pushed the meven/diagnose-connection branch 3 times, most recently from c5b139f to 94b6a90 Compare December 19, 2024 08:37
@meven meven requested a review from vmiklos December 19, 2024 08:52
@meven meven force-pushed the meven/diagnose-connection branch from 94b6a90 to e60b0f9 Compare December 19, 2024 08:54
@meven meven requested a review from mmeeks December 19, 2024 08:55
@meven meven force-pushed the meven/diagnose-connection branch from e60b0f9 to 9f6a583 Compare December 19, 2024 08:57
@meven meven marked this pull request as ready for review December 19, 2024 09:01
Copy link
Contributor

@vmiklos vmiklos left a comment

Choose a reason for hiding this comment

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

  1. CI still needs to pass

  2. I see that the PR has many commits, with commit messages that suggest you would squash this before merging. But if you do so, you'll get a quite large commit. It would be nice to split this to pieces: A) the net/ infrastructure you add (e.g. add socket to the connect fail callback) B) add the new endpoint with a single detected problem C) add the rest of the problems

  3. I see that all the sync poco network code is gone, good.

  4. The sync poco DNS code is also gone, good.

  5. For something as simple as a JSON dictionary with a single entry, I would suggest to use escapeJSONValue() and build the reply manually, no need to go via poco JSON.

@meven meven force-pushed the meven/diagnose-connection branch from 9f6a583 to 97d94f4 Compare December 19, 2024 10:51
@meven
Copy link
Contributor Author

meven commented Dec 20, 2024

2. I see that the PR has many commits, with commit messages that suggest you would squash this before merging. But if you do so, you'll get a quite large commit. It would be nice to split this to pieces: A) the net/ infrastructure you add (e.g. add socket to the connect fail callback) B) add the new endpoint with a single detected problem C) add the rest of the problems

Now split in three first is A, the two seconds are C (mostly optional just code improvement) and the fourth commit is B, the actual endpoint.

5. For something as simple as a JSON dictionary with a single entry, I would suggest to use escapeJSONValue() and build the reply manually, no need to go via poco JSON.

Taken care of. Thanks.

net/HttpRequest.hpp Outdated Show resolved Hide resolved
net/HttpRequest.hpp Outdated Show resolved Hide resolved
net/HttpRequest.hpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Show resolved Hide resolved
Copy link
Contributor

@vmiklos vmiklos left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, I think you did address the points from the previous round. I left some inline comments for nits, but it's fine to not block on those, this PR had many iterations already.

Please wait with merging till you also get a review from @mmeeks . Thanks.

wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
@meven meven force-pushed the meven/diagnose-connection branch from 97d94f4 to c000a0f Compare December 20, 2024 11:41
Copy link
Contributor

@vmiklos vmiklos left a comment

Choose a reason for hiding this comment

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

OK, CI is green, that's promising.

wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mmeeks mmeeks left a comment

Choose a reason for hiding this comment

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

Ok - this is a lot better, looks async - which is nice ;-) and I guess will really help if we can get the Nextcloud (and other integration) side working nicely - with some SDK documentation for the test end-point and so on :-)

There is a follow-on fix there that would be good to have.

Thanks Meven, Miklos & all.

wsd/ClientRequestDispatcher.cpp Outdated Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Show resolved Hide resolved
wsd/ClientRequestDispatcher.cpp Show resolved Hide resolved
And add Session parameter to ConnectFailCallback.

Signed-off-by: Méven Car <meven.car@collabora.com>
Change-Id: Ic541b379bad3beccaa867a95be298e6b807338eb
Signed-off-by: Méven Car <meven.car@collabora.com>
Change-Id: I6c0005a6360bcb63a6712c395a3d7084c2bb26ce
Signed-off-by: Méven Car <meven.car@collabora.com>
Change-Id: Ifa832978040ab1bfa156b0c639605a0e8a971324
@meven meven force-pushed the meven/diagnose-connection branch from c000a0f to 16523a3 Compare December 20, 2024 16:30
The endpoint is accessible at /hosting/wopiAccessCheck

You send it a POST with input json:
{
"callbackUrl: "https://wopi-host/files/"
}

For instance:
curl -ki https://localhost:9980/hosting/wopiAccessCheck --header "Content-Type: application/json" -d '{"callbackUrl":"https://cool.local"}'

Returns json such as:
{
"details": "OK"
}
With status and details giving hints to what went wrong if it did.

The callbackUrl needs just to be an HTTP(S) endpoint, it does not need to be the actual wopi service but can be only a healthcheck url for instance.

Signed-off-by: Méven Car <meven.car@collabora.com>
Change-Id: I0f08456d38738ea1195dda21f2bfd3636ae3fde7
@meven meven force-pushed the meven/diagnose-connection branch from 16523a3 to 3c27c11 Compare December 20, 2024 16:31
@meven
Copy link
Contributor Author

meven commented Dec 20, 2024

Ok - this is a lot better, looks async - which is nice ;-) and I guess will really help if we can get the Nextcloud (and other integration) side working nicely - with some SDK documentation for the test end-point and so on :-)

Documentation is my next step and informing Julius.

@meven meven enabled auto-merge (rebase) December 20, 2024 16:32
@meven meven merged commit e116ee8 into CollaboraOnline:master Dec 20, 2024
13 checks passed
@meven meven deleted the meven/diagnose-connection branch December 23, 2024 09:14
@juliusknorr
Copy link
Member

Very nice to see this merged, thanks a lot.

I started a first draft PR to integrate with richdocuments (nextcloud/richdocuments#4361).

One thing I noticed would be that we have not reliable way to detect if the collabora server supports this check endpoint. Can we maybe expose it in /hosting/discovery similar to how it is done for capabilities:

Screenshot 2024-12-27 at 15 49 59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants