Only rescue errors explicitly

There are a lot of places in the code where we rescue any error, at all. That's quite dangerous, as it could conceal bugs. I've changed it so that we always specify which error class we want to catch. Mostly it's been a minor change, but there are two API changes: * When it comes to loading data I've had to introduce an explicit list of protocols which we can load json over (otherwise it's possible to have a uri with a scheme that makes no sense - it'd still be a valid uri but loading from it is impossible). I've used the list of protocols that addressable recognizes for now. * No matter what JSON parser you use, we now always raise a JSON::Schema::JsonParseError. Without doing this it would be tricky to handle parser errors identically, for all parsers (the other option would be to catch a long list of potential parse errors, but this seems more sensible).
voxpupuli · Mar 6, 2015 · cb52269 · cb52269
1 parent 8497d4c
commit cb52269
Show file tree

Hide file tree

Showing 11 changed files with 83 additions and 54 deletions.
diff --git a/lib/json-schema/attributes/formats/date.rb b/lib/json-schema/attributes/formats/date.rb
@@ -11,7 +11,8 @@ def self.validate(current_schema, data, fragments, processor, validator, options
           if REGEXP.match(data)
             begin
               Date.parse(data)
-            rescue Exception
+            rescue ArgumentError => e
+              raise e unless e.message == 'invalid date'
               validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
             end
           else

diff --git a/lib/json-schema/attributes/formats/date_time.rb b/lib/json-schema/attributes/formats/date_time.rb
@@ -14,18 +14,16 @@ def self.validate(current_schema, data, fragments, processor, validator, options
 
             begin
               Date.parse(parts[0])
-            rescue Exception
+            rescue ArgumentError => e
+              raise e unless e.message == 'invalid date'
               validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
               return
             end
 
-            begin
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[1].to_i > 23
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[2].to_i > 59
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[3].to_i > 59
-            rescue Exception
-              validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
-            end
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m.length < 4
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[1].to_i > 23
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[2].to_i > 59
+            validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors]) and return if m[3].to_i > 59
           else
             validation_error(processor, error_message, fragments, current_schema, self, options[:record_errors])
           end

diff --git a/lib/json-schema/attributes/not.rb b/lib/json-schema/attributes/not.rb
@@ -17,7 +17,7 @@ def self.validate(current_schema, data, fragments, processor, validator, options
             message = "The property '#{build_fragment(fragments)}' of type #{data.class} matched the disallowed schema"
             failed = false
           end
-        rescue
+        rescue ValidationError
           # Yay, we failed validation.
         end
 

diff --git a/lib/json-schema/errors/json_load_error.rb b/lib/json-schema/errors/json_load_error.rb
@@ -0,0 +1,6 @@
+module JSON
+  class Schema
+    class JsonLoadError < StandardError
+    end
+  end
+end
diff --git a/lib/json-schema/errors/schema_parse_error.rb b/lib/json-schema/errors/schema_parse_error.rb
@@ -0,0 +1,8 @@
+require 'json/common'
+
+module JSON
+  class Schema
+    class SchemaParseError < JSON::ParserError
+    end
+  end
+end
diff --git a/lib/json-schema/schema/reader.rb b/lib/json-schema/schema/reader.rb
@@ -57,8 +57,8 @@ def initialize(options = {})
       # @param location [#to_s] The location from which to read the schema
       # @return [JSON::Schema]
       # @raise [JSON::Schema::ReadRefused] if +accept_uri+ or +accept_file+
-      #   indicated the schema should not be readed
-      # @raise [JSON::ParserError] if the schema was not a valid JSON object
+      #   indicated the schema could not be read
+      # @raise [JSON::Schema::ParseError] if the schema was not a valid JSON object
       def read(location)
         uri  = Addressable::URI.parse(location.to_s)
         body = if uri.scheme.nil? || uri.scheme == 'file'

diff --git a/lib/json-schema/util/uri.rb b/lib/json-schema/util/uri.rb
@@ -1,6 +1,8 @@
 module JSON
   module Util
     module URI
+      SUPPORTED_PROTOCOLS = %w(http https ftp tftp sftp ssh svn+ssh telnet nntp gopher wais ldap prospero)
+
       def self.normalized_uri(uri)
         uri = Addressable::URI.parse(uri) unless uri.is_a?(Addressable::URI)
         # Check for absolute path

diff --git a/lib/json-schema/validator.rb b/lib/json-schema/validator.rb
@@ -9,6 +9,8 @@
 
 require 'json-schema/schema/reader'
 require 'json-schema/errors/schema_error'
+require 'json-schema/errors/schema_parse_error'
+require 'json-schema/errors/json_load_error'
 require 'json-schema/errors/json_parse_error'
 
 module JSON
@@ -54,17 +56,13 @@ def initialize(schema_data, data, opts={})
 
       # validate the schema, if requested
       if @options[:validate_schema]
-        begin
-          if @base_schema.schema["$schema"]
-            base_validator = JSON::Validator.validator_for_name(@base_schema.schema["$schema"])
-          end
-          metaschema = base_validator ? base_validator.metaschema : validator.metaschema
-          # Don't clear the cache during metaschema validation!
-          meta_validator = JSON::Validator.new(metaschema, @base_schema.schema, {:clear_cache => false})
-          meta_validator.validate
-        rescue JSON::Schema::ValidationError, JSON::Schema::SchemaError
-          raise $!
+        if @base_schema.schema["$schema"]
+          base_validator = JSON::Validator.validator_for_name(@base_schema.schema["$schema"])
         end
+        metaschema = base_validator ? base_validator.metaschema : validator.metaschema
+        # Don't clear the cache during metaschema validation!
+        meta_validator = JSON::Validator.new(metaschema, @base_schema.schema, {:clear_cache => false})
+        meta_validator.validate
       end
 
       # If the :fragment option is set, try and validate against the fragment
@@ -415,15 +413,27 @@ def json_backend=(backend)
 
       def parse(s)
         if defined?(MultiJson)
-          MultiJson.respond_to?(:adapter) ? MultiJson.load(s) : MultiJson.decode(s)
+          begin
+            MultiJson.respond_to?(:adapter) ? MultiJson.load(s) : MultiJson.decode(s)
+          rescue MultiJson::ParseError => e
+            raise JSON::Schema::JsonParseError.new(e.message)
+          end
         else
           case @@json_backend.to_s
           when 'json'
-            JSON.parse(s, :quirks_mode => true)
+            begin
+              JSON.parse(s, :quirks_mode => true)
+            rescue JSON::ParserError => e
+              raise JSON::Schema::JsonParseError.new(e.message)
+            end
           when 'yajl'
-            json = StringIO.new(s)
-            parser = Yajl::Parser.new
-            parser.parse(json) or raise JSON::Schema::JsonParseError.new("The JSON could not be parsed by yajl")
+            begin
+              json = StringIO.new(s)
+              parser = Yajl::Parser.new
+              parser.parse(json) or raise JSON::Schema::JsonParseError.new("The JSON could not be parsed by yajl")
+            rescue Yajl::ParseError => e
+              raise JSON::Schema::JsonParseError.new(e.message)
+            end
           else
             raise JSON::Schema::JsonParseError.new("No supported JSON parsers found. The following parsers are suported:\n * yajl-ruby\n * json")
           end
@@ -520,7 +530,7 @@ def initialize_schema(schema)
             schema = schema.to_array_schema
           end
           Validator.add_schema(schema)
-        rescue
+        rescue JSON::Schema::JsonParseError
           # Build a uri for it
           schema_uri = Util::URI.normalized_uri(schema)
           if !self.class.schema_loaded?(schema_uri)
@@ -551,7 +561,7 @@ def initialize_schema(schema)
         end
         Validator.add_schema(schema)
       else
-        raise "Invalid schema - must be either a string or a hash"
+        raise SchemaParseError, "Invalid schema - must be either a string or a hash"
       end
 
       schema
@@ -567,12 +577,12 @@ def initialize_data(data)
         elsif data.is_a?(String)
           begin
             data = JSON::Validator.parse(data)
-          rescue
+          rescue JSON::Schema::JsonParseError
             begin
               json_uri = Util::URI.normalized_uri(data)
               data = JSON::Validator.parse(custom_open(json_uri))
-            rescue
-              # Silently discard the error - the data will not change
+            rescue JSON::Schema::JsonLoadError
+              # Silently discard the error - use the data as-is
             end
           end
         end
@@ -582,10 +592,18 @@ def initialize_data(data)
 
     def custom_open(uri)
       uri = Util::URI.normalized_uri(uri) if uri.is_a?(String)
-      if uri.absolute? && uri.scheme != 'file'
-        open(uri.to_s).read
+      if uri.absolute? && Util::URI::SUPPORTED_PROTOCOLS.include?(uri.scheme)
+        begin
+          open(uri.to_s).read
+        rescue OpenURI::HTTPError, Timeout::Error => e
+          raise JSON::Schema::JsonLoadError, e.message
+        end
       else
-        File.read(Addressable::URI.unescape(uri.path))
+        begin
+          File.read(Addressable::URI.unescape(uri.path))
+        rescue SystemCallError => e
+          raise JSON::Schema::JsonLoadError, e.message
+        end
       end
     end
   end

diff --git a/test/test_helper.rb b/test/test_helper.rb
@@ -34,14 +34,4 @@ def refute_valid(schema, data, options = {})
     errors = JSON::Validator.fully_validate(schema, data, options)
     refute_equal([], errors, "#{data.inspect} should be invalid for schema:\n#{schema.inspect}")
   end
-
-  def parser_error
-    if defined?(::Yajl)
-      Yajl::ParseError
-    elsif defined?(::MultiJson)
-      MultiJson::ParseError
-    else
-      JSON::ParserError
-    end
-  end
 end
diff --git a/test/test_initialize_data.rb b/test/test_initialize_data.rb
@@ -10,11 +10,11 @@ def test_parse_character_string
 
     assert(JSON::Validator.validate(schema, data, :parse_data => false))
 
-    assert_raises(parser_error) do
+    assert_raises(JSON::Schema::JsonParseError) do
       JSON::Validator.validate(schema, data, :json => true)
     end
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_integer_string
@@ -27,7 +27,7 @@ def test_parse_integer_string
 
     assert(JSON::Validator.validate(schema, data, :json => true))
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_hash_string
@@ -40,7 +40,7 @@ def test_parse_hash_string
 
     assert(JSON::Validator.validate(schema, data, :json => true))
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_json_string
@@ -53,7 +53,7 @@ def test_parse_json_string
 
     assert(JSON::Validator.validate(schema, data, :json => true))
 
-    assert_raises(Errno::ENOENT) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_valid_uri_string
@@ -66,7 +66,7 @@ def test_parse_valid_uri_string
 
     assert(JSON::Validator.validate(schema, data, :parse_data => false))
 
-    assert_raises(parser_error) do
+    assert_raises(JSON::Schema::JsonParseError) do
       JSON::Validator.validate(schema, data, :json => true)
     end
 
@@ -83,11 +83,17 @@ def test_parse_invalid_uri_string
 
     assert(JSON::Validator.validate(schema, data, :parse_data => false))
 
-    assert_raises(parser_error) do
+    stub_request(:get, "foo.bar").to_return(:status => [500, "Internal Server Error"])
+
+    assert(JSON::Validator.validate(schema, data))
+
+    assert(JSON::Validator.validate(schema, data, :parse_data => false))
+
+    assert_raises(JSON::Schema::JsonParseError) do
       JSON::Validator.validate(schema, data, :json => true)
     end
 
-    assert_raises(Timeout::Error) { JSON::Validator.validate(schema, data, :uri => true) }
+    assert_raises(JSON::Schema::JsonLoadError) { JSON::Validator.validate(schema, data, :uri => true) }
   end
 
   def test_parse_integer

diff --git a/test/test_schema_loader.rb b/test/test_schema_loader.rb
@@ -67,7 +67,7 @@ def test_parse_error
 
     reader = JSON::Schema::Reader.new
 
-    assert_raises(parser_error) do
+    assert_raises(JSON::Schema::JsonParseError) do
       reader.read(ADDRESS_SCHEMA_URI)
     end
   end