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

Use a separate ExecutorService to fix #71. #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bturner
Copy link

@bturner bturner commented Mar 1, 2018

  • 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

<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.atlassian.templaterenderer</groupId>
<artifactId>atlassian-template-renderer-api</artifactId>
<version>1.5.7</version>
Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

<dependency>
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>commons-lang</groupId>
<artifactId>commons-lang</artifactId>
Copy link
Author

Choose a reason for hiding this comment

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

This was unused

@@ -99,47 +114,9 @@
</exclusions>
<scope>test</scope>
</dependency>

<!-- WIRED TEST RUNNER DEPENDENCIES -->
Copy link
Author

Choose a reason for hiding this comment

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

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.

</dependencies>

<build>
<testSourceDirectory>${project.basedir}/src/test/java/ut</testSourceDirectory>
Copy link
Author

Choose a reason for hiding this comment

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

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.)

<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>
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

}

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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)

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());
Copy link
Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

@@ -31,12 +30,12 @@ public Response get(@PathParam("projectKey") String projectKey, @PathParam("repo
return Response.ok(ImmutableMap.of(
"requiredReviews", config.getRequiredReviews() == null ? "" : config.getRequiredReviews(),
"blockMergeIfPrNeedsWork", config.getBlockMergeIfPrNeedsWork() == null ? "" : config.getBlockMergeIfPrNeedsWork(),
"requiredReviewers", utils.dereferenceUsers(newArrayList(newHashSet(concat(
Copy link
Author

Choose a reason for hiding this comment

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

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.)

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

@@ -1,3 +1,3 @@
#!/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"
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks!

- 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 bturner changed the title Use a separate ExecutorService to fix #73. Use a separate ExecutorService to fix #71. Mar 1, 2018
@bturner
Copy link
Author

bturner commented Mar 1, 2018

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
Copy link
Author

bturner commented Mar 1, 2018

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
Copy link
Author

bturner commented Mar 8, 2018

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 ([email protected]) 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.

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

Choose a reason for hiding this comment

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

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.


//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();
Copy link
Author

Choose a reason for hiding this comment

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

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.)

@@ -29,7 +28,7 @@ public boolean isPullRequestApproved(PullRequest pr) {
return requiredReviews == null || seenReviewers(pr).size() >= requiredReviews;
}

public Set<String> missingRevieiwers(PullRequest pr) {
public Set<String> missingReviewers(PullRequest pr) {
Copy link
Author

Choose a reason for hiding this comment

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

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

@@ -16,8 +14,4 @@ public boolean match(List<String> regexes, String value) {
}
return false;
}

public String formatBranchName(String refspec) {
Copy link
Author

Choose a reason for hiding this comment

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

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.


public class UserUtils {
private static final int RESULTS_PER_REQUEST = 25;
private static final int RESULTS_PER_REQUEST = 50;
Copy link
Author

Choose a reason for hiding this comment

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

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.

}

public List<String> dereferenceGroups(Collection<String> groups) {
Copy link
Author

Choose a reason for hiding this comment

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

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

}

public String getUserDisplayNameByName(String username) {
return ofNullable(userService.getUserByName(username))
.map(ApplicationUser::getDisplayName)
.orElse(username);
}

public ApplicationUser getApplicationUserByName(String username) {
Copy link
Author

Choose a reason for hiding this comment

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

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

<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"/>
Copy link
Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

}

@Test
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));
Copy link
Author

Choose a reason for hiding this comment

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

verify is implicitly times(1)

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.

2 participants