Skip to content

GraphQL createDiffNote forces clients to compute redundant information

Summary

The API requires the client to send (and often compute) redundant information that is available in the API.

I believe this is the case for REST API as well.

Example

The most obvious example is when creating a note on an unchanged line, the client needs to compute the position of the line on both sides of the diff. On the following image, you can see that the unchanged line has index 134 on the old diff and 149 on the new diff.

image

The createDiffNote requires both of those lines even though one should be sufficient. The API anyway calculates the other line because it fails validation if they don't match.

DiffPositionInput argument for the createDiffNote mutation

Property Type Description
headSha String! SHA of the HEAD at the time the comment was made.
baseSha String Merge base of the branch the comment was made on.
startSha String! SHA of the branch being compared against.
paths DiffPathsInput! The paths of the file that was changed. Both of the properties of this input are optional, but at least one of them is required
oldLine Int Line on start SHA that was changed.
newLine Int Line on HEAD SHA that was changed.

DiffPathsInput

Property Type Description
oldPath String The path of the file on the start sha.
newPath String The path of the file on the head sha.

Steps to reproduce

I use https://gitlab.com/-/graphql-explorer to run the queries.

Mutation requires both paths

When the user creates a note on the new part of the diff, only the new path should be required (and vice versa).

mutation CreateDiffNote6 {
  createDiffNote(
    input: {
      noteableId: "gid://gitlab/MergeRequest/87196674"
      body: "new note"
      position: {
        baseSha: "5e6dffa282c5129aa67cd227a0429be21bfdaf80"
        headSha: "a2616e0fea06c8b5188e4064a76d623afbf97b0c"
        startSha: "5e6dffa282c5129aa67cd227a0429be21bfdaf80"
        paths: { newPath: "test.ts" }
        newLine: 15
      }
    }
  ) {
    errors
    note {
      id
    }
  }
}

(notice only the newPath being present)

This results in 500 Internal Server Error. It only works when we change the paths to paths: { newPath: "test.ts", oldPath: "test.js" } (specifying both paths).

Forces user to compute both line indexes for unchanged lines:

mutation CreateDiffNoteNonChanged {
  createDiffNote(
    input: {
      noteableId: "gid://gitlab/MergeRequest/87196674"
      body: "new note"
      position: {
        baseSha: "5e6dffa282c5129aa67cd227a0429be21bfdaf80"
        headSha: "a2616e0fea06c8b5188e4064a76d623afbf97b0c"
        startSha: "5e6dffa282c5129aa67cd227a0429be21bfdaf80"
        paths: { oldPath: "test.js", newPath: "test.ts" }
        oldLine: 17
      }
    }
  ) {
    errors
  }
}

fails with message

{
  "data": {
    "createDiffNote": {
      "errors": [
        "Line code can't be blank",
        "Line code must be a valid line code"
      ]
    }
  }
}

Only adding the newLine: 18 fixes the issue. The client needs to keep track of the fact that the new line index has been shifted by 1

Example Project

I use viktomas/test-project!5 (diffs)

Output of checks

This bug happens on GitLab.com

Possible fixes

Edited by 🤖 GitLab Bot 🤖