Use BadRequirementError and reduce usage of nilable

Author: amazimbe

maven/lib/dependabot/maven/new_version.rb

@@ -24,16 +24,19 @@ class NewVersion
       sig { returns(Dependabot::Maven::TokenBucket) }
       attr_accessor :token_bucket
 
-      sig { params(version: T.nilable(String)).returns(T::Boolean) }
+      sig { params(version: String).returns(T::Boolean) }
       def self.correct?(version)
-        return false if version.nil? || version.empty?
+        return false if version.empty?
 
         Dependabot::Maven::VersionParser.parse(version.to_s).to_a.any?
+      rescue Dependabot::BadRequirementError
+        Dependabot.logger.info("Malformed version string - #{version}")
+        false
       end
 
-      sig { params(version: T.nilable(String)).void }
+      sig { params(version: String).void }
       def initialize(version)
-        @version_string = T.let(version, T.nilable(String))
+        @version_string = T.let(version, String)
         @token_bucket = T.let(Dependabot::Maven::VersionParser.parse(version), Dependabot::Maven::TokenBucket)
       end
 
@@ -44,7 +47,7 @@ def inspect
 
       sig { returns(String) }
       def to_s
-        T.must(version_string)
+        version_string
       end
 
       sig { returns(T::Boolean) }
@@ -61,7 +64,7 @@ def <=>(other)
 
       private
 
-      sig { returns(T.nilable(String)) }
+      sig { returns(String) }
       attr_reader :version_string
     end
   end

maven/lib/dependabot/maven/version_parser.rb

@@ -22,8 +22,8 @@ class VersionParser
 
       sig { params(version: T.nilable(String)).returns(TokenBucket) }
       def self.parse(version)
-        raise ArgumentError, "Malformed version string - string is nil" if version.nil?
-        raise ArgumentError, "Malformed version string - string is empty" if version.empty?
+        raise BadRequirementError, "Malformed version string - string is nil" if version.nil?
+        raise BadRequirementError, "Malformed version string - string is empty" if version.empty?
 
         new(version).parse
       end
@@ -40,7 +40,8 @@ def initialize(version)
       def parse
         parse_version(false)
 
-        raise ArgumentError, "Malformed version string #{version}" if parse_result.to_a.empty?
+        # no tokens: version is just one of the tokens we split on e.g '.' or '-'
+        raise BadRequirementError, "Malformed version string - #{version}" if parse_result.to_a.empty?
 
         T.must(parse_result)
       end
@@ -129,7 +130,7 @@ def parse_version(number_begins_partition) # rubocop:disable Metrics/AbcSize,Met
             number_begins_partition = true
 
           else
-            raise ArgumentError, "Malformed version string - #{version}"
+            raise BadRequirementError, "Malformed version string - #{version}"
           end
         end
       end

maven/spec/dependabot/maven/new_version_spec.rb

@@ -30,19 +30,16 @@
       it { is_expected.to be(true) }
     end
 
-    context "with a nil version" do
-      let(:version_string) { nil }
+    context "with an empty string" do
+      let(:version_string) { "" }
 
       it { is_expected.to be(false) }
     end
 
     context "with a malformed version string" do
       let(:version_string) { "-" }
-      let(:err_msg) { "Malformed version string #{version_string}" }
 
-      it "raises an exception" do
-        expect { version }.to raise_error(ArgumentError, err_msg)
-      end
+      it { is_expected.to be(false) }
     end
   end
 
@@ -85,21 +82,30 @@
       it { is_expected.to eq("1.0.0_pre1") }
     end
 
-    context "with a null version" do
-      let(:version_string) { nil }
-      let(:err_msg) { "Malformed version string - string is nil" }
+    context "with space as version" do
+      let(:version_string) { "" }
+      let(:err_msg) { "Malformed version string - string is empty" }
 
       it "raises an exception" do
-        expect { version }.to raise_error(ArgumentError, err_msg)
+        expect { version }.to raise_error(Dependabot::BadRequirementError, err_msg)
       end
     end
 
-    context "with a malformed version string" do
+    context "with a dot as version" do
+      let(:version_string) { "." }
+      let(:err_msg) { "Malformed version string - #{version_string}" }
+
+      it "raises an exception" do
+        expect { version }.to raise_error(Dependabot::BadRequirementError, err_msg)
+      end
+    end
+
+    context "with a hyphen as a version" do
       let(:version_string) { "-" }
-      let(:err_msg) { "Malformed version string #{version_string}" }
+      let(:err_msg) { "Malformed version string - #{version_string}" }
 
       it "raises an exception" do
-        expect { version }.to raise_error(ArgumentError, err_msg)
+        expect { version }.to raise_error(Dependabot::BadRequirementError, err_msg)
       end
     end
   end

maven/spec/dependabot/maven/version_parser_spec.rb

@@ -46,7 +46,7 @@
       let(:err_msg) { "Malformed version string - string is nil" }
 
       it "raises an exception" do
-        expect { described_class.parse(version) }.to raise_error(ArgumentError, err_msg)
+        expect { described_class.parse(version) }.to raise_error(Dependabot::BadRequirementError, err_msg)
       end
     end
 
@@ -56,7 +56,16 @@
       let(:err_msg) { "Malformed version string - string is empty" }
 
       it "raises an exception" do
-        expect { described_class.parse(version) }.to raise_error(ArgumentError, err_msg)
+        expect { described_class.parse(version) }.to raise_error(Dependabot::BadRequirementError, err_msg)
+      end
+    end
+
+    context "with a malformed version" do
+      let(:version) { "RELEASE&802" }
+      let(:err_msg) { "Malformed version string - #{version}" }
+
+      it "raises an exception" do
+        expect { described_class.parse(version) }.to raise_error(Dependabot::BadRequirementError, err_msg)
       end
     end
   end