Use a separate ExecutorService to fix #71. #72

bturner · 2018-03-01T04:31:56Z

Updated AsyncProcessor to look at the Bitbucket Server verson and choose between using the shared ExecutorService (5.9+) or creating its own ExecutorService (previous versions)
- Bitbucket Server 5.9 includes a fix which prevents add-ons using the shared BucketedExecutor from blocking hook callbacks
Removed some dead code in AsyncProcessor and PullRequestListener
Simplified Maven structure and reconfigured how integration and unit tests are distinguished
- Moved unit tests from src/test/java/ut to src/test/java
- Added a simple UserResourceTest which requests configuration for a repository which hasn't had any applied
- This verifies that the PR Harmony plugin has been installed and enabled successfully, and that its REST resource is available
Simplified some of the processing in UserUtils to use the API more efficiently and avoid various IDEA inspection warnings

bturner · 2018-03-01T04:34:04Z

pom.xml

    <dependency>
      <groupId>com.atlassian.templaterenderer</groupId>
      <artifactId>atlassian-template-renderer-api</artifactId>
-      <version>1.5.7</version>


In general, if you have a <scope>import</scope> for bitbucket-parent, you shouldn't define explicit versions on your dependencies. Our parent POM defines the versions of the libraries that we actually ship with, so using those versions ensures you're (for example) using the "right" APIs for Bitbucket Server versions you target.

Thanks for the tip. The POM was largely copied from Atlassian's example plugin project, so if you haven't already you might want to update that to prevent others from doing this.

bturner · 2018-03-01T04:34:15Z

pom.xml

    </dependency>
-    <dependency>
-      <groupId>commons-lang</groupId>
-      <artifactId>commons-lang</artifactId>


This was unused

bturner · 2018-03-01T04:35:20Z

pom.xml

      <scope>test</scope>
    </dependency>
-
-    <!-- WIRED TEST RUNNER DEPENDENCIES -->


I've replaced all of these dependencies with new dependencies on Bitbucket Server's common test modules (bitbucket-it-common and bitbucket-page-objects). Those will transitively pull in all the dependencies you need to write browser and REST integration tests.

bturner · 2018-03-01T04:36:29Z

pom.xml

  </dependencies>

  <build>
-    <testSourceDirectory>${project.basedir}/src/test/java/ut</testSourceDirectory>


Rather than separate the source like this, I've switch the setup to have Surefire (used to run unit tests) ignore everything under src/test/java/it, and have AMPS (which uses Failsafe to run integration tests) ignore everything that's not under src/test/java/it.

(Feel free to not do it this way; this is just how we write our own plugins.)

bturner · 2018-03-01T04:37:43Z

pom.xml

+    <bitbucket.data.version>${bitbucket.version}</bitbucket.data.version>
+    <!-- This separate version allows testing the plugin with a different version of the system than
+         it's compiled against, which is useful for testing compatibility with multiple versions -->
+    <bitbucket.test.version>${bitbucket.version}</bitbucket.test.version>


I'd introduced this separate version to allow, for example, compiling PR Harmony against Bitbucket Server 4.8 (which is the lower bound of the compatibility range you've set on Marketplace), but running the tests against Bitbucket Server 5.8 (the upper bound).

You can run the integration tests against a given version with mvn clean install -DskipITs=false -Dbitbucket.test.version=5.8.1

bturner · 2018-03-01T04:42:20Z

src/main/java/com/monitorjbl/plugins/UserUtils.java

  public String getUserDisplayNameByName(String username) {
-	    ApplicationUser user = userService.getUserByName(username);  
-	    return user.getDisplayName() == null ? username : user.getDisplayName();
+    return ofNullable(userService.getUserByName(username))


This change guards against getUserByName returning null (It's marked @Nullable in our documentation)

Actually, from looking at the open issues for the plugin, this change will fix #69 (Or, at least, will prevent the NullPointerException from that issue from occurring)

bturner · 2018-03-01T04:43:46Z

src/main/java/com/monitorjbl/plugins/UserUtils.java

-        users.addAll(results.stream()
-            .map(ApplicationUser::getSlug)
-            .collect(Collectors.toList()));
-        results = newArrayList(userService.findUsersByGroup(group, new PageRequestImpl(i * RESULTS_PER_REQUEST, RESULTS_PER_REQUEST)).getValues());


So the code was requesting 25 users at a time. If 24 were returned (meaning there wasn't a 25th, and indicating there are no more users), because results.size() would still be >0, you'd do an extra page load trying to go "beyond" the end. When that returned empty, you'd stop.

bturner · 2018-03-01T04:44:33Z

src/main/java/com/monitorjbl/plugins/UserUtils.java

+    return groups.stream()
+        //Replace each group with a stream of the users in that group
+        .flatMap(group ->
+            streamIterable(new PagedIterable<>(


Those code switches to PagedIterable, which automatically applies correct pagination and concatenates the pages together, on demand, into a single Iterable.

bturner · 2018-03-01T04:46:42Z

src/main/java/com/monitorjbl/plugins/config/UserResource.java

    return Response.ok(ImmutableMap.of(
        "requiredReviews", config.getRequiredReviews() == null ? "" : config.getRequiredReviews(),
        "blockMergeIfPrNeedsWork", config.getBlockMergeIfPrNeedsWork() == null ? "" : config.getBlockMergeIfPrNeedsWork(),
-        "requiredReviewers", utils.dereferenceUsers(newArrayList(newHashSet(concat(


This code was loading the dereferencing the groups, which implicitly returns "dereferenced" users by name, and then dereferencing those users again together with any explicitly configured users. I've changed it to not re-dereference users from dereferenced groups. Instead, it only dereferences explicitly configured users.

(Note that this code could, and still can, return duplicates, if a user was in a configured group and configured by name.)

The HashSet was guarding against duplicates. Was there a reason you removed that part?

Ah, of course. You were deduping on the input side, before. The old code didn't have any deduping on the output, but since you eliminated them from the input it wouldn't matter.

I'll re-add the set, but around the user/group call instead of inside. That way group users still don't get dereferenced twice.

Turns out I'd introduced a more subtle breakage than that, in that dereferenceGroups and dereferenceUsers return different types (Strings and Users, respectively). I've fixed that now too.

However, noticing that led me to notice a different bug. Specifically, the Strings returned by dereferenceGroups are user slugs, not user names. But in CommitBlockerHook, they're being compared to UserProfile.getUsername(). That means in any instance where users' names and slugs are not identical, configuring excluded groups will not actually work. So I fixed that too.

The best fix for that is to replace references to SAL's UserManager with Bitbucket Server's AuthenticationContext, which returns an ApplicationUser directly (and allows access to its slug). Using AuthenticationContext also simplifies ConfigServlet and removes the need for UserUtils.getApplicationUserByName, so I removed that too.

bturner · 2018-03-01T04:47:10Z

start-test-server.sh

 #!/bin/bash

-atlas-run -u 6.3.0 --jvmargs "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005"
+atlas-run -u 6.3.14 --jvmargs "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005"


Just updated to match the AMPS version upgrade I applied in your pom.xml.

- Updated AsyncProcessor to look at the Bitbucket Server verson and choose between using the shared ExecutorService (5.9+) or creating its own ExecutorService (previous versions) - Bitbucket Server 5.9 includes a fix which prevents add-ons using the shared BucketedExecutor from blocking hook callbacks - Removed some dead code in AsyncProcessor and PullRequestListener - Simplified Maven structure and reconfigured how integration and unit tests are distinguished - Moved unit tests from src/test/java/ut to src/test/java - Added a simple UserResourceTest which requests configuration for a repository which hasn't had any applied - This verifies that the PR Harmony plugin has been installed and enabled successfully, and that its REST resource is available - Simplified some of the processing in UserUtils to use the API more efficiently and avoid various IDEA inspection warnings

bturner · 2018-03-01T04:52:37Z

I've added a single REST integration test, which effectively verifies that the plugin starts and enables without issue. That means my code changes are structurally correct. However, I don't really have a good setup for actually testing the plugin. I apologize for the hassle, but you'll probably want to verify these changes yourself before you merge.

bturner · 2018-03-01T22:57:47Z

Please hold off on this for a moment; I'm fixing some bugs I introduced (and adding more integration and unit tests as penance for doing so).

- Added more integration tests - ConfigResource and UserResource are both fully tested - CommitBlockerHook is now tested using real pushes, in addition to its existing unit tests - Fixed deduplication for users in UserResource.get's response - Also fixed an unintentional change in the "shape" of the payload where group members would have been returned as simple strings instead of Users - Fixed an issue in CommitBlockerHook where checking for users excluded by groups was comparing usernames against slugs - UserUtils.dereferenceGroups returns _slugs_, not usernames - Also fixed the hook so it doesn't apply its rules to tags - Revised how the hook applies its checks to avoid dereferencing users and groups on every iteration of the loop, even if the ref being tested doesn't match any blocking pattern - Revised the rejection wording to include all blocked branches when a single push attempts to update more than one - Removed RegexUtils.formatBranchName and instead simplified callsites to use MinimalRef.getDisplayId, which already omits refs/heads/ - Updated ConfigServlet to use AuthenticationContext to retrieve the current user instead of the SAL UserManager - This simplifies applying permission checks later - Updated component-imports in atlassian-plugin.xml to ensure every component the add-on uses is explicitly imported

bturner · 2018-03-08T01:11:02Z

Okay, I've pushed all my updates. I've verified my changes against Bitbucket Server 4.8.6 and 5.8.0, the oldest and newest versions the add-on claims to support. All seems well. (However, my validation has been based on my tests, so some things I haven't changed, like pull request auto-merging, haven't been tested. It's likely you'll still want to do your own validation.)

Apologies for the size of this pull request. I've probably gone too far fixing up issues. To try and increase your confidence in accepting my changes, I've added some new unit tests as well as several integration tests. ConfigResource and UserResource now have full REST tests, and CommitBlockerHook has an integration test which configures blocking and then validates handling of actual pushes end-to-end.

If my changes are too broad, you're welcome to pick them apart and use what suits you. I've generally tried to stick with what I perceive to be your code style (two space indenting, etc), but I may not have gotten everything quite right.

There's still one fairly pervasive issue with how the plugin is written that I haven't really tried to fix, and that's the inconsistent use of usernames and user slugs. For example, your User object has a slug field, but what you're actually putting in that field is the user's username, not their slug. I fixed a username/slug mismatch in CommitBlockerHook (which really should be PushBlockerHook, since Bitbucket Server cannot block commits; those are made on developers' workstations and then pushed to Bitbucket Server, at which point it can reject them), but there are almost certainly more. In many systems a user's username and slug are identical, but that's not guaranteed. For example, e-mail addresses (user@example.com) are valid usernames, but the slug for such a user would replace the @ with an _ (user_example.com). You might try creating a user like that and then doing some testing.

bturner · 2018-03-08T01:12:16Z

src/main/java/com/monitorjbl/plugins/CommitBlockerHook.java

-      if(regexUtils.match(config.getBlockedCommits(), branch) && !excluded.contains(user.getUsername())) {
-        hookResponse.err().write("\n" +
-                "******************************\n" +
-                "*    !! Commit Rejected !!   *\n" +


You can, of course, change this back, if you prefer, but it's really inaccurate. Bitbucket Server can't reject commits. They're authored remotely. What this is blocking is the push, preventing the new commits from being applied. It's a subtle distinction, perhaps, but it's an important one.

bturner · 2018-03-08T01:13:46Z

src/main/java/com/monitorjbl/plugins/CommitBlockerHook.java

+
+    //Otherwise, if the user is pushing to at least one branch which does not allow direct commits, check
+    //whether the they've been excluded from the restriction
+    ApplicationUser user = authenticationContext.getCurrentUser();


We need the full ApplicationUser, rather than a SAL UserProfile, because exclusion groups need to be checked against the slug. UserUtils.dereferenceGroups returns a collection of slugs, not usernames. (And it did before I started making changes, just to be clear.)

bturner · 2018-03-08T01:14:11Z

src/main/java/com/monitorjbl/plugins/PullRequestApproval.java

  }

-  public Set<String> missingRevieiwers(PullRequest pr) {
+  public Set<String> missingReviewers(PullRequest pr) {


These two methods were misspelled. (Sorry, personal quirk)

bturner · 2018-03-08T01:16:38Z

src/main/java/com/monitorjbl/plugins/RegexUtils.java

    return false;
  }
-
-  public String formatBranchName(String refspec) {


You should never need to do something like this, in any modern (4.0+) version of the system. Where RefChange in Stash 3.11 and prior only included ref IDs, Bitbucket Server 4.0 replaced that with a MinimalRef, which exposes both the ID (refs/heads/example) and the display ID (just example).

I've removed this method, and updated all the callers to just use getDisplayId() instead.

bturner · 2018-03-08T01:17:46Z

src/main/java/com/monitorjbl/plugins/UserUtils.java


 public class UserUtils {
-  private static final int RESULTS_PER_REQUEST = 25;
+  private static final int RESULTS_PER_REQUEST = 50;


I've made your pages a little bigger to improve performance. Bitbucket Server itself uses 25 as the default for some page sizes, but goes as high as 100 for some things.

bturner · 2018-03-08T01:18:41Z

src/main/java/com/monitorjbl/plugins/UserUtils.java

+        .collect(toSet());
  }

-  public List<String> dereferenceGroups(Collection<String> groups) {


I've changed this to return a Set because that optimizes contains checks

bturner · 2018-03-08T01:19:58Z

src/main/java/com/monitorjbl/plugins/UserUtils.java

        .orElse(username);
  }
-
-  public ApplicationUser getApplicationUserByName(String username) {


With the switch from SAL's UserManager to Bitbucket Server's AuthenticationContext (which is what our implementation for SAL's UserManager uses under the covers), you don't need to do this lookup anymore when checking permissions in ConfigServlet; you already have a full ApplicationUser

bturner · 2018-03-08T01:21:26Z

src/main/resources/atlassian-plugin.xml

+  <component-import key="securityService" interface="com.atlassian.bitbucket.user.SecurityService"/>
+  <component-import key="templateRenderer"
+                    interface="com.atlassian.templaterenderer.velocity.one.six.VelocityTemplateRenderer"/>
+  <component-import key="userService" interface="com.atlassian.bitbucket.user.UserService"/>


I've adjusted your component-imports to ensure every thing the plugin uses is explicitly imported.

Because this is a transformed plugin (we turn it into an OSGi bundle on your behalf when it's installed), the system can automatically fix missing imports in some places. But it's better to just import all your dependencies explicitly, rather than relying on that.

bturner · 2018-03-08T01:22:46Z

src/test/java/com/monitorjbl/plugins/PullRequestListenerTest.java

+  private RegexUtils regexUtils;
+  @Spy
+  @SuppressWarnings("unused") //Used by @InjectMocks
+  private SecurityService securityService = new DummySecurityService();


Using our DummySecurityService makes your test a little simpler; no need for MockSecurityContext and related setup

bturner · 2018-03-08T01:23:29Z

src/test/java/com/monitorjbl/plugins/config/ConfigServletTest.java

  public void testFoundRepo() throws Exception {
-    sut.handleRequest("/plugins/servlet/pr-harmony/PRJ/repo1", "user1", response);
-    verify(response, times(1)).setStatus(HttpServletResponse.SC_OK);
-    verify(renderer, times(1)).render(anyString(), any(Map.class), any(Writer.class));


verify is implicitly times(1)

bturner commented Mar 1, 2018

View reviewed changes

bturner changed the title ~~Use a separate ExecutorService to fix #73.~~ Use a separate ExecutorService to fix #71. Mar 1, 2018

bturner force-pushed the issue-73 branch from b45a931 to 3c9142c Compare March 1, 2018 04:49

bturner mentioned this pull request Mar 1, 2018

Performance issues on build status updates #71

Open

bturner commented Mar 8, 2018

View reviewed changes

Use a separate ExecutorService to fix #71. #72

Are you sure you want to change the base?

Use a separate ExecutorService to fix #71. #72

Uh oh!

Conversation

bturner commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bturner commented Mar 1, 2018

Uh oh!

bturner commented Mar 1, 2018

Uh oh!

bturner commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants