wip more validations

Author: Samze

app/messages/deployment_create_message.rb

@@ -10,20 +10,21 @@ class DeploymentCreateMessage < MetadataBaseMessage
       options
     ]
 
+    ALLOWED_OPTION_KEYS = %i[
+      canary
+      max_in_flight
+    ].freeze
+
     ALLOWED_STEP_KEYS = [
       :instance_weight
-    ]
+    ].freeze
 
     validates_with NoAdditionalKeysValidator
     validates :strategy,
               inclusion: { in: %w[rolling canary], message: "'%<value>s' is not a supported deployment strategy" },
               allow_nil: true
     validate :mutually_exclusive_droplet_sources
 
-    # validates :options,
-    #           allow_nil: true,
-    #           hash: true
-
     validate :validate_options
 
     def app_guid
@@ -51,13 +52,16 @@ def mutually_exclusive_droplet_sources
     end
 
     def validate_options
-      return unless options.present?
+      return if options.blank?
 
       unless options.is_a?(Hash)
         errors.add(:options, 'must be an object')
         return
       end
 
+      disallowed_keys = options.keys - ALLOWED_OPTION_KEYS
+      errors.add(:options, "has unsupported key(s): #{disallowed_keys.join(', ')}") if disallowed_keys.present?
+
       validate_max_in_flight if options[:max_in_flight]
       validate_canary if options[:canary]
     end
@@ -98,6 +102,12 @@ def validate_steps
         errors.add(:'options.canary.steps', "has unsupported key(s): #{disallowed_keys.join(', ')}") if disallowed_keys.present?
       end
 
+      validate_step_instance_weights
+    end
+
+    def validate_step_instance_weights
+      steps = options[:canary][:steps]
+
       errors.add(:'options.canary.steps', 'missing key: "instance_weight"') if steps.any? { |step| !step.key?(:instance_weight) }
 
       if steps.any? { |step| !step[:instance_weight].is_a?(Integer) }
@@ -107,7 +117,7 @@ def validate_steps
 
       errors.add(:'options.canary.steps.instance_weight', 'must be an Integer between 1-100 (inclusive)') if steps.any? { |step| (1..100).exclude?(step[:instance_weight]) }
 
-      weights = steps.map { |step| step[:instance_weight] }
+      weights = steps.pluck(:instance_weight)
       return unless weights.sort != weights
 
       errors.add(:'options.canary.steps.instance_weight', 'must be sorted in ascending order')

app/messages/validators.rb

@@ -257,46 +257,6 @@ def validate(record)
     end
   end
 
-  class DeploymentOptionsValidator < ActiveModel::Validator
-    def validate(record)
-      options = record.options
-      return unless options.present?
-
-      unless options.is_a?(Hash)
-        record.errors.add(:options, 'must be an object')
-        return
-      end
-
-      max_in_flight = options[:max_in_flight]
-
-      # TODO: figure out if we should allow max_in_flight set to nil explicitly
-      record.errors.add(:max_in_flight, 'must be an integer greater than 0') if options.key?(:max_in_flight) && (!max_in_flight.is_a?(Integer) || max_in_flight < 1)
-
-      canary_options = options[:canary]
-      return if canary_options.nil?
-
-      unless canary_options.is_a?(Hash)
-        record.errors.add(:'options.canary', 'must be an object')
-        return
-      end
-
-      # binding.pry
-
-      strategy = record.strategy
-
-      if canary_options && strategy != 'canary'
-        record.errors.add(:'options.canary', 'are only valid for Canary deployments')
-        return
-      end
-
-      steps = canary_options[:steps]
-
-      return unless steps && (!steps.is_a?(Array) || steps.any? { |step| !step.is_a?(Hash) })
-
-      record.errors.add(:'options.canary.steps', 'must be an array of objects')
-    end
-  end
-
   class ToOneRelationshipValidator < ActiveModel::EachValidator
     def validate_each(record, attribute, relationship)
       if has_correct_structure?(relationship)

spec/unit/messages/deployment_create_message_spec.rb

@@ -167,6 +167,13 @@ module VCAP::CloudController
           expect(message.errors[:'options.canary']).to include('are only valid for Canary deployments')
         end
 
+        it 'errors when there is an unknown option' do
+          body['options'] = { foo: 'bar', baz: 'boo' }
+          message = DeploymentCreateMessage.new(body)
+          expect(message).not_to be_valid
+          expect(message.errors[:options]).to include('has unsupported key(s): foo, baz')
+        end
+
         context 'steps' do
           it 'errors when is not an array' do
             body['options'] = { canary: { steps: 'foo' } }
@@ -175,7 +182,7 @@ module VCAP::CloudController
             expect(message.errors[:'options.canary.steps']).to include('must be an array of objects')
           end
 
-          it 'is valid when is an array' do
+          it 'is valid when is an empty array' do
             body['options'] = { canary: { steps: [] } }
             message = DeploymentCreateMessage.new(body)
             expect(message).to be_valid
@@ -187,12 +194,6 @@ module VCAP::CloudController
             expect(message).to be_valid
           end
 
-          it 'is valid if instance_weights are Integers between 1-100 in ascending order' do
-            body['options'] = { canary: { steps: [{ instance_weight: 1 }, { instance_weight: 2 }, { instance_weight: 50 }, { instance_weight: 99 }, { instance_weight: 100 }] } }
-            message = DeploymentCreateMessage.new(body)
-            expect(message).to be_valid
-          end
-
           it 'errors if not an array of objects' do
             body['options'] = { canary: { steps: [{ instance_weight: 1 }, 'foo'] } }
             message = DeploymentCreateMessage.new(body)
@@ -209,20 +210,53 @@ module VCAP::CloudController
           end
 
           context 'instance_weights' do
+            it 'is valid if instance_weights are Integers between 1-100 in ascending order' do
+              body['options'] = { canary: { steps: [{ instance_weight: 1 }, { instance_weight: 2 }, { instance_weight: 50 }, { instance_weight: 99 }, { instance_weight: 100 }] } }
+              message = DeploymentCreateMessage.new(body)
+              expect(message).to be_valid
+            end
+
+            it 'is valid if there are duplicate instance_weights' do
+              body['options'] = { canary: { steps: [{ instance_weight: 10 }, { instance_weight: 10 }, { instance_weight: 50 }, { instance_weight: 50 }] } }
+              message = DeploymentCreateMessage.new(body)
+              expect(message).to be_valid
+            end
+
             it 'errors if steps are missing instance_weight' do
               body['options'] = { canary: { steps: [{ instance_weight: 1 }, { foo: 'bar' }] } }
               message = DeploymentCreateMessage.new(body)
               expect(message).not_to be_valid
               expect(message.errors[:'options.canary.steps']).to include('missing key: "instance_weight"')
             end
 
-            it 'errors if any instance_weight is less than or equal to 0' do
+            it 'errors if steps are missing instance_weight with empty hash' do
+              body['options'] = { canary: { steps: [{ instance_weight: 1 }, {}] } }
+              message = DeploymentCreateMessage.new(body)
+              expect(message).not_to be_valid
+              expect(message.errors[:'options.canary.steps']).to include('missing key: "instance_weight"')
+            end
+
+            it 'errors if any instance_weight is not an Integer' do
+              body['options'] = { canary: { steps: [{ instance_weight: 'foo' }] } }
+              message = DeploymentCreateMessage.new(body)
+              expect(message).not_to be_valid
+              expect(message.errors[:'options.canary.steps.instance_weight']).to include('must be an Integer between 1-100 (inclusive)')
+            end
+
+            it 'errors if any instance_weight is equal to 0' do
               body['options'] = { canary: { steps: [{ instance_weight: 0 }, { instance_weight: 50 }] } }
               message = DeploymentCreateMessage.new(body)
               expect(message).not_to be_valid
               expect(message.errors[:'options.canary.steps.instance_weight']).to include('must be an Integer between 1-100 (inclusive)')
             end
 
+            it 'errors if any instance_weight is less than 0' do
+              body['options'] = { canary: { steps: [{ instance_weight: -5 }, { instance_weight: 50 }] } }
+              message = DeploymentCreateMessage.new(body)
+              expect(message).not_to be_valid
+              expect(message.errors[:'options.canary.steps.instance_weight']).to include('must be an Integer between 1-100 (inclusive)')
+            end
+
             it 'errors if any instance_weight is greater than 100' do
               body['options'] = { canary: { steps: [{ instance_weight: 50 }, { instance_weight: 101 }] } }
               message = DeploymentCreateMessage.new(body)
@@ -237,8 +271,7 @@ module VCAP::CloudController
               expect(message.errors[:'options.canary.steps.instance_weight']).to include('must be sorted in ascending order')
             end
 
-            it 'errors if any instance_weights are not an Integer' do
-              # TODO: should we coerce values like 25.0 to 25? only error on 25.1 etc?
+            it 'errors if any instance_weights are not a non-integer numeric' do
               body['options'] = { canary: { steps: [{ instance_weight: 2 }, { instance_weight: 25.0 }] } }
               message = DeploymentCreateMessage.new(body)
               expect(message).not_to be_valid
@@ -300,7 +333,7 @@ module VCAP::CloudController
 
       context 'when options is specified, but not max_in_flight' do
         before do
-          body['options'] = { other_key: 'other_value' }
+          body['options'] = { canary: nil }
         end
 
         it 'returns the default value of 1' do
@@ -334,13 +367,5 @@ module VCAP::CloudController
         end
       end
     end
-
-    context 'validations' do
-      context 'when one instance_weight is below 0'
-      context 'when one instance_weight is above 100'
-      context 'when an instance weight is a decimal or non-integer'
-      context 'when canary steps is an empty array'
-      context 'when canary steps are non-increasing (e.g. 1,50,20)'
-    end
   end
 end