fix rebase creating PR for wrong dependency

[sachin-sandhu]

What are you trying to accomplish?

Preface: This issue is related with closing and creating new pull requests.

Issue: Azure Dev Ops team (likely not limited to) is facing an issue where is closing and recreating an existing PR which is resulting in multiple PRs being generated every day. i.e.

+-------------------------------------------------------------------------------------+
|                         Changes to Dependabot Pull Requests                         |
+------------------------------+------------------------------------------------------+
| closed: dependencies_changed | express,path-to-regexp,react-router,react-router-dom |
| created                      | express ( from 4.18.2 to 4.21.0 )                    |
+------------------------------+------------------------------------------------------+

Here the existing pull request already exists for lead dependency express. This can be seen from job snapshot as following:

job:
  dependencies:
  - express
  - path-to-regexp
  - react-router
  - react-router-dom
  existing-pull-requests:
  - - dependency-name: express
      dependency-version: 4.21.0
    - dependency-name: path-to-regexp
      dependency-version: 0.1.10
    - dependency-name: react-router-doms
      dependency-version: 6.26.2
  security-updates-only: true

On dependabot scanning existing PR, it does not takes the existing PR snapshot into account. i.e. the rerun pr log is as following:

2024/10/07 16:04:22 INFO Checking and updating security pull requests...
2024/10/07 16:04:22 INFO Checking if express 4.18.2 needs updating
2024/10/07 16:04:23 INFO Latest version is 4.21.0
2024/10/07 16:04:30 INFO Requirements to unlock own
2024/10/07 16:04:30 INFO Requirements update strategy bump_versions
2024/10/07 16:04:30 INFO Updating express from 4.18.2 to 4.21.0
2024/10/07 16:04:40 INFO Telling backend to close pull request for express, path-to-regexp, react-router, react-router-dom - dependencies changed
2024/10/07 16:04:40 INFO Submitting express pull request for creation
+-------------------------------------------------------------------------------------+
|                         Changes to Dependabot Pull Requests                         |
+------------------------------+------------------------------------------------------+
| closed: dependencies_changed | express,path-to-regexp,react-router,react-router-dom |
| created                      | express ( from 4.18.2 to 4.21.0 )                    |
+------------------------------+------------------------------------------------------+

Issues:

There are multiple issues with this error which are described as following:

1. The preceding snapshot is as following:

job:
  dependencies:
  - path-to-regexp
  security-advisories:
  - dependency-name: path-to-regexp
    affected-versions:
    - '>= 0, < 0.1.10'
    - '>= 0.2.0, < 8.0.0'
  security-updates-only: true
  commit-message-options:

Here it can be seen that the lead dependency is mentioned as path-to-regexp as compared to subsequent job (mentioned on top) where the lead dependency is express . Here the dependencies list is sorted alphabetically which is cause of this issue. Due to this mismatch the resulting job creates and erroneous situation that results in closing and creating a new PR. The ideal result in this situation should be to update the existing PR.

2. A situation where the decision to takes to create/update/close PR has a logical issue. The number of dependencies modified vs initial job list are not compared in logical order (alphabetically) which results in inaccurate results. The error is at line

Solution: The fix intends to stop recreation of PRs if same version update is found in existing PR snapshot. It is seen that this issue exists for lead dependencies (first in the group) right now. The targeted version update can be matched with the one in pr snapshot, if the versions are a match, then a new PR creation can be avoided. i.e.

2024/10/07 17:04:19 INFO Matching entry found in existing PR. Dependency name: express, version: 4.21.0
2024/10/07 17:04:19 INFO Lead dependency (express) version (4.21.0) is  already upto date in pull request, skipping updating pull request.

1. First part of solution is to take security advisory dependency as a lead dependency, which is primary dependency on which the preceding PR was created. This solves the issue of inaccurate lead dependency.

2. The comparison of dependencies updated vs initial dependencies in job are now sorted in alphabetical order to accurately compare the two data sets (string array). This should resolve issue of inaccurate results in case same number of dependencies are updated but not in order.

+---------------------------------------------------------------------------------------------------------------------------------+
|                                               Changes to Dependabot Pull Requests                                               |
+---------+-----------------------------------------------------------------------------------------------------------------------+
| updated | path-to-regexp ( from 0.1.7 to 0.1.10 ), react-router-dom ( from 5.3.4 to 6.27.0 ), express ( from 4.18.2 to 4.21.1 ) |
+---------+-----------------------------------------------------------------------------------------------------------------------+

Additional information:

To mitigate any issues that might come up with this fix, a feature flag lead_security_dependency is used to quickly roll back in case of any issues.

Telemetry data collection:
To assess the behaviour, telemetry log has been inserted at various places to collect information on impact of this issue. This is to conduct impact assessment of the issue later and facilitate next steps if required.

Plan of execution:

• The feature will be turned on only for Azure Dev Ops related PRs. Feature flag handling will be by ADO team.
• Afterwards, logs will be observed for sample repos set to ensure that fix is working as intended
• If there is not breaking impact, then telemetry data will be collected from AWS CloudWatch logs.
• Data collected will be analyzed to see the impact of this behaviour.
• Post analysis steps can be taken if required.

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

We should see a larger drop in dropping and recreating new PRs on affected repos.

Checklist

• I have run the complete test suite to ensure all tests and linters pass.
• I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
• I have written clear and descriptive commit messages.
• I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
• I have ensured that the code is well-documented and easy to understand.

[abdulapopoola]

thanks @sachin-sandhu , please let's get @jakecoffman , @Nishnha , and @landongrindheim to review this as they worked on this area a bit.

[kbukum1]

I left some comments.

[jakecoffman]

I'm not entirely sure what the problem is. Customer has Dependabot run daily and so it does, and finds an update to an existing PR:

created                      | express ( from 4.18.2 to 4.21.0 ) 

So it supersedes the PR. That seems like the behavior Dependabot has always had.

Is that a correct summary of the issue?

If the dependencies are updating too frequently the solution would be to have Dependabot run less frequent than daily, maybe weekly.

Edit: Ah ok this repo makes the problem pretty clear: https://github.com/brettfo/dependabot-npm-closing

[jakecoffman]

It appears this PR will break rebases. We do need Dependabot to rebase jobs even if an existing PR exists with the same version, that's a feature of rebasing.

For example: Dependabot posts the PR, and new commits go to the main branch. Users will want to @dependabot rebase the PR before merging. With this change it seems like that will no longer work assuming the dependency version hasn't changed.

It seems the bug here is that a rebase job is producing different results from the job that created it, probably because of the grouped nature of the update. So we need to track down why that is happening instead of preventing the rebase.

[jakecoffman]

The top of this file has a comment:

DO NOT ADD NEW TESTS TO THIS FILE

This would be a perfect candidate for a "silent test" for multidir-rebase. Here's a multidir-create test you can modify to add a rebase: https://github.com/dependabot/dependabot-core/blob/e63dc2cf45f6d2d6ca29ed49433a26eb74c5fcfc/silent/tests/testdata/su-multidir.txt

[sachin-sandhu]

I have moved tests cases to relevant file and also added silent tests for security rebases

[jakecoffman]

I would suggest turning the experiment on in these tests as we believe this is the right way to do security updates with a single dependency going forward.

[sachin-sandhu]

added a new test for single dependency update, this particular test case is intended towards deprecation notices

[jakecoffman]

Nice work!