Bitbucket Server 8.0 Early Access Program (EAP) release

RomanStoffel · April 5, 2022, 9:44am

Thanks @bturner for the details

Indeed, when creating a file and commit it, the publish API does work for publishing the new commit.

It looks like it depends on the previous command we execute to get the repo in the right state.
We do:
git checkout -b COMMIT_SHA branch-name

Or in code:

GitWorkTreeBuilderFactory factory = ...
factory.builder(repo)
        .alternate(source)
        .checkoutType(GitCheckoutType.NONE)
        .execute(worktree-> doGitOperations(worktree, sourceBranch));
       .execute(worktree -> {

                        // Works in Bitbucket 7, but not 8
            String checkoutResult = workTree.builder()
                    .author(author)
                    .command("checkout")
                    .argument("-b")
                    .argument(source.getId())
                    .argument(source.getLatestCommit())
                    .build(new SingleLineOutputHandler())
                    .call();
      
        String noExistingCommit = "0000000000000000000000000000000000000000";
        workTree.publish(new PublishGitWorkTreeParameters.Builder(invoker)
                .branch("newly-published-branch", noExistingCommit)
                .build());
     }

Here is the code to demonstrate the issue:
https://bitbucket.org/RomanStoffel/bb8-worktree-issue/src/86bf5ae345454058ffa60e05da0cd0f361470dd0/src/main/java/ch/mibex/gitworktree/reproduce/api/TestGitWorktree.java#lines-91

It can be triggered by invoking the servlet:

curl -X POST --user admin:admin 'http://localhost:7990/bitbucket/plugins/servlet/mibex-git-worktree/project-admin/?proj=PROJECT_1&source=rep_1&target=test-repo'

Where there is a ‘source=’ is the source repo from where the latest commit is used, and ‘target’ is the empty repo.

From the Bitbucket mesh logs we get only this line:

2022-04-05 11:44:07,789 WARN  [grpc-server:thread-5] admin 4T2JO83Qx704x26x2 @19Y6H0Wx704x24x0 127.0.0.1 "PlumbingService/CallStdin" (>1 <0) c.a.b.mesh.git.GrpcPlumbingService 'git checkout' is deprecated is not supported on remote repositories. Update the feature to use the higher level APIs if possible.

Not sure about this warning, we seem to get that one for all operations?

bturner · April 5, 2022, 11:01am

@RomanStoffel,

The work tree API expects to handle the checkout itself, which is why workTree.builder() doesn’t have a type-safe checkout() sub-builder. On your GitWorkTreeBuilder, alongside the checkoutType, pass commit(source.getLatestCommitId()). (If it’s an empty repository and you’re going to create the first commit, either don’t call commit at all or call commit(null) to tell the work tree underpinnings that you don’t want anything checked out.) You don’t need to set up a branch name; the work tree underpinnings know what branch is being updated. publish takes the target branch you want to update, but it figures out the source by itself. If you did happen to need to refer to it, use "HEAD".

(And with that change, the warning about not using git checkout will go away, because Mesh will manage it for you.)

Best regards,
Bryan Turner
Atlassian Bitbucket

RomanStoffel · April 5, 2022, 12:36pm

@bturner

Thanks for the answer. Unfortunately, I’m still doing something wrong. I used the .commit() for the checkout, and then did no further git operations. It still fails with the same error.

Note that the commit comes from the ‘alternative’ repo. It seems that is the source of the crash:

            worktrees.builder(targetRepo)
                    .alternate(sourceRepo)
                    .checkoutType(GitCheckoutType.NONE)
                    .commit(sourceBranch.getLatestCommit()) // Commit from .alternate(sourceRepo).
                    .execute(worktree ->{

                       String noExistingCommit = "0000000000000000000000000000000000000000";
                      workTree.publish(new PublishGitWorkTreeParameters.Builder(invoker)
                         .branch("newly-published-branch", noExistingCommit)
                         .build());
                });

Source: Bitbucket

So, the overall trigger of the crash seems to be that the commits / content come from the alternative repo.

Maybe, I take a step back and explain what we want to do. Maybe we took the wrong path. Basically we want to ‘clone’ content from a source repo to target repo.

Something like in git CLI:

git clone {source-repo}
git push {to-target-repo} {selection-of-multiple-branches}

And in other instances we want to take a Git tree from a source repo and push only that.

git clone {source-repo}
COMMIT_SHA = $(git commit-tree {TREE_SHA} -m "initial-commit") # Create a new commit, with the exiting tree, but with no history.
git checkout -b $COMMIT_SHA new-branch-name # Create that as a branch
git push {to-target-repo} new-branch-name # Push it.

So, we don’t need an actual ‘work-tree’ to edit files. We need to be able to take commits from a source repo and push them to another repo. So maybe there is a better API for that.

bturner · April 6, 2022, 9:40am

@RomanStoffel,

Apologies for the slow response; I’m juggling too many things.

For the second one (the commit-tree one), try replacing git checkout -b with git update-ref HEAD <SHA>. You don’t really need (or want) to check out the commit you created, and you don’t need to create random branches in the work tree. The branches that exist have nothing to do with what’s published; you pass the ref you want to update in the target repository explicitly when you call publish. Updating HEAD (which will be a symref in the work tree to a refs/heads/master branch; the name of the branch doesn’t really matter to the work tree internals) in the work tree is going to control what commit(s) get published to whatever branch you target.

Also, for that task, you won’t want to pass commit(sourceBranch.getLatestCommit()) to the GitWorkTreeBuilder since you don’t actually care what commit the work tree starts at (given you’re trying to create an initial commit).

The first one, you may want to consider whether you can move away from using the work tree API at all. I’d invert your approach there and instead of trying to git clone and then git push, I’d replace both with a git fetch <source> <refspecs for branches> executed in the target repository. You can still build git fetch commands directly via GitScmCommandBuilder. (In general I’d encourage app developers to always fetch instead of pushing.) I don’t know whether you want PreRepositoryHooks and PostRepositoryHooks to run for the ref updates you’re doing as part of your clone+push-now-implemented-as-fetch. If you don’t, then that’s how it works “for free”. If you do, you’ll need to use RepositoryHookService yourself to trigger them. For your refspecs you pass to git fetch, if you don’t pass a target ref (e.g. git fetch <source> refs/heads/example vs refs/heads/example:refs/heads/example) the fetch won’t update any refs and, instead, will update FETCH_HEAD and write its updates to stderr (I think; it may be stdout). You can use that output to build your own RefChanges to trigger hooks.

I need to tinker a bit more locally and see what I can get working, conceptually, given those scenarios. I’ll get back to you when I can, unless you tell me you’ve got it all working.

Best regards,
Bryan Turner
Atlassian Bitbucket

YannisArgyropoulos · April 6, 2022, 12:41pm

Hi,
Using the old ScmCommandBuilder with 8.0.0-eap05 I noticed that the mergeBase() method is not returning a common ancestor, the following scrpt will return null, while it used to work with bitbucket 7.0.0 to 7.21.0

def mergeBase = gitCommandBuilderFactory.builder(repository)
            .mergeBase()
            .between(pullRequest.fromRef.id, pullRequest.toRef.id)
            .exitHandler(new ErrorRecordingCommandExitHandler(i18nService, recorder))
            .build(new StringCommandOutputHandler()).call().trim()

The merge-base command is not listed among the deprecated methods, so I had the impression it would keep working.

Should I be able to keep using the old api for ‘merge-base’, or should I switch to WorkTree API for this?

Many thanks,
Yannis

RomanStoffel · April 6, 2022, 1:57pm

@bturner Thanks for your help. We are getting closer.

The ‘fetch’ replacement for the ‘push’ approach does work. Thanks a lot. No GitWorkingTree required, so that is even better.

Update/Edit: I could make the ‘commit-tree’ scenario work. If it do not use GitWorkingTreeBuilder.alternative(sourceRepo), and do a manual fetch, then I can make it work:

        // Missing: A way to limit the fetching, like --depth 1
        String fetchResult =  workTree.builder()
                .fetch()
                .repository(sourceRepo)
                .refspec(source.getId())
                .build(new SingleLineOutputHandler())
                .call();
        String commitHash = workTree.builder()
                .author(author)
                .command("commit-tree")
                .argument(source.getLatestCommit() + "^{tree}")
                .argument("-m")
                .argument("initial commit")
                .build(new SingleLineOutputHandler())
                .call();

        workTree.builder()
                .updateRef()
                .set("HEAD", commitHash)
                .build()
                .call();

        String noExistingCommit = "0000000000000000000000000000000000000000";
        workTree.publish(new PublishGitWorkTreeParameters.Builder(invoker)
                .author(author)
                .branch("newly-published-branch", noExistingCommit)
                .build());

Wonderful.
It feels like that GitWorkingTreeBuilder.alternative() is a trap at the moment. It always causes crashes for me.

There is one improvement left. I don’t really need to ‘fetch’ everything for the operation.
But looks like there is not ‘–depth’ argument for the fetch? I do not need the commit history, as we throw it away right away.

Thanks again for all the help,
I’ll experiment a bit more =)

bturner · April 6, 2022, 9:18pm

@RomanStoffel,

We’ve finally hit a point where I got all the building blocks I needed to root cause what’s going on. It’s a bug in some Mesh code, and only triggered when alternates are present. I’ll get a fix up for review internally today, and it should be present when the final 8.0.0 build is released. With that fix, you’ll definitely want to continue using the alternates-based approach rather than fetching. (While the fetch works, it’s not something I think we’ve coded to account for very well and it’s going to end up moving around a lot more objects than you might expect–or want.)

Best regards,
Bryan Turner
Atlassian Bitbucket

bturner · April 6, 2022, 9:29pm

@YannisArgyropoulos,

merge-base is not deprecated and will not be removed. You definitely should not use a GitWorkTree for that. We also haven’t changed anything about how merge-base works, which implies there must be more to your situation than you’re showing. We’ve got extensive unit testing around merge-base, and our own pull request internals use it extensively. For example, we have this test, which passes on every version of Git we support:

    @Test
    public void testMergeBase() {
        Set<String> mergeBases = builderFactory.builder(repository).mergeBase()
                .between("branch_that_has_same_text_file_modified_on_both_trgt",
                        "branch_that_has_same_text_file_modified_on_both_src")
                .build(new MergeBaseStdoutHandler())
                .call();
        assertNotNull(mergeBases);
        assertEquals(1, mergeBases.size());
        assertEquals("030da685a756bba584b0a0165cf618eda9364146", Iterables.getOnlyElement(mergeBases));
    }

Just off the top of my head, looking at the minimal snippet you’ve posted:

You shouldn’t use fromRef.id and toRef.id; you should use fromRef.latestCommit and toRef.latestCommit
For a cross-repository pull request, your code won’t work. You need to add an alternate(fromRef.repository) in your builder chain (after between and before build)
What does your ErrorRecordingCommandExitHandler do? Is it possible it’s suppressing some sort of error or non-zero exit from merge-base?
You actually need handling for the case where a pull request’s refs are not related. I fixed BSERV-12429 in Bitbucket Server 7.21, so it’s entirely possible the branches involved in a pull request actually don’t have a merge-base

Rather than running merge-base yourself, the same way you’re injecting GitCommandBuilderFactory, you can also inject GitCommandFactory and call its commonAncestor command. You can use fromRef.latestCommit and toRef.latestCommit as the IDs, and pass fromRef.repository as your secondaryRepository. As noted above, though, the result of the returned GitCommand may be null if the pull request refs don’t share a common ancestor.

If that doesn’t help get you moving forward, please provide me with a minimal reproduction that I can use to debug it.

Best regards,
Bryan Turner
Atlassian Bitbucket

YannisArgyropoulos · April 7, 2022, 4:22pm

Thank you @bturner this helped a lot!

You were right, there was more to my situation. Turns out that our output handler was throwing abstract method exceptions, related to the change in CommandOutputHandler
I replaced it with SingleLineOutputHandler and I’m getting back the expected results.

Thank you very much for your quick and very detailed reply, much appreciated!

Best regards,
Yannis

YuriKarnilaev · April 8, 2022, 12:08pm

Hello,

Thanks for the Bitbucket 8 EAP. I have a question about this release.

In our app Submodule Changes for Bitbucket we are using the following endpoint in the branch compare diff context to show submodule changes:
https://{bitbucketHost}/rest/api/latest/projects/{projectName}/repos/{repoSlug}/compare/changes?{params}
In the Bitbucket 8 EAP this endpoint was changed. The part /rest/api/latest was missed from the URL. It looks like this for now:
https://{bitbucketHost}/projects/{projectName}/repos/{repoSlug}/compare/changes?{params}

The new URL doesn’t work when I open it in a browser but works inside the Bitbucket:

There are similar endpoints in the pull requests diff context:
https://{bitbucketHost}/rest/api/latest/projects/{projectName}/repos/{repoSlug}/pull-requests/{pullRequestId}/changes?{params}
and in the commit diff context:
http://{bitbucketUrl}/rest/api/latest/projects/{projectName}/repos/{repoSlug}/commits/{commitHash}/changes?{params}
They have not lost /rest/api/latest part.

Could you please say, is it accidentally that the endpoint from the branch compare diff context has lost /rest/api/latest part, or is it intentionally?

The format of these URLs is very important for our app.

bturner · April 8, 2022, 6:48pm

@YuriKarnilaev,

8.0 includes some UI improvements for compare, updating it to use the same React-based diff view that our pull requests have used since 7.0, but no URLs were updated and no REST endpoints have been removed. Have you checked your server logs and verified some other app or customization is not interfering?

The version of the compare URL without rest/api/latest is the web UI for compare, mapped to a. SpringMVC @Controller, and has always been at that URL. There is no changes URL for the web UI, which is why you get an HTML-rendered 404–that’s an invalid URL, and the error page is rendered for user viewing.

$BASE_URL/rest/api/latest/project/{key}/repos/{slug}/compare/changes is provided by CompareResource, part of our bitbucket-rest bundled plugin, exists unchanged in 8.0. I’ve confirmed this with Git:

bturner@C02C701AMD6T server % git diff bitbucket-parent-7.21.0 bitbucket-parent-8.0.0-eap05 -- plugins/rest/src/main/java/com/atlassian/stash/internal/rest/compare
bturner@C02C701AMD6T server %

As you can see, there are literally no modifications between the 7.21 tag and the EAP5 tag, for the compare REST endpoints. (There are changes that will be included in the final 8.0 release, but they’re just source changes related to replacing our old REST doclet with Swagger for our REST documentation.) Further, I have set up EAP 5 locally and am able to use compare, both via the UI and via REST, without issue.

I’ll note that if you look at XHR requests in the Developer Tools for your browser when using compare, you may observe that the compare UI makes requests to the same compare URL as the UI page–e.g., without /rest/api/latest. That’s because we have an evil, legacy URL rewrite in place that detects requests with Accept: application/json directed at our SpringMVC controllers and internally redirects them to /rest/api/latest:

<rule>
    <!-- Rewrite things that would normally go to /mvc to /rest/api/latest if they are looking for json. -->
    <condition type="header" name="Accept" next="or">application/json</condition>
    <condition type="header" name="Accept" next="or">application/x-json</condition>
    <condition type="header" name="Accept" next="or">text/json</condition>
    <condition type="header" name="Accept">text/x-json</condition>
    <from>^/((?!s/|mvc|rest|plugins|download|status|test|git).+)$</from>
    <to last="true">/rest/api/1.0/$1</to>
</rule>

Under the hood, it’s the same CompareResource REST endpoint that gets hit either way.

Whatever behavior you’re seeing, it’s not a change in the product. There must be something more going on.

Best regards,
Bryan Turner
Atlassian Bitbucket

UlrichKuhnhardtIzym1 · April 11, 2022, 1:40am

Thanks @rlander - after upgrading local git version to 2.35 atlas-debug works out of the box!

UlrichKuhnhardtIzym1 · April 11, 2022, 5:45am

@khughes @bturner

[UPDATE: Once the escalated permission is REPO_READ the code retrieves correct build results, ]

I see the access denied when accessing build results as LICENSED USER

[INFO] 2022-04-11 15:38:34,190 DEBUG [threadpool:thread-15]  c.a.s.i.c.StateTransferringCallable Error while processing asynchronous task
[INFO] com.atlassian.bitbucket.AuthorisationException: You are not permitted to access this resource
[INFO] 	at com.atlassian.stash.internal.aop.ExceptionRewriteAdvice.afterThrowing(ExceptionRewriteAdvice.java:37)
[INFO] 	at com.atlassian.bitbucket.internal.build.status.DefaultBuildStatusService.validateSearchRequest(DefaultBuildStatusService.java:614)
[INFO] 	at com.atlassian.bitbucket.internal.build.status.DefaultBuildStatusService.searchInternal(DefaultBuildStatusService.java:353)
[INFO] 	at com.atlassian.bitbucket.internal.build.status.DefaultBuildStatusService.search(DefaultBuildStatusService.java:322)
[INFO] 	at com.atlassian.plugin.util.ContextClassLoaderSettingInvocationHandler.invoke(ContextClassLoaderSettingInvocationHandler.java:26)
[INFO] 	at org.eclipse.gemini.blueprint.service.importer.support.internal.aop.ServiceInvoker.doInvoke(ServiceInvoker.java:56)
[INFO] 	at org.eclipse.gemini.blueprint.service.importer.support.internal.aop.ServiceInvoker.invoke(ServiceInvoker.java:60)
[INFO] 	at org.eclipse.gemini.blueprint.service.util.internal.aop.ServiceTCCLInterceptor.invokeUnprivileged(ServiceTCCLInterceptor.java:70)
[INFO] 	at org.eclipse.gemini.blueprint.service.util.internal.aop.ServiceTCCLInterceptor.invoke(ServiceTCCLInterceptor.java:53)
[INFO] 	at org.eclipse.gemini.blueprint.service.importer.support.LocalBundleContextAdvice.invoke(LocalBundleContextAdvice.java:57)
[INFO] 	at com.izymes.workzone.listener.BuildEventListener$1.perform(BuildEventListener.java:179)
[INFO] 	at com.izymes.workzone.listener.BuildEventListener$1.perform(BuildEventListener.java:170)
[INFO] 	at com.atlassian.stash.internal.user.DefaultEscalatedSecurityContext.call(DefaultEscalatedSecurityContext.java:59)

Workzone code looks like

UncheckedOperation<Page<RepositoryBuildStatus>> op = () -> {
  final Page<RepositoryBuildStatus> buildStatusPage = 
    repositoryBuildStatusService.search(searchRequest,
          new PageRequestImpl(0, 256));
}
securityService.withPermission(
Permission.LICENSED_USER, 
"Escalated permission for anonymous 2LO"
).call(op)

This used to work in 7.x

See also [BSERV-12101] PullRequestService.searchByCommit should require same permission as PullRequestService.search - Create and track feature requests for Atlassian products. for a similar issue in the past.

Workzone scans build results in a scheduled job context, without an authenticated user. It needs to be able to search for build status in this context.

YuriKarnilaev · April 11, 2022, 7:44am

@bturner, thank you for your answer!

Submodule Changes for Bitbucket uses requests from the Bitbucket frontend to trigger submodule diff calculation.

There is a difference between URL composing to get changes in the sources of Bitbucket 8.0.0-eap05.
For commit changes diff plugins/frontend/src/feature/commit/commit-changes/commit-changes.jsx:47:

const changesUrlBuilder = useMemo(
    () =>
        nav
            .rest()
            .repository(repository)
            .commits()
            .addPathComponents(commit.id, 'changes')
            .withDefinedParams({ since: activeParent && activeParent.id }),
    [repository, commit.id, activeParent]
);

For pull request changes diff plugins/frontend/src/feature/pull-request/pull-request-changes/pull-request-changes.jsx:15:

const urlBuilder = useMemo(() => {
    let builder = nav.rest().pullRequest(pullRequest).changes().withParams({
        avatarSize: AvatarSize.DEFAULT,
        markup: true,
    });

    if (commit) {
        const sinceId = sinceCommitId ? sinceCommitId : get(commit, 'parents.0.id');

        builder = builder.withParams({
            changeScope: 'RANGE',
            untilId: commit.id,
            sinceId,
        });
    }

    return builder;
}, [commit, sinceCommitId, pullRequest]);

For branch compare changes diff plugins/frontend/src/feature/compare/compare-changes/compare-changes.jsx:17

const changesUrlBuilder = useMemo(
    () =>
        nav.repository(targetRepo).compare().changes().withDefinedParams({
            to: targetRef.latestCommit,
            fromRepo: sourceRepo.id,
            from: sourceRef.latestCommit,
        }),
    [targetRepo, targetRef, sourceRepo, sourceRef]
);

There is no .rest() part in the branch compare changes diff.

Is it possible to standardize all the places where Bitbucket composes changes diff URL and add the .rest() part to the branch compare changes diff URL forming code?

bturner · April 11, 2022, 8:21am

@YuriKarnilaev,

It’s not a part of the product I work with. The URLs likely aren’t standardized because for how the app works they don’t need to be. The request without rest() will still end up hitting the /rest/api/1.0 URL because the Accept: application/json rewrite on the server-side will send it there, so the UI logic all ends up working as expected.

I’ll forward this comment to a few people internally who actually worked on the changes to the commit and compare diffs (updating them to use the pull request React code) and they’ll say what they say.

Best regards,
Bryan Turner
Atlassian Bitbucket

bturner · April 11, 2022, 8:28am

@UlrichKuhnhardtIzym1,

I’m not across any changes to the build status logic. Kristy is out on leave, but I’ll forward your question internally to some folks who have worked in that area. Hopefully one of them can reply.

One thing your question doesn’t show is what searchRequest's type is; search has two overloads, one for BuildStatusRepositorySearchRequest and one for BuildStatusPullRequestSearchRequest.

With that said, I wouldn’t expect LICENSED_USER to have ever been enough here. The RepositoryBuildStatusResource that exposes this over REST escalates to REPO_READ for anonymous 2LO, for example. I suspect the answer is going to end up being that REPO_READ is what your app should be using (and likely should always have been using), but we’ll see what they say.

Best regards,
Bryan Turner
Atlassian Bitbucket

UlrichKuhnhardtIzym1 · April 12, 2022, 1:03am

@khughes , @bturner - here is the snippet how we build the search request.

      final String latestCommit = pullRequest.getFromRef().getLatestCommit();
      BuildStatusPullRequestSearchRequest searchRequest = new BuildStatusPullRequestSearchRequest
          .Builder(pullRequest)
          .commitId(latestCommit)
          .buildOrder(BuildOrder.NEWEST)
          .build();

CarlosSanchezRosa · April 12, 2022, 3:54am

For this particular case it will work with or without .rest() , there is no reason why not to explicitly use the .rest() , I agree it will be better to standardize it since it is only doing a request to the API to get the changes and we could explicitly show that from the code, improving the code readability, by not using .rest() it leaves some room for interpretation, like thinking the URL could be use for some other reasons like linking to another FE page.

Some weeks ago I worked on the navbuilder library that generates that URL and we removed a lot of its deprecated functionality, I will create ticket to apply this fix, and do some work on finding other places it is used in different ways.

CarlosSanchezRosa · April 12, 2022, 4:57am

This issue will be fixed with 8.0

MarkusSutterMibexSof · April 12, 2022, 1:04pm

Is the source code available for the Bitbucket 8 EAP for vendors?
It would help as a lot in the transition to CSE for the PR create page changes.
For instances what are the required arguments for setReviewers for the Extension point: bitbucket.ui.create.pullrequest.form
?

Thanks.