[rubygems/rubygems] Fix locked gems being upgraded when locked dependencies are incorrect

Resolver had internal logic to prioritize locked versions when sorting
versions, however part of it was not being actually hit because of how
unlocking worked in the resolver: a package was allow to be unlocked
when that was explicit requested or when the list of unlocks was empty.
That did not make a lot of sense and other cases were working because
the explicit list of unlocks was getting "artificially filled".

Now we consider a package unlocked when explicitly requested (`bundle
update <package>`), or when everything is being unlocked (`bundle
install` with no lockfile or `bundle update`).

This makes things simpler and gets the edge case added as a test case
working as expected.

https://github.com/rubygems/rubygems/commit/b8e55087f0

lib/bundler/definition.rb

@@ -625,7 +625,7 @@ module Bundler
       @resolution_packages ||= begin
         last_resolve = converge_locked_specs
         remove_invalid_platforms!
-        packages = Resolver::Base.new(source_requirements, expanded_dependencies, last_resolve, @platforms, locked_specs: @originally_locked_specs, unlock: @gems_to_unlock, prerelease: gem_version_promoter.pre?, prefer_local: @prefer_local)
+        packages = Resolver::Base.new(source_requirements, expanded_dependencies, last_resolve, @platforms, locked_specs: @originally_locked_specs, unlock: @unlocking_all || @gems_to_unlock, prerelease: gem_version_promoter.pre?, prefer_local: @prefer_local, new_platforms: @new_platforms)
         packages = additional_base_requirements_to_prevent_downgrades(packages, last_resolve)
         packages = additional_base_requirements_to_force_updates(packages)
         packages
@@ -768,6 +768,7 @@ module Bundler
       @most_specific_non_local_locked_ruby_platform = find_most_specific_locked_ruby_platform
       return if @most_specific_non_local_locked_ruby_platform
 
+      @new_platforms << local_platform
       @platforms << local_platform
       true
     end
@@ -952,6 +953,7 @@ module Bundler
           locked_specs = @originally_locked_specs[name]
 
           if locked_specs.any? && !dep.matches_spec?(locked_specs.first)
+            @gems_to_unlock << name
             dep_changed = true
           elsif locked_specs.empty? && dep_changed == false
             @missing_lockfile_dep = name
@@ -1023,11 +1025,6 @@ module Bundler
           end
         end
 
-        if dep.nil? && requested_dep = requested_dependencies.find {|d| name == d.name }
-          @gems_to_unlock << name
-          deps << requested_dep
-        end
-
         converged << s
       end
 

lib/bundler/gem_version_promoter.rb

@@ -132,8 +132,6 @@ module Bundler
     # Specific version moves can't always reliably be done during sorting
     # as not all elements are compared against each other.
     def post_sort(result, unlock, locked_version)
-      # default :major behavior in Bundler does not do this
-      return result if major?
       if unlock || locked_version.nil?
         result
       else

lib/bundler/resolver/package.rb

@@ -15,7 +15,7 @@ module Bundler
     class Package
       attr_reader :name, :platforms, :dependency, :locked_version
 
-      def initialize(name, platforms, locked_specs:, unlock:, prerelease: false, prefer_local: false, dependency: nil)
+      def initialize(name, platforms, locked_specs:, unlock:, prerelease: false, prefer_local: false, dependency: nil, new_platforms: [])
         @name = name
         @platforms = platforms
         @locked_version = locked_specs.version_for(name)
@@ -24,10 +24,14 @@ module Bundler
         @top_level = !dependency.nil?
         @prerelease = @dependency.prerelease? || @locked_version&.prerelease? || prerelease ? :consider_first : :ignore
         @prefer_local = prefer_local
+        @new_platforms = new_platforms
       end
 
       def platform_specs(specs)
-        platforms.map {|platform| GemHelpers.select_best_platform_match(specs, platform, prefer_locked: !unlock?) }
+        platforms.map do |platform|
+          prefer_locked = @new_platforms.include?(platform) ? false : !unlock?
+          GemHelpers.select_best_platform_match(specs, platform, prefer_locked: prefer_locked)
+        end
       end
 
       def to_s
@@ -55,7 +59,7 @@ module Bundler
       end
 
       def unlock?
-        @unlock.empty? || @unlock.include?(name)
+        @unlock == true || @unlock.include?(name)
       end
 
       def ignores_prereleases?

spec/bundler/bundler/gem_version_promoter_spec.rb

@@ -20,13 +20,13 @@ RSpec.describe Bundler::GemVersionPromoter do
       end
     end
 
-    def build_package(name, version, locked = [])
+    def build_package(name, version, unlock)
-      Bundler::Resolver::Package.new(name, [], locked_specs: Bundler::SpecSet.new(build_spec(name, version)), unlock: locked)
+      Bundler::Resolver::Package.new(name, [], locked_specs: Bundler::SpecSet.new(build_spec(name, version)), unlock: unlock)
     end
 
-    def sorted_versions(candidates:, current:, name: "foo", locked: [])
+    def sorted_versions(candidates:, current:, unlock: true)
       gvp.sort_versions(
-        build_package(name, current, locked),
+        build_package("foo", current, unlock),
         build_candidates(candidates)
       ).flatten.map(&:version).map(&:to_s)
     end
@@ -127,7 +127,7 @@ RSpec.describe Bundler::GemVersionPromoter do
       before { gvp.level = :minor }
 
       it "keeps the current version first" do
-        versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.1.0 2.0.1], current: "0.3.0", locked: ["bar"])
+        versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.1.0 2.0.1], current: "0.3.0", unlock: [])
         expect(versions.first).to eq("0.3.0")
       end
     end

spec/bundler/lock/lockfile_spec.rb

@@ -1875,6 +1875,62 @@ RSpec.describe "the lockfile format" do
     L
   end
 
+  it "automatically fixes the lockfile when it has incorrect deps, keeping the locked version" do
+    build_repo4 do
+      build_gem "net-smtp", "0.5.0" do |s|
+        s.add_dependency "net-protocol"
+      end
+
+      build_gem "net-smtp", "0.5.1" do |s|
+        s.add_dependency "net-protocol"
+      end
+
+      build_gem "net-protocol", "0.2.2"
+    end
+
+    gemfile <<~G
+      source "#{file_uri_for(gem_repo4)}"
+      gem "net-smtp"
+    G
+
+    lockfile <<~L
+      GEM
+        remote: #{file_uri_for(gem_repo4)}/
+        specs:
+          net-protocol (0.2.2)
+          net-smtp (0.5.0)
+
+      PLATFORMS
+        #{lockfile_platforms}
+
+      DEPENDENCIES
+        net-smtp
+
+      BUNDLED WITH
+         #{Bundler::VERSION}
+    L
+
+    bundle "install"
+
+    expect(lockfile).to eq <<~L
+      GEM
+        remote: #{file_uri_for(gem_repo4)}/
+        specs:
+          net-protocol (0.2.2)
+          net-smtp (0.5.0)
+            net-protocol
+
+      PLATFORMS
+        #{lockfile_platforms}
+
+      DEPENDENCIES
+        net-smtp
+
+      BUNDLED WITH
+         #{Bundler::VERSION}
+    L
+  end
+
   shared_examples_for "a lockfile missing dependent specs" do
     it "auto-heals" do
       build_repo4 do

spec/bundler/resolver/basic_spec.rb

@@ -211,12 +211,12 @@ RSpec.describe "Resolving" do
 
     it "resolves all gems to latest patch" do
       # strict is not set, so bar goes up a minor version due to dependency from foo 1.4.5
-      should_conservative_resolve_and_include :patch, [], %w[foo-1.4.5 bar-2.1.1]
+      should_conservative_resolve_and_include :patch, true, %w[foo-1.4.5 bar-2.1.1]
     end
 
     it "resolves all gems to latest patch strict" do
       # strict is set, so foo can only go up to 1.4.4 to avoid bar going up a minor version, and bar can go up to 2.0.5
-      should_conservative_resolve_and_include [:patch, :strict], [], %w[foo-1.4.4 bar-2.0.5]
+      should_conservative_resolve_and_include [:patch, :strict], true, %w[foo-1.4.4 bar-2.0.5]
     end
 
     it "resolves foo only to latest patch - same dependency case" do
@@ -256,20 +256,20 @@ RSpec.describe "Resolving" do
 
     it "resolves all gems to latest minor" do
       # strict is not set, so bar goes up a major version due to dependency from foo 1.4.5
-      should_conservative_resolve_and_include :minor, [], %w[foo-1.5.1 bar-3.0.0]
+      should_conservative_resolve_and_include :minor, true, %w[foo-1.5.1 bar-3.0.0]
     end
 
     it "resolves all gems to latest minor strict" do
       # strict is set, so foo can only go up to 1.5.0 to avoid bar going up a major version
-      should_conservative_resolve_and_include [:minor, :strict], [], %w[foo-1.5.0 bar-2.1.1]
+      should_conservative_resolve_and_include [:minor, :strict], true, %w[foo-1.5.0 bar-2.1.1]
     end
 
     it "resolves all gems to latest major" do
-      should_conservative_resolve_and_include :major, [], %w[foo-2.0.0 bar-3.0.0]
+      should_conservative_resolve_and_include :major, true, %w[foo-2.0.0 bar-3.0.0]
     end
 
     it "resolves all gems to latest major strict" do
-      should_conservative_resolve_and_include [:major, :strict], [], %w[foo-2.0.0 bar-3.0.0]
+      should_conservative_resolve_and_include [:major, :strict], true, %w[foo-2.0.0 bar-3.0.0]
     end
 
     # Why would this happen in real life? If bar 2.2 has a bug that the author of foo wants to bypass
@@ -292,21 +292,21 @@ RSpec.describe "Resolving" do
       end
 
       it "could revert to a previous version level patch" do
-        should_conservative_resolve_and_include :patch, [], %w[foo-1.4.4 bar-2.1.1]
+        should_conservative_resolve_and_include :patch, true, %w[foo-1.4.4 bar-2.1.1]
       end
 
       it "cannot revert to a previous version in strict mode level patch" do
         # fall back to the locked resolution since strict means we can't regress either version
-        should_conservative_resolve_and_include [:patch, :strict], [], %w[foo-1.4.3 bar-2.2.3]
+        should_conservative_resolve_and_include [:patch, :strict], true, %w[foo-1.4.3 bar-2.2.3]
       end
 
       it "could revert to a previous version level minor" do
-        should_conservative_resolve_and_include :minor, [], %w[foo-1.5.0 bar-2.0.5]
+        should_conservative_resolve_and_include :minor, true, %w[foo-1.5.0 bar-2.0.5]
       end
 
       it "cannot revert to a previous version in strict mode level minor" do
         # fall back to the locked resolution since strict means we can't regress either version
-        should_conservative_resolve_and_include [:minor, :strict], [], %w[foo-1.4.3 bar-2.2.3]
+        should_conservative_resolve_and_include [:minor, :strict], true, %w[foo-1.4.3 bar-2.2.3]
       end
     end
   end

spec/bundler/support/indexes.rb

@@ -66,7 +66,6 @@ module Spec
     end
 
     def should_conservative_resolve_and_include(opts, unlock, specs)
-      # empty unlock means unlock all
       opts = Array(opts)
       search = Bundler::GemVersionPromoter.new.tap do |s|
         s.level = opts.first