[go: up one dir, main page]
Skip to content

Move check from MergeRequests::BuildService to MergeRequest model validation

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

What it is

MergeRequests::BuildService is used to determine if we're going to show the preview of a new merge request, pre-filling some data in the form and ask the user to enter the other information in order to create a merge request.

What to do

Following the conversation from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9263#note_23507289, I propose that we:

  • Remove MergeRequest#can_be_created
  • Move all the checks in MergeRequests::BuildService to be validations on MergeRequest
  • Replace the check to MergeRequest#can_be_created to be MergeRequest#valid?
  • Make MergeRequests::BuildService only pre-fill data and handle error messages if extra action needed.

The validation could look something like:

# Note that the below check should happen after `validate_branches` and so on
validate :validate_commit_existence, on: :create

def validate_commit_existence
  unless source_project.commit(source_branch)
    errors.add(:source_commit, '...')
  end

  unless target_project.commit(target_branch)
    errors.add(:target_commit, '...')
  end
end

And update the error messages accordingly.

Rationale behind this

MergeRequest#can_be_created is really a bad way to pass data. The way it is right now is like saving a local variable in another object, while the object saving the data has no idea what it is. Ideally, validations should not be a state in the record, but the validation itself. Something like:

valid = Validate::MergeRequest.new(merge_request).validate

Would work much better because if we're ever going to validate twice (or use a different validation because, see MergeRequest#allow_broken), we just need to create a new validation object instead of clearing the state on the record. This works better:

valid = Validate::MergeRequest.new(merge_request).validate
merge_request.update(params)
valid_again = Validate::MergeRequest.new(merge_request).validate

Than this:

valid = merge_request.valid?
merge_request.update(params)
merge_request.reset_validation # BOO
valid_again = merge_request.valid?

However we're just stuck with Rails, and in this particular case, the object, that is the merge request should totally know if the branches are valid or not, which is the only check used for MergeRequest#can_be_created.

Checking the other validations used in MergeRequest:

  • validates :source_project, presence: true Of course we also want this when previewing
  • validates :source_branch, presence: true Same
  • validates :target_project, presence: true Same
  • validates :target_branch, presence: true Same
  • validate :validate_branches Why are we doing similar thing in MergeRequests::BuildService too? This is where we should merge the checks.
  • validate :validate_fork This won't hurt

That means we certainly also want the above to pass even if we're previewing the merge request.

All the other attr_accessor should probably also be removed from MergeRequest, but each one would have their own story and would be another topic.

/cc @felipe_artur @smcgivern

p.s. MergeRequests::BuildService is really in a very confusing state, probably only better than CreatesCommit

Edited by 🤖 GitLab Bot 🤖