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 isAsync() for All Handlers #378

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

vic-ma
Copy link
Contributor

@vic-ma vic-ma commented Dec 24, 2020

Fixes #312

Handler Async? Notes
AuthPostHandler No
ClientUrlGetHandler No A unit test expects it to be async.
GrafanaDashboardUrlGetHandler No
GrafanaDatasourceUrlGetHandler No
RecordingDeleteHandler No
RecordingGetHandler Yes ctx.vertx().filesystem().exists() is async.
RecordingsGetHandler No
RecordingsPostHandler Yes validateRecording() and saveRecording() are async, but this handler already has isAsync() set to false.
RecordingUploadPostHandler No Already has isAsync() set to false.
ReportGetHandler No Already has isAsync() set to false.
TargetEventsGetHandler No
TargetRecordingDeleteHandler No
TargetRecordingGetHandler No Already has isAsync() set to false.
TargetRecordingOptionsGetHandler No Already has isAsync() set to false.
TargetRecordingOptionsPatchHandler No
TargetRecordingPatchHandler No Already has isAsync() set to false.
TargetRecordingsGetHandler No
TargetRecordingsPostHandler No Already has isAsync() set to false.
TargetRecordingUploadPostHandler No Already has isAsync() set to false.
TargetReportGetHandler No Already has isAsync() set to false.
TargetsGetHandler No
TargetSnapshotPostHandler No
TargetTemplateGetHandler No
TargetTemplatesGetHandler No
TemplateDeleteHandler No
TemplatesPostHandler No
CertificatePostHandler (V2) No Already has isAsync() set to false.
TargetEventsSearchGetHandler (V2) No Already has isAsync() set to false.
TargetRecordingOptionsListGetHandler (V2) No Already has isAsync() set to false.
TargetSnapshotPostHandler (V2) No

AuthPostHandler, GrafanaDashboardUrlGetHandler, and GrafanaDatasourceUrlGetHandler are not async, but they're all simple handlers, so I've set their isAsync() values to true anyway, as suggested.

ClientUrlGetHandler doesn't seem to be async from what I can tell, but there's a unit test that expects it to be async, so I didn't change it.

RecordingsPostHandler seems to have async helper functions, but it already had its isAsync() set to false, so I didn't change it.

Edit: TargetRecordingsPostHandler didn't actually have isAsync() set to false already, but it does appear to be synchronous.

@vic-ma vic-ma requested a review from andrewazores December 24, 2020 16:14
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Nice work. I read over all of your notes and glanced over the implementations and I agree with all of your findings. Please also prepare a PR to backport this to the v1 branch.

@andrewazores andrewazores merged commit 5437d94 into cryostatio:main Jan 6, 2021
@vic-ma vic-ma deleted the check-handlers-async branch January 6, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate whether HTTP handlers are all correctly async
2 participants