Add support for global/project level webhooks #10

kzh · 2019-10-28T15:25:34Z

This PR adds support for global and project scoped webhooks.

REST Endpoints:

<bitbucket url>/rest/sourcegraph-admin/1.0/webhook

Listing:

$ curl <bitbucket url>/rest/sourcegraph-admin/1.0/webhook

Creating:

$ curl <bitbucket url>/rest/sourcegraph-admin/1.0/webhook -d 'scope=<scope>&identifier=<identifier>&events=<events>&external=<external>'
- scope: global | project | repository
- identifier: name of project or repository (empty if scope is global)
- events: comma separated list of events
- external: URL at which the events data will be sent

Deleting:

$ curl -X DELETE <bitbucket url>/rest/sourcegraph-admin/1.0/webhook -d 'id=<id>'
$ curl -X DELETE <bitbucket url>/rest/sourcegraph-admin/1.0/webhook -d 'name=<name>'

For authorization, include these flags: -u admin:admin -H "X-Atlassian-Token: no-check".

The event payload aims to follow these JSON schemas:

Todo:

Persistence layer for storing Webhooks via ActiveObjects
Expose REST endpoint (/webhooks) for CRUDing Webhooks
Serialize events to JSON via google/gson and send out HTTP requests to external URLs
Support more event listeners for PullRequestEvent
Use project/repository ID instead of name slug for unique identifiers
Support webhook secret to validate HTTP requests
Add unit tests
Document available events and general behaviour

This is a part of sourcegraph/sourcegraph#6091.

src/main/java/com/sourcegraph/webhook/EventSerializer.java

kzh · 2019-11-06T07:10:08Z

This PR is ready for first round review. cc @nicksnyder @tsenart @unknwon

unknwon

Thanks for making this!

I am really not sure how should I review this... because all of them LGTM.

Maybe add more comments in the code explain things like:

How would the method be used/invoked.
What role a class/method is.

tsenart · 2019-11-06T08:56:38Z

@kzh: Would it be possible to remove the whitespace / formatting changes or put them in a separate commit?

src/main/java/com/sourcegraph/webhook/Dispatcher.java

src/main/java/com/sourcegraph/webhook/EventSerializer.java

tsenart · 2019-11-06T09:27:44Z

src/main/java/com/sourcegraph/webhook/EventSerializer.java

+    private JsonObject payload;
+
+    private static JsonElement render(Object o) {
+        String raw = renderer.render(o, new HashMap<>());


Java noob question: What is a HashMap doing here? Aren't we rendering an Object directly to a JSON String?

The HashMap is for the options parameter of the render function. I think passing in a null will throw a null pointer exception, so I just pass in an empty one.

I'm not exactly sure how this parameter works either since the docs don't say much 🤔https://docs.atlassian.com/bitbucket-server/javadoc/4.0.2/spi/reference/com/atlassian/bitbucket/json/JsonRenderer.html

I had a similar question. Can you assign this to a variable so it is clearer?

Suggested change

String raw = renderer.render(o, new HashMap<>());

HashMap<> options = new HashMap<>();

String raw = renderer.render(o, options);

Do we really have to indirect through a JSON string? There is no method to go directly from a regular object to a JsonElement?

The bitbucket API seems to only expose the render function which converts bitbucket objects (PullRequest, User, Ref, etc) directly to a JSON string, so there isn't much flexibility.

The other approach would be manually serializing these objects like how events are currently being handled to avoid incurring this redundant operation.

src/main/java/com/sourcegraph/webhook/EventSerializer.java

src/main/java/com/sourcegraph/webhook/WebhookListener.java

src/main/java/com/sourcegraph/webhook/WebhookRouter.java

kzh · 2019-11-06T22:51:50Z

Thanks for the review! I'll ping to request the second round :)

src/main/java/com/sourcegraph/webhook/EventSerializer.java

mrnugget

Leaving this comment here to say: Thorsten was here 😄 I don't know enough about Bitbucket Server's plugin architecture to spot foundational mistakes and my knowledge of Java is superficial, but, that being said, I can't spot obvious mistakes here.

I'm wondering whether we should add some developer/debug experience features such as logging of succesful deliveries, sending a test even for a given PR?

tsenart

Approving to unblock this PR but left some additional feedback, some of which can definitely be addressed in follow up PRs.

A more general question I have: How has all of this been tested? I think we'd truly benefit from at least one high level system level integration test to make our future lives easier in changing this code.

src/main/java/com/sourcegraph/webhook/Dispatcher.java

tsenart · 2019-11-11T19:10:03Z

src/main/java/com/sourcegraph/webhook/WebhookRouter.java

+
+    @POST
+    @Consumes(MediaType.APPLICATION_JSON)
+    public Response put(String raw) throws WebhookException {


We'll need some basic documentation on how to use this API (but that can come in a follow up PR).

Question that comes to mind: Are requests authenticated in the same way as the normal Bitbucket Server API?

Yep!

Authentication Any authentication that works against Bitbucket will work against the REST API. The preferred authentication methods are HTTP Basic (when using SSL) and OAuth. Other supported methods include: HTTP Cookies and Trusted Applications. You can find OAuth code samples in several programming languages at bitbucket.org/atlassian_tutorial/atlassian-oauth-examples. The log-in page uses cookie-based authentication, so if you are using Bitbucket in a browser you can call REST from JavaScript on the page and rely on the authentication that the browser has established.

(https://docs.atlassian.com/bitbucket-server/rest/5.16.0/bitbucket-rest.html)

This talks about the REST API. Does this plugin "extend" the same REST API? Or is it served under a different path? I guess all of these usage questions need to be documented.

Yes, I believe the plugin extend the native bitbucket REST API. They're both served under the /rest/ path which requires authentication (else 401s).

nicksnyder

No need to block on my review. I glanced through the code and here are some notes. My biggest ask is to add a lot of documentation.

nicksnyder · 2019-11-11T19:04:25Z

src/main/java/com/sourcegraph/webhook/Dispatcher.java

+import java.util.concurrent.ExecutorService;
+
+@Component
+public class Dispatcher {


Please add class and method doc comments here and everywhere else.

src/main/java/com/sourcegraph/webhook/Dispatcher.java

nicksnyder · 2019-11-11T19:16:15Z

src/main/java/com/sourcegraph/webhook/EventSerializer.java

+    private JsonObject payload;
+
+    private static JsonElement render(Object o) {
+        String raw = renderer.render(o, new HashMap<>());


I had a similar question. Can you assign this to a variable so it is clearer?

Suggested change

String raw = renderer.render(o, new HashMap<>());

HashMap<> options = new HashMap<>();

String raw = renderer.render(o, options);

Do we really have to indirect through a JSON string? There is no method to go directly from a regular object to a JsonElement?

src/main/java/com/sourcegraph/webhook/EventSerializer.java

src/main/java/com/sourcegraph/webhook/registry/Webhook.java

Co-Authored-By: Nick Snyder <[email protected]>

src/main/java/com/sourcegraph/webhook/Dispatcher.java

tsenart · 2019-11-13T14:02:04Z

@kzh: Can you please summarise the current state of this PR? What's left to do and what's your expected timeline?

tsenart · 2019-11-13T14:04:34Z

@christinaforney: By the way, since this plugin isn't tied to our release cycle, there isn't any hard requirement for us to ship this at the same time as 3.10. We have to implement support in Sourcegraph for receiving and processing these webhooks in 3.11 anyways, so no rush necessary here.

kzh · 2019-11-13T14:23:41Z

This PR is good to merge. Everything is fully functioning. I intend to include the flushed out docs (README.md for webhooks) + unit/integration tests + comments in a followup PR.

A more general question I have: How has all of this been tested? I think we'd truly benefit from at least one high level system level integration test to make our future lives easier in changing this code.

So far, the tests have been manual. I spun up a small webserver that dumps incoming http requests:

package main

import (
	"log"
	"net/http"
	"net/http/httputil"
)

func main() {
	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		dump, err := httputil.DumpRequest(r, true)
		if err != nil {
			log.Println(err)
		} else {
			log.Println(string(dump))
		}
	})
	log.Fatal(http.ListenAndServe(":80", nil))
}

I registered a webhook that listens to all PR events and sends it to the webserver.
[{"id":1,"name":"test","scope":"global","identifier":"","events":["pr"],"endpoint":"<endpoint>","secret":"secret"}]

This is the output on the webserver when I trigger an event (ex. merging a pr).

tsenart · 2019-11-13T14:31:20Z

This PR is good to merge. Everything is fully functioning. I intend to include the docs (README.md for webhooks) + unit/integration tests in a followup PR.

Great. Will that follow up PR also include Java docs that @nicksnyder asked for? If so, let's merge this!

tsenart · 2019-11-13T14:46:25Z

@kzh: I'd share that screenshot in our #progress channel! Thank you for working on this 🙇

kzh marked this pull request as ready for review November 6, 2019 06:16

kzh requested review from nicksnyder, tsenart, a team and unknwon November 6, 2019 06:17

kzh force-pushed the kz/webhooks branch from 16907dc to 603c50e Compare November 6, 2019 06:34

kzh commented Nov 6, 2019

View reviewed changes

src/main/java/com/sourcegraph/webhook/EventSerializer.java Show resolved Hide resolved

unknwon approved these changes Nov 6, 2019

View reviewed changes

tsenart reviewed Nov 6, 2019

View reviewed changes

nicksnyder removed their request for review November 6, 2019 15:28

mrnugget reviewed Nov 7, 2019

View reviewed changes

src/main/java/com/sourcegraph/webhook/EventSerializer.java Show resolved Hide resolved

mrnugget reviewed Nov 7, 2019

View reviewed changes

Add support for global/project level webhooks

dc5606a

kzh force-pushed the kz/webhooks branch from 603c50e to dc5606a Compare November 7, 2019 23:16

kzh added 3 commits November 7, 2019 23:25

Check for invalid scope, repository, and project

d62b653

Improve exception handling

289a9b4

HMAC signing + JSON endpoint

a76c23b

tsenart approved these changes Nov 11, 2019

View reviewed changes

nicksnyder reviewed Nov 11, 2019

View reviewed changes

kzh and others added 7 commits November 11, 2019 14:09

Update src/main/java/com/sourcegraph/webhook/Dispatcher.java

ec7f52b

Co-Authored-By: Nick Snyder <[email protected]>

Address comments

cd01a27

Fix ending newlines

34e3762

Clean imports

67bf8c3

Add ReopenedEvent

e419d04

Support webhook delete by name + change header signature name

99b191d

Exclude secret from being serialized

a109cc7

tsenart reviewed Nov 13, 2019

View reviewed changes

src/main/java/com/sourcegraph/webhook/Dispatcher.java Outdated Show resolved Hide resolved

kzh merged commit fe2491f into master Nov 13, 2019

tsenart deleted the kz/webhooks branch November 13, 2019 14:46

mrnugget mentioned this pull request Nov 20, 2019

a8n: Upsert ChangesetEvents from Bitbucket Server webhooks sourcegraph/sourcegraph-public-snapshot#6726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for global/project level webhooks #10

Add support for global/project level webhooks #10

kzh commented Oct 28, 2019 •

edited

Loading

kzh commented Nov 6, 2019

unknwon left a comment

tsenart commented Nov 6, 2019

tsenart Nov 6, 2019

kzh Nov 6, 2019 •

edited

Loading

nicksnyder Nov 11, 2019

kzh Nov 13, 2019 •

edited

Loading

kzh commented Nov 6, 2019 •

edited

Loading

mrnugget left a comment

tsenart left a comment

tsenart Nov 11, 2019

tsenart Nov 11, 2019

kzh Nov 13, 2019

tsenart Nov 13, 2019

kzh Nov 13, 2019 •

edited

Loading

nicksnyder left a comment

nicksnyder Nov 11, 2019

nicksnyder Nov 11, 2019

tsenart commented Nov 13, 2019

tsenart commented Nov 13, 2019

kzh commented Nov 13, 2019 •

edited

Loading

tsenart commented Nov 13, 2019

tsenart commented Nov 13, 2019

	String raw = renderer.render(o, new HashMap<>());
	HashMap<> options = new HashMap<>();
	String raw = renderer.render(o, options);

Add support for global/project level webhooks #10

Add support for global/project level webhooks #10

Conversation

kzh commented Oct 28, 2019 • edited Loading

REST Endpoints:

Todo:

kzh commented Nov 6, 2019

unknwon left a comment

Choose a reason for hiding this comment

tsenart commented Nov 6, 2019

Choose a reason for hiding this comment

kzh Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzh Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

kzh commented Nov 6, 2019 • edited Loading

mrnugget left a comment

Choose a reason for hiding this comment

tsenart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzh Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

nicksnyder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsenart commented Nov 13, 2019

tsenart commented Nov 13, 2019

kzh commented Nov 13, 2019 • edited Loading

tsenart commented Nov 13, 2019

tsenart commented Nov 13, 2019

kzh commented Oct 28, 2019 •

edited

Loading

kzh Nov 6, 2019 •

edited

Loading

kzh Nov 13, 2019 •

edited

Loading

kzh commented Nov 6, 2019 •

edited

Loading

kzh Nov 13, 2019 •

edited

Loading

kzh commented Nov 13, 2019 •

edited

Loading