Union fix

README.md

@@ -1,60 +1,56 @@
 # Thrift::Validator
 
-Recursive [thrift][] struct validator. The thrift library out of the
-box does not validated nested structs, this library fixes that
-problem. It does not monkey-patch the Thrift code. Instead this
-library includes a class to recursively validate objects.
+Recursive [thrift][] struct validator. The thrift library out of the box does
+not validated nested structs; This library fixes that problem. Rather than
+monkey-patching Thrift, this library includes a class to validate objects
+recursively.
 
-Here's an example from this libraries test. Take a look at this
-protocol:
+Here's an example from this library's tests. Given the following schema:
 
 ```thrift
 struct SimpleStruct  {
-  1: required string required_string
-  2: optional string optional_string
+  1: required string required_string,
+  2: optional string optional_string,
 }
 
 struct NestedExample {
-  1: required SimpleStruct required_struct
-  2: optional SimpleStruct optional_struct
+  1: required SimpleStruct required_struct,
+  2: optional SimpleStruct optional_struct,
 }
 ```
 
-Then ran some ruby:
+This library provides:
 
 ```ruby
 struct = SimpleStruct.new
-nested = NestedStruct.new required_struct: struct
+nested = NestedStruct.new(required_struct: struct)
 
-# Method defined by the thrift library
+# Method defined by the Thrift library
 struct.validate # => Thrift::ProtocolException
 
-# Thrift only validate fields set as required on this instance.
-# so since required_struct is non-nil validation succeeds.
-# Also note that thrift does not validate the semantics of
-# the assigned objects, so also assigning and invalid struct will
-# pass its validation method.
+# Thrift only validates fields set as required on this instance. Since
+# `required_struct` is non-nil, validation succeeds. Also note that Thrift
+# does not validate semantics of the assigned objects, so assigning an
+# invalid struct will pass its validation method.
 nested.validate # => true
 
 # With the validator
-validator = Thrift::Validator.new
-validator.validate nested # => Thrift::ProtocolException
+Thrift::Validator.new.validate(nested) # => Thrift::ProtocolException
 ```
 
-## Semantics
+## Semantics enforced
 
-* Original thrift validation smenatics enforces
-* `optional` or `required` `struct` types pass validation
-* `optional` or `required` `list<struct>` items pass validation
-* `optional` or `required` `set<struct>` items pass validation
-* `optional` or `required` `map` type using a `struct` for key or
-  value pass validation
+* all original Thrift validation semantics
+* `optional` or `required` `struct` types
+* `optional` or `required` `list<struct>` items
+* `optional` or `required` `set<struct>` items
+* `optional` or `required` `map` types with `struct` keys and/or values
 
 ## Exception handling
 
 Due to the recursive nature of `thrift-validator`, the validation
-errors raised by Thrift have become less than helpful. In order
-to provide more information, the `Thrift::ProtocolException`'s
+errors raised by Thrift become less than helpful. In order
+to provide more information, the resulting `Thrift::ProtocolException`
 message is prefixed with the type where the error occurred. For
 example:
 
@@ -68,8 +64,8 @@ Thrift::ProtocolException: Required field required_string is unset!
 Thrift::ProtocolException: SimpleStruct: Required field required_string is unset!
 ```
 
-This feature becomes especially useful with nested structures,
-where the validation may fail at any given depth.
+This feature becomes especially useful with nested structs, where validation
+may fail at any depth.
 
 ## Installation
 
@@ -89,13 +85,14 @@ Or install it yourself as:
 
 ## Testing
 
-First install `thrift` compilier on your platform.
+First, install a `thrift` compiler on your platform (e.g., `brew install
+thrift`, `sudo apt install thrift`). Then run:
 
-	$ make test
+    $ rake test
 
 ## Contributing
 
-1. Fork it ( https://github.com/saltside/thrift-validator/fork )
+1. Fork it ( https://github.com/saltside/thrift-validator-ruby/fork )
 2. Create your feature branch (`git checkout -b my-new-feature`)
 3. Commit your changes (`git commit -am 'Add some feature'`)
 4. Push to the branch (`git push origin my-new-feature`)

Rakefile

@@ -1,8 +1,23 @@
-require "bundler/gem_tasks"
-require "rake/testtask"
+require 'bundler/gem_tasks'
+require 'rake/testtask'
+
+THRIFT_IN = 'test/test.thrift'
+THRIFT_OUT = Rake::FileList['test/gen-rb/test_constants.rb', 'test/gen-rb/test_types.rb']
+
+THRIFT_OUT.each do |output|
+  file output => THRIFT_IN do
+    sh 'thrift', '-o', 'test', '--gen', 'rb', THRIFT_IN
+  end
+end
+
+desc 'Cleans generated files from workspace'
+task :clean do
+  rm THRIFT_OUT
+end
 
 Rake::TestTask.new do |t|
   t.test_files = Rake::FileList['test/**/*_test.rb']
 end
+task test: THRIFT_OUT # add dependency
 
 task default: :test

lib/thrift/validator.rb

@@ -3,47 +3,30 @@
 
 module Thrift
   class Validator
-    def validate(source)
-      begin
-        source.validate
-      rescue Thrift::ProtocolException => ex
-        message = "#{source.class.name}: #{ex}"
-        raise Thrift::ProtocolException.new(ex.type, message)
-      end
-
-      source.struct_fields.each_pair do |_, field|
-        type, name = field.fetch(:type), field.fetch(:name)
-        case type
-        when Types::STRUCT
-          next if source.kind_of?(Union) && source.get_set_field.to_s != name
-
-          if source.send(name)
-            validate source.send(name)
-          end
-        when Types::LIST, Types::SET
-          if recurse? field.fetch(:element)
-            Array(source.send(name)).each do |item|
-              validate item
-            end
-          end
-        when Types::MAP
-          if recurse? field.fetch(:key)
-            Hash(source.send(name)).each_key do |key_value|
-              validate key_value
-            end
-          end
+    DEFAULT_TYPE = Thrift::ProtocolException::UNKNOWN
 
-          if recurse? field.fetch(:value)
-            Hash(source.send(name)).each_value do |value_value|
-              validate value_value
-            end
+    # @param structs [Object] any Thrift value -- struct, primitive, or a collection thereof
+    # @raise [Thrift::ProtocolException] if any deviation from schema was detected
+    # @return [nil] if no problems were detected; note that this does not include type checks
+    def validate(structs)
+      # handle anything -- Struct, Union, List, Set, Map, primitives...
+      Array(structs).flatten.each do |struct|
+        begin
+          # only Structs/Unions can be validated (see Thrift.type_checking for another option)
+          next unless struct.is_a?(Struct_Union)
+          # raises a ProtocolException if this specific struct is invalid
+          struct.validate
+          # recursively validate all fields except unset union fields
+          struct.struct_fields.each_value do |f|
+            next if struct.is_a?(Union) && struct.get_set_field != f[:name].to_sym
+            validate(struct.send(f[:name]))
           end
+        rescue ProtocolException => e
+          raise ProtocolException.new(e.type, "#{struct.class}: #{e.message}")
+        rescue => e # union validation raises StandardError...
+          raise ProtocolException.new(DEFAULT_TYPE, "#{struct.class}: #{e.message}")
         end
       end
     end
-
-    def recurse?(field)
-      field[:class] && field[:class] < ::Thrift::Struct
-    end
   end
 end

lib/thrift/validator/version.rb

@@ -1,5 +1,5 @@
 module Thrift
   class Validator
-    VERSION = "0.1.3"
+    VERSION = '0.2.0'.freeze
   end
 end

test/acceptance_test.rb

@@ -126,20 +126,18 @@ def test_passes_if_optional_and_required_valid_map_values_are_given
 
   def test_fails_if_no_union_fields_set
     union = UnionExample.new
-    assert_raises(StandardError) do
-      Thrift::Validator.new.validate(union)
-    end
+    refute_valid union
   end
 
   def test_fails_if_union_set_field_is_invalid
     union = UnionExample.new
-    union.primary = SimpleStruct.new
+    union.primary = [SimpleStruct.new]
     refute_valid union
   end
 
   def test_passes_if_union_set_field_is_valid
     union = UnionExample.new
-    union.primary = SimpleStruct.new required_string: 'foo'
+    union.primary = [SimpleStruct.new(required_string: 'foo')]
     assert_valid union
   end
 

test.thrift → test/test.thrift

@@ -37,6 +37,6 @@ struct MapValueExample {
 }
 
 union UnionExample {
-  1: SimpleStruct primary
+  1: list<SimpleStruct> primary
   2: string alternate
 }

test/test_helper.rb

@@ -1,8 +1,6 @@
 require 'bundler/setup'
 
-root = File.expand_path '../..', __FILE__
-
-$LOAD_PATH << "#{root}/vendor/gen-rb"
+$LOAD_PATH << File.expand_path('../gen-rb', __FILE__)
 
 require 'thrift-validator'
 

thrift-validator.gemspec

@@ -4,22 +4,22 @@ $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
 require 'thrift/validator/version'
 
 Gem::Specification.new do |spec|
-  spec.name          = "thrift-validator"
+  spec.name          = 'thrift-validator'
   spec.version       = Thrift::Validator::VERSION
-  spec.authors       = ["ahawkins"]
-  spec.email         = ["[email protected]"]
+  spec.authors       = ['ahawkins', 'anujdas']
+  spec.email         = ['[email protected]', '[email protected]']
   spec.summary       = %q{Recursive thrift struct validator}
   spec.description   = %q{}
-  spec.homepage      = "https://github.com/saltside/thrift-validator-ruby"
-  spec.license       = "MIT"
+  spec.homepage      = 'https://github.com/saltside/thrift-validator-ruby'
+  spec.license       = 'MIT'
 
   spec.files         = `git ls-files -z`.split("\x0")
-  spec.executables   = spec.files.grep(%r{^bin/}) { |f| File.basename(f) }
-  spec.test_files    = spec.files.grep(%r{^(test|spec|features)/})
-  spec.require_paths = ["lib"]
+  spec.test_files    = spec.files.grep(%r{^test/})
+  spec.require_paths = ['lib']
 
-  spec.add_dependency "thrift"
+  spec.add_dependency 'thrift'
 
-  spec.add_development_dependency "bundler", "~> 1.7"
-  spec.add_development_dependency "rake", "~> 10.0"
+  spec.add_development_dependency 'bundler', '~> 1.7'
+  spec.add_development_dependency 'rake', '~> 10.0'
+  spec.add_development_dependency 'minitest', '~> 5.11'
 end