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

Bitbucket Cloud PR Decoration - Annotation summary is displaying a URL instead of the user message #237

Merged
merged 1 commit into from
Oct 24, 2020

Conversation

ksl67
Copy link
Contributor

@ksl67 ksl67 commented Sep 6, 2020

The message displayed in the annotation on the pull request is a URL which points to the issue in SonarQube.

This is because the summary field of the annotation body is serialising link. See screenshot.

public class CloudAnnotation extends CodeInsightsAnnotation {
    @JsonProperty("external_id")
    private final String externalId;
    @JsonProperty("summary")
    private final String link;
    @JsonProperty("annotation_type")
    private final String annotationType;

Is this intentional? Would it not be better to serialise the message instead? Then the analysis message is visible directly in the PR.

Furthermore, I can't see anywhere in the Bitbucket API where message is specified in annotation body.

public class CodeInsightsAnnotation {
    @JsonProperty("line")
    private final int line;
    @JsonProperty("message")
    private final String message;

Software Versions

  • SonarQube Version: 8.1
  • Plugin Version: [1.4.0]
    Screenshot 2020-08-23 at 11 23 38

@ksl67 ksl67 marked this pull request as ready for review September 6, 2020 09:43
@ksl67
Copy link
Contributor Author

ksl67 commented Sep 6, 2020

See issue #233.

Copy link
Contributor

@marvin-w marvin-w left a comment

Choose a reason for hiding this comment

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

Looks good for me!

@mc1arke mc1arke merged commit ff725c6 into mc1arke:master Oct 24, 2020
@mc1arke
Copy link
Owner

mc1arke commented Oct 24, 2020

Merged. Thanks for the contribution!

@mc1arke mc1arke added the backport candidate This feature or fix should be included in another release branch label Oct 26, 2020
@@ -26,7 +26,7 @@
public class CodeInsightsAnnotation {
@JsonProperty("line")
private final int line;
@JsonProperty("message")
Copy link

@tobilarscheid tobilarscheid Oct 29, 2020

Choose a reason for hiding this comment

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

@mc1arke @ksl67 @marvin-w unfortunately since this change PR decoration fails with the following stracktrace for me:

sonarqube    | com.github.mc1arke.sonarqube.plugin.ce.pullrequest.bitbucket.client.BitbucketException: The field 'message' is required
sonarqube    | 	at com.github.mc1arke.sonarqube.plugin.ce.pullrequest.bitbucket.client.BitbucketServerClient.validate(BitbucketServerClient.java:189)
sonarqube    | 	at com.github.mc1arke.sonarqube.plugin.ce.pullrequest.bitbucket.client.BitbucketServerClient.uploadAnnotations(BitbucketServerClient.java:114)
sonarqube    | 	at com.github.mc1arke.sonarqube.plugin.ce.pullrequest.bitbucket.BitbucketPullRequestDecorator.updateAnnotations(BitbucketPullRequestDecorator.java:153)
sonarqube    | 	at com.github.mc1arke.sonarqube.plugin.ce.pullrequest.bitbucket.BitbucketPullRequestDecorator.decorateQualityGateStatus(BitbucketPullRequestDecorator.java:98)
sonarqube    | 	at com.github.mc1arke.sonarqube.plugin.ce.pullrequest.PullRequestPostAnalysisTask.finished(PullRequestPostAnalysisTask.java:160)

The Code Insights API expects the field to be called message: https://docs.atlassian.com/bitbucket-server/rest/7.7.0/bitbucket-code-insights-rest.html#idp12

Maybe an additional field for summary is needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a difference between Cloud and Server that went unnoticed in this PR - the Cloud API description lists the summary property, but not message property: https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Bworkspace%7D/%7Brepo_slug%7D/commit/%7Bcommit%7D/reports/%7BreportId%7D/annotations

mossroy added a commit to mossroy/sonarqube-community-branch-plugin that referenced this pull request Nov 27, 2020
This is a backport of mc1arke#237 on branch 1.3 (for SonarQube 7.8 to 8.0)
@mossroy
Copy link

mossroy commented Nov 27, 2020

Many thanks for this fix.
I see there is a "backport candidate" label on this PR, but it doesn't seem to be backported for now
I think it would deserve to be included in a version 1.3.3, so that it works properly on the latest LTS version of SonarQube (7.9.x)

I have done this backport on branch release/1_3 (see https://github.com/mossroy/sonarqube-community-branch-plugin/tree/release/1_3 ), and tested it on SonarQube 7.9.5 : it works great.
Should I submit another PR for this backport?

@marvin-w
Copy link
Contributor

Yes, please create a PR 👍

@mc1arke
Copy link
Owner

mc1arke commented Nov 27, 2020

@mossroy You don't need to raise a pull request - I'll cherry-pick the commits when I come to do a release 😄

@marvin-w
Copy link
Contributor

Well or that :) thanks!

@mossroy
Copy link

mossroy commented Nov 27, 2020

I've just created PR #277 : feel free to close it if you prefer to cherry-pick. It's indeed the exact same changes, but on branch release/1_3

mc1arke pushed a commit that referenced this pull request Jan 23, 2021
In #237 the property names of the annotation model were changed to fix a bug with how code annotations are reported to Bitbucket Cloud. This PR however did not take into consideration that the previous naming was actually correct and required for Bitbucket Server. With this change Bitbucket Cloud will continue to receive the message as summary (API docs), while Bitbucket Server receives the message as message again (API docs).

The @JsonProperty annotations have been moved to the getters so that they support overrides, as mixing property-level and method-level annotations does not work for this use case. Technically this is only needed for the message property, but for consistency reasons this has been done for all fields and has been verified that this is mapped correctly for both cloud and server.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidate This feature or fix should be included in another release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants