Draft: Modify VulnerabilityFeedbackController to create Vulnerability

What does this MR do and why?

Describe in detail what your merge request does and why.

After #324857 (closed) is merged, we should make sure to create a new Vulnerability when a user interacts with an unpersisted Finding.

This MR Modifies the Projects::VulnerabilityFeedbackController to create Vulnerability on the fly for a given Finding.

• This will not create a new feedback object but only create a vulnerability.
• We will need to set the present_on_default_branch to false for these new vulnerabilities

This MR is related to issue #324860 (closed) #368311 (closed)

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

• I have evaluated the MR acceptance checklist for this MR.

changed milestone to %15.2

added backend devopssecure groupthreat insights maintenancerefactor sectionsec typemaintenance labels

assigned to @mc_rocha

Suggested Reviewers (beta)

The individuals below may be good candidates to participate in the review based on various factors.

You can use slash commands in comments to quickly assign /assign_reviewer @user1.

Suggested Reviewers
@rspeicher, @mparuszewski, @dzaporozhets, @kerrizor, @rymai

If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/357923.

Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".

	1 Warning
	8398ee38: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

Reviewer roulette

Changes that require review have been detected!

Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

Category	Reviewer	Maintainer
backend	Serhii Yarynovskyi (@syarynovskyi) (UTC+3)	Alex Pooley (@alexpooley) (UTC+8)

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

added 835 commits

• bc43a6ff...880c0e93 - 833 commits from branch master
• 6a51c8c2 - Modify VulnerabilityFeedbackController to create Vulnerability
• 08686891 - Fix spec error

Compare with previous version

changed the description

added 794 commits

• 08686891...3b48b7a8 - 787 commits from branch master
• 182958a2 - Modify VulnerabilityFeedbackController to create Vulnerability
• dd78ce8f - Fix spec error
• c686c956 - Add a new service to create vulnerabilities for issue
• 066fe6ad - Modify VulnerabilityFeedbackController to create Vulnerability
• c5730454 - Update VulnerabilityFeedbackController create endpoint
• 1f2c2d43 - Create Merge Request from Vulnerability
• 67f6ddc6 - Create Merge Request from Vulnerability

Compare with previous version

changed the description

changed milestone to %15.3

added missed:15.2 label

added 4963 commits

• 67f6ddc6...ddd3a035 - 4961 commits from branch master
• a7127240 - Merge branch 'master' of gitlab.com:gitlab-org/gitlab into...
• 619079dc - Add Create Merge Request Service

Compare with previous version

added 9 commits

• 86384b35 - Modify VulnerabilityFeedbackController to create Vulnerability
• c209cdac - Fix spec error
• 2c930b4a - Add a new service to create vulnerabilities for issue
• f96c0d04 - Modify VulnerabilityFeedbackController to create Vulnerability
• d5cda6c3 - Update VulnerabilityFeedbackController create endpoint
• fb4a20f3 - Create Merge Request from Vulnerability
• 5e2f3b48 - Create Merge Request from Vulnerability
• 8398ee38 - Add Create Merge Request Service
• 8bb01f79 - Add Create Merge Request Service specs

Compare with previous version

Hi @Quintasan,

I was trying to wrap the vulnerability, merge_request, and merge_request_link creation in a transaction.

Like we did for the vulnerability, issue, and issue_link.

But I got the following error:

Vulnerabilities::SecurityFinding::CreateMergeRequestService#execute when there is an existing vulnerability for the security finding does create a new MergeRequestLink
     Failure/Error: NewMergeRequestWorker.perform_async(issuable.id, current_user.id)
     Sidekiq::Worker::EnqueueFromTransactionError:
       NewMergeRequestWorker.perform_async cannot be enqueued inside a transaction as this can lead to
       race conditions when the worker runs before the transaction is committed and
       tries to access a model that has not been saved yet.
       Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead.

Is it okay to create them without a transaction? Should we delete the merge_request manually if we can't create a vulnerability or a merge_request_link?

cc: @subashis

include AfterCommitQueue and use a run_after_commit block instead.

@mc_rocha did you investigate whether that would solve our problem here? I'd rather not skip transactions here.

changed milestone to %15.4

added missed:15.3 label

added 630 commits

• 8bb01f79...b8425f20 - 620 commits from branch master
• 0e35fdf9 - Modify VulnerabilityFeedbackController to create Vulnerability
• 378136f0 - Fix spec error
• faa608d8 - Add a new service to create vulnerabilities for issue
• 9dc0f80f - Modify VulnerabilityFeedbackController to create Vulnerability
• 9d78fa36 - Update VulnerabilityFeedbackController create endpoint
• 52dc6836 - Create Merge Request from Vulnerability
• 506c5845 - Create Merge Request from Vulnerability
• f06edb59 - Add Create Merge Request Service
• 556f03f7 - Add Create Merge Request Service specs
• 1b0f0a16 - Wrap related objects creation in a transaction

Compare with previous version

	1 Warning
	8e9e77e1: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

Reviewer roulette

Changes that require review have been detected!

Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

Category	Reviewer	Maintainer
backend	Roy Zwambag (@rzwambag) (UTC+2)	Pavel Shutsin (@pshutsin) (UTC+2)

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

added 1641 commits

• 1b0f0a16...0788478c - 1630 commits from branch master
• 17b92abd - Modify VulnerabilityFeedbackController to create Vulnerability
• a7bd2ff6 - Fix spec error
• fce5b9ac - Add a new service to create vulnerabilities for issue
• 0225d3cd - Modify VulnerabilityFeedbackController to create Vulnerability
• 3b52ef3c - Update VulnerabilityFeedbackController create endpoint
• ce5ba8c2 - Create Merge Request from Vulnerability
• e988d0c1 - Create Merge Request from Vulnerability
• 8e9e77e1 - Add Create Merge Request Service
• 05f52247 - Add Create Merge Request Service specs
• 1f3e25ae - Wrap related objects creation in a transaction
• c55793ef - Update vulnerability_feedback_controller specs

Compare with previous version

Allure report

allure-report-publisher generated test report!

review-qa-blocking: test report for c55793ef

expand test summary

+-----------------------------------------------------------------------------------------+
|                                     suites summary                                      |
+------------------------------------+--------+--------+---------+-------+-------+--------+
|                                    | passed | failed | skipped | flaky | total | result |
+------------------------------------+--------+--------+---------+-------+-------+--------+
| Verify                             | 12     | 0      | 1       | 7     | 13    | ❗     |
| Create                             | 28     | 0      | 1       | 8     | 29    | ❗     |
| Manage                             | 46     | 0      | 3       | 16    | 49    | ❗     |
| Plan                               | 47     | 0      | 1       | 10    | 48    | ❗     |
| Protect                            | 2      | 0      | 0       | 1     | 2     | ❗     |
| Feature flag handler sanity checks | 9      | 0      | 0       | 0     | 9     | ✅     |
| Secure                             | 2      | 0      | 0       | 2     | 2     | ❗     |
| Version sanity check               | 0      | 0      | 1       | 0     | 1     | ➖     |
| Package                            | 0      | 0      | 1       | 0     | 1     | ➖     |
| Configure                          | 0      | 0      | 1       | 0     | 1     | ➖     |
+------------------------------------+--------+--------+---------+-------+-------+--------+
| Total                              | 146    | 0      | 9       | 44    | 155   | ❗     |
+------------------------------------+--------+--------+---------+-------+-------+--------+

added 1 commit

• efa65116 - Update vulnerability_feedback_controller specs

Compare with previous version

mentioned in merge request !96921 (merged)

added devopsgovern label and removed devopssecure label

changed milestone to %15.5

added missed:15.4 label

changed milestone to %15.6

added missed:15.5 label

As commented here, I am assigning this to myself to continue rest of the work after !96921 (merged) this is merged.

assigned to @subashis and unassigned @mc_rocha

mentioned in merge request !102907 (merged)

changed milestone to %15.7

added missed:15.6 label

changed milestone to %15.8

added missed:15.7 label

changed milestone to %15.9

added missed:15.8 label

changed milestone to %15.10

added missed:15.9 label

The changes for creating vulnerability is already covered in other MRs for this issue. We can close this MR as we are moving towards replacing the controller actions with GraphQL mutations. If we do want to change the feedback creation service before the GraphQL replacement, we do have this Replace/Update feedback create service with a n... (#390156) • Unassigned • Backlog. But this code will not be needed in both of the scenario.

closed