code review feedback

Author: saghm

lib/mongo/uri.rb

@@ -435,9 +435,9 @@ def self.uri_option(uri_key, name, extra = {})
 
     # Write Options
     uri_option 'w', :w, :group => :write
-    uri_option 'journal', :j, :group => :write, :type => :bool
+    uri_option 'journal', :j, :group => :write, :type => :journal
     uri_option 'fsync', :fsync, :group => :write
-    uri_option 'wtimeoutms', :timeout, :group => :write, :type => :unsigned_int
+    uri_option 'wtimeoutms', :timeout, :group => :write, :type => :wtimeout
 
     # Read Options
     uri_option 'readpreference', :mode, :group => :read, :type => :read_mode
@@ -452,7 +452,7 @@ def self.uri_option(uri_key, name, extra = {})
     # Security Options
     uri_option 'ssl', :ssl
     uri_option 'tls', :ssl
-    uri_option 'tlsallowinvalidcertificates', :ssl_verify, :type => :inverse_bool
+    uri_option 'tlsallowinvalidcertificates', :ssl_verify, :type => :ssl_verify
     uri_option 'tlscafilepath', :ssl_ca_cert
     uri_option 'tlsclientcertfilepath', :ssl_cert
     uri_option 'tlsclientkeyfilepath', :ssl_key
@@ -471,7 +471,7 @@ def self.uri_option(uri_key, name, extra = {})
     uri_option 'appname', :app_name
     uri_option 'compressors', :compressors, :type => :array
     uri_option 'readconcernlevel', :read_concern
-    uri_option 'retrywrites', :retry_writes, :type => :bool
+    uri_option 'retrywrites', :retry_writes, :type => :retry_writes
     uri_option 'zlibcompressionlevel', :zlib_compression_level, :type => :zlib_compression_level
 
     # Casts option values that do not have a specifically provided
@@ -582,7 +582,9 @@ def auth_source(value)
     #
     # @return [Symbol] The transformed authentication mechanism.
     def auth_mech(value)
-      AUTH_MECH_MAP[value.upcase] || log_warn("#{value} is not a valid auth mechanism")
+      AUTH_MECH_MAP[value.upcase].tap do |mech|
+        log_warn("#{value} is not a valid auth mechanism") unless mech
+      end
     end
 
     # Read preference mode transformation.
@@ -631,9 +633,9 @@ def auth_mech_props(value)
     # @param value [ String ] The zlib compression level string.
     #
     # @return [ Integer | nil ] The compression level value if it is between -1 and 9 (inclusive),
-    #   otherwise nil (and a warning will be raised).
+    #   otherwise nil (and a warning will be logged).
     def zlib_compression_level(value)
-      if /^-?\d+$/ =~ value
+      if /\A-?\d+\z/ =~ value
         i = value.to_i
 
         if i >= -1 && i <= 9
@@ -645,47 +647,69 @@ def zlib_compression_level(value)
       nil
     end
 
+    # Parses the journal value.
+    #
+    # @param value [ String ] The journal value.
+    #
+    # @return [ true | false | nil ] The journal value parsed out, otherwise nil (and a warning
+    #   will be raised).
+    def journal(value)
+      bool('journal', value)
+    end
+
+    # Parses the ssl_verify value. Note that this will be the inverse of the value of
+    # tlsAllowInvalidCertificates (if present).
+    #
+    # @param value [ String ] The tlsAllowInvalidCertificates value.
+    #
+    # @return [ true | false | nil ] The ssl_verify value parsed out, otherwise nil (and a warning
+    #   will be raised).
+    def ssl_verify(value)
+      b = bool('tlsAllowInvalidCertificates', value)
+
+      if b.nil?
+        nil
+      else
+        !b
+      end
+    end
+
+    # Parses the retryWrites value.
+    #
+    # @param value [ String ] The retryWrites value.
+    #
+    # @return [ true | false | nil ] The boolean value parsed out, otherwise nil (and a warning
+    #   will be raised).
+    def retry_writes(value)
+      bool('retryWrites', value)
+    end
 
     # Parses a boolean value.
     #
     # @param value [ String ] The URI option value.
     #
     # @return [ true | false | nil ] The boolean value parsed out, otherwise nil (and a warning
     #   will be raised).
-    def bool(value)
+    def bool(name, value)
       case value
       when "true"
         true
       when "false"
         false
       else
-        log_warn("invalid boolean: #{value}")
+        log_warn("invalid boolean option for #{name}: #{value}")
         nil
       end
     end
 
-    # Parses a boolean value and returns its inverse. This is used for options where the spec
-    # defines the boolean value semantics of enabling/disabling something in the reverse way that
-    # the driver does. For instance, the client option `ssl_verify` will disable certificate
-    # checking when the value is false, but the spec defines the option
-    # `tlsAllowInvalidCertificates`, which disables certificate checking when the value is true.
-    #
-    # @param value [ String ] The URI option.
-    #
-    # @return [ true | false | nil ] The inverse of boolean value parsed out, otherwise nil (and a
-    #   warning will be raised).
-    def inverse_bool(value)
-      !bool(value)
-    end
-
     # Parses the max staleness value, which must be either "0" or an integer greater or equal to 90.
     #
     # @param value [ String ] The max staleness string.
     #
     # @return [ Integer | nil ] The max staleness integer parsed out if it is valid, otherwise nil
-    #   (and a warning will be raised).
+    #   (and a warning will be logged).
     def max_staleness(value)
-      if /^\d+$/ =~ value
+      if /\A\d+\z/ =~ value
         int = value.to_i
 
         if int >= 0 && int < 90
@@ -705,9 +729,9 @@ def max_staleness(value)
     #
     # @return [ Integer | nil ] The integer parsed out, otherwise nil (and a warning will be
     #   raised).
-    def unsigned_int(value)
-     unless /^?\d+$/ =~ value
-        log_warn("Invalid unsigned integer value: #{value}")
+    def wtimeout(value)
+     unless /\A\d+\z/ =~ value
+        log_warn("Invalid wtimeoutMS value: #{value}")
         return nil
      end
 
@@ -724,7 +748,7 @@ def unsigned_int(value)
     #
     # @since 2.0.0
     def ms_convert(value)
-      unless /^-?\d+(\.\d+)?$/ =~ value
+      unless /\A-?\d+(\.\d+)?\z/ =~ value
         log_warn("Invalid ms value: #{value}")
         return nil
       end

spec/spec_tests/uri_options_spec.rb

@@ -23,25 +23,6 @@ def create_resolver(timeout, ssl_options)
               FAMILY_MAP[info.first[4]].new(info[3], port, host)
             end
           end
-
-          class Server
-
-            # The constructor keeps the same API, but does not instantiate a
-            # monitor and run it.
-            alias :original_initialize :initialize
-            def initialize(address, cluster, monitoring, event_listeners, options = {})
-              @address = address
-              @cluster = cluster
-              @monitoring = monitoring
-              @options = options.freeze
-              @monitor = Monitor.new(address, event_listeners, Monitoring.new, options)
-            end
-
-            # Disconnect simply needs to return true since we have no monitor and
-            # no connection.
-            alias :original_disconnect! :disconnect!
-            def disconnect!; true; end
-          end
         end
       end
 
@@ -54,14 +35,6 @@ class Address
             alias :create_resolver :original_create_resolver
             remove_method(:original_create_resolver)
           end
-
-          class Server
-            alias :initialize :original_initialize
-            remove_method(:original_initialize)
-
-            alias :disconnect! :original_disconnect!
-            remove_method(:original_disconnect!)
-          end
         end
       end
 
@@ -83,26 +56,7 @@ class Server
             end
           end
 
-          context 'when the uri should warn', if: test.warn? do
-
-            before do
-              expect(Mongo::Logger.logger).to receive(:warn)
-            end
-
-            it 'warns' do
-              expect(test.client).to be_a(Mongo::Client)
-            end
-          end
-
-          context 'when the uri is valid', if: test.valid? && !test.warn? do
-
-            before do
-              expect(Mongo::Logger.logger).not_to receive(:warn)
-            end
-
-            it 'does not raise an exception or warning' do
-              expect(test.uri).to be_a(Mongo::URI)
-            end
+          context 'when the uri is valid', if: test.valid? do
 
             it 'creates a client with the correct hosts' do
               expect(test.client).to have_hosts(test)
@@ -115,6 +69,28 @@ class Server
             it 'creates a client with the correct options' do
               expect(test.client).to match_options(test)
             end
+
+            context 'when the uri should not warn', if: !test.warn? do
+
+              before do
+                expect(Mongo::Logger.logger).not_to receive(:warn)
+              end
+
+              it 'does not raise an exception or warning' do
+                expect(test.uri).to be_a(Mongo::URI)
+              end
+            end
+
+            context 'when the uri should warn', if: test.warn? do
+
+              before do
+                expect(Mongo::Logger.logger).to receive(:warn)
+              end
+
+              it 'warns' do
+                expect(test.client).to be_a(Mongo::Client)
+              end
+            end
           end
         end
       end