Multiple flaws in "Discussions API / Create new merge request thread"

Summary

The ""Discussions API / Create new merge request thread" has multiple flaws - both design and implementation. More precisely (item numbers are used later in the ticket to refer to specific issues

  1. API requires excessive parameters that may be uniquely concluded from other parameters
  2. API doesn't properly use some of its parameters
  3. API return competely useless error messages in case of errors and sometimes results in the "%00 - internal Server Error"

Steps to reproduce

May be reproduced on any merge request containing added/deleted/modified files.

Example Project

What is the current bug behavior?

To start a discussion thread related to the specific place in the file you need to specify both new and old paths to file and for lines unchanged in this merge request both old and new number. Which values have to be specified is not always obvious (see #35935 (closed)). As the mandatory API attributes, merge request IID and the SHA-triplet (base, head and start SHA), uniquely identify the merge request and its state (including a state of each affected file), no need to specify both new and old paths exist - for added files only "new path" makes sense, for deleted files only "old path", for all others each of them may be safely concluded from other. The same holds true for line numbers.

  1. and 3. As the merge_request_iid attribute uniquely specifies the (position[base_sha], position[head_sha], position[base_sha] triplet corresponding to the current state of the merge request, such triplet, passed in an API request may be used in one of two ways
  • either to ensure that the discussion thread to be created is intended for the current state of the merge request. In such case the API request shall fail on mismatch with a clear error message (like "400 Bad request - Passed SHA-s do not match the merge request state)
  • or to match the discussion thread to the current or one of the previous merge request state,. If not such matcb is found, a clear error message shall be returned. At the time being the situation is different - some request with wrong SHA (e,g, position[base_sha] in the past) succeed, some produce position
{
    "message": "500 Internal Server Error"
}

and some (pretty useless)

{
    "message": "400 (Bad request) \"Note {:line_code=>[\"can't be blank\", \"must be a valid line code\"]}\" not given"
}

All of it depending of the correctness of other API request attributes (position[new_path], position[old_path], position[new_line], position[old_line]). BTW, messages from the close related API (Discussions API / Create new commit thread) are much better, e.g.

{
    "message": "400 (Bad request) \"Note {:commit_id=>[\"does not match the diff refs\"], :line_code=>[\"can't be blank\", \"must be a valid line code\"]}\" not given"
}

What is the expected correct behavior?

Predictable behavior, clear and useful error messages, no necessity to pass attributes that are uniquely identified by other passed attributes. See as well #35935 (closed)

Relevant logs and/or screenshots

(Paste any relevant logs - please use code blocks (```) to format console output, logs, and code as it's tough to read otherwise.)

Output of checks

(If you are reporting a bug on GitLab.com, write: This bug happens on GitLab.com)

Results of GitLab environment info

Expand for output related to GitLab environment info 
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:env:info)


(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production)

Results of GitLab application Check

Expand for output related to the GitLab application check 
(For installations with omnibus-gitlab package run and paste the output of:
sudo gitlab-rake gitlab:check SANITIZE=true)


(For installations from source run and paste the output of:
sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true)


(we will only investigate if the tests are passing)

Possible fixes

(If you can, link to the line of code that might be responsible for the problem)

Edited by 🤖 GitLab Bot 🤖