From 9291b57764341007ae68213514b4d86f2592d32c Mon Sep 17 00:00:00 2001
From: Alberto Rios <arios@pivotal.io>
Date: Wed, 12 Jul 2017 16:51:13 +0100
Subject: [PATCH 01/15] Adding update instance schema

[#146082145]

Signed-off-by: Luis Urraca <lurraca@pivotal.io>
---
 app/models/services/service_plan.rb           |   4 +-
 app/presenters/v2/service_plan_presenter.rb   |   6 +
 ...instance_update_schema_to_service_plans.rb |   5 +
 .../service_brokers/service_manager.rb        |   3 +-
 .../service_brokers/v2/catalog_schemas.rb     |   3 +-
 .../broker_api_v2.13_spec.rb                  |  13 +-
 spec/api/api_version_spec.rb                  |   2 +-
 spec/api/documentation/events_api_spec.rb     |   6 +-
 .../documentation/service_plans_api_spec.rb   |   2 +-
 spec/api/documentation/services_api_spec.rb   |   2 +-
 .../services/service_plans_controller_spec.rb |   2 +-
 .../service_brokers/service_manager_spec.rb   |  14 +-
 .../v2/catalog_schemas_spec.rb                | 384 ++++++++++--------
 .../unit/models/services/service_plan_spec.rb |  28 +-
 .../v2/service_plan_presenter_spec.rb         |  48 ++-
 15 files changed, 324 insertions(+), 198 deletions(-)
 create mode 100644 db/migrations/20170712153045_add_instance_update_schema_to_service_plans.rb

diff --git a/app/models/services/service_plan.rb b/app/models/services/service_plan.rb
index 557969f103f..6e74d6d8c6e 100644
--- a/app/models/services/service_plan.rb
+++ b/app/models/services/service_plan.rb
@@ -6,11 +6,11 @@ class ServicePlan < Sequel::Model
 
     add_association_dependencies service_plan_visibilities: :destroy
 
-    export_attributes :name, :free, :description, :service_guid, :extra, :unique_id, :public, :bindable, :active, :create_instance_schema
+    export_attributes :name, :free, :description, :service_guid, :extra, :unique_id, :public, :bindable, :active, :create_instance_schema, :update_instance_schema
 
     export_attributes_from_methods bindable: :bindable?
 
-    import_attributes :name, :free, :description, :service_guid, :extra, :unique_id, :public, :bindable, :create_instance_schema
+    import_attributes :name, :free, :description, :service_guid, :extra, :unique_id, :public, :bindable, :create_instance_schema, :update_instance_schema
 
     strip_attributes :name
 
diff --git a/app/presenters/v2/service_plan_presenter.rb b/app/presenters/v2/service_plan_presenter.rb
index 6b6652a4ed5..6d4afdf47ce 100644
--- a/app/presenters/v2/service_plan_presenter.rb
+++ b/app/presenters/v2/service_plan_presenter.rb
@@ -12,6 +12,7 @@ def entity_hash(controller, plan, opts, depth, parents, orphans=nil)
           schemas = present_schemas(plan)
           entity.merge!(schemas)
           entity.delete('create_instance_schema')
+          entity.delete('update_instance_schema')
 
           entity
         end
@@ -20,11 +21,16 @@ def entity_hash(controller, plan, opts, depth, parents, orphans=nil)
 
         def present_schemas(plan)
           create_instance_schema = parse_schema(plan.create_instance_schema)
+          update_instance_schema = parse_schema(plan.update_instance_schema)
           {
             'schemas' => {
               'service_instance' => {
                 'create' => {
                   'parameters' => create_instance_schema
+                },
+                'update' => {
+                    'parameters' => update_instance_schema
+
                 }
               }
             }
diff --git a/db/migrations/20170712153045_add_instance_update_schema_to_service_plans.rb b/db/migrations/20170712153045_add_instance_update_schema_to_service_plans.rb
new file mode 100644
index 00000000000..ffbdbd39730
--- /dev/null
+++ b/db/migrations/20170712153045_add_instance_update_schema_to_service_plans.rb
@@ -0,0 +1,5 @@
+Sequel.migration do
+  change do
+    add_column :service_plans, :update_instance_schema, :text, null: true
+  end
+end
diff --git a/lib/services/service_brokers/service_manager.rb b/lib/services/service_brokers/service_manager.rb
index 600e5ef7716..aa30621abcd 100644
--- a/lib/services/service_brokers/service_manager.rb
+++ b/lib/services/service_brokers/service_manager.rb
@@ -65,7 +65,8 @@ def update_or_create_plans(catalog)
           bindable:    catalog_plan.bindable,
           active:      true,
           extra:       catalog_plan.metadata.try(:to_json),
-          create_instance_schema: catalog_plan.schemas.create_instance.try(:to_json)
+          create_instance_schema: catalog_plan.schemas.create_instance.try(:to_json),
+          update_instance_schema: catalog_plan.schemas.update_instance.try(:to_json)
         })
         @services_event_repository.with_service_plan_event(plan) do
           plan.save(changed: true)
diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index 374742e08e6..bb47ea3801b 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -3,11 +3,12 @@
 module VCAP::Services::ServiceBrokers::V2
   MAX_SCHEMA_SIZE = 65_536
   class CatalogSchemas
-    attr_reader :errors, :create_instance
+    attr_reader :errors, :create_instance, :update_instance
 
     def initialize(schema)
       @errors = VCAP::Services::ValidationErrors.new
       validate_and_populate_create_instance(schema)
+      @update_instance = {}
     end
 
     def valid?
diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index 76d064d19fb..923f6ee5ba5 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -5,12 +5,14 @@
     include VCAP::CloudController::BrokerApiHelper
 
     let(:create_instance_schema) { { 'type': 'object' } }
+    let(:update_instance_schema) { {} }
     let(:schemas) {
       {
         'service_instance' => {
           'create' => {
             'parameters' => create_instance_schema
-          }
+          },
+          'update' => {}
         }
       }
     }
@@ -23,7 +25,7 @@
       @broker = VCAP::CloudController::ServiceBroker.find guid: @broker_guid
     end
 
-    context 'when a broker catalog defines plan schemas' do
+    context 'when a broker catalog defines a create plan schema' do
       let(:create_instance_schema) {
         {
           '$schema' => 'http://json-schema.org/draft-04/schema#',
@@ -37,7 +39,10 @@
             json_headers(admin_headers))
 
         parsed_body = MultiJson.load(last_response.body)
-        expect(parsed_body['entity']['schemas']).to eq schemas
+        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => {
+            'create' => { 'parameters' => {'$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object'} },
+            'update' => { 'parameters' => {} }
+        } })
       end
     end
 
@@ -48,7 +53,7 @@
             json_headers(admin_headers))
 
         parsed_body = MultiJson.load(last_response.body)
-        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => { 'create' => { 'parameters' => {} } } })
+        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => { 'create' => { 'parameters' => {} }, 'update' => { 'parameters' => {} } } })
       end
     end
 
diff --git a/spec/api/api_version_spec.rb b/spec/api/api_version_spec.rb
index 9a57f522a3f..6bdfda22eac 100644
--- a/spec/api/api_version_spec.rb
+++ b/spec/api/api_version_spec.rb
@@ -2,7 +2,7 @@
 require 'vcap/digester'
 
 RSpec.describe 'Stable API warning system', api_version_check: true do
-  API_FOLDER_CHECKSUM = 'cfb1df934fc87e70db6e62322612d860bc7dadc7'.freeze
+  API_FOLDER_CHECKSUM = '5d13e2d3b8855457ad523efceb8cee63e94bbda8'.freeze
 
   it 'tells the developer if the API specs change' do
     api_folder = File.expand_path('..', __FILE__)
diff --git a/spec/api/documentation/events_api_spec.rb b/spec/api/documentation/events_api_spec.rb
index aa40a54f2b3..7d21884d148 100644
--- a/spec/api/documentation/events_api_spec.rb
+++ b/spec/api/documentation/events_api_spec.rb
@@ -622,7 +622,8 @@
         public:                 true,
         active:                 true,
         bindable:               true,
-        create_instance_schema: '{}'
+        create_instance_schema: '{}',
+        update_instance_schema: '{}'
       )
       service_event_repository.with_service_plan_event(new_plan) do
         new_plan.save
@@ -648,7 +649,8 @@
           'public'                 => new_plan.public,
           'active'                 => new_plan.active,
           'bindable'               => new_plan.bindable,
-          'create_instance_schema' => new_plan.create_instance_schema
+          'create_instance_schema' => new_plan.create_instance_schema,
+          'update_instance_schema' => new_plan.update_instance_schema
         }
       }
     end
diff --git a/spec/api/documentation/service_plans_api_spec.rb b/spec/api/documentation/service_plans_api_spec.rb
index 43577ed3cd9..bc6adc90d9a 100644
--- a/spec/api/documentation/service_plans_api_spec.rb
+++ b/spec/api/documentation/service_plans_api_spec.rb
@@ -11,7 +11,7 @@
     field :guid, 'The guid of the service plan', required: false
     field :public, 'A boolean describing that the plan is visible to the all users', required: false, default: true
 
-    expected_attributes = VCAP::CloudController::ServicePlan.new.export_attrs - [:create_instance_schema] + [:schemas]
+    expected_attributes = VCAP::CloudController::ServicePlan.new.export_attrs - [:create_instance_schema] - [:update_instance_schema] + [:schemas]
 
     standard_model_list(:service_plans, VCAP::CloudController::ServicePlansController, export_attributes: expected_attributes)
     standard_model_get(:service_plans, export_attributes: expected_attributes)
diff --git a/spec/api/documentation/services_api_spec.rb b/spec/api/documentation/services_api_spec.rb
index e8298aecaf1..a807a754db5 100644
--- a/spec/api/documentation/services_api_spec.rb
+++ b/spec/api/documentation/services_api_spec.rb
@@ -72,7 +72,7 @@
         VCAP::CloudController::ServicePlan.make(service: service)
       end
 
-      expected_attributes = VCAP::CloudController::ServicePlan.new.export_attrs - [:create_instance_schema] + [:schemas]
+      expected_attributes = VCAP::CloudController::ServicePlan.new.export_attrs - [:create_instance_schema] - [:update_instance_schema] + [:schemas]
       standard_model_list :service_plan, VCAP::CloudController::ServicePlansController, { outer_model: :service, export_attributes: expected_attributes }
     end
   end
diff --git a/spec/unit/controllers/services/service_plans_controller_spec.rb b/spec/unit/controllers/services/service_plans_controller_spec.rb
index 9955efd180e..075692ae94c 100644
--- a/spec/unit/controllers/services/service_plans_controller_spec.rb
+++ b/spec/unit/controllers/services/service_plans_controller_spec.rb
@@ -181,7 +181,7 @@ module VCAP::CloudController
         schemas = decoded_response.fetch('entity')['schemas']
 
         expect(schemas).to_not be_nil
-        expect(schemas).to eq({ 'service_instance' => { 'create' => { 'parameters' => {} } } })
+        expect(schemas).to eq({ 'service_instance' => { 'create' => { 'parameters' => {} }, 'update' => { 'parameters' => {} } } })
       end
 
       context 'when the plan does not set bindable' do
diff --git a/spec/unit/lib/services/service_brokers/service_manager_spec.rb b/spec/unit/lib/services/service_brokers/service_manager_spec.rb
index affd97b1330..12e8c8ddafb 100644
--- a/spec/unit/lib/services/service_brokers/service_manager_spec.rb
+++ b/spec/unit/lib/services/service_brokers/service_manager_spec.rb
@@ -40,6 +40,9 @@ module VCAP::Services::ServiceBrokers
                       'parameters' => {
                           '$schema' => 'http://json-schema.org/draft-04/schema', 'type' => 'object'
                       }
+                  },
+                  'update' => {
+                      'parameters' => {}
                   }
               }
           }
@@ -172,6 +175,7 @@ module VCAP::Services::ServiceBrokers
           'active' => service_plan.active,
           'bindable' => true,
           'create_instance_schema' => '{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}',
+          'update_instance_schema' => '{}'
         })
       end
 
@@ -207,6 +211,7 @@ module VCAP::Services::ServiceBrokers
           expect(plan.description).to eq(plan_description)
           expect(JSON.parse(plan.extra)).to eq({ 'cost' => '0.0' })
           expect(plan.create_instance_schema).to eq('{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}')
+          expect(plan.update_instance_schema).to eq('{}')
 
           expect(plan.free).to be false
         end
@@ -245,6 +250,7 @@ module VCAP::Services::ServiceBrokers
           service_manager.sync_services_and_plans(catalog)
           plan = VCAP::CloudController::ServicePlan.last
           expect(plan.create_instance_schema).to be_nil
+          expect(plan.update_instance_schema).to eq('{}')
         end
       end
 
@@ -255,6 +261,7 @@ module VCAP::Services::ServiceBrokers
           service_manager.sync_services_and_plans(catalog)
           plan = VCAP::CloudController::ServicePlan.last
           expect(plan.create_instance_schema).to be_nil
+          expect(plan.update_instance_schema).to eq('{}')
         end
       end
 
@@ -265,6 +272,7 @@ module VCAP::Services::ServiceBrokers
           service_manager.sync_services_and_plans(catalog)
           plan = VCAP::CloudController::ServicePlan.last
           expect(plan.create_instance_schema).to be_nil
+          expect(plan.update_instance_schema).to eq('{}')
         end
       end
 
@@ -326,7 +334,8 @@ module VCAP::Services::ServiceBrokers
               unique_id: plan_id,
               free: true,
               bindable: false,
-              create_instance_schema: nil
+              create_instance_schema: nil,
+              update_instance_schema: nil
             )
           end
 
@@ -336,6 +345,7 @@ module VCAP::Services::ServiceBrokers
             expect(plan.free).to be true
             expect(plan.bindable).to be false
             expect(plan.create_instance_schema).to be_nil
+            expect(plan.update_instance_schema).to be_nil
 
             expect {
               service_manager.sync_services_and_plans(catalog)
@@ -347,6 +357,7 @@ module VCAP::Services::ServiceBrokers
             expect(plan.free).to be false
             expect(plan.bindable).to be true
             expect(plan.create_instance_schema).to eq('{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}')
+            expect(plan.update_instance_schema).to eq('{}')
           end
 
           it 'creates service audit events for each service plan updated' do
@@ -372,6 +383,7 @@ module VCAP::Services::ServiceBrokers
               'bindable' => true,
               'free' => false,
               'create_instance_schema' => '{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}',
+              'update_instance_schema' => '{}'
             })
           end
 
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index c7145ae2ac9..38d14eaa8d0 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -2,226 +2,268 @@
 
 module VCAP::Services::ServiceBrokers::V2
   RSpec.describe CatalogSchemas do
-    describe 'initializing' do
-      let(:path) { 'service_instance.create.parameters' }
-      let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
-      let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => create_instance_schema } } } }
+    describe 'initializing catalog schemas' do
 
       subject { CatalogSchemas.new(attrs) }
 
-      context 'when attrs have nil value' do
-        {
-          'Schemas': nil,
-          'Schemas service_instance': { 'service_instance' => nil },
-          'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },
-          'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
-        }.each do |name, test|
-          context "for property #{name}" do
-            let(:attrs) { test }
-
-            its(:create_instance) { should be_nil }
-            its(:errors) { should be_empty }
-            its(:valid?) { should be true }
+      context 'when catalog has a create schema' do
+        let(:path) { 'service_instance.create.parameters' }
+        let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
+        let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => create_instance_schema } } } }
+
+        context 'when attrs have nil value' do
+          {
+              'Schemas': nil,
+              'Schemas service_instance': { 'service_instance' => nil },
+              'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },
+              'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:create_instance) { should be_nil }
+              its(:errors) { should be_empty }
+              its(:valid?) { should be true }
+            end
           end
         end
-      end
 
-      context 'when attrs have a missing key' do
-        {
-          'Schemas': {},
-          'Schemas service_instance': { 'service_instance' => {} },
-          'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
-        }.each do |name, test|
-          context "for property #{name}" do
-            let(:attrs) { test }
-
-            its(:create_instance) { should be_nil }
-            its(:errors) { should be_empty }
-            its(:valid?) { should be true }
+        context 'when attrs have a missing key' do
+          {
+              'Schemas': {},
+              'Schemas service_instance': { 'service_instance' => {} },
+              'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:create_instance) { should be_nil }
+              its(:errors) { should be_empty }
+              its(:valid?) { should be true }
+            end
           end
         end
-      end
 
-      context 'when attrs have an invalid type' do
-        {
-          'Schemas': true,
-          'Schemas service_instance': { 'service_instance' => true },
-          'Schemas service_instance.create': { 'service_instance' => { 'create' => true } },
-          'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => true } } },
-        }.each do |name, test|
-          context "for property #{name}" do
-            let(:attrs) { test }
-
-            its(:create_instance) { should be_nil }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
-            its(:valid?) { should be false }
+        context 'when attrs have an invalid type' do
+          {
+              'Schemas': true,
+              'Schemas service_instance': { 'service_instance' => true },
+              'Schemas service_instance.create': { 'service_instance' => { 'create' => true } },
+              'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => true } } },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:create_instance) { should be_nil }
+              its('errors.messages') { should have(1).items }
+              its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
+              its(:valid?) { should be false }
+            end
           end
         end
-      end
 
-      context 'when attrs has a potentially dangerous uri' do
-        let(:attrs) {
-          {
-            'service_instance' => {
-              'create' => {
-                'parameters' => 'https://example.com/hax0r'
-              }
+        context 'when attrs has a potentially dangerous uri' do
+          let(:attrs) {
+            {
+                'service_instance' => {
+                    'create' => {
+                        'parameters' => 'https://example.com/hax0r'
+                    }
+                }
             }
           }
-        }
 
-        its(:create_instance) { should be_nil }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
-        its(:valid?) { should be false }
-      end
+          its(:create_instance) { should be_nil }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
+          its(:valid?) { should be false }
+        end
 
-      context 'when the schema does not conform to JSON Schema Draft 04' do
-        let(:create_instance_schema) { { 'properties': true } }
+        context 'when the schema does not conform to JSON Schema Draft 04' do
+          let(:create_instance_schema) { { 'properties': true } }
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') {
-          should match "Schema #{path} is not valid\. Must conform to JSON Schema Draft 04.+properties"
-        }
-      end
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. Must conform to JSON Schema Draft 04.+properties"
+          }
+        end
 
-      context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-        let(:create_instance_schema) { { 'type': 'foo', 'properties': true } }
+        context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
+          let(:create_instance_schema) { { 'type': 'foo', 'properties': true } }
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(2).items }
-        its('errors.messages.first') { should match 'properties' }
-        its('errors.messages.second') { should match 'type' }
-      end
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(2).items }
+          its('errors.messages.first') { should match 'properties' }
+          its('errors.messages.second') { should match 'type' }
+        end
 
-      context 'when the schema has an external schema' do
-        let(:create_instance_schema) { { '$schema': 'http://example.com/schema' } }
+        context 'when the schema has an external schema' do
+          let(:create_instance_schema) { { '$schema': 'http://example.com/schema' } }
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') {
-          should match "Schema #{path} is not valid\. Custom meta schemas are not supported.+http://example.com/schema"
-        }
-      end
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. Custom meta schemas are not supported.+http://example.com/schema"
+          }
+        end
 
-      context 'when the schema has an external uri reference' do
-        let(:create_instance_schema) { { '$ref': 'http://example.com/ref' } }
+        context 'when the schema has an external uri reference' do
+          let(:create_instance_schema) { { '$ref': 'http://example.com/ref' } }
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') {
-          should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
-        }
-      end
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
+          }
+        end
 
-      context 'when the schema has an external file reference' do
-        let(:create_instance_schema) { { '$ref': 'path/to/schema.json' } }
+        context 'when the schema has an external file reference' do
+          let(:create_instance_schema) { { '$ref': 'path/to/schema.json' } }
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') {
-          should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
-        }
-      end
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
+          }
+        end
 
-      context 'when the schema has an internal reference' do
-        let(:create_instance_schema) {
-          {
-            'type' => 'object',
-            'properties': {
-              'foo': { 'type': 'integer' },
-              'bar': { '$ref': '#/properties/foo' }
+        context 'when the schema has an internal reference' do
+          let(:create_instance_schema) {
+            {
+                'type' => 'object',
+                'properties': {
+                    'foo': { 'type': 'integer' },
+                    'bar': { '$ref': '#/properties/foo' }
+                }
             }
           }
-        }
 
-        its(:valid?) { should be true }
-        its(:errors) { should be_empty }
-      end
-
-      context 'when the schema has an unknown parse error' do
-        before do
-          allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
+          its(:valid?) { should be true }
+          its(:errors) { should be_empty }
         end
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') { should match "Schema #{path} is not valid\.+ some unknown error" }
-      end
+        context 'when the schema has an unknown parse error' do
+          before do
+            allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
+          end
 
-      context 'when the schema has multiple valid constraints ' do
-        let(:create_instance_schema) {
-          { :'$schema' => 'http://json-schema.org/draft-04/schema#',
-            'type' => 'object',
-            :properties => { 'foo': { 'type': 'string' } },
-            :required => ['foo']
-        }
-        }
-        its(:valid?) { should be true }
-        its(:errors) { should be_empty }
-      end
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ some unknown error" }
+        end
 
-      describe 'schema sizes' do
-        def create_schema_of_size(bytes)
-          surrounding_bytes = 26
-          {
-            'type' => 'object',
-            'foo' => 'x' * (bytes - surrounding_bytes)
+        context 'when the schema has multiple valid constraints ' do
+          let(:create_instance_schema) {
+            { :'$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object',
+              :properties => { 'foo': { 'type': 'string' } },
+              :required => ['foo']
+            }
           }
+          its(:valid?) { should be true }
+          its(:errors) { should be_empty }
         end
 
-        context 'that are valid' do
-          {
-            'well below the limit': 1,
-            'just below the limit': 63,
-            'on the limit': 64,
-          }.each do |desc, size_in_kb|
-            context "when the schema is #{desc}" do
-              let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+        describe 'schema sizes' do
+          def create_schema_of_size(bytes)
+            surrounding_bytes = 26
+            {
+                'type' => 'object',
+                'foo' => 'x' * (bytes - surrounding_bytes)
+            }
+          end
 
-              its(:valid?) { should be true }
-              its(:errors) { should be_empty }
+          context 'that are valid' do
+            {
+                'well below the limit': 1,
+                'just below the limit': 63,
+                'on the limit': 64,
+            }.each do |desc, size_in_kb|
+              context "when the schema is #{desc}" do
+                let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                its(:valid?) { should be true }
+                its(:errors) { should be_empty }
+
+                it 'does perform further validation' do
+                  expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
+                  expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
+                  subject
+                end
+              end
+            end
+          end
 
-              it 'does perform further validation' do
-                expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
-                expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
-                subject
+          context 'that are invalid' do
+            {
+                'just above the limit': 65,
+                'well above the limit': 10 * 1024,
+            }.each do |desc, size_in_kb|
+              context "when the schema is #{desc}" do
+                path = 'service_instance.create.parameters'
+                let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                its(:valid?) { should be false }
+                its('errors.messages') { should have(1).items }
+                its('errors.messages.first') { should match "Schema #{path} is larger than 64KB" }
+
+                it 'does not perform further validation' do
+                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
+                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
+                  subject
+                end
               end
             end
           end
         end
 
-        context 'that are invalid' do
-          {
-            'just above the limit': 65,
-            'well above the limit': 10 * 1024,
-          }.each do |desc, size_in_kb|
-            context "when the schema is #{desc}" do
-              path = 'service_instance.create.parameters'
-              let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+        context 'when the schema does not have a type field' do
+          let(:create_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
 
-              its(:valid?) { should be false }
-              its('errors.messages') { should have(1).items }
-              its('errors.messages.first') { should match "Schema #{path} is larger than 64KB" }
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
+        end
+      end
 
-              it 'does not perform further validation' do
-                expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
-                expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
-                subject
-              end
+      context 'when catalog has an update schema' do
+        let(:path) { 'service_instance.update.parameters' }
+        let(:update_instance_schema) { {} }
+        let(:attrs) { { 'service_instance' => { 'update' => { 'parameters' => update_instance_schema } } } }
+
+        context 'when attrs have nil value' do
+          {
+              'Schemas': nil,
+              'Schemas service_instance': { 'service_instance' => nil },
+              'Schemas service_instance.update': { 'service_instance' => { 'update' => nil } },
+              'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => nil } } },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:update_instance) { should eq ({}) }
+              its(:errors) { should be_empty }
+              its(:valid?) { should be true }
             end
           end
         end
-      end
-
-      context 'when the schema does not have a type field' do
-        let(:create_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
 
-        its(:valid?) { should be false }
-        its('errors.messages') { should have(1).items }
-        its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
+        context 'when attrs have a missing key' do
+          {
+              'Schemas': {},
+              'Schemas service_instance': { 'service_instance' => {} },
+              'Schemas service_instance.update': { 'service_instance' => { 'update' => {} } },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:update_instance) { should eq ({}) }
+              its(:errors) { should be_empty }
+              its(:valid?) { should be true }
+            end
+          end
+        end
       end
     end
   end
diff --git a/spec/unit/models/services/service_plan_spec.rb b/spec/unit/models/services/service_plan_spec.rb
index d1bf4242498..69f78c06859 100644
--- a/spec/unit/models/services/service_plan_spec.rb
+++ b/spec/unit/models/services/service_plan_spec.rb
@@ -45,8 +45,32 @@ module VCAP::CloudController
     end
 
     describe 'Serialization' do
-      it { is_expected.to export_attributes :name, :free, :description, :service_guid, :extra, :unique_id, :public, :bindable, :active, :create_instance_schema }
-      it { is_expected.to import_attributes :name, :free, :description, :service_guid, :extra, :unique_id, :public, :bindable, :create_instance_schema }
+      it 'exports these attributes' do
+        is_expected.to export_attributes :name,
+                                         :free,
+                                         :description,
+                                         :service_guid,
+                                         :extra,
+                                         :unique_id,
+                                         :public,
+                                         :bindable,
+                                         :active,
+                                         :create_instance_schema,
+                                         :update_instance_schema
+      end
+
+      it 'imports these attributes' do
+        is_expected.to import_attributes :name,
+                                         :free,
+                                         :description,
+                                         :service_guid,
+                                         :extra,
+                                         :unique_id,
+                                         :public,
+                                         :bindable,
+                                         :create_instance_schema,
+                                         :update_instance_schema
+      end
     end
 
     describe '#save' do
diff --git a/spec/unit/presenters/v2/service_plan_presenter_spec.rb b/spec/unit/presenters/v2/service_plan_presenter_spec.rb
index 280c96b402b..b8ef0385e6b 100644
--- a/spec/unit/presenters/v2/service_plan_presenter_spec.rb
+++ b/spec/unit/presenters/v2/service_plan_presenter_spec.rb
@@ -17,10 +17,12 @@ module CloudController::Presenters::V2
       end
 
       let(:service_plan) do
-        VCAP::CloudController::ServicePlan.make(create_instance_schema: create_instance_schema)
+        VCAP::CloudController::ServicePlan.make(create_instance_schema: create_instance_schema,
+                                                update_instance_schema: update_instance_schema)
       end
 
       let(:create_instance_schema) { nil }
+      let(:update_instance_schema) { nil }
 
       before do
         allow(RelationsPresenter).to receive(:new).and_return(relations_presenter)
@@ -37,19 +39,30 @@ module CloudController::Presenters::V2
            'name' => service_plan.name,
            'public' => true,
            'relationship_url' => 'http://relationship.example.com',
-           'schemas' => { 'service_instance' => { 'create' => { 'parameters' => {} } } },
+           'schemas' => {
+               'service_instance' => {
+                'create' => { 'parameters' => {} },
+                'update' => { 'parameters' => {} }
+               }
+           },
            'service_guid' => service_plan.service_guid,
            'unique_id' => service_plan.unique_id
           }
         )
       end
 
-      context 'when the plan create_instance_schema is nil' do
+      context 'when the plan create_instance_schema and update_instance_schema are nil' do
         let(:create_instance_schema) { nil }
+        let(:update_instance_schema) { nil }
         it 'returns an empty schema in the correct format' do
           expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
             {
-             'schemas' => { 'service_instance' => { 'create' => { 'parameters' =>  {} } } },
+              'schemas' => {
+                'service_instance' => {
+                  'create' => { 'parameters' => {} },
+                  'update' => { 'parameters' => {} }
+                }
+              }
             }
           )
         end
@@ -60,9 +73,7 @@ module CloudController::Presenters::V2
         let(:create_instance_schema) { schema.to_json }
         it 'returns the service plan entity with the schema in the correct format' do
           expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
-            {
-             'schemas' => { 'service_instance' => { 'create' => { 'parameters' =>  schema } } },
-            }
+           { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' => schema }, 'update' => { 'parameters' => {} } } } }
           )
         end
       end
@@ -71,9 +82,26 @@ module CloudController::Presenters::V2
         let(:create_instance_schema) { '{' }
         it 'returns an empty schema in the correct format' do
           expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
-            {
-             'schemas' => { 'service_instance' => { 'create' => { 'parameters' =>  {} } } },
-            }
+            { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' => {} }, 'update' => { 'parameters' => {} } } } }
+          )
+        end
+      end
+
+      context 'when the plan update_instance_schema is valid json' do
+        schema = { '$schema' => 'example.com/schema' }
+        let(:update_instance_schema) { schema.to_json }
+        it 'returns the service plan entity with the schema in the correct format' do
+          expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
+            { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' =>  {} }, 'update' => { 'parameters' => schema } } } }
+          )
+        end
+      end
+
+      context 'when the plan update_instance_schema is invalid json' do
+        let(:update_instance_schema) { '{' }
+        it 'returns an empty schema in the correct format' do
+          expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
+            { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' => {} }, 'update' => { 'parameters' => {} } } } }
           )
         end
       end

From f36e3d382cd2681e780976739c8f63d6f4d58994 Mon Sep 17 00:00:00 2001
From: Alberto Rios <arios@pivotal.io>
Date: Thu, 13 Jul 2017 14:24:23 +0100
Subject: [PATCH 02/15] Adding update schema

[#146082145]

Signed-off-by: Alex Blease <ablease@pivotal.io>
---
 .../broker_api_compatibility/broker_api_v2.13_spec.rb        | 2 +-
 .../lib/services/service_brokers/v2/catalog_schemas_spec.rb  | 5 ++---
 spec/unit/presenters/v2/service_plan_presenter_spec.rb       | 4 ++--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index 923f6ee5ba5..169d205ebd5 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -40,7 +40,7 @@
 
         parsed_body = MultiJson.load(last_response.body)
         expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => {
-            'create' => { 'parameters' => {'$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object'} },
+            'create' => { 'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } },
             'update' => { 'parameters' => {} }
         } })
       end
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 38d14eaa8d0..37f67972eb8 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -3,7 +3,6 @@
 module VCAP::Services::ServiceBrokers::V2
   RSpec.describe CatalogSchemas do
     describe 'initializing catalog schemas' do
-
       subject { CatalogSchemas.new(attrs) }
 
       context 'when catalog has a create schema' do
@@ -242,7 +241,7 @@ def create_schema_of_size(bytes)
             context "for property #{name}" do
               let(:attrs) { test }
 
-              its(:update_instance) { should eq ({}) }
+              its(:update_instance) { should eq({}) }
               its(:errors) { should be_empty }
               its(:valid?) { should be true }
             end
@@ -258,7 +257,7 @@ def create_schema_of_size(bytes)
             context "for property #{name}" do
               let(:attrs) { test }
 
-              its(:update_instance) { should eq ({}) }
+              its(:update_instance) { should eq({}) }
               its(:errors) { should be_empty }
               its(:valid?) { should be true }
             end
diff --git a/spec/unit/presenters/v2/service_plan_presenter_spec.rb b/spec/unit/presenters/v2/service_plan_presenter_spec.rb
index b8ef0385e6b..21505d43aa2 100644
--- a/spec/unit/presenters/v2/service_plan_presenter_spec.rb
+++ b/spec/unit/presenters/v2/service_plan_presenter_spec.rb
@@ -73,7 +73,7 @@ module CloudController::Presenters::V2
         let(:create_instance_schema) { schema.to_json }
         it 'returns the service plan entity with the schema in the correct format' do
           expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
-           { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' => schema }, 'update' => { 'parameters' => {} } } } }
+            { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' => schema }, 'update' => { 'parameters' => {} } } } }
           )
         end
       end
@@ -92,7 +92,7 @@ module CloudController::Presenters::V2
         let(:update_instance_schema) { schema.to_json }
         it 'returns the service plan entity with the schema in the correct format' do
           expect(subject.entity_hash(controller, service_plan, opts, depth, parents, orphans)).to include(
-            { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' =>  {} }, 'update' => { 'parameters' => schema } } } }
+            { 'schemas' => { 'service_instance' => { 'create' => { 'parameters' => {} }, 'update' => { 'parameters' => schema } } } }
           )
         end
       end

From 4845ef6c869db2621de7e10d00b9746e15c242bc Mon Sep 17 00:00:00 2001
From: Alberto Rios <arios@pivotal.io>
Date: Fri, 14 Jul 2017 12:27:09 +0100
Subject: [PATCH 03/15] Adding single plan to update service instance schema

[#146082143]

Signed-off-by: Alex Blease <ablease@pivotal.io>
---
 .../service_brokers/v2/catalog_schemas.rb     | 40 +++++++++++++-----
 .../broker_api_v2.13_spec.rb                  | 25 ++++++++++-
 .../service_brokers/service_manager_spec.rb   | 18 ++++----
 .../v2/catalog_schemas_spec.rb                | 42 +++++++++++++++++--
 4 files changed, 103 insertions(+), 22 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index bb47ea3801b..b89f6a03f93 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -8,7 +8,7 @@ class CatalogSchemas
     def initialize(schema)
       @errors = VCAP::Services::ValidationErrors.new
       validate_and_populate_create_instance(schema)
-      @update_instance = {}
+      populate_update_instance(schema)
     end
 
     def valid?
@@ -17,26 +17,46 @@ def valid?
 
     private
 
-    def validate_and_populate_create_instance(schemas)
-      return unless schemas
-      unless schemas.is_a? Hash
-        errors.add("Schemas must be a hash, but has value #{schemas.inspect}")
+    def populate_update_instance(schema)
+      return unless schema
+      unless schema.is_a? Hash
+        return
+      end
+
+      path = []
+      ['service_instance', 'update', 'parameters'].each do |key|
+        path += [key]
+        schema = schema[key]
+        return nil unless schema
+        unless schema.is_a? Hash
+          return nil
+        end
+      end
+
+      update_instance_schema = schema
+      @update_instance = update_instance_schema
+    end
+
+    def validate_and_populate_create_instance(schema)
+      return unless schema
+      unless schema.is_a? Hash
+        errors.add("Schemas must be a hash, but has value #{schema.inspect}")
         return
       end
 
       path = []
       ['service_instance', 'create', 'parameters'].each do |key|
         path += [key]
-        schemas = schemas[key]
-        return nil unless schemas
+        schema = schema[key]
+        return nil unless schema
 
-        unless schemas.is_a? Hash
-          errors.add("Schemas #{path.join('.')} must be a hash, but has value #{schemas.inspect}")
+        unless schema.is_a? Hash
+          errors.add("Schemas #{path.join('.')} must be a hash, but has value #{schema.inspect}")
           return nil
         end
       end
 
-      create_instance_schema = schemas
+      create_instance_schema = schema
       create_instance_path = path.join('.')
 
       validate_schema(create_instance_path, create_instance_schema)
diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index 169d205ebd5..d7846fb77a4 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -12,7 +12,9 @@
           'create' => {
             'parameters' => create_instance_schema
           },
-          'update' => {}
+          'update' => {
+              'parameters' => update_instance_schema
+          }
         }
       }
     }
@@ -46,6 +48,27 @@
       end
     end
 
+    context 'when a broker catalog defines an update plan schema' do
+      let(:update_instance_schema) {
+        {
+          'key' => 'value',
+          'type' => 'object'
+        }
+      }
+
+      it 'responds with the schema for a service plan entry' do
+        get("/v2/service_plans/#{@plan_guid}",
+            {}.to_json,
+            json_headers(admin_headers))
+
+        parsed_body = MultiJson.load(last_response.body)
+        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => {
+            'create' => { 'parameters' => { 'type' => 'object' } },
+            'update' => { 'parameters' => { 'key' => 'value', 'type' => 'object' } }
+        } })
+      end
+    end
+
     context 'when the broker catalog defines a plan without schemas' do
       it 'responds with an empty schema' do
         get("/v2/service_plans/#{@large_plan_guid}",
diff --git a/spec/unit/lib/services/service_brokers/service_manager_spec.rb b/spec/unit/lib/services/service_brokers/service_manager_spec.rb
index 12e8c8ddafb..b9a2d01fb12 100644
--- a/spec/unit/lib/services/service_brokers/service_manager_spec.rb
+++ b/spec/unit/lib/services/service_brokers/service_manager_spec.rb
@@ -42,7 +42,9 @@ module VCAP::Services::ServiceBrokers
                       }
                   },
                   'update' => {
-                      'parameters' => {}
+                      'parameters' => {
+                          'key' => 'value'
+                      }
                   }
               }
           }
@@ -175,7 +177,7 @@ module VCAP::Services::ServiceBrokers
           'active' => service_plan.active,
           'bindable' => true,
           'create_instance_schema' => '{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}',
-          'update_instance_schema' => '{}'
+          'update_instance_schema' => '{"key":"value"}'
         })
       end
 
@@ -211,7 +213,7 @@ module VCAP::Services::ServiceBrokers
           expect(plan.description).to eq(plan_description)
           expect(JSON.parse(plan.extra)).to eq({ 'cost' => '0.0' })
           expect(plan.create_instance_schema).to eq('{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}')
-          expect(plan.update_instance_schema).to eq('{}')
+          expect(plan.update_instance_schema).to eq('{"key":"value"}')
 
           expect(plan.free).to be false
         end
@@ -250,7 +252,7 @@ module VCAP::Services::ServiceBrokers
           service_manager.sync_services_and_plans(catalog)
           plan = VCAP::CloudController::ServicePlan.last
           expect(plan.create_instance_schema).to be_nil
-          expect(plan.update_instance_schema).to eq('{}')
+          expect(plan.update_instance_schema).to be_nil
         end
       end
 
@@ -261,7 +263,7 @@ module VCAP::Services::ServiceBrokers
           service_manager.sync_services_and_plans(catalog)
           plan = VCAP::CloudController::ServicePlan.last
           expect(plan.create_instance_schema).to be_nil
-          expect(plan.update_instance_schema).to eq('{}')
+          expect(plan.update_instance_schema).to be_nil
         end
       end
 
@@ -272,7 +274,7 @@ module VCAP::Services::ServiceBrokers
           service_manager.sync_services_and_plans(catalog)
           plan = VCAP::CloudController::ServicePlan.last
           expect(plan.create_instance_schema).to be_nil
-          expect(plan.update_instance_schema).to eq('{}')
+          expect(plan.update_instance_schema).to be_nil
         end
       end
 
@@ -357,7 +359,7 @@ module VCAP::Services::ServiceBrokers
             expect(plan.free).to be false
             expect(plan.bindable).to be true
             expect(plan.create_instance_schema).to eq('{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}')
-            expect(plan.update_instance_schema).to eq('{}')
+            expect(plan.update_instance_schema).to eq('{"key":"value"}')
           end
 
           it 'creates service audit events for each service plan updated' do
@@ -383,7 +385,7 @@ module VCAP::Services::ServiceBrokers
               'bindable' => true,
               'free' => false,
               'create_instance_schema' => '{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}',
-              'update_instance_schema' => '{}'
+              'update_instance_schema' => '{"key":"value"}'
             })
           end
 
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 37f67972eb8..2b51dff0361 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -154,12 +154,24 @@ module VCAP::Services::ServiceBrokers::V2
 
         context 'when the schema has multiple valid constraints ' do
           let(:create_instance_schema) {
-            { :'$schema' => 'http://json-schema.org/draft-04/schema#',
+            {
+              :'$schema' => 'http://json-schema.org/draft-04/schema#',
               'type' => 'object',
               :properties => { 'foo': { 'type': 'string' } },
               :required => ['foo']
             }
           }
+
+          its(:create_instance) {
+            should eq(
+              {
+                :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object',
+                :properties => { foo: { type: 'string' } },
+                :required => ['foo']
+              }
+            )
+          }
           its(:valid?) { should be true }
           its(:errors) { should be_empty }
         end
@@ -241,7 +253,7 @@ def create_schema_of_size(bytes)
             context "for property #{name}" do
               let(:attrs) { test }
 
-              its(:update_instance) { should eq({}) }
+              its(:update_instance) { should be_nil }
               its(:errors) { should be_empty }
               its(:valid?) { should be true }
             end
@@ -257,12 +269,36 @@ def create_schema_of_size(bytes)
             context "for property #{name}" do
               let(:attrs) { test }
 
-              its(:update_instance) { should eq({}) }
+              its(:update_instance) { should be_nil }
               its(:errors) { should be_empty }
               its(:valid?) { should be true }
             end
           end
         end
+
+        context 'when the schema has multiple valid constraints ' do
+          let(:update_instance_schema) {
+            {
+              :'$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object',
+              :properties => { 'foo': { 'type': 'string' } },
+              :required => ['foo']
+            }
+          }
+
+          its(:update_instance) {
+            should eq(
+              {
+                :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object',
+                :properties => { foo: { type: 'string' } },
+                :required => ['foo']
+              }
+            )
+          }
+          its(:valid?) { should be true }
+          its(:errors) { should be_empty }
+        end
       end
     end
   end

From 07d7f8425260d5b1dba83e56cbf04e85373ea505 Mon Sep 17 00:00:00 2001
From: Alex Blease <ablease@pivotal.io>
Date: Fri, 14 Jul 2017 15:52:56 +0100
Subject: [PATCH 04/15] Adding validation for object type on update schema

[#146082137]

Signed-off-by: Alberto Rios <arios@pivotal.io>
---
 lib/services/service_brokers/v2/catalog_schemas.rb     |  9 +++++++--
 .../broker_api_compatibility/broker_api_v2.13_spec.rb  |  4 ++--
 .../services/service_brokers/service_manager_spec.rb   | 10 +++++-----
 .../service_brokers/v2/catalog_schemas_spec.rb         |  8 ++++++++
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index b89f6a03f93..107c69ae168 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -8,7 +8,7 @@ class CatalogSchemas
     def initialize(schema)
       @errors = VCAP::Services::ValidationErrors.new
       validate_and_populate_create_instance(schema)
-      populate_update_instance(schema)
+      validate_and_populate_update_instance(schema)
     end
 
     def valid?
@@ -17,7 +17,7 @@ def valid?
 
     private
 
-    def populate_update_instance(schema)
+    def validate_and_populate_update_instance(schema)
       return unless schema
       unless schema.is_a? Hash
         return
@@ -34,6 +34,11 @@ def populate_update_instance(schema)
       end
 
       update_instance_schema = schema
+      update_instance_path = path.join('.')
+
+      validate_schema_type(update_instance_path, update_instance_schema)
+      return unless errors.empty?
+
       @update_instance = update_instance_schema
     end
 
diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index d7846fb77a4..422c02aa079 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -5,7 +5,7 @@
     include VCAP::CloudController::BrokerApiHelper
 
     let(:create_instance_schema) { { 'type': 'object' } }
-    let(:update_instance_schema) { {} }
+    let(:update_instance_schema) { { 'type': 'object' } }
     let(:schemas) {
       {
         'service_instance' => {
@@ -43,7 +43,7 @@
         parsed_body = MultiJson.load(last_response.body)
         expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => {
             'create' => { 'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } },
-            'update' => { 'parameters' => {} }
+            'update' => { 'parameters' => { 'type' => 'object' } }
         } })
       end
     end
diff --git a/spec/unit/lib/services/service_brokers/service_manager_spec.rb b/spec/unit/lib/services/service_brokers/service_manager_spec.rb
index b9a2d01fb12..3d42e8c8ab0 100644
--- a/spec/unit/lib/services/service_brokers/service_manager_spec.rb
+++ b/spec/unit/lib/services/service_brokers/service_manager_spec.rb
@@ -43,7 +43,7 @@ module VCAP::Services::ServiceBrokers
                   },
                   'update' => {
                       'parameters' => {
-                          'key' => 'value'
+                          'type' => 'object'
                       }
                   }
               }
@@ -177,7 +177,7 @@ module VCAP::Services::ServiceBrokers
           'active' => service_plan.active,
           'bindable' => true,
           'create_instance_schema' => '{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}',
-          'update_instance_schema' => '{"key":"value"}'
+          'update_instance_schema' => '{"type":"object"}'
         })
       end
 
@@ -213,7 +213,7 @@ module VCAP::Services::ServiceBrokers
           expect(plan.description).to eq(plan_description)
           expect(JSON.parse(plan.extra)).to eq({ 'cost' => '0.0' })
           expect(plan.create_instance_schema).to eq('{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}')
-          expect(plan.update_instance_schema).to eq('{"key":"value"}')
+          expect(plan.update_instance_schema).to eq('{"type":"object"}')
 
           expect(plan.free).to be false
         end
@@ -359,7 +359,7 @@ module VCAP::Services::ServiceBrokers
             expect(plan.free).to be false
             expect(plan.bindable).to be true
             expect(plan.create_instance_schema).to eq('{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}')
-            expect(plan.update_instance_schema).to eq('{"key":"value"}')
+            expect(plan.update_instance_schema).to eq('{"type":"object"}')
           end
 
           it 'creates service audit events for each service plan updated' do
@@ -385,7 +385,7 @@ module VCAP::Services::ServiceBrokers
               'bindable' => true,
               'free' => false,
               'create_instance_schema' => '{"$schema":"http://json-schema.org/draft-04/schema","type":"object"}',
-              'update_instance_schema' => '{"key":"value"}'
+              'update_instance_schema' => '{"type":"object"}'
             })
           end
 
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 2b51dff0361..8c29543c2de 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -299,6 +299,14 @@ def create_schema_of_size(bytes)
           its(:valid?) { should be true }
           its(:errors) { should be_empty }
         end
+
+        context 'when the schema does not have a type field' do
+          let(:update_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
+        end
       end
     end
   end

From 42379558b39b81f93590d70779821e9939ffeff2 Mon Sep 17 00:00:00 2001
From: Sam Gunaratne <sgunaratne@pivotal.io>
Date: Thu, 20 Jul 2017 17:04:33 +0100
Subject: [PATCH 05/15] Adding validations for update broker schema

[#146082133]

Signed-off-by: Alberto Rios <arios@pivotal.io>
---
 .../service_brokers/v2/catalog_schemas.rb     |  92 +++---
 .../broker_api_v2.13_spec.rb                  |  69 +++--
 .../v2/catalog_schemas_spec.rb                | 262 ++++++++++++++----
 3 files changed, 310 insertions(+), 113 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index 107c69ae168..2d82cc32837 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -5,10 +5,17 @@ module VCAP::Services::ServiceBrokers::V2
   class CatalogSchemas
     attr_reader :errors, :create_instance, :update_instance
 
-    def initialize(schema)
+    def initialize(schemas)
       @errors = VCAP::Services::ValidationErrors.new
-      validate_and_populate_create_instance(schema)
-      validate_and_populate_update_instance(schema)
+      @schemas = schemas
+
+      return unless validate_structure([])
+
+      service_instance_path = ['service_instance']
+      return unless validate_structure(service_instance_path)
+
+      @create_instance = validate_and_populate_create(service_instance_path)
+      @update_instance = validate_and_populate_update(service_instance_path)
     end
 
     def valid?
@@ -17,57 +24,50 @@ def valid?
 
     private
 
-    def validate_and_populate_update_instance(schema)
-      return unless schema
+    attr_reader :schemas
+
+    def validate_structure(path)
+      schema = path.reduce(@schemas) { |current, key|
+        return false unless current.key?(key)
+        current.fetch(key)
+      }
+      return false unless schema
+
       unless schema.is_a? Hash
-        return
+        add_schema_type_error_msg(path, schema)
+        return false
       end
+      true
+    end
 
-      path = []
-      ['service_instance', 'update', 'parameters'].each do |key|
-        path += [key]
-        schema = schema[key]
-        return nil unless schema
-        unless schema.is_a? Hash
-          return nil
-        end
-      end
+    def validate_and_populate_create(path)
+      create_path = path + ['create']
+      return unless validate_structure(create_path)
 
-      update_instance_schema = schema
-      update_instance_path = path.join('.')
+      create_parameter_path = create_path + ['parameters']
+      return unless validate_structure(create_parameter_path)
 
-      validate_schema_type(update_instance_path, update_instance_schema)
-      return unless errors.empty?
+      create_parameters = @schemas['service_instance']['create']['parameters']
 
-      @update_instance = update_instance_schema
-    end
+      validate_schema(create_parameter_path, create_parameters)
 
-    def validate_and_populate_create_instance(schema)
-      return unless schema
-      unless schema.is_a? Hash
-        errors.add("Schemas must be a hash, but has value #{schema.inspect}")
-        return
-      end
+      return unless errors.empty?
 
-      path = []
-      ['service_instance', 'create', 'parameters'].each do |key|
-        path += [key]
-        schema = schema[key]
-        return nil unless schema
+      create_parameters
+    end
 
-        unless schema.is_a? Hash
-          errors.add("Schemas #{path.join('.')} must be a hash, but has value #{schema.inspect}")
-          return nil
-        end
-      end
+    def validate_and_populate_update(path)
+      update_path = path + ['update']
+      return unless validate_structure(update_path)
 
-      create_instance_schema = schema
-      create_instance_path = path.join('.')
+      update_parameter_path = update_path + ['parameters']
+      return unless validate_structure(update_parameter_path)
 
-      validate_schema(create_instance_path, create_instance_schema)
+      update_parameters = @schemas['service_instance']['update']['parameters']
+      validate_schema(update_parameter_path, update_parameters)
       return unless errors.empty?
 
-      @create_instance = create_instance_schema
+      update_parameters
     end
 
     def validate_schema(path, schema)
@@ -91,7 +91,7 @@ def validate_schema_type(path, schema)
     end
 
     def validate_schema_size(path, schema)
-      errors.add("Schema #{path} is larger than 64KB") if schema.to_json.length > MAX_SCHEMA_SIZE
+      add_schema_error_msg(path, 'Must not be larger than 64KB') if schema.to_json.length > MAX_SCHEMA_SIZE
     end
 
     def validate_metaschema(path, schema)
@@ -129,7 +129,13 @@ def validate_no_external_references(path, schema)
     end
 
     def add_schema_error_msg(path, err)
-      errors.add("Schema #{path} is not valid. #{err}")
+      path = path.empty? ? '' : " #{path.join('.')}"
+      errors.add("Schema#{path} is not valid. #{err}")
+    end
+
+    def add_schema_type_error_msg(path, value)
+      path = path.empty? ? '' : " #{path.join('.')}"
+      errors.add("Schemas#{path} must be a hash, but has value #{value.inspect}")
     end
   end
 end
diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index 422c02aa079..2ed563f6dce 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -4,18 +4,18 @@
   describe 'v2.13' do
     include VCAP::CloudController::BrokerApiHelper
 
-    let(:create_instance_schema) { { 'type': 'object' } }
-    let(:update_instance_schema) { { 'type': 'object' } }
+    let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
+    let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
     let(:schemas) {
       {
-        'service_instance' => {
-          'create' => {
-            'parameters' => create_instance_schema
-          },
-          'update' => {
-              'parameters' => update_instance_schema
+          'service_instance' => {
+              'create' => {
+                  'parameters' => create_instance_schema
+              },
+              'update' => {
+                  'parameters' => update_instance_schema
+              }
           }
-        }
       }
     }
 
@@ -30,8 +30,8 @@
     context 'when a broker catalog defines a create plan schema' do
       let(:create_instance_schema) {
         {
-          '$schema' => 'http://json-schema.org/draft-04/schema#',
-          'type' => 'object'
+            '$schema' => 'http://json-schema.org/draft-04/schema#',
+            'type' => 'object'
         }
       }
 
@@ -41,18 +41,23 @@
             json_headers(admin_headers))
 
         parsed_body = MultiJson.load(last_response.body)
-        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => {
-            'create' => { 'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } },
-            'update' => { 'parameters' => { 'type' => 'object' } }
-        } })
+        create_schema = parsed_body['entity']['schemas']['service_instance']['create']
+        expect(create_schema).to eq({
+                                        'parameters' =>
+                                            {
+                                                '$schema' => 'http://json-schema.org/draft-04/schema#',
+                                                'type' => 'object'
+                                            }
+                                    }
+                                 )
       end
     end
 
     context 'when a broker catalog defines an update plan schema' do
       let(:update_instance_schema) {
         {
-          'key' => 'value',
-          'type' => 'object'
+            '$schema' => 'http://json-schema.org/draft-04/schema#',
+            'type' => 'object'
         }
       }
 
@@ -62,14 +67,19 @@
             json_headers(admin_headers))
 
         parsed_body = MultiJson.load(last_response.body)
-        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => {
-            'create' => { 'parameters' => { 'type' => 'object' } },
-            'update' => { 'parameters' => { 'key' => 'value', 'type' => 'object' } }
-        } })
+        update_schema = parsed_body['entity']['schemas']['service_instance']['update']
+        expect(update_schema).to eq({
+                                        'parameters' =>
+                                            {
+                                                '$schema' => 'http://json-schema.org/draft-04/schema#',
+                                                'type' => 'object'
+                                            }
+                                    }
+                                 )
       end
     end
 
-    context 'when the broker catalog defines a plan without schemas' do
+    context 'when the broker catalog defines a plan without plan schemas' do
       it 'responds with an empty schema' do
         get("/v2/service_plans/#{@large_plan_guid}",
             {}.to_json,
@@ -80,7 +90,7 @@
       end
     end
 
-    context 'when the broker catalog has an invalid schema' do
+    context 'when the broker catalog has an invalid create plan schema' do
       before do
         update_broker(default_catalog(plan_schemas: { 'service_instance' => { 'create' => true } }))
       end
@@ -92,5 +102,18 @@
         expect(parsed_body['description']).to include('Schemas service_instance.create must be a hash, but has value true')
       end
     end
+
+    context 'when the broker catalog has an invalid update plan schema' do
+      before do
+        update_broker(default_catalog(plan_schemas: { 'service_instance' => { 'update' => true } }))
+      end
+
+      it 'returns an error' do
+        parsed_body = MultiJson.load(last_response.body)
+
+        expect(parsed_body['code']).to eq(270012)
+        expect(parsed_body['description']).to include('Schemas service_instance.update must be a hash, but has value true')
+      end
+    end
   end
 end
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 8c29543c2de..e456dd627bf 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -5,6 +5,42 @@ module VCAP::Services::ServiceBrokers::V2
     describe 'initializing catalog schemas' do
       subject { CatalogSchemas.new(attrs) }
 
+      context 'when catalog has schemas' do
+        let(:attrs) { { 'service_instance' => {} } }
+
+        context 'when schemas is not set' do
+          {
+              'Schemas is nil': nil,
+              'Schemas is empty': {},
+              'Schemas service_instance is nil': { 'service_instance' => nil },
+              'Schemas service_instance is empty': { 'service_instance' => {} },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:update_instance) { should be_nil }
+              its(:errors) { should be_empty }
+              its(:valid?) { should be true }
+            end
+          end
+        end
+
+        context 'when schemas is invalid' do
+          {
+              'Schemas': true,
+              'Schemas service_instance': { 'service_instance' => true }
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its('errors.messages') { should have(1).items }
+              its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
+              its(:valid?) { should be false }
+            end
+          end
+        end
+      end
+
       context 'when catalog has a create schema' do
         let(:path) { 'service_instance.create.parameters' }
         let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
@@ -12,8 +48,6 @@ module VCAP::Services::ServiceBrokers::V2
 
         context 'when attrs have nil value' do
           {
-              'Schemas': nil,
-              'Schemas service_instance': { 'service_instance' => nil },
               'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },
               'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
           }.each do |name, test|
@@ -29,8 +63,6 @@ module VCAP::Services::ServiceBrokers::V2
 
         context 'when attrs have a missing key' do
           {
-              'Schemas': {},
-              'Schemas service_instance': { 'service_instance' => {} },
               'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
           }.each do |name, test|
             context "for property #{name}" do
@@ -45,8 +77,6 @@ module VCAP::Services::ServiceBrokers::V2
 
         context 'when attrs have an invalid type' do
           {
-              'Schemas': true,
-              'Schemas service_instance': { 'service_instance' => true },
               'Schemas service_instance.create': { 'service_instance' => { 'create' => true } },
               'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => true } } },
           }.each do |name, test|
@@ -61,23 +91,6 @@ module VCAP::Services::ServiceBrokers::V2
           end
         end
 
-        context 'when attrs has a potentially dangerous uri' do
-          let(:attrs) {
-            {
-                'service_instance' => {
-                    'create' => {
-                        'parameters' => 'https://example.com/hax0r'
-                    }
-                }
-            }
-          }
-
-          its(:create_instance) { should be_nil }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
-          its(:valid?) { should be false }
-        end
-
         context 'when the schema does not conform to JSON Schema Draft 04' do
           let(:create_instance_schema) { { 'properties': true } }
 
@@ -107,6 +120,23 @@ module VCAP::Services::ServiceBrokers::V2
           }
         end
 
+        context 'when attrs has a potentially dangerous uri' do
+          let(:attrs) {
+            {
+                'service_instance' => {
+                    'create' => {
+                        'parameters' => 'https://example.com/hax0r'
+                    }
+                }
+            }
+          }
+
+          its(:create_instance) { should be_nil }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
+          its(:valid?) { should be false }
+        end
+
         context 'when the schema has an external uri reference' do
           let(:create_instance_schema) { { '$ref': 'http://example.com/ref' } }
 
@@ -142,16 +172,6 @@ module VCAP::Services::ServiceBrokers::V2
           its(:errors) { should be_empty }
         end
 
-        context 'when the schema has an unknown parse error' do
-          before do
-            allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
-          end
-
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ some unknown error" }
-        end
-
         context 'when the schema has multiple valid constraints ' do
           let(:create_instance_schema) {
             {
@@ -177,14 +197,6 @@ module VCAP::Services::ServiceBrokers::V2
         end
 
         describe 'schema sizes' do
-          def create_schema_of_size(bytes)
-            surrounding_bytes = 26
-            {
-                'type' => 'object',
-                'foo' => 'x' * (bytes - surrounding_bytes)
-            }
-          end
-
           context 'that are valid' do
             {
                 'well below the limit': 1,
@@ -217,7 +229,7 @@ def create_schema_of_size(bytes)
 
                 its(:valid?) { should be false }
                 its('errors.messages') { should have(1).items }
-                its('errors.messages.first') { should match "Schema #{path} is larger than 64KB" }
+                its('errors.messages.first') { should match "Schema #{path} is not valid. Must not be larger than 64KB" }
 
                 it 'does not perform further validation' do
                   expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
@@ -236,17 +248,25 @@ def create_schema_of_size(bytes)
           its('errors.messages') { should have(1).items }
           its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
         end
+
+        context 'when the schema has an unknown parse error' do
+          before do
+            allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
+          end
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ some unknown error" }
+        end
       end
 
       context 'when catalog has an update schema' do
         let(:path) { 'service_instance.update.parameters' }
-        let(:update_instance_schema) { {} }
+        let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
         let(:attrs) { { 'service_instance' => { 'update' => { 'parameters' => update_instance_schema } } } }
 
         context 'when attrs have nil value' do
           {
-              'Schemas': nil,
-              'Schemas service_instance': { 'service_instance' => nil },
               'Schemas service_instance.update': { 'service_instance' => { 'update' => nil } },
               'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => nil } } },
           }.each do |name, test|
@@ -262,8 +282,6 @@ def create_schema_of_size(bytes)
 
         context 'when attrs have a missing key' do
           {
-              'Schemas': {},
-              'Schemas service_instance': { 'service_instance' => {} },
               'Schemas service_instance.update': { 'service_instance' => { 'update' => {} } },
           }.each do |name, test|
             context "for property #{name}" do
@@ -276,6 +294,103 @@ def create_schema_of_size(bytes)
           end
         end
 
+        context 'when attrs have an invalid type' do
+          {
+              'Schemas service_instance.update': { 'service_instance' => { 'update' => true } },
+              'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => true } } },
+          }.each do |name, test|
+            context "for property #{name}" do
+              let(:attrs) { test }
+
+              its(:update_instance) { should be_nil }
+              its('errors.messages') { should have(1).items }
+              its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
+              its(:valid?) { should be false }
+            end
+          end
+        end
+
+        context 'when the schema does not conform to JSON Schema Draft 04' do
+          let(:update_instance_schema) { { 'properties': true } }
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. Must conform to JSON Schema Draft 04.+properties"
+          }
+        end
+
+        context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
+          let(:update_instance_schema) { { 'type': 'foo', 'properties': true } }
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(2).items }
+          its('errors.messages.first') { should match 'properties' }
+          its('errors.messages.second') { should match 'type' }
+        end
+
+        context 'when the schema has an external schema' do
+          let(:update_instance_schema) { { '$schema': 'http://example.com/schema' } }
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. Custom meta schemas are not supported.+http://example.com/schema"
+          }
+        end
+
+        context 'when attrs has a potentially dangerous uri' do
+          let(:attrs) {
+            {
+                'service_instance' => {
+                    'update' => {
+                        'parameters' => 'https://example.com/hax0r'
+                    }
+                }
+            }
+          }
+
+          its(:update_instance) { should be_nil }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
+          its(:valid?) { should be false }
+        end
+
+        context 'when the schema has an external uri reference' do
+          let(:update_instance_schema) { { '$ref': 'http://example.com/ref' } }
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
+          }
+        end
+
+        context 'when the schema has an external file reference' do
+          let(:update_instance_schema) { { '$ref': 'path/to/schema.json' } }
+
+          its(:valid?) { should be false }
+          its('errors.messages') { should have(1).items }
+          its('errors.messages.first') {
+            should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
+          }
+        end
+
+        context 'when the schema has an internal reference' do
+          let(:update_instance_schema) {
+            {
+                'type' => 'object',
+                'properties': {
+                    'foo': { 'type': 'integer' },
+                    'bar': { '$ref': '#/properties/foo' }
+                }
+            }
+          }
+
+          its(:valid?) { should be true }
+          its(:errors) { should be_empty }
+        end
+
         context 'when the schema has multiple valid constraints ' do
           let(:update_instance_schema) {
             {
@@ -300,6 +415,51 @@ def create_schema_of_size(bytes)
           its(:errors) { should be_empty }
         end
 
+        describe 'schema sizes' do
+          context 'that are valid' do
+            {
+                'well below the limit': 1,
+                'just below the limit': 63,
+                'on the limit': 64,
+            }.each do |desc, size_in_kb|
+              context "when the schema is #{desc}" do
+                let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                its(:valid?) { should be true }
+                its(:errors) { should be_empty }
+
+                it 'does perform further validation' do
+                  expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
+                  expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
+                  subject
+                end
+              end
+            end
+          end
+
+          context 'that are invalid' do
+            {
+                'just above the limit': 65,
+                'well above the limit': 10 * 1024,
+            }.each do |desc, size_in_kb|
+              context "when the schema is #{desc}" do
+                path = 'service_instance.update.parameters'
+                let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                its(:valid?) { should be false }
+                its('errors.messages') { should have(1).items }
+                its('errors.messages.first') { should match "Schema #{path} is not valid. Must not be larger than 64KB" }
+
+                it 'does not perform further validation' do
+                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
+                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
+                  subject
+                end
+              end
+            end
+          end
+        end
+
         context 'when the schema does not have a type field' do
           let(:update_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
 
@@ -308,6 +468,14 @@ def create_schema_of_size(bytes)
           its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
         end
       end
+
+      def create_schema_of_size(bytes)
+        surrounding_bytes = 26
+        {
+            'type' => 'object',
+            'foo' => 'x' * (bytes - surrounding_bytes)
+        }
+      end
     end
   end
 end

From ae6079855d19eed989ee872676ce7394e6772bc2 Mon Sep 17 00:00:00 2001
From: Luis Urraca <lurraca@pivotal.io>
Date: Tue, 25 Jul 2017 14:19:53 +0100
Subject: [PATCH 06/15] Adding top level error when validating the schema

[#149395975]

Signed-off-by: Alberto Rios <arios@pivotal.io>
---
 .../service_brokers/v2/catalog_schemas.rb     |   4 +-
 .../v2/catalog_schemas_spec.rb                | 772 +++++++++---------
 2 files changed, 407 insertions(+), 369 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index 2d82cc32837..48a90d532f0 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -101,14 +101,14 @@ def validate_metaschema(path, schema)
       metaschema = JSON.parse(file)
 
       begin
-        errors = JSON::Validator.fully_validate(metaschema, schema)
+        errors = JSON::Validator.fully_validate(metaschema, schema, errors_as_objects: true)
       rescue => e
         add_schema_error_msg(path, e)
         return nil
       end
 
       errors.each do |error|
-        add_schema_error_msg(path, "Must conform to JSON Schema Draft 04: #{error}")
+        add_schema_error_msg(path, "Must conform to JSON Schema Draft 04: #{error[:message]}")
       end
     end
 
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index e456dd627bf..6752cf5b679 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -5,467 +5,505 @@ module VCAP::Services::ServiceBrokers::V2
     describe 'initializing catalog schemas' do
       subject { CatalogSchemas.new(attrs) }
 
-      context 'when catalog has schemas' do
-        let(:attrs) { { 'service_instance' => {} } }
-
-        context 'when schemas is not set' do
-          {
-              'Schemas is nil': nil,
-              'Schemas is empty': {},
-              'Schemas service_instance is nil': { 'service_instance' => nil },
-              'Schemas service_instance is empty': { 'service_instance' => {} },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
-
-              its(:update_instance) { should be_nil }
-              its(:errors) { should be_empty }
-              its(:valid?) { should be true }
+      context 'service instance' do
+        context 'when catalog has schemas' do
+          let(:attrs) { { 'service_instance' => {} } }
+
+          context 'when schemas is not set' do
+            {
+                'Schemas is nil': nil,
+                'Schemas is empty': {},
+                'Schemas service_instance is nil': { 'service_instance' => nil },
+                'Schemas service_instance is empty': { 'service_instance' => {} },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
+
+                its(:update_instance) { should be_nil }
+                its(:errors) { should be_empty }
+                its(:valid?) { should be true }
+              end
             end
           end
-        end
 
-        context 'when schemas is invalid' do
-          {
-              'Schemas': true,
-              'Schemas service_instance': { 'service_instance' => true }
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
-
-              its('errors.messages') { should have(1).items }
-              its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
-              its(:valid?) { should be false }
+          context 'when schemas is invalid' do
+            {
+                'Schemas': true,
+                'Schemas service_instance': { 'service_instance' => true }
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
+
+                its('errors.messages') { should have(1).items }
+                its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
+                its(:valid?) { should be false }
+              end
             end
           end
         end
-      end
 
-      context 'when catalog has a create schema' do
-        let(:path) { 'service_instance.create.parameters' }
-        let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
-        let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => create_instance_schema } } } }
-
-        context 'when attrs have nil value' do
-          {
-              'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },
-              'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
-
-              its(:create_instance) { should be_nil }
-              its(:errors) { should be_empty }
-              its(:valid?) { should be true }
-            end
+        context 'when catalog has a create schema' do
+          let(:path) { 'service_instance.create.parameters' }
+          let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
+          let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => create_instance_schema } } } }
+
+          context 'when attrs have an invalid value as type' do
+            let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
+
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
+                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+            }
           end
-        end
 
-        context 'when attrs have a missing key' do
-          {
-              'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
+          context 'when attrs have nil value' do
+            {
+                'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },
+                'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
 
-              its(:create_instance) { should be_nil }
-              its(:errors) { should be_empty }
-              its(:valid?) { should be true }
+                its(:create_instance) { should be_nil }
+                its(:errors) { should be_empty }
+                its(:valid?) { should be true }
+              end
             end
           end
-        end
 
-        context 'when attrs have an invalid type' do
-          {
-              'Schemas service_instance.create': { 'service_instance' => { 'create' => true } },
-              'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => true } } },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
-
-              its(:create_instance) { should be_nil }
-              its('errors.messages') { should have(1).items }
-              its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
-              its(:valid?) { should be false }
+          context 'when attrs have a missing key' do
+            {
+                'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
+
+                its(:create_instance) { should be_nil }
+                its(:errors) { should be_empty }
+                its(:valid?) { should be true }
+              end
             end
           end
-        end
 
-        context 'when the schema does not conform to JSON Schema Draft 04' do
-          let(:create_instance_schema) { { 'properties': true } }
-
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. Must conform to JSON Schema Draft 04.+properties"
-          }
-        end
+          context 'when attrs have an invalid type' do
+            {
+                'Schemas service_instance.create': { 'service_instance' => { 'create' => true } },
+                'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => true } } },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
 
-        context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-          let(:create_instance_schema) { { 'type': 'foo', 'properties': true } }
+                its(:create_instance) { should be_nil }
+                its('errors.messages') { should have(1).items }
+                its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
+                its(:valid?) { should be false }
+              end
+            end
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(2).items }
-          its('errors.messages.first') { should match 'properties' }
-          its('errors.messages.second') { should match 'type' }
-        end
+          context 'when the schema does not conform to JSON Schema Draft 04' do
+            let(:create_instance_schema) { { 'properties': true } }
 
-        context 'when the schema has an external schema' do
-          let(:create_instance_schema) { { '$schema': 'http://example.com/schema' } }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
+                          "The property '#/properties' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#"
+            }
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. Custom meta schemas are not supported.+http://example.com/schema"
-          }
-        end
+          context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
+            let(:create_instance_schema) { { 'type': 'foo', 'properties': true } }
 
-        context 'when attrs has a potentially dangerous uri' do
-          let(:attrs) {
-            {
-                'service_instance' => {
-                    'create' => {
-                        'parameters' => 'https://example.com/hax0r'
-                    }
-                }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(2).items }
+            its('errors.messages.first') {
+              should eq 'Schema service_instance.create.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
             }
-          }
+            its('errors.messages.second') {
+              should eq 'Schema service_instance.create.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
+            }
+          end
 
-          its(:create_instance) { should be_nil }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
-          its(:valid?) { should be false }
-        end
+          context 'when the schema has an external schema' do
+            let(:create_instance_schema) { { '$schema': 'http://example.com/schema' } }
 
-        context 'when the schema has an external uri reference' do
-          let(:create_instance_schema) { { '$ref': 'http://example.com/ref' } }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Custom meta schemas are not supported: Schema not found: http://example.com/schema"
+            }
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
-          }
-        end
+          context 'when attrs has a potentially dangerous uri' do
+            let(:attrs) {
+              {
+                  'service_instance' => {
+                      'create' => {
+                          'parameters' => 'https://example.com/hax0r'
+                      }
+                  }
+              }
+            }
 
-        context 'when the schema has an external file reference' do
-          let(:create_instance_schema) { { '$ref': 'path/to/schema.json' } }
+            its(:create_instance) { should be_nil }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
+            its(:valid?) { should be false }
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
-          }
-        end
+          context 'when the schema has an external uri reference' do
+            let(:create_instance_schema) { { '$ref': 'http://example.com/ref' } }
 
-        context 'when the schema has an internal reference' do
-          let(:create_instance_schema) {
-            {
-                'type' => 'object',
-                'properties': {
-                    'foo': { 'type': 'integer' },
-                    'bar': { '$ref': '#/properties/foo' }
-                }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. No external references are allowed: Read of URI at http://example.com/ref refused"
             }
-          }
+          end
 
-          its(:valid?) { should be true }
-          its(:errors) { should be_empty }
-        end
+          context 'when the schema has an external file reference' do
+            let(:create_instance_schema) { { '$ref': 'path/to/schema.json' } }
 
-        context 'when the schema has multiple valid constraints ' do
-          let(:create_instance_schema) {
-            {
-              :'$schema' => 'http://json-schema.org/draft-04/schema#',
-              'type' => 'object',
-              :properties => { 'foo': { 'type': 'string' } },
-              :required => ['foo']
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should match "Schema #{path} is not valid. No external references are allowed.+path/to/schema.json"
             }
-          }
+          end
 
-          its(:create_instance) {
-            should eq(
+          context 'when the schema has an internal reference' do
+            let(:create_instance_schema) {
               {
-                :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                'type' => 'object',
-                :properties => { foo: { type: 'string' } },
-                :required => ['foo']
+                  'type' => 'object',
+                  'properties': {
+                      'foo': { 'type': 'integer' },
+                      'bar': { '$ref': '#/properties/foo' }
+                  }
               }
-            )
-          }
-          its(:valid?) { should be true }
-          its(:errors) { should be_empty }
-        end
+            }
 
-        describe 'schema sizes' do
-          context 'that are valid' do
-            {
-                'well below the limit': 1,
-                'just below the limit': 63,
-                'on the limit': 64,
-            }.each do |desc, size_in_kb|
-              context "when the schema is #{desc}" do
-                let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+            its(:valid?) { should be true }
+            its(:errors) { should be_empty }
+          end
 
-                its(:valid?) { should be true }
-                its(:errors) { should be_empty }
+          context 'when the schema has multiple valid constraints ' do
+            let(:create_instance_schema) {
+              {
+                  :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                  'type' => 'object',
+                  :properties => { 'foo': { 'type': 'string' } },
+                  :required => ['foo']
+              }
+            }
 
-                it 'does perform further validation' do
-                  expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
-                  expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
-                  subject
+            its(:create_instance) {
+              should eq(
+                {
+                    :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                    'type' => 'object',
+                    :properties => { foo: { type: 'string' } },
+                    :required => ['foo']
+                }
+                     )
+            }
+            its(:valid?) { should be true }
+            its(:errors) { should be_empty }
+          end
+
+          describe 'schema sizes' do
+            context 'that are valid' do
+              {
+                  'well below the limit': 1,
+                  'just below the limit': 63,
+                  'on the limit': 64,
+              }.each do |desc, size_in_kb|
+                context "when the schema is #{desc}" do
+                  let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                  its(:valid?) { should be true }
+                  its(:errors) { should be_empty }
+
+                  it 'does perform further validation' do
+                    expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
+                    expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
+                    subject
+                  end
+                end
+              end
+            end
+
+            context 'that are invalid' do
+              {
+                  'just above the limit': 65,
+                  'well above the limit': 10 * 1024,
+              }.each do |desc, size_in_kb|
+                context "when the schema is #{desc}" do
+                  path = 'service_instance.create.parameters'
+                  let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                  its(:valid?) { should be false }
+                  its('errors.messages') { should have(1).items }
+                  its('errors.messages.first') { should eq "Schema #{path} is not valid. Must not be larger than 64KB" }
+
+                  it 'does not perform further validation' do
+                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
+                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
+                    subject
+                  end
                 end
               end
             end
           end
 
-          context 'that are invalid' do
-            {
-                'just above the limit': 65,
-                'well above the limit': 10 * 1024,
-            }.each do |desc, size_in_kb|
-              context "when the schema is #{desc}" do
-                path = 'service_instance.create.parameters'
-                let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+          context 'when the schema does not have a type field' do
+            let(:create_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
 
-                its(:valid?) { should be false }
-                its('errors.messages') { should have(1).items }
-                its('errors.messages.first') { should match "Schema #{path} is not valid. Must not be larger than 64KB" }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should eq "Schema #{path} is not valid. must have field \"type\", with value \"object\"" }
+          end
 
-                it 'does not perform further validation' do
-                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
-                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
-                  subject
-                end
-              end
+          context 'when the schema has an unknown parse error' do
+            before do
+              allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
             end
+
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should eq "Schema #{path} is not valid. some unknown error" }
           end
         end
 
-        context 'when the schema does not have a type field' do
-          let(:create_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
+        context 'when catalog has an update schema' do
+          let(:path) { 'service_instance.update.parameters' }
+          let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
+          let(:attrs) { { 'service_instance' => { 'update' => { 'parameters' => update_instance_schema } } } }
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
-        end
+          context 'when attrs have an invalid value as type' do
+            let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
 
-        context 'when the schema has an unknown parse error' do
-          before do
-            allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
+                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+            }
           end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ some unknown error" }
-        end
-      end
+          context 'when attrs have nil value' do
+            {
+                'Schemas service_instance.update': { 'service_instance' => { 'update' => nil } },
+                'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => nil } } },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
 
-      context 'when catalog has an update schema' do
-        let(:path) { 'service_instance.update.parameters' }
-        let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
-        let(:attrs) { { 'service_instance' => { 'update' => { 'parameters' => update_instance_schema } } } }
-
-        context 'when attrs have nil value' do
-          {
-              'Schemas service_instance.update': { 'service_instance' => { 'update' => nil } },
-              'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => nil } } },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
-
-              its(:update_instance) { should be_nil }
-              its(:errors) { should be_empty }
-              its(:valid?) { should be true }
+                its(:update_instance) { should be_nil }
+                its(:errors) { should be_empty }
+                its(:valid?) { should be true }
+              end
             end
           end
-        end
 
-        context 'when attrs have a missing key' do
-          {
-              'Schemas service_instance.update': { 'service_instance' => { 'update' => {} } },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
+          context 'when attrs have a missing key' do
+            {
+                'Schemas service_instance.update': { 'service_instance' => { 'update' => {} } },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
 
-              its(:update_instance) { should be_nil }
-              its(:errors) { should be_empty }
-              its(:valid?) { should be true }
+                its(:update_instance) { should be_nil }
+                its(:errors) { should be_empty }
+                its(:valid?) { should be true }
+              end
             end
           end
-        end
 
-        context 'when attrs have an invalid type' do
-          {
-              'Schemas service_instance.update': { 'service_instance' => { 'update' => true } },
-              'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => true } } },
-          }.each do |name, test|
-            context "for property #{name}" do
-              let(:attrs) { test }
-
-              its(:update_instance) { should be_nil }
-              its('errors.messages') { should have(1).items }
-              its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
-              its(:valid?) { should be false }
+          context 'when attrs have an invalid type' do
+            {
+                'Schemas service_instance.update': { 'service_instance' => { 'update' => true } },
+                'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => true } } },
+            }.each do |name, test|
+              context "for property #{name}" do
+                let(:attrs) { test }
+
+                its(:update_instance) { should be_nil }
+                its('errors.messages') { should have(1).items }
+                its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
+                its(:valid?) { should be false }
+              end
             end
           end
-        end
-
-        context 'when the schema does not conform to JSON Schema Draft 04' do
-          let(:update_instance_schema) { { 'properties': true } }
-
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. Must conform to JSON Schema Draft 04.+properties"
-          }
-        end
 
-        context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-          let(:update_instance_schema) { { 'type': 'foo', 'properties': true } }
+          context 'when the schema does not conform to JSON Schema Draft 04' do
+            let(:update_instance_schema) { { 'properties': true } }
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(2).items }
-          its('errors.messages.first') { should match 'properties' }
-          its('errors.messages.second') { should match 'type' }
-        end
-
-        context 'when the schema has an external schema' do
-          let(:update_instance_schema) { { '$schema': 'http://example.com/schema' } }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/properties' " \
+                          'of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
+            }
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. Custom meta schemas are not supported.+http://example.com/schema"
-          }
-        end
+          context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
+            let(:update_instance_schema) { { 'type': 'foo', 'properties': true } }
 
-        context 'when attrs has a potentially dangerous uri' do
-          let(:attrs) {
-            {
-                'service_instance' => {
-                    'update' => {
-                        'parameters' => 'https://example.com/hax0r'
-                    }
-                }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(2).items }
+            its('errors.messages.first') {
+              should eq 'Schema service_instance.update.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
             }
-          }
+            its('errors.messages.second') {
+              should eq 'Schema service_instance.update.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
+            }
+          end
 
-          its(:update_instance) { should be_nil }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
-          its(:valid?) { should be false }
-        end
+          context 'when the schema has an external schema' do
+            let(:update_instance_schema) { { '$schema': 'http://example.com/schema' } }
 
-        context 'when the schema has an external uri reference' do
-          let(:update_instance_schema) { { '$ref': 'http://example.com/ref' } }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Custom meta schemas are not supported: Schema not found: http://example.com/schema"
+            }
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
-          }
-        end
+          context 'when attrs has a potentially dangerous uri' do
+            let(:attrs) {
+              {
+                  'service_instance' => {
+                      'update' => {
+                          'parameters' => 'https://example.com/hax0r'
+                      }
+                  }
+              }
+            }
 
-        context 'when the schema has an external file reference' do
-          let(:update_instance_schema) { { '$ref': 'path/to/schema.json' } }
+            its(:update_instance) { should be_nil }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
+            its(:valid?) { should be false }
+          end
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') {
-            should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
-          }
-        end
+          context 'when the schema has an external uri reference' do
+            let(:update_instance_schema) { { '$ref': 'http://example.com/ref' } }
 
-        context 'when the schema has an internal reference' do
-          let(:update_instance_schema) {
-            {
-                'type' => 'object',
-                'properties': {
-                    'foo': { 'type': 'integer' },
-                    'bar': { '$ref': '#/properties/foo' }
-                }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
             }
-          }
+          end
 
-          its(:valid?) { should be true }
-          its(:errors) { should be_empty }
-        end
+          context 'when the schema has an external file reference' do
+            let(:update_instance_schema) { { '$ref': 'path/to/schema.json' } }
 
-        context 'when the schema has multiple valid constraints ' do
-          let(:update_instance_schema) {
-            {
-              :'$schema' => 'http://json-schema.org/draft-04/schema#',
-              'type' => 'object',
-              :properties => { 'foo': { 'type': 'string' } },
-              :required => ['foo']
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') {
+              should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
             }
-          }
+          end
 
-          its(:update_instance) {
-            should eq(
+          context 'when the schema has an internal reference' do
+            let(:update_instance_schema) {
               {
-                :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                'type' => 'object',
-                :properties => { foo: { type: 'string' } },
-                :required => ['foo']
+                  'type' => 'object',
+                  'properties': {
+                      'foo': { 'type': 'integer' },
+                      'bar': { '$ref': '#/properties/foo' }
+                  }
               }
-            )
-          }
-          its(:valid?) { should be true }
-          its(:errors) { should be_empty }
-        end
+            }
 
-        describe 'schema sizes' do
-          context 'that are valid' do
-            {
-                'well below the limit': 1,
-                'just below the limit': 63,
-                'on the limit': 64,
-            }.each do |desc, size_in_kb|
-              context "when the schema is #{desc}" do
-                let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+            its(:valid?) { should be true }
+            its(:errors) { should be_empty }
+          end
 
-                its(:valid?) { should be true }
-                its(:errors) { should be_empty }
+          context 'when the schema has multiple valid constraints ' do
+            let(:update_instance_schema) {
+              {
+                  :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                  'type' => 'object',
+                  :properties => { 'foo': { 'type': 'string' } },
+                  :required => ['foo']
+              }
+            }
 
-                it 'does perform further validation' do
-                  expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
-                  expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
-                  subject
+            its(:update_instance) {
+              should eq(
+                {
+                    :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                    'type' => 'object',
+                    :properties => { foo: { type: 'string' } },
+                    :required => ['foo']
+                }
+                     )
+            }
+            its(:valid?) { should be true }
+            its(:errors) { should be_empty }
+          end
+
+          describe 'schema sizes' do
+            context 'that are valid' do
+              {
+                  'well below the limit': 1,
+                  'just below the limit': 63,
+                  'on the limit': 64,
+              }.each do |desc, size_in_kb|
+                context "when the schema is #{desc}" do
+                  let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                  its(:valid?) { should be true }
+                  its(:errors) { should be_empty }
+
+                  it 'does perform further validation' do
+                    expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
+                    expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
+                    subject
+                  end
                 end
               end
             end
-          end
-
-          context 'that are invalid' do
-            {
-                'just above the limit': 65,
-                'well above the limit': 10 * 1024,
-            }.each do |desc, size_in_kb|
-              context "when the schema is #{desc}" do
-                path = 'service_instance.update.parameters'
-                let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
 
-                its(:valid?) { should be false }
-                its('errors.messages') { should have(1).items }
-                its('errors.messages.first') { should match "Schema #{path} is not valid. Must not be larger than 64KB" }
-
-                it 'does not perform further validation' do
-                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
-                  expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
-                  subject
+            context 'that are invalid' do
+              {
+                  'just above the limit': 65,
+                  'well above the limit': 10 * 1024,
+              }.each do |desc, size_in_kb|
+                context "when the schema is #{desc}" do
+                  path = 'service_instance.update.parameters'
+                  let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+                  its(:valid?) { should be false }
+                  its('errors.messages') { should have(1).items }
+                  its('errors.messages.first') { should eq "Schema #{path} is not valid. Must not be larger than 64KB" }
+
+                  it 'does not perform further validation' do
+                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
+                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
+                    subject
+                  end
                 end
               end
             end
           end
-        end
 
-        context 'when the schema does not have a type field' do
-          let(:update_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
+          context 'when the schema does not have a type field' do
+            let(:update_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
 
-          its(:valid?) { should be false }
-          its('errors.messages') { should have(1).items }
-          its('errors.messages.first') { should match "Schema #{path} is not valid\.+ must have field \"type\", with value \"object\"" }
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should eq "Schema #{path} is not valid. must have field \"type\", with value \"object\"" }
+          end
         end
       end
 

From c85c32299a35a5e5f7205683ae77471191c55afc Mon Sep 17 00:00:00 2001
From: Alberto Rios <arios@pivotal.io>
Date: Tue, 25 Jul 2017 14:55:13 +0100
Subject: [PATCH 07/15] Adding test cases for when schema has multiple invalid
 types

[#149396085]

Signed-off-by: Luis Urraca <lurraca@pivotal.io>
---
 .../v2/catalog_schemas_spec.rb                | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 6752cf5b679..573b6290f5e 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -58,6 +58,30 @@ module VCAP::Services::ServiceBrokers::V2
             }
           end
 
+          context 'when schema has multiple invalid types' do
+            let(:create_instance_schema) {
+              {
+                '$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object',
+                'properties' => {
+                  'test' => { 'type' => 'notatype' },
+                  'b2' => { 'type' => 'alsonotatype' }
+                }
+              }
+            }
+
+            its(:valid?) { should be false }
+            its('errors.messages') { should have(2).items }
+            its('errors.messages.first') {
+              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
+                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+            }
+            its('errors.messages.last') {
+              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
+                          "The property '#/properties/b2/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+            }
+          end
+
           context 'when attrs have nil value' do
             {
                 'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },

From 8b1d9350fa86ff6c75b3991f397b4fff12a7ae09 Mon Sep 17 00:00:00 2001
From: Alberto Rios <arios@pivotal.io>
Date: Wed, 26 Jul 2017 12:12:36 +0100
Subject: [PATCH 08/15] Updating error message when custom metaschema is not
 valid

[#149396075]

Signed-off-by: Alex Blease <ablease@pivotal.io>
---
 lib/services/service_brokers/v2/catalog_schemas.rb            | 4 ++--
 .../lib/services/service_brokers/v2/catalog_schemas_spec.rb   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index 48a90d532f0..7ce3c552e08 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -117,8 +117,8 @@ def validate_no_external_references(path, schema)
 
       begin
         JSON::Validator.validate!(schema, {})
-      rescue JSON::Schema::SchemaError => e
-        add_schema_error_msg(path, "Custom meta schemas are not supported: #{e}")
+      rescue JSON::Schema::SchemaError
+        add_schema_error_msg(path, 'Custom meta schemas are not supported.')
       rescue JSON::Schema::ReadRefused => e
         add_schema_error_msg(path, "No external references are allowed: #{e}")
       rescue JSON::Schema::ValidationError
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 573b6290f5e..7715975149e 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -159,7 +159,7 @@ module VCAP::Services::ServiceBrokers::V2
             its(:valid?) { should be false }
             its('errors.messages') { should have(1).items }
             its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Custom meta schemas are not supported: Schema not found: http://example.com/schema"
+              should eq "Schema #{path} is not valid. Custom meta schemas are not supported."
             }
           end
 
@@ -396,7 +396,7 @@ module VCAP::Services::ServiceBrokers::V2
             its(:valid?) { should be false }
             its('errors.messages') { should have(1).items }
             its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Custom meta schemas are not supported: Schema not found: http://example.com/schema"
+              should eq "Schema #{path} is not valid. Custom meta schemas are not supported."
             }
           end
 

From 09818dabd7f2a7bf5e16e3639bcf93a707f422f8 Mon Sep 17 00:00:00 2001
From: Alberto Rios <arios@pivotal.io>
Date: Thu, 27 Jul 2017 12:19:27 +0100
Subject: [PATCH 09/15] refactor contexts in broker api

Signed-off-by: Alex Blease <ablease@pivotal.io>
---
 .../broker_api_v2.13_spec.rb                  | 135 ++++++++----------
 1 file changed, 63 insertions(+), 72 deletions(-)

diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index 2ed563f6dce..ac46a1525fb 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -27,92 +27,83 @@
       @broker = VCAP::CloudController::ServiceBroker.find guid: @broker_guid
     end
 
-    context 'when a broker catalog defines a create plan schema' do
-      let(:create_instance_schema) {
-        {
-            '$schema' => 'http://json-schema.org/draft-04/schema#',
-            'type' => 'object'
+    context 'when a broker catalog defines a service instance' do
+      context 'with a create plan schema' do
+        let(:create_instance_schema) {
+          {
+              '$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object'
+          }
         }
-      }
 
-      it 'responds with the schema for a service plan entry' do
-        get("/v2/service_plans/#{@plan_guid}",
-            {}.to_json,
-            json_headers(admin_headers))
-
-        parsed_body = MultiJson.load(last_response.body)
-        create_schema = parsed_body['entity']['schemas']['service_instance']['create']
-        expect(create_schema).to eq({
-                                        'parameters' =>
-                                            {
-                                                '$schema' => 'http://json-schema.org/draft-04/schema#',
-                                                'type' => 'object'
-                                            }
-                                    }
-                                 )
+        it 'responds with the schema for a service plan entry' do
+          get("/v2/service_plans/#{@plan_guid}",
+              {}.to_json,
+              json_headers(admin_headers))
+
+          parsed_body = MultiJson.load(last_response.body)
+          create_schema = parsed_body['entity']['schemas']['service_instance']['create']
+          expect(create_schema).to eq({
+                                          'parameters' =>
+                                              {
+                                                  '$schema' => 'http://json-schema.org/draft-04/schema#',
+                                                  'type' => 'object'
+                                              }
+                                      }
+                                   )
+        end
       end
-    end
 
-    context 'when a broker catalog defines an update plan schema' do
-      let(:update_instance_schema) {
-        {
-            '$schema' => 'http://json-schema.org/draft-04/schema#',
-            'type' => 'object'
+      context 'with an update plan schema' do
+        let(:update_instance_schema) {
+          {
+              '$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object'
+          }
         }
-      }
 
-      it 'responds with the schema for a service plan entry' do
-        get("/v2/service_plans/#{@plan_guid}",
-            {}.to_json,
-            json_headers(admin_headers))
-
-        parsed_body = MultiJson.load(last_response.body)
-        update_schema = parsed_body['entity']['schemas']['service_instance']['update']
-        expect(update_schema).to eq({
-                                        'parameters' =>
-                                            {
-                                                '$schema' => 'http://json-schema.org/draft-04/schema#',
-                                                'type' => 'object'
-                                            }
-                                    }
-                                 )
+        it 'responds with the schema for a service plan entry' do
+          get("/v2/service_plans/#{@plan_guid}",
+              {}.to_json,
+              json_headers(admin_headers))
+
+          parsed_body = MultiJson.load(last_response.body)
+          update_schema = parsed_body['entity']['schemas']['service_instance']['update']
+          expect(update_schema).to eq({
+                                          'parameters' =>
+                                              {
+                                                  '$schema' => 'http://json-schema.org/draft-04/schema#',
+                                                  'type' => 'object'
+                                              }
+                                      }
+                                   )
+        end
       end
-    end
 
-    context 'when the broker catalog defines a plan without plan schemas' do
-      it 'responds with an empty schema' do
-        get("/v2/service_plans/#{@large_plan_guid}",
-            {}.to_json,
-            json_headers(admin_headers))
+      context 'with an invalid create plan schema' do
+        before do
+          update_broker(default_catalog(plan_schemas: { 'service_instance' => { 'create' => true } }))
+        end
 
-        parsed_body = MultiJson.load(last_response.body)
-        expect(parsed_body['entity']['schemas']).to eq({ 'service_instance' => { 'create' => { 'parameters' => {} }, 'update' => { 'parameters' => {} } } })
-      end
-    end
-
-    context 'when the broker catalog has an invalid create plan schema' do
-      before do
-        update_broker(default_catalog(plan_schemas: { 'service_instance' => { 'create' => true } }))
-      end
+        it 'returns an error' do
+          parsed_body = MultiJson.load(last_response.body)
 
-      it 'returns an error' do
-        parsed_body = MultiJson.load(last_response.body)
-
-        expect(parsed_body['code']).to eq(270012)
-        expect(parsed_body['description']).to include('Schemas service_instance.create must be a hash, but has value true')
+          expect(parsed_body['code']).to eq(270012)
+          expect(parsed_body['description']).to include('Schemas service_instance.create must be a hash, but has value true')
+        end
       end
-    end
 
-    context 'when the broker catalog has an invalid update plan schema' do
-      before do
-        update_broker(default_catalog(plan_schemas: { 'service_instance' => { 'update' => true } }))
-      end
+      context 'with an invalid update plan schema' do
+        before do
+          update_broker(default_catalog(plan_schemas: { 'service_instance' => { 'update' => true } }))
+        end
 
-      it 'returns an error' do
-        parsed_body = MultiJson.load(last_response.body)
+        it 'returns an error' do
+          parsed_body = MultiJson.load(last_response.body)
 
-        expect(parsed_body['code']).to eq(270012)
-        expect(parsed_body['description']).to include('Schemas service_instance.update must be a hash, but has value true')
+          expect(parsed_body['code']).to eq(270012)
+          expect(parsed_body['description']).to include('Schemas service_instance.update must be a hash, but has value true')
+        end
       end
     end
   end

From 9d759cedf639dfb20f2ba1b16aef0d7e2a404adb Mon Sep 17 00:00:00 2001
From: Alex Blease <ablease@pivotal.io>
Date: Fri, 28 Jul 2017 13:58:33 +0100
Subject: [PATCH 10/15] Refactor broker schema validation [#149550919]

Signed-off-by: Luis Urraca <lurraca@pivotal.io>
---
 .../service_brokers/v2/catalog_schemas.rb     | 102 +++++----
 spec/acceptance/service_broker_spec.rb        | 200 ++++++++++++++++++
 .../v2/catalog_schemas_spec.rb                |  35 +--
 3 files changed, 262 insertions(+), 75 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index 7ce3c552e08..a3c4018cffd 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -14,8 +14,22 @@ def initialize(schemas)
       service_instance_path = ['service_instance']
       return unless validate_structure(service_instance_path)
 
-      @create_instance = validate_and_populate_create(service_instance_path)
-      @update_instance = validate_and_populate_update(service_instance_path)
+      create_instance_hash = validate_and_populate_create(service_instance_path)
+      update_instance_hash = validate_and_populate_update(service_instance_path)
+
+      if create_instance_hash
+        @create_instance = Schema.new(create_instance_hash, 'service_instance.create.parameters')
+        if !create_instance.validate
+          create_instance.errors.messages.each { |key, value| value.each { |error| errors.add(error) } }
+        end
+      end
+
+      if update_instance_hash
+        @update_instance = Schema.new(update_instance_hash, 'service_instance.update.parameters')
+        if !update_instance.validate
+          update_instance.errors.messages.each { |key, value| value.each { |error| errors.add(error) } }
+        end
+      end
     end
 
     def valid?
@@ -47,13 +61,7 @@ def validate_and_populate_create(path)
       create_parameter_path = create_path + ['parameters']
       return unless validate_structure(create_parameter_path)
 
-      create_parameters = @schemas['service_instance']['create']['parameters']
-
-      validate_schema(create_parameter_path, create_parameters)
-
-      return unless errors.empty?
-
-      create_parameters
+      @schemas['service_instance']['create']['parameters']
     end
 
     def validate_and_populate_update(path)
@@ -63,79 +71,81 @@ def validate_and_populate_update(path)
       update_parameter_path = update_path + ['parameters']
       return unless validate_structure(update_parameter_path)
 
-      update_parameters = @schemas['service_instance']['update']['parameters']
-      validate_schema(update_parameter_path, update_parameters)
-      return unless errors.empty?
-
-      update_parameters
+      @schemas['service_instance']['update']['parameters']
     end
 
-    def validate_schema(path, schema)
-      schema_validations.each do |validation|
-        break if errors.present?
-        send(validation, path, schema)
-      end
+    def add_schema_type_error_msg(path, value)
+      path = path.empty? ? '' : " #{path.join('.')}"
+      errors.add("Schemas#{path} must be a hash, but has value #{value.inspect}")
     end
+  end
 
-    def schema_validations
-      [
-        :validate_schema_size,
-        :validate_metaschema,
-        :validate_no_external_references,
-        :validate_schema_type
-      ]
-    end
+  class Schema
+    include ActiveModel::Validations
+
+    validate :validate_schema_size, :validate_metaschema, :validate_no_external_references, :validate_schema_type
 
-    def validate_schema_type(path, schema)
-      add_schema_error_msg(path, 'must have field "type", with value "object"') if schema['type'] != 'object'
+    def initialize(schema, path)
+      @schema = schema
+      @path = path
     end
 
-    def validate_schema_size(path, schema)
-      add_schema_error_msg(path, 'Must not be larger than 64KB') if schema.to_json.length > MAX_SCHEMA_SIZE
+    def validate_schema_size
+      return unless errors.blank?
+      add_schema_error_msg('Must not be larger than 64KB') if @schema.to_json.length > MAX_SCHEMA_SIZE
     end
 
-    def validate_metaschema(path, schema)
+    def validate_metaschema
+      return unless errors.blank?
       JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
       file = File.read(JSON::Validator.validator_for_name('draft4').metaschema)
 
       metaschema = JSON.parse(file)
 
       begin
-        errors = JSON::Validator.fully_validate(metaschema, schema, errors_as_objects: true)
+        errors = JSON::Validator.fully_validate(metaschema, @schema, errors_as_objects: true)
       rescue => e
-        add_schema_error_msg(path, e)
+        add_schema_error_msg(e)
         return nil
       end
 
       errors.each do |error|
-        add_schema_error_msg(path, "Must conform to JSON Schema Draft 04: #{error[:message]}")
+        add_schema_error_msg("Must conform to JSON Schema Draft 04: #{error[:message]}")
       end
     end
 
-    def validate_no_external_references(path, schema)
+    def validate_no_external_references
+      return unless errors.blank?
       JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
 
       begin
-        JSON::Validator.validate!(schema, {})
+        JSON::Validator.validate!(@schema, {})
       rescue JSON::Schema::SchemaError
-        add_schema_error_msg(path, 'Custom meta schemas are not supported.')
+        add_schema_error_msg('Custom meta schemas are not supported.')
       rescue JSON::Schema::ReadRefused => e
-        add_schema_error_msg(path, "No external references are allowed: #{e}")
+        add_schema_error_msg("No external references are allowed: #{e}")
       rescue JSON::Schema::ValidationError
         # We don't care if our input fails validation on broker schema
       rescue => e
-        add_schema_error_msg(path, e)
+        add_schema_error_msg(e)
       end
     end
 
-    def add_schema_error_msg(path, err)
-      path = path.empty? ? '' : " #{path.join('.')}"
-      errors.add("Schema#{path} is not valid. #{err}")
+    def validate_schema_type
+      return unless errors.blank?
+      add_schema_error_msg('must have field "type", with value "object"') if @schema['type'] != 'object'
     end
 
-    def add_schema_type_error_msg(path, value)
-      path = path.empty? ? '' : " #{path.join('.')}"
-      errors.add("Schemas#{path} must be a hash, but has value #{value.inspect}")
+    def add_schema_error_msg(err)
+      errors.add(:base, "Schema #{@path} is not valid. #{err}")
+    end
+
+    def add_schema_type_error_msg(value)
+      errors.add(:base, "Schemas #{@path} must be a hash, but has value #{value.inspect}")
+    end
+
+    def to_json
+      @schema.to_json
     end
   end
 end
diff --git a/spec/acceptance/service_broker_spec.rb b/spec/acceptance/service_broker_spec.rb
index befa351122e..4e3fa334491 100644
--- a/spec/acceptance/service_broker_spec.rb
+++ b/spec/acceptance/service_broker_spec.rb
@@ -300,6 +300,206 @@ def build_service(attrs={})
         expect(VCAP::CloudController::ServiceDashboardClient.count).to eq(0)
       end
     end
+
+    context 'when a service instance schema' do
+      ['create', 'update'].each do |service_instance_type|
+        context 'is not present' do
+          {
+              "#{service_instance_type} is nil": { 'service_instance' => { service_instance_type => nil } },
+              "#{service_instance_type} is nil": { 'service_instance' => { service_instance_type => { 'parameters' => nil } } },
+              "#{service_instance_type} is empty object": { 'service_instance' => { service_instance_type => {} } },
+          }.each do |desc, schema|
+            context "#{desc} #{schema}" do
+              before do
+                stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+              end
+              it 'responds with invalid' do
+                post('/v2/service_brokers', {
+                    name: 'some-guid',
+                    broker_url: 'http://broker-url',
+                    auth_username: 'username',
+                    auth_password: 'password'
+                }.to_json, admin_headers)
+
+                expect(last_response.status).to eql(201)
+              end
+            end
+          end
+        end
+
+        context 'has an invalid type' do
+          {
+              "service_instance.#{service_instance_type}": { 'service_instance' => { service_instance_type => true } },
+              "service_instance.#{service_instance_type}.parameters": { 'service_instance' => { service_instance_type => { 'parameters' => true } } },
+          }.each do |path, schema|
+
+            context "operator receives an error about #{path} #{schema}" do
+              before do
+                stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+              end
+              it 'responds with invalid' do
+                post('/v2/service_brokers', {
+                    name: 'some-guid',
+                    broker_url: 'http://broker-url',
+                    auth_username: 'username',
+                    auth_password: 'password'
+                }.to_json, admin_headers)
+
+                expect(last_response.status).to eql(502)
+                expect(decoded_response['code']).to eql(270012)
+                expect(decoded_response['description']).to eql("Service broker catalog is invalid: \n" \
+              "Service MySQL\n" \
+              "  Plan small\n" \
+              "    Schemas\n" \
+              "      Schemas #{path} must be a hash, but has value true\n")
+              end
+            end
+          end
+        end
+
+        context 'does not conform to JSON Schema Draft 04' do
+          let(:path) { "service_instance.#{service_instance_type}.parameters" }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'properties': true } } } } }
+
+          before do
+            stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+          end
+
+          it 'responds with invalid' do
+            post('/v2/service_brokers', {
+                name: 'some-guid',
+                broker_url: 'http://broker-url',
+                auth_username: 'username',
+                auth_password: 'password'
+            }.to_json, admin_headers)
+
+            expect(last_response.status).to eql(502)
+            expect(decoded_response['code']).to eql(270012)
+            expect(decoded_response['description']).to eql("Service broker catalog is invalid: \n" \
+          "Service MySQL\n" \
+          "  Plan small\n" \
+          "    Schemas\n" \
+          "      Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/properties' " \
+                      "of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#\n")
+          end
+        end
+
+        context 'does not conform to JSON Schema Draft 04 with multiple problems' do
+          let(:path) { "service_instance.#{service_instance_type}.parameters" }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'type': 'foo', 'properties': true } } } } }
+
+          before do
+            stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+          end
+          it 'responds with invalid' do
+            post('/v2/service_brokers', {
+                name: 'some-guid',
+                broker_url: 'http://broker-url',
+                auth_username: 'username',
+                auth_password: 'password'
+            }.to_json, admin_headers)
+
+            expect(last_response.status).to eql(502)
+            expect(decoded_response['code']).to eql(270012)
+            expect(decoded_response['description']).to eql("Service broker catalog is invalid: \n" \
+          "Service MySQL\n" \
+          "  Plan small\n" \
+          "    Schemas\n" \
+          "      Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/properties' " \
+                      "of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#\n"\
+          "      Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/type' " \
+                      "of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#\n")
+          end
+        end
+
+        context 'has an external schema' do
+          let(:path) { "service_instance.#{service_instance_type}.parameters" }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { '$schema': 'http://example.com/schema' } } } } }
+
+          before do
+            stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+          end
+
+          it 'responds with invalid' do
+            post('/v2/service_brokers', {
+                name: 'some-guid',
+                broker_url: 'http://broker-url',
+                auth_username: 'username',
+                auth_password: 'password'
+            }.to_json, admin_headers)
+
+            expect(last_response.status).to eql(502)
+            expect(decoded_response['code']).to eql(270012)
+            expect(decoded_response['description']).to eql("Service broker catalog is invalid: \n" \
+          "Service MySQL\n" \
+          "  Plan small\n" \
+          "    Schemas\n" \
+          "      Schema #{path} is not valid. Custom meta schemas are not supported.\n"
+                                                       )
+          end
+        end
+
+        context 'has an external uri reference' do
+          let(:path) { "service_instance.#{service_instance_type}.parameters" }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { '$ref': 'http://example.com/ref' } } } } }
+
+          before do
+            stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+          end
+          it 'responds with invalid' do
+            post('/v2/service_brokers', {
+                name: 'some-guid',
+                broker_url: 'http://broker-url',
+                auth_username: 'username',
+                auth_password: 'password'
+            }.to_json, admin_headers)
+
+            expect(last_response.status).to eql(502)
+            expect(decoded_response['code']).to eql(270012)
+            expect(decoded_response['description']).to eql("Service broker catalog is invalid: \n" \
+          "Service MySQL\n" \
+          "  Plan small\n" \
+          "    Schemas\n" \
+          "      Schema #{path} is not valid. No external references are allowed: Read of URI at http://example.com/ref refused\n"
+                                                       )
+          end
+        end
+      end
+    end
+
+    context 'when multiple schemas have validation issues' do
+      let(:schema) {
+        {
+          'service_instance' => {
+            'create' => { 'parameters' => { '$ref': 'http://example.com/ref' } },
+            'update' => { 'parameters' => { '$ref': 'http://example.com/ref' } }
+          }
+        }
+      }
+
+      before do
+        stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
+      end
+
+      it 'reports all issues' do
+        post('/v2/service_brokers', {
+            name: 'some-guid',
+            broker_url: 'http://broker-url',
+            auth_username: 'username',
+            auth_password: 'password'
+        }.to_json, admin_headers)
+
+        expect(last_response.status).to eql(502)
+        expect(decoded_response['code']).to eql(270012)
+        expect(decoded_response['description']).to eql("Service broker catalog is invalid: \n" \
+          "Service MySQL\n" \
+          "  Plan small\n" \
+          "    Schemas\n" \
+          "      Schema service_instance.create.parameters is not valid. No external references are allowed: Read of URI at http://example.com/ref refused\n" \
+          "      Schema service_instance.update.parameters is not valid. No external references are allowed: Read of URI at http://example.com/ref refused\n"
+                                                   )
+      end
+    end
   end
 
   describe 'updating a service broker' do
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 7715975149e..4af2f3973bc 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -225,16 +225,17 @@ module VCAP::Services::ServiceBrokers::V2
               }
             }
 
-            its(:create_instance) {
+            its('create_instance.to_json') {
               should eq(
                 {
                     :'$schema' => 'http://json-schema.org/draft-04/schema#',
                     'type' => 'object',
                     :properties => { foo: { type: 'string' } },
                     :required => ['foo']
-                }
-                     )
+                }.to_json
+              )
             }
+
             its(:valid?) { should be true }
             its(:errors) { should be_empty }
           end
@@ -251,12 +252,6 @@ module VCAP::Services::ServiceBrokers::V2
 
                   its(:valid?) { should be true }
                   its(:errors) { should be_empty }
-
-                  it 'does perform further validation' do
-                    expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
-                    expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
-                    subject
-                  end
                 end
               end
             end
@@ -273,12 +268,6 @@ module VCAP::Services::ServiceBrokers::V2
                   its(:valid?) { should be false }
                   its('errors.messages') { should have(1).items }
                   its('errors.messages.first') { should eq "Schema #{path} is not valid. Must not be larger than 64KB" }
-
-                  it 'does not perform further validation' do
-                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
-                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
-                    subject
-                  end
                 end
               end
             end
@@ -462,14 +451,14 @@ module VCAP::Services::ServiceBrokers::V2
               }
             }
 
-            its(:update_instance) {
+            its('update_instance.to_json') {
               should eq(
                 {
                     :'$schema' => 'http://json-schema.org/draft-04/schema#',
                     'type' => 'object',
                     :properties => { foo: { type: 'string' } },
                     :required => ['foo']
-                }
+                }.to_json
                      )
             }
             its(:valid?) { should be true }
@@ -488,12 +477,6 @@ module VCAP::Services::ServiceBrokers::V2
 
                   its(:valid?) { should be true }
                   its(:errors) { should be_empty }
-
-                  it 'does perform further validation' do
-                    expect_any_instance_of(CatalogSchemas).to receive(:validate_metaschema)
-                    expect_any_instance_of(CatalogSchemas).to receive(:validate_no_external_references)
-                    subject
-                  end
                 end
               end
             end
@@ -510,12 +493,6 @@ module VCAP::Services::ServiceBrokers::V2
                   its(:valid?) { should be false }
                   its('errors.messages') { should have(1).items }
                   its('errors.messages.first') { should eq "Schema #{path} is not valid. Must not be larger than 64KB" }
-
-                  it 'does not perform further validation' do
-                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_metaschema)
-                    expect_any_instance_of(CatalogSchemas).to_not receive(:validate_no_external_references)
-                    subject
-                  end
                 end
               end
             end

From ec28ab5af11bdd01802be95d2a62a07c0870f8c6 Mon Sep 17 00:00:00 2001
From: Alex Blease <ablease@pivotal.io>
Date: Fri, 28 Jul 2017 16:35:28 +0100
Subject: [PATCH 11/15] Refactor CatalogSchema validations

* Change Schema length validation to use ActiveModel

[#149550919]

Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>
---
 .../service_brokers/v2/catalog_schemas.rb     | 93 +++++++++----------
 .../v2/catalog_schemas_spec.rb                |  6 +-
 2 files changed, 48 insertions(+), 51 deletions(-)

diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index a3c4018cffd..db7bf480807 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -8,40 +8,41 @@ class CatalogSchemas
     def initialize(schemas)
       @errors = VCAP::Services::ValidationErrors.new
       @schemas = schemas
+      return unless schemas
 
-      return unless validate_structure([])
-
+      return unless validate_structure(@schemas, [])
       service_instance_path = ['service_instance']
-      return unless validate_structure(service_instance_path)
+      return unless validate_structure(@schemas, service_instance_path)
 
-      create_instance_hash = validate_and_populate_create(service_instance_path)
-      update_instance_hash = validate_and_populate_update(service_instance_path)
+      create_schema = get_method('create', @schemas)
+      @create_instance = Schema.new(create_schema) if create_schema
 
-      if create_instance_hash
-        @create_instance = Schema.new(create_instance_hash, 'service_instance.create.parameters')
-        if !create_instance.validate
-          create_instance.errors.messages.each { |key, value| value.each { |error| errors.add(error) } }
-        end
+      update_schema = get_method('update', @schemas)
+      @update_instance = Schema.new(update_schema) if update_schema
+    end
+
+    def valid?
+      return false unless errors.empty?
+
+      if create_instance && !create_instance.validate
+        add_schema_validation_errors(create_instance.errors, 'service_instance.create.parameters')
       end
 
-      if update_instance_hash
-        @update_instance = Schema.new(update_instance_hash, 'service_instance.update.parameters')
-        if !update_instance.validate
-          update_instance.errors.messages.each { |key, value| value.each { |error| errors.add(error) } }
-        end
+      if update_instance && !update_instance.validate
+        add_schema_validation_errors(update_instance.errors, 'service_instance.update.parameters')
       end
-    end
 
-    def valid?
       errors.empty?
     end
 
     private
 
-    attr_reader :schemas
-
-    def validate_structure(path)
-      schema = path.reduce(@schemas) { |current, key|
+    def validate_structure(schemas, path)
+      unless schemas.is_a? Hash
+        add_schema_type_error_msg(path, schemas)
+        return false
+      end
+      schema = path.reduce(schemas) { |current, key|
         return false unless current.key?(key)
         current.fetch(key)
       }
@@ -51,27 +52,26 @@ def validate_structure(path)
         add_schema_type_error_msg(path, schema)
         return false
       end
+
       true
     end
 
-    def validate_and_populate_create(path)
-      create_path = path + ['create']
-      return unless validate_structure(create_path)
+    def get_method(method, schema)
+      path = ['service_instance', method]
+      return unless validate_structure(schema, path)
 
-      create_parameter_path = create_path + ['parameters']
-      return unless validate_structure(create_parameter_path)
+      path = ['service_instance', method, 'parameters']
+      return unless validate_structure(schema, path)
 
-      @schemas['service_instance']['create']['parameters']
+      schema['service_instance'][method]['parameters']
     end
 
-    def validate_and_populate_update(path)
-      update_path = path + ['update']
-      return unless validate_structure(update_path)
-
-      update_parameter_path = update_path + ['parameters']
-      return unless validate_structure(update_parameter_path)
-
-      @schemas['service_instance']['update']['parameters']
+    def add_schema_validation_errors(schema_errors, path)
+      schema_errors.messages.each do |_, error_list|
+        error_list.each do |error_msg|
+          errors.add("Schema #{path} is not valid. #{error_msg}")
+        end
+      end
     end
 
     def add_schema_type_error_msg(path, value)
@@ -83,18 +83,19 @@ def add_schema_type_error_msg(path, value)
   class Schema
     include ActiveModel::Validations
 
-    validate :validate_schema_size, :validate_metaschema, :validate_no_external_references, :validate_schema_type
+    validates :to_json, length: { maximum: MAX_SCHEMA_SIZE, message: 'Must not be larger than 64KB' }
+    validate :validate_metaschema, :validate_no_external_references, :validate_schema_type
 
-    def initialize(schema, path)
+    def initialize(schema)
       @schema = schema
-      @path = path
     end
 
-    def validate_schema_size
-      return unless errors.blank?
-      add_schema_error_msg('Must not be larger than 64KB') if @schema.to_json.length > MAX_SCHEMA_SIZE
+    def to_json
+      @schema.to_json
     end
 
+    private
+
     def validate_metaschema
       return unless errors.blank?
       JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
@@ -137,15 +138,7 @@ def validate_schema_type
     end
 
     def add_schema_error_msg(err)
-      errors.add(:base, "Schema #{@path} is not valid. #{err}")
-    end
-
-    def add_schema_type_error_msg(value)
-      errors.add(:base, "Schemas #{@path} must be a hash, but has value #{value.inspect}")
-    end
-
-    def to_json
-      @schema.to_json
+      errors.add(:base, err.to_s)
     end
   end
 end
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 4af2f3973bc..807e672e812 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -3,7 +3,11 @@
 module VCAP::Services::ServiceBrokers::V2
   RSpec.describe CatalogSchemas do
     describe 'initializing catalog schemas' do
-      subject { CatalogSchemas.new(attrs) }
+      subject do
+        catalog_schema = CatalogSchemas.new(attrs)
+        catalog_schema.valid?
+        catalog_schema
+      end
 
       context 'service instance' do
         context 'when catalog has schemas' do

From 18709a61f73682e7a24dff0fec908b7370e3cf60 Mon Sep 17 00:00:00 2001
From: Sam Gunaratne <sgunaratne@pivotal.io>
Date: Fri, 28 Jul 2017 17:05:54 +0100
Subject: [PATCH 12/15] Add separate unit tests for Broker Schema class

[#149550919]

Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>
---
 .../v2/catalog_schemas_spec.rb                | 167 +++++++++++++++++-
 1 file changed, 166 insertions(+), 1 deletion(-)

diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 807e672e812..2cf379af3c2 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -2,7 +2,7 @@
 
 module VCAP::Services::ServiceBrokers::V2
   RSpec.describe CatalogSchemas do
-    describe 'initializing catalog schemas' do
+    describe 'validating catalog schemas' do
       subject do
         catalog_schema = CatalogSchemas.new(attrs)
         catalog_schema.valid?
@@ -521,4 +521,169 @@ def create_schema_of_size(bytes)
       end
     end
   end
+
+  RSpec.describe Schema do
+    describe 'validating schema' do
+      subject do
+        schema = Schema.new(raw_schema)
+        schema.validate
+        schema
+      end
+      let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
+
+      context 'when attrs have an invalid value as type' do
+        let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+        }
+      end
+
+      context 'when the schema does not conform to JSON Schema Draft 04' do
+        let(:raw_schema) { { 'properties': true } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should eq "Must conform to JSON Schema Draft 04: The property '#/properties' " \
+                          'of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
+        }
+      end
+
+      context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
+        let(:raw_schema) { { 'type': 'foo', 'properties': true } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(2).items }
+        its('errors.full_messages.first') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
+        }
+        its('errors.full_messages.second') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
+        }
+      end
+
+      context 'when the schema has an external schema' do
+        let(:raw_schema) { { '$schema': 'http://example.com/schema' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should eq 'Custom meta schemas are not supported.'
+        }
+      end
+
+      context 'when the schema has an external uri reference' do
+        let(:raw_schema) { { '$ref': 'http://example.com/ref' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should match 'No external references are allowed.+http://example.com/ref'
+        }
+      end
+
+      context 'when the schema has an external file reference' do
+        let(:raw_schema) { { '$ref': 'path/to/schema.json' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should match 'No external references are allowed.+path/to/schema.json'
+        }
+      end
+
+      context 'when the schema has an internal reference' do
+        let(:raw_schema) {
+          {
+              'type' => 'object',
+              'properties': {
+                  'foo': { 'type': 'integer' },
+                  'bar': { '$ref': '#/properties/foo' }
+              }
+          }
+        }
+
+        its(:validate) { should be true }
+        its(:errors) { should be_empty }
+      end
+
+      context 'when the schema has multiple valid constraints ' do
+        let(:raw_schema) {
+          {
+              :'$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object',
+              :properties => { 'foo': { 'type': 'string' } },
+              :required => ['foo']
+          }
+        }
+
+        its(:to_json) {
+          should eq(
+            {
+                :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object',
+                :properties => { foo: { type: 'string' } },
+                :required => ['foo']
+            }.to_json
+                 )
+        }
+        its(:validate) { should be true }
+        its(:errors) { should be_empty }
+      end
+
+      describe 'schema sizes' do
+        context 'that are valid' do
+          {
+              'well below the limit': 1,
+              'just below the limit': 63,
+              'on the limit': 64,
+          }.each do |desc, size_in_kb|
+            context "when the schema is #{desc}" do
+              let(:raw_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+              its(:validate) { should be true }
+              its(:errors) { should be_empty }
+            end
+          end
+        end
+
+        context 'that are invalid' do
+          {
+              'just above the limit': 65,
+              'well above the limit': 10 * 1024,
+          }.each do |desc, size_in_kb|
+            context "when the schema is #{desc}" do
+              let(:raw_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+              its(:validate) { should be false }
+              its('errors.full_messages') { should have(1).items }
+              its('errors.full_messages.first') { should eq 'To json Must not be larger than 64KB' }
+            end
+          end
+        end
+      end
+
+      context 'when the schema does not have a type field' do
+        let(:raw_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') { should eq 'must have field "type", with value "object"' }
+      end
+
+      def create_schema_of_size(bytes)
+        surrounding_bytes = 26
+        {
+            'type' => 'object',
+            'foo' => 'x' * (bytes - surrounding_bytes)
+        }
+      end
+    end
+  end
 end

From 548ebfacf67474c34dcc1e72bc2e9d3dd8df53e8 Mon Sep 17 00:00:00 2001
From: Alex Blease <ablease@pivotal.io>
Date: Mon, 31 Jul 2017 17:08:35 +0100
Subject: [PATCH 13/15] Extracted Schema into it's own class along with the
 tests.

[#149550919]

Signed-off-by: Luis Urraca <lurraca@pivotal.io>
---
 lib/services/service_brokers/v2.rb            |   1 +
 .../service_brokers/v2/catalog_schemas.rb     |  65 ---
 lib/services/service_brokers/v2/schema.rb     |  68 +++
 .../v2/catalog_schemas_spec.rb                | 552 ++----------------
 .../service_brokers/v2/schema_spec.rb         | 202 +++++++
 5 files changed, 321 insertions(+), 567 deletions(-)
 create mode 100644 lib/services/service_brokers/v2/schema.rb
 create mode 100644 spec/unit/lib/services/service_brokers/v2/schema_spec.rb

diff --git a/lib/services/service_brokers/v2.rb b/lib/services/service_brokers/v2.rb
index 54ab723306e..14e98b34597 100644
--- a/lib/services/service_brokers/v2.rb
+++ b/lib/services/service_brokers/v2.rb
@@ -5,6 +5,7 @@ module VCAP::Services::ServiceBrokers::V2 end
 require 'services/service_brokers/v2/catalog_service'
 require 'services/service_brokers/v2/catalog_plan'
 require 'services/service_brokers/v2/catalog_schemas'
+require 'services/service_brokers/v2/schema'
 require 'services/service_brokers/v2/http_client'
 require 'services/service_brokers/v2/client'
 require 'services/service_brokers/v2/orphan_mitigator'
diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index db7bf480807..c311badf8f9 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -1,7 +1,4 @@
-require 'json-schema'
-
 module VCAP::Services::ServiceBrokers::V2
-  MAX_SCHEMA_SIZE = 65_536
   class CatalogSchemas
     attr_reader :errors, :create_instance, :update_instance
 
@@ -79,66 +76,4 @@ def add_schema_type_error_msg(path, value)
       errors.add("Schemas#{path} must be a hash, but has value #{value.inspect}")
     end
   end
-
-  class Schema
-    include ActiveModel::Validations
-
-    validates :to_json, length: { maximum: MAX_SCHEMA_SIZE, message: 'Must not be larger than 64KB' }
-    validate :validate_metaschema, :validate_no_external_references, :validate_schema_type
-
-    def initialize(schema)
-      @schema = schema
-    end
-
-    def to_json
-      @schema.to_json
-    end
-
-    private
-
-    def validate_metaschema
-      return unless errors.blank?
-      JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
-      file = File.read(JSON::Validator.validator_for_name('draft4').metaschema)
-
-      metaschema = JSON.parse(file)
-
-      begin
-        errors = JSON::Validator.fully_validate(metaschema, @schema, errors_as_objects: true)
-      rescue => e
-        add_schema_error_msg(e)
-        return nil
-      end
-
-      errors.each do |error|
-        add_schema_error_msg("Must conform to JSON Schema Draft 04: #{error[:message]}")
-      end
-    end
-
-    def validate_no_external_references
-      return unless errors.blank?
-      JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
-
-      begin
-        JSON::Validator.validate!(@schema, {})
-      rescue JSON::Schema::SchemaError
-        add_schema_error_msg('Custom meta schemas are not supported.')
-      rescue JSON::Schema::ReadRefused => e
-        add_schema_error_msg("No external references are allowed: #{e}")
-      rescue JSON::Schema::ValidationError
-        # We don't care if our input fails validation on broker schema
-      rescue => e
-        add_schema_error_msg(e)
-      end
-    end
-
-    def validate_schema_type
-      return unless errors.blank?
-      add_schema_error_msg('must have field "type", with value "object"') if @schema['type'] != 'object'
-    end
-
-    def add_schema_error_msg(err)
-      errors.add(:base, err.to_s)
-    end
-  end
 end
diff --git a/lib/services/service_brokers/v2/schema.rb b/lib/services/service_brokers/v2/schema.rb
new file mode 100644
index 00000000000..2eda0195a11
--- /dev/null
+++ b/lib/services/service_brokers/v2/schema.rb
@@ -0,0 +1,68 @@
+require 'json-schema'
+
+module VCAP::Services::ServiceBrokers::V2
+  class Schema
+    include ActiveModel::Validations
+    attr_reader :schema
+
+    MAX_SCHEMA_SIZE = 65_536
+
+    validates :to_json, length: { maximum: MAX_SCHEMA_SIZE, message: 'Must not be larger than 64KB' }
+    validate :validate_metaschema, :validate_no_external_references, :validate_schema_type
+
+    def initialize(schema)
+      @schema = schema
+    end
+
+    def to_json
+      @schema.to_json
+    end
+
+    private
+
+    def validate_metaschema
+      return unless errors.blank?
+      JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
+      file = File.read(JSON::Validator.validator_for_name('draft4').metaschema)
+
+      metaschema = JSON.parse(file)
+
+      begin
+        errors = JSON::Validator.fully_validate(metaschema, @schema, errors_as_objects: true)
+      rescue => e
+        add_schema_error_msg(e)
+        return nil
+      end
+
+      errors.each do |error|
+        add_schema_error_msg("Must conform to JSON Schema Draft 04: #{error[:message]}")
+      end
+    end
+
+    def validate_no_external_references
+      return unless errors.blank?
+      JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
+
+      begin
+        JSON::Validator.validate!(@schema, {})
+      rescue JSON::Schema::SchemaError
+        add_schema_error_msg('Custom meta schemas are not supported.')
+      rescue JSON::Schema::ReadRefused => e
+        add_schema_error_msg("No external references are allowed: #{e}")
+      rescue JSON::Schema::ValidationError
+        # We don't care if our input fails validation on broker schema
+      rescue => e
+        add_schema_error_msg(e)
+      end
+    end
+
+    def validate_schema_type
+      return unless errors.blank?
+      add_schema_error_msg('must have field "type", with value "object"') if @schema['type'] != 'object'
+    end
+
+    def add_schema_error_msg(err)
+      errors.add(:base, err.to_s)
+    end
+  end
+end
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index 2cf379af3c2..e9efac20be9 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -47,46 +47,13 @@ module VCAP::Services::ServiceBrokers::V2
         end
 
         context 'when catalog has a create schema' do
-          let(:path) { 'service_instance.create.parameters' }
-          let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
-          let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => create_instance_schema } } } }
+          context 'and the schema structure is valid' do
+            let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } } } } }
 
-          context 'when attrs have an invalid value as type' do
-            let(:create_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
-                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
-            }
-          end
-
-          context 'when schema has multiple invalid types' do
-            let(:create_instance_schema) {
-              {
-                '$schema' => 'http://json-schema.org/draft-04/schema#',
-                'type' => 'object',
-                'properties' => {
-                  'test' => { 'type' => 'notatype' },
-                  'b2' => { 'type' => 'alsonotatype' }
-                }
-              }
-            }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(2).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
-                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
-            }
-            its('errors.messages.last') {
-              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
-                          "The property '#/properties/b2/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
-            }
+            its(:create_instance) { should be_a(Schema) }
           end
 
-          context 'when attrs have nil value' do
+          context 'and it has nil value' do
             {
                 'Schemas service_instance.create': { 'service_instance' => { 'create' => nil } },
                 'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
@@ -101,6 +68,23 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
+          context 'when the create instance schema is not valid' do
+            let(:attrs) {
+              {
+                  'service_instance' => {
+                      'create' => {
+                          'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => true }
+                      }
+                  }
+              }
+            }
+            let(:path) { 'service_instance.create.parameters' }
+
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should match "Schema #{path} is not valid" }
+            its(:valid?) { should be false }
+          end
+
           context 'when attrs have a missing key' do
             {
                 'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
@@ -131,43 +115,8 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
-          context 'when the schema does not conform to JSON Schema Draft 04' do
-            let(:create_instance_schema) { { 'properties': true } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
-                          "The property '#/properties' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#"
-            }
-          end
-
-          context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-            let(:create_instance_schema) { { 'type': 'foo', 'properties': true } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(2).items }
-            its('errors.messages.first') {
-              should eq 'Schema service_instance.create.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
-            }
-            its('errors.messages.second') {
-              should eq 'Schema service_instance.create.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
-            }
-          end
-
-          context 'when the schema has an external schema' do
-            let(:create_instance_schema) { { '$schema': 'http://example.com/schema' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Custom meta schemas are not supported."
-            }
-          end
-
           context 'when attrs has a potentially dangerous uri' do
+            let(:path) { 'service_instance.create.parameters' }
             let(:attrs) {
               {
                   'service_instance' => {
@@ -183,133 +132,21 @@ module VCAP::Services::ServiceBrokers::V2
             its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
             its(:valid?) { should be false }
           end
+        end
 
-          context 'when the schema has an external uri reference' do
-            let(:create_instance_schema) { { '$ref': 'http://example.com/ref' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. No external references are allowed: Read of URI at http://example.com/ref refused"
-            }
-          end
-
-          context 'when the schema has an external file reference' do
-            let(:create_instance_schema) { { '$ref': 'path/to/schema.json' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should match "Schema #{path} is not valid. No external references are allowed.+path/to/schema.json"
-            }
-          end
-
-          context 'when the schema has an internal reference' do
-            let(:create_instance_schema) {
+        context 'when catalog has an update schema' do
+          context 'and the schema structure is valid' do
+            let(:attrs) {
               {
-                  'type' => 'object',
-                  'properties': {
-                      'foo': { 'type': 'integer' },
-                      'bar': { '$ref': '#/properties/foo' }
+                  'service_instance' => {
+                      'update' => {
+                          'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' }
+                      }
                   }
               }
             }
 
-            its(:valid?) { should be true }
-            its(:errors) { should be_empty }
-          end
-
-          context 'when the schema has multiple valid constraints ' do
-            let(:create_instance_schema) {
-              {
-                  :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                  'type' => 'object',
-                  :properties => { 'foo': { 'type': 'string' } },
-                  :required => ['foo']
-              }
-            }
-
-            its('create_instance.to_json') {
-              should eq(
-                {
-                    :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                    'type' => 'object',
-                    :properties => { foo: { type: 'string' } },
-                    :required => ['foo']
-                }.to_json
-              )
-            }
-
-            its(:valid?) { should be true }
-            its(:errors) { should be_empty }
-          end
-
-          describe 'schema sizes' do
-            context 'that are valid' do
-              {
-                  'well below the limit': 1,
-                  'just below the limit': 63,
-                  'on the limit': 64,
-              }.each do |desc, size_in_kb|
-                context "when the schema is #{desc}" do
-                  let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
-
-                  its(:valid?) { should be true }
-                  its(:errors) { should be_empty }
-                end
-              end
-            end
-
-            context 'that are invalid' do
-              {
-                  'just above the limit': 65,
-                  'well above the limit': 10 * 1024,
-              }.each do |desc, size_in_kb|
-                context "when the schema is #{desc}" do
-                  path = 'service_instance.create.parameters'
-                  let(:create_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
-
-                  its(:valid?) { should be false }
-                  its('errors.messages') { should have(1).items }
-                  its('errors.messages.first') { should eq "Schema #{path} is not valid. Must not be larger than 64KB" }
-                end
-              end
-            end
-          end
-
-          context 'when the schema does not have a type field' do
-            let(:create_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') { should eq "Schema #{path} is not valid. must have field \"type\", with value \"object\"" }
-          end
-
-          context 'when the schema has an unknown parse error' do
-            before do
-              allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
-            end
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') { should eq "Schema #{path} is not valid. some unknown error" }
-          end
-        end
-
-        context 'when catalog has an update schema' do
-          let(:path) { 'service_instance.update.parameters' }
-          let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
-          let(:attrs) { { 'service_instance' => { 'update' => { 'parameters' => update_instance_schema } } } }
-
-          context 'when attrs have an invalid value as type' do
-            let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: " \
-                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
-            }
+            its(:update_instance) { should be_a(Schema) }
           end
 
           context 'when attrs have nil value' do
@@ -327,6 +164,23 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
+          context 'when the update instance schema is not valid' do
+            let(:attrs) {
+              {
+                  'service_instance' => {
+                      'update' => {
+                          'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => true }
+                      }
+                  }
+              }
+            }
+            let(:path) { 'service_instance.update.parameters' }
+
+            its('errors.messages') { should have(1).items }
+            its('errors.messages.first') { should match "Schema #{path} is not valid" }
+            its(:valid?) { should be false }
+          end
+
           context 'when attrs have a missing key' do
             {
                 'Schemas service_instance.update': { 'service_instance' => { 'update' => {} } },
@@ -357,43 +211,10 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
-          context 'when the schema does not conform to JSON Schema Draft 04' do
-            let(:update_instance_schema) { { 'properties': true } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/properties' " \
-                          'of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
-            }
-          end
-
-          context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-            let(:update_instance_schema) { { 'type': 'foo', 'properties': true } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(2).items }
-            its('errors.messages.first') {
-              should eq 'Schema service_instance.update.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
-            }
-            its('errors.messages.second') {
-              should eq 'Schema service_instance.update.parameters is not valid. Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
-            }
-          end
-
-          context 'when the schema has an external schema' do
-            let(:update_instance_schema) { { '$schema': 'http://example.com/schema' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should eq "Schema #{path} is not valid. Custom meta schemas are not supported."
-            }
-          end
+          # TODO: Look into schema path is not valid error tests
 
           context 'when attrs has a potentially dangerous uri' do
+            let(:path) { 'service_instance.update.parameters' }
             let(:attrs) {
               {
                   'service_instance' => {
@@ -409,281 +230,8 @@ module VCAP::Services::ServiceBrokers::V2
             its('errors.messages.first') { should eq "Schemas #{path} must be a hash, but has value \"https://example.com/hax0r\"" }
             its(:valid?) { should be false }
           end
-
-          context 'when the schema has an external uri reference' do
-            let(:update_instance_schema) { { '$ref': 'http://example.com/ref' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should match "Schema #{path} is not valid\. No external references are allowed.+http://example.com/ref"
-            }
-          end
-
-          context 'when the schema has an external file reference' do
-            let(:update_instance_schema) { { '$ref': 'path/to/schema.json' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') {
-              should match "Schema #{path} is not valid\. No external references are allowed.+path/to/schema.json"
-            }
-          end
-
-          context 'when the schema has an internal reference' do
-            let(:update_instance_schema) {
-              {
-                  'type' => 'object',
-                  'properties': {
-                      'foo': { 'type': 'integer' },
-                      'bar': { '$ref': '#/properties/foo' }
-                  }
-              }
-            }
-
-            its(:valid?) { should be true }
-            its(:errors) { should be_empty }
-          end
-
-          context 'when the schema has multiple valid constraints ' do
-            let(:update_instance_schema) {
-              {
-                  :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                  'type' => 'object',
-                  :properties => { 'foo': { 'type': 'string' } },
-                  :required => ['foo']
-              }
-            }
-
-            its('update_instance.to_json') {
-              should eq(
-                {
-                    :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                    'type' => 'object',
-                    :properties => { foo: { type: 'string' } },
-                    :required => ['foo']
-                }.to_json
-                     )
-            }
-            its(:valid?) { should be true }
-            its(:errors) { should be_empty }
-          end
-
-          describe 'schema sizes' do
-            context 'that are valid' do
-              {
-                  'well below the limit': 1,
-                  'just below the limit': 63,
-                  'on the limit': 64,
-              }.each do |desc, size_in_kb|
-                context "when the schema is #{desc}" do
-                  let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
-
-                  its(:valid?) { should be true }
-                  its(:errors) { should be_empty }
-                end
-              end
-            end
-
-            context 'that are invalid' do
-              {
-                  'just above the limit': 65,
-                  'well above the limit': 10 * 1024,
-              }.each do |desc, size_in_kb|
-                context "when the schema is #{desc}" do
-                  path = 'service_instance.update.parameters'
-                  let(:update_instance_schema) { create_schema_of_size(size_in_kb * 1024) }
-
-                  its(:valid?) { should be false }
-                  its('errors.messages') { should have(1).items }
-                  its('errors.messages.first') { should eq "Schema #{path} is not valid. Must not be larger than 64KB" }
-                end
-              end
-            end
-          end
-
-          context 'when the schema does not have a type field' do
-            let(:update_instance_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
-
-            its(:valid?) { should be false }
-            its('errors.messages') { should have(1).items }
-            its('errors.messages.first') { should eq "Schema #{path} is not valid. must have field \"type\", with value \"object\"" }
-          end
         end
       end
-
-      def create_schema_of_size(bytes)
-        surrounding_bytes = 26
-        {
-            'type' => 'object',
-            'foo' => 'x' * (bytes - surrounding_bytes)
-        }
-      end
-    end
-  end
-
-  RSpec.describe Schema do
-    describe 'validating schema' do
-      subject do
-        schema = Schema.new(raw_schema)
-        schema.validate
-        schema
-      end
-      let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
-
-      context 'when attrs have an invalid value as type' do
-        let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(1).items }
-        its('errors.full_messages.first') {
-          should eq 'Must conform to JSON Schema Draft 04: ' \
-                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
-        }
-      end
-
-      context 'when the schema does not conform to JSON Schema Draft 04' do
-        let(:raw_schema) { { 'properties': true } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(1).items }
-        its('errors.full_messages.first') {
-          should eq "Must conform to JSON Schema Draft 04: The property '#/properties' " \
-                          'of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
-        }
-      end
-
-      context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-        let(:raw_schema) { { 'type': 'foo', 'properties': true } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(2).items }
-        its('errors.full_messages.first') {
-          should eq 'Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
-        }
-        its('errors.full_messages.second') {
-          should eq 'Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
-        }
-      end
-
-      context 'when the schema has an external schema' do
-        let(:raw_schema) { { '$schema': 'http://example.com/schema' } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(1).items }
-        its('errors.full_messages.first') {
-          should eq 'Custom meta schemas are not supported.'
-        }
-      end
-
-      context 'when the schema has an external uri reference' do
-        let(:raw_schema) { { '$ref': 'http://example.com/ref' } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(1).items }
-        its('errors.full_messages.first') {
-          should match 'No external references are allowed.+http://example.com/ref'
-        }
-      end
-
-      context 'when the schema has an external file reference' do
-        let(:raw_schema) { { '$ref': 'path/to/schema.json' } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(1).items }
-        its('errors.full_messages.first') {
-          should match 'No external references are allowed.+path/to/schema.json'
-        }
-      end
-
-      context 'when the schema has an internal reference' do
-        let(:raw_schema) {
-          {
-              'type' => 'object',
-              'properties': {
-                  'foo': { 'type': 'integer' },
-                  'bar': { '$ref': '#/properties/foo' }
-              }
-          }
-        }
-
-        its(:validate) { should be true }
-        its(:errors) { should be_empty }
-      end
-
-      context 'when the schema has multiple valid constraints ' do
-        let(:raw_schema) {
-          {
-              :'$schema' => 'http://json-schema.org/draft-04/schema#',
-              'type' => 'object',
-              :properties => { 'foo': { 'type': 'string' } },
-              :required => ['foo']
-          }
-        }
-
-        its(:to_json) {
-          should eq(
-            {
-                :'$schema' => 'http://json-schema.org/draft-04/schema#',
-                'type' => 'object',
-                :properties => { foo: { type: 'string' } },
-                :required => ['foo']
-            }.to_json
-                 )
-        }
-        its(:validate) { should be true }
-        its(:errors) { should be_empty }
-      end
-
-      describe 'schema sizes' do
-        context 'that are valid' do
-          {
-              'well below the limit': 1,
-              'just below the limit': 63,
-              'on the limit': 64,
-          }.each do |desc, size_in_kb|
-            context "when the schema is #{desc}" do
-              let(:raw_schema) { create_schema_of_size(size_in_kb * 1024) }
-
-              its(:validate) { should be true }
-              its(:errors) { should be_empty }
-            end
-          end
-        end
-
-        context 'that are invalid' do
-          {
-              'just above the limit': 65,
-              'well above the limit': 10 * 1024,
-          }.each do |desc, size_in_kb|
-            context "when the schema is #{desc}" do
-              let(:raw_schema) { create_schema_of_size(size_in_kb * 1024) }
-
-              its(:validate) { should be false }
-              its('errors.full_messages') { should have(1).items }
-              its('errors.full_messages.first') { should eq 'To json Must not be larger than 64KB' }
-            end
-          end
-        end
-      end
-
-      context 'when the schema does not have a type field' do
-        let(:raw_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
-
-        its(:validate) { should be false }
-        its('errors.full_messages') { should have(1).items }
-        its('errors.full_messages.first') { should eq 'must have field "type", with value "object"' }
-      end
-
-      def create_schema_of_size(bytes)
-        surrounding_bytes = 26
-        {
-            'type' => 'object',
-            'foo' => 'x' * (bytes - surrounding_bytes)
-        }
-      end
     end
   end
 end
diff --git a/spec/unit/lib/services/service_brokers/v2/schema_spec.rb b/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
new file mode 100644
index 00000000000..b03eedcaa81
--- /dev/null
+++ b/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
@@ -0,0 +1,202 @@
+require 'spec_helper'
+
+module VCAP::Services::ServiceBrokers::V2
+  RSpec.describe Schema do
+    describe 'validating schema' do
+      subject do
+        schema = Schema.new(raw_schema)
+        schema.validate
+        schema
+      end
+      let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
+
+      context 'when attrs have an invalid value as type' do
+        let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object', 'properties' => { 'test' => { 'type' => 'notatype' } } } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+        }
+      end
+
+      context 'when schema has multiple invalid types' do
+        let(:raw_schema) {
+          {
+              '$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object',
+              'properties' => {
+                  'test' => { 'type' => 'notatype' },
+                  'b2' => { 'type' => 'alsonotatype' }
+              }
+          }
+        }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(2).items }
+        its('errors.full_messages.first') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          "The property '#/properties/test/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+        }
+        its('errors.full_messages.last') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          "The property '#/properties/b2/type' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#"
+        }
+      end
+
+      context 'when the schema does not conform to JSON Schema Draft 04' do
+        let(:raw_schema) { { 'properties': true } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should eq "Must conform to JSON Schema Draft 04: The property '#/properties' " \
+                          'of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
+        }
+      end
+
+      context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
+        let(:raw_schema) { { 'type': 'foo', 'properties': true } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(2).items }
+        its('errors.full_messages.first') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/properties\' of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#'
+        }
+        its('errors.full_messages.second') {
+          should eq 'Must conform to JSON Schema Draft 04: ' \
+                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
+        }
+      end
+
+      context 'when the schema has an external schema' do
+        let(:raw_schema) { { '$schema': 'http://example.com/schema' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should eq 'Custom meta schemas are not supported.'
+        }
+      end
+
+      context 'when the schema has an external uri reference' do
+        let(:raw_schema) { { '$ref': 'http://example.com/ref' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should match 'No external references are allowed.+http://example.com/ref'
+        }
+      end
+
+      context 'when the schema has an external file reference' do
+        let(:raw_schema) { { '$ref': 'path/to/schema.json' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') {
+          should match 'No external references are allowed.+path/to/schema.json'
+        }
+      end
+
+      context 'when the schema has an internal reference' do
+        let(:raw_schema) {
+          {
+              'type' => 'object',
+              'properties': {
+                  'foo': { 'type': 'integer' },
+                  'bar': { '$ref': '#/properties/foo' }
+              }
+          }
+        }
+
+        its(:validate) { should be true }
+        its(:errors) { should be_empty }
+      end
+
+      context 'when the schema has multiple valid constraints ' do
+        let(:raw_schema) {
+          {
+              :'$schema' => 'http://json-schema.org/draft-04/schema#',
+              'type' => 'object',
+              :properties => { 'foo': { 'type': 'string' } },
+              :required => ['foo']
+          }
+        }
+
+        its(:to_json) {
+          should eq(
+            {
+                :'$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object',
+                :properties => { foo: { type: 'string' } },
+                :required => ['foo']
+            }.to_json
+                 )
+        }
+        its(:validate) { should be true }
+        its(:errors) { should be_empty }
+      end
+
+      describe 'schema sizes' do
+        context 'that are valid' do
+          {
+              'well below the limit': 1,
+              'just below the limit': 63,
+              'on the limit': 64,
+          }.each do |desc, size_in_kb|
+            context "when the schema is #{desc}" do
+              let(:raw_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+              its(:validate) { should be true }
+              its(:errors) { should be_empty }
+            end
+          end
+        end
+
+        context 'that are invalid' do
+          {
+              'just above the limit': 65,
+              'well above the limit': 10 * 1024,
+          }.each do |desc, size_in_kb|
+            context "when the schema is #{desc}" do
+              let(:raw_schema) { create_schema_of_size(size_in_kb * 1024) }
+
+              its(:validate) { should be false }
+              its('errors.full_messages') { should have(1).items }
+              its('errors.full_messages.first') { should eq 'To json Must not be larger than 64KB' }
+            end
+          end
+        end
+      end
+
+      context 'when the schema does not have a type field' do
+        let(:raw_schema) { { '$schema': 'http://json-schema.org/draft-04/schema#' } }
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') { should eq 'must have field "type", with value "object"' }
+      end
+
+      context 'when the schema has an unknown parse error' do
+        before do
+          allow(JSON::Validator).to receive(:validate!) { raise 'some unknown error' }
+        end
+
+        its(:validate) { should be false }
+        its('errors.full_messages') { should have(1).items }
+        its('errors.full_messages.first') { should eq 'some unknown error' }
+      end
+
+      def create_schema_of_size(bytes)
+        surrounding_bytes = 26
+        {
+            'type' => 'object',
+            'foo' => 'x' * (bytes - surrounding_bytes)
+        }
+      end
+    end
+  end
+end

From f7bf24938a473aa0d299f62dac74e0438e9cac95 Mon Sep 17 00:00:00 2001
From: Luis Urraca <lurraca@pivotal.io>
Date: Tue, 1 Aug 2017 14:59:10 +0100
Subject: [PATCH 14/15] Perform schema validations in required order

[#149550919]

Signed-off-by: Luis Urraca <lurraca@pivotal.io>
---
 lib/services/service_brokers/v2/schema.rb     | 12 ++--
 spec/acceptance/service_broker_spec.rb        | 18 ++---
 .../service_brokers/v2/schema_spec.rb         | 72 +++++++++++++++++--
 3 files changed, 81 insertions(+), 21 deletions(-)

diff --git a/lib/services/service_brokers/v2/schema.rb b/lib/services/service_brokers/v2/schema.rb
index 2eda0195a11..96c2e692902 100644
--- a/lib/services/service_brokers/v2/schema.rb
+++ b/lib/services/service_brokers/v2/schema.rb
@@ -8,7 +8,7 @@ class Schema
     MAX_SCHEMA_SIZE = 65_536
 
     validates :to_json, length: { maximum: MAX_SCHEMA_SIZE, message: 'Must not be larger than 64KB' }
-    validate :validate_metaschema, :validate_no_external_references, :validate_schema_type
+    validate :validate_schema_type, :validate_metaschema, :validate_no_external_references
 
     def initialize(schema)
       @schema = schema
@@ -20,6 +20,11 @@ def to_json
 
     private
 
+    def validate_schema_type
+      return unless errors.blank?
+      add_schema_error_msg('must have field "type", with value "object"') if @schema['type'] != 'object'
+    end
+
     def validate_metaschema
       return unless errors.blank?
       JSON::Validator.schema_reader = JSON::Schema::Reader.new(accept_uri: false, accept_file: false)
@@ -56,11 +61,6 @@ def validate_no_external_references
       end
     end
 
-    def validate_schema_type
-      return unless errors.blank?
-      add_schema_error_msg('must have field "type", with value "object"') if @schema['type'] != 'object'
-    end
-
     def add_schema_error_msg(err)
       errors.add(:base, err.to_s)
     end
diff --git a/spec/acceptance/service_broker_spec.rb b/spec/acceptance/service_broker_spec.rb
index 4e3fa334491..baf9b0d731a 100644
--- a/spec/acceptance/service_broker_spec.rb
+++ b/spec/acceptance/service_broker_spec.rb
@@ -127,7 +127,7 @@ def build_service(attrs={})
                   schemas: {
                     service_instance: {
                       create: {
-                        parameters: { properties: true }
+                        parameters: { type: 'object', properties: true }
                       }
                     }
                   }
@@ -359,7 +359,7 @@ def build_service(attrs={})
 
         context 'does not conform to JSON Schema Draft 04' do
           let(:path) { "service_instance.#{service_instance_type}.parameters" }
-          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'properties': true } } } } }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'type': 'object', 'properties': true } } } } }
 
           before do
             stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
@@ -386,7 +386,7 @@ def build_service(attrs={})
 
         context 'does not conform to JSON Schema Draft 04 with multiple problems' do
           let(:path) { "service_instance.#{service_instance_type}.parameters" }
-          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'type': 'foo', 'properties': true } } } } }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'type': 'object', 'properties': true, 'anyOf': true } } } } }
 
           before do
             stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
@@ -407,14 +407,14 @@ def build_service(attrs={})
           "    Schemas\n" \
           "      Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/properties' " \
                       "of type boolean did not match the following type: object in schema http://json-schema.org/draft-04/schema#\n"\
-          "      Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/type' " \
-                      "of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#\n")
+          "      Schema #{path} is not valid. Must conform to JSON Schema Draft 04: The property '#/anyOf' " \
+                      "of type boolean did not match the following type: array in schema http://json-schema.org/draft-04/schema#\n")
           end
         end
 
         context 'has an external schema' do
           let(:path) { "service_instance.#{service_instance_type}.parameters" }
-          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { '$schema': 'http://example.com/schema' } } } } }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { '$schema': 'http://example.com/schema', 'type': 'object' } } } } }
 
           before do
             stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
@@ -441,7 +441,7 @@ def build_service(attrs={})
 
         context 'has an external uri reference' do
           let(:path) { "service_instance.#{service_instance_type}.parameters" }
-          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { '$ref': 'http://example.com/ref' } } } } }
+          let(:schema) { { 'service_instance' => { service_instance_type => { 'parameters' => { 'type': 'object', '$ref': 'http://example.com/ref' } } } } }
 
           before do
             stub_catalog_fetch(200, default_catalog(plan_schemas: schema))
@@ -471,8 +471,8 @@ def build_service(attrs={})
       let(:schema) {
         {
           'service_instance' => {
-            'create' => { 'parameters' => { '$ref': 'http://example.com/ref' } },
-            'update' => { 'parameters' => { '$ref': 'http://example.com/ref' } }
+            'create' => { 'parameters' => { 'type': 'object', '$ref': 'http://example.com/ref' } },
+            'update' => { 'parameters' => { 'type': 'object', '$ref': 'http://example.com/ref' } }
           }
         }
       }
diff --git a/spec/unit/lib/services/service_brokers/v2/schema_spec.rb b/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
index b03eedcaa81..3ff746f20ef 100644
--- a/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
@@ -46,7 +46,7 @@ module VCAP::Services::ServiceBrokers::V2
       end
 
       context 'when the schema does not conform to JSON Schema Draft 04' do
-        let(:raw_schema) { { 'properties': true } }
+        let(:raw_schema) { { 'type' => 'object', 'properties': true } }
 
         its(:validate) { should be false }
         its('errors.full_messages') { should have(1).items }
@@ -57,7 +57,7 @@ module VCAP::Services::ServiceBrokers::V2
       end
 
       context 'when the schema does not conform to JSON Schema Draft 04 with multiple problems' do
-        let(:raw_schema) { { 'type': 'foo', 'properties': true } }
+        let(:raw_schema) { { 'type' => 'object', 'properties': true, 'anyOf': true } }
 
         its(:validate) { should be false }
         its('errors.full_messages') { should have(2).items }
@@ -67,12 +67,12 @@ module VCAP::Services::ServiceBrokers::V2
         }
         its('errors.full_messages.second') {
           should eq 'Must conform to JSON Schema Draft 04: ' \
-                          'The property \'#/type\' of type string did not match one or more of the required schemas in schema http://json-schema.org/draft-04/schema#'
+                          'The property \'#/anyOf\' of type boolean did not match the following type: array in schema http://json-schema.org/draft-04/schema#'
         }
       end
 
       context 'when the schema has an external schema' do
-        let(:raw_schema) { { '$schema': 'http://example.com/schema' } }
+        let(:raw_schema) { { 'type' => 'object', '$schema': 'http://example.com/schema' } }
 
         its(:validate) { should be false }
         its('errors.full_messages') { should have(1).items }
@@ -82,7 +82,7 @@ module VCAP::Services::ServiceBrokers::V2
       end
 
       context 'when the schema has an external uri reference' do
-        let(:raw_schema) { { '$ref': 'http://example.com/ref' } }
+        let(:raw_schema) { { 'type' => 'object', '$ref': 'http://example.com/ref' } }
 
         its(:validate) { should be false }
         its('errors.full_messages') { should have(1).items }
@@ -92,7 +92,7 @@ module VCAP::Services::ServiceBrokers::V2
       end
 
       context 'when the schema has an external file reference' do
-        let(:raw_schema) { { '$ref': 'path/to/schema.json' } }
+        let(:raw_schema) { { 'type' => 'object', '$ref': 'path/to/schema.json' } }
 
         its(:validate) { should be false }
         its('errors.full_messages') { should have(1).items }
@@ -190,6 +190,66 @@ module VCAP::Services::ServiceBrokers::V2
         its('errors.full_messages.first') { should eq 'some unknown error' }
       end
 
+      describe 'validation ordering' do
+        context 'when an invalid schema fails multiple validations' do
+          context 'schema size and schema type' do
+            let(:raw_schema) do
+              schema = create_schema_of_size(64 * 1024)
+              schema['type'] = 'notobject'
+              schema
+            end
+
+            its(:validate) { should be false }
+            its('errors.full_messages') { should have(1).items }
+            its('errors.full_messages.first') { should match 'Must not be larger than 64KB' }
+          end
+
+          context 'schema size and external reference' do
+            let(:raw_schema) do
+              schema = create_schema_of_size(64 * 1024)
+              schema['$ref'] = 'http://example.com/ref'
+              schema
+            end
+
+            its(:validate) { should be false }
+            its('errors.full_messages') { should have(1).items }
+            its('errors.full_messages.first') { should match 'Must not be larger than 64KB' }
+          end
+
+          context 'schema size and does not conform to Json Schema Draft 4' do
+            let(:raw_schema) do
+              schema = create_schema_of_size(64 * 1024)
+              schema['properties'] = true
+              schema
+            end
+
+            its(:validate) { should be false }
+            its('errors.full_messages') { should have(1).items }
+            its('errors.full_messages.first') { should match 'Must not be larger than 64KB' }
+          end
+
+          context 'schema type and does not conform to JSON Schema Draft 4' do
+            let(:raw_schema) { { 'type' => 'notobject', 'properties' => true } }
+
+            its(:validate) { should be false }
+            its('errors.full_messages') { should have(1).items }
+            its('errors.full_messages.first') { should match 'must have field "type", with value "object"' }
+          end
+
+          context 'does not conform to JSON Schema Draft 4 and external references' do
+            let(:raw_schema) { { 'type' => 'object', 'properties' => true, '$ref' => 'http://example.com/ref' } }
+
+            its(:validate) { should be false }
+            its('errors.full_messages') { should have(1).items }
+            its('errors.full_messages.first') {
+              should match 'Must conform to JSON Schema Draft 04: ' \
+                  'The property \'#/properties\' of type boolean did not match the following type: ' \
+                  'object in schema http://json-schema.org/draft-04/schema#'
+            }
+          end
+        end
+      end
+
       def create_schema_of_size(bytes)
         surrounding_bytes = 26
         {

From 54b0e10b094865fed729b16a753ebe5a20de0bd9 Mon Sep 17 00:00:00 2001
From: Luis Urraca <lurraca@pivotal.io>
Date: Tue, 1 Aug 2017 16:58:40 +0100
Subject: [PATCH 15/15] Fix indentation and naming issues for broker schema
 [#149550919]

Signed-off-by: Sam Gunaratne <sgunaratne@pivotal.io>
---
 app/presenters/v2/service_plan_presenter.rb   |  1 -
 .../service_brokers/v2/catalog_schemas.rb     |  9 ++-
 lib/services/service_brokers/v2/schema.rb     |  1 -
 .../broker_api_v2.13_spec.rb                  | 56 ++++++++++---------
 .../v2/catalog_schemas_spec.rb                | 46 +++++++--------
 .../service_brokers/v2/schema_spec.rb         |  1 +
 6 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/app/presenters/v2/service_plan_presenter.rb b/app/presenters/v2/service_plan_presenter.rb
index 6d4afdf47ce..cefe543f00a 100644
--- a/app/presenters/v2/service_plan_presenter.rb
+++ b/app/presenters/v2/service_plan_presenter.rb
@@ -30,7 +30,6 @@ def present_schemas(plan)
                 },
                 'update' => {
                     'parameters' => update_instance_schema
-
                 }
               }
             }
diff --git a/lib/services/service_brokers/v2/catalog_schemas.rb b/lib/services/service_brokers/v2/catalog_schemas.rb
index c311badf8f9..6f0ddbb5edd 100644
--- a/lib/services/service_brokers/v2/catalog_schemas.rb
+++ b/lib/services/service_brokers/v2/catalog_schemas.rb
@@ -4,17 +4,16 @@ class CatalogSchemas
 
     def initialize(schemas)
       @errors = VCAP::Services::ValidationErrors.new
-      @schemas = schemas
       return unless schemas
 
-      return unless validate_structure(@schemas, [])
+      return unless validate_structure(schemas, [])
       service_instance_path = ['service_instance']
-      return unless validate_structure(@schemas, service_instance_path)
+      return unless validate_structure(schemas, service_instance_path)
 
-      create_schema = get_method('create', @schemas)
+      create_schema = get_method('create', schemas)
       @create_instance = Schema.new(create_schema) if create_schema
 
-      update_schema = get_method('update', @schemas)
+      update_schema = get_method('update', schemas)
       @update_instance = Schema.new(update_schema) if update_schema
     end
 
diff --git a/lib/services/service_brokers/v2/schema.rb b/lib/services/service_brokers/v2/schema.rb
index 96c2e692902..8b878e97bb4 100644
--- a/lib/services/service_brokers/v2/schema.rb
+++ b/lib/services/service_brokers/v2/schema.rb
@@ -3,7 +3,6 @@
 module VCAP::Services::ServiceBrokers::V2
   class Schema
     include ActiveModel::Validations
-    attr_reader :schema
 
     MAX_SCHEMA_SIZE = 65_536
 
diff --git a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
index ac46a1525fb..aea784d29e4 100644
--- a/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
+++ b/spec/acceptance/broker_api_compatibility/broker_api_v2.13_spec.rb
@@ -8,14 +8,14 @@
     let(:update_instance_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
     let(:schemas) {
       {
-          'service_instance' => {
-              'create' => {
-                  'parameters' => create_instance_schema
-              },
-              'update' => {
-                  'parameters' => update_instance_schema
-              }
+        'service_instance' => {
+          'create' => {
+            'parameters' => create_instance_schema
+          },
+          'update' => {
+            'parameters' => update_instance_schema
           }
+        }
       }
     }
 
@@ -31,8 +31,8 @@
       context 'with a create plan schema' do
         let(:create_instance_schema) {
           {
-              '$schema' => 'http://json-schema.org/draft-04/schema#',
-              'type' => 'object'
+            '$schema' => 'http://json-schema.org/draft-04/schema#',
+            'type' => 'object'
           }
         }
 
@@ -43,22 +43,23 @@
 
           parsed_body = MultiJson.load(last_response.body)
           create_schema = parsed_body['entity']['schemas']['service_instance']['create']
-          expect(create_schema).to eq({
-                                          'parameters' =>
-                                              {
-                                                  '$schema' => 'http://json-schema.org/draft-04/schema#',
-                                                  'type' => 'object'
-                                              }
-                                      }
-                                   )
+          expect(create_schema).to eq(
+            {
+              'parameters' =>
+              {
+                '$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object'
+              }
+            }
+          )
         end
       end
 
       context 'with an update plan schema' do
         let(:update_instance_schema) {
           {
-              '$schema' => 'http://json-schema.org/draft-04/schema#',
-              'type' => 'object'
+            '$schema' => 'http://json-schema.org/draft-04/schema#',
+            'type' => 'object'
           }
         }
 
@@ -69,14 +70,15 @@
 
           parsed_body = MultiJson.load(last_response.body)
           update_schema = parsed_body['entity']['schemas']['service_instance']['update']
-          expect(update_schema).to eq({
-                                          'parameters' =>
-                                              {
-                                                  '$schema' => 'http://json-schema.org/draft-04/schema#',
-                                                  'type' => 'object'
-                                              }
-                                      }
-                                   )
+          expect(update_schema).to eq(
+            {
+              'parameters' =>
+              {
+                '$schema' => 'http://json-schema.org/draft-04/schema#',
+                'type' => 'object'
+              }
+            }
+          )
         end
       end
 
diff --git a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
index e9efac20be9..07b4f03d607 100644
--- a/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/catalog_schemas_spec.rb
@@ -4,14 +4,14 @@ module VCAP::Services::ServiceBrokers::V2
   RSpec.describe CatalogSchemas do
     describe 'validating catalog schemas' do
       subject do
-        catalog_schema = CatalogSchemas.new(attrs)
+        catalog_schema = CatalogSchemas.new(schemas)
         catalog_schema.valid?
         catalog_schema
       end
 
       context 'service instance' do
         context 'when catalog has schemas' do
-          let(:attrs) { { 'service_instance' => {} } }
+          let(:schemas) { { 'service_instance' => {} } }
 
           context 'when schemas is not set' do
             {
@@ -21,7 +21,7 @@ module VCAP::Services::ServiceBrokers::V2
                 'Schemas service_instance is empty': { 'service_instance' => {} },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:update_instance) { should be_nil }
                 its(:errors) { should be_empty }
@@ -36,7 +36,7 @@ module VCAP::Services::ServiceBrokers::V2
                 'Schemas service_instance': { 'service_instance' => true }
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its('errors.messages') { should have(1).items }
                 its('errors.messages.first') { should eq "#{name} must be a hash, but has value true" }
@@ -48,7 +48,7 @@ module VCAP::Services::ServiceBrokers::V2
 
         context 'when catalog has a create schema' do
           context 'and the schema structure is valid' do
-            let(:attrs) { { 'service_instance' => { 'create' => { 'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } } } } }
+            let(:schemas) { { 'service_instance' => { 'create' => { 'parameters' => { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } } } } }
 
             its(:create_instance) { should be_a(Schema) }
           end
@@ -59,7 +59,7 @@ module VCAP::Services::ServiceBrokers::V2
                 'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => nil } } },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:create_instance) { should be_nil }
                 its(:errors) { should be_empty }
@@ -69,7 +69,7 @@ module VCAP::Services::ServiceBrokers::V2
           end
 
           context 'when the create instance schema is not valid' do
-            let(:attrs) {
+            let(:schemas) {
               {
                   'service_instance' => {
                       'create' => {
@@ -85,12 +85,12 @@ module VCAP::Services::ServiceBrokers::V2
             its(:valid?) { should be false }
           end
 
-          context 'when attrs have a missing key' do
+          context 'when schemas have a missing key' do
             {
                 'Schemas service_instance.create': { 'service_instance' => { 'create' => {} } },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:create_instance) { should be_nil }
                 its(:errors) { should be_empty }
@@ -99,13 +99,13 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
-          context 'when attrs have an invalid type' do
+          context 'when schemas have an invalid type' do
             {
                 'Schemas service_instance.create': { 'service_instance' => { 'create' => true } },
                 'Schemas service_instance.create.parameters': { 'service_instance' => { 'create' => { 'parameters' => true } } },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:create_instance) { should be_nil }
                 its('errors.messages') { should have(1).items }
@@ -115,9 +115,9 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
-          context 'when attrs has a potentially dangerous uri' do
+          context 'when schemas has a potentially dangerous uri' do
             let(:path) { 'service_instance.create.parameters' }
-            let(:attrs) {
+            let(:schemas) {
               {
                   'service_instance' => {
                       'create' => {
@@ -136,7 +136,7 @@ module VCAP::Services::ServiceBrokers::V2
 
         context 'when catalog has an update schema' do
           context 'and the schema structure is valid' do
-            let(:attrs) {
+            let(:schemas) {
               {
                   'service_instance' => {
                       'update' => {
@@ -149,13 +149,13 @@ module VCAP::Services::ServiceBrokers::V2
             its(:update_instance) { should be_a(Schema) }
           end
 
-          context 'when attrs have nil value' do
+          context 'when schemas have nil value' do
             {
                 'Schemas service_instance.update': { 'service_instance' => { 'update' => nil } },
                 'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => nil } } },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:update_instance) { should be_nil }
                 its(:errors) { should be_empty }
@@ -165,7 +165,7 @@ module VCAP::Services::ServiceBrokers::V2
           end
 
           context 'when the update instance schema is not valid' do
-            let(:attrs) {
+            let(:schemas) {
               {
                   'service_instance' => {
                       'update' => {
@@ -181,12 +181,12 @@ module VCAP::Services::ServiceBrokers::V2
             its(:valid?) { should be false }
           end
 
-          context 'when attrs have a missing key' do
+          context 'when schemas have a missing key' do
             {
                 'Schemas service_instance.update': { 'service_instance' => { 'update' => {} } },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:update_instance) { should be_nil }
                 its(:errors) { should be_empty }
@@ -195,13 +195,13 @@ module VCAP::Services::ServiceBrokers::V2
             end
           end
 
-          context 'when attrs have an invalid type' do
+          context 'when schemas have an invalid type' do
             {
                 'Schemas service_instance.update': { 'service_instance' => { 'update' => true } },
                 'Schemas service_instance.update.parameters': { 'service_instance' => { 'update' => { 'parameters' => true } } },
             }.each do |name, test|
               context "for property #{name}" do
-                let(:attrs) { test }
+                let(:schemas) { test }
 
                 its(:update_instance) { should be_nil }
                 its('errors.messages') { should have(1).items }
@@ -213,9 +213,9 @@ module VCAP::Services::ServiceBrokers::V2
 
           # TODO: Look into schema path is not valid error tests
 
-          context 'when attrs has a potentially dangerous uri' do
+          context 'when schemas has a potentially dangerous uri' do
             let(:path) { 'service_instance.update.parameters' }
-            let(:attrs) {
+            let(:schemas) {
               {
                   'service_instance' => {
                       'update' => {
diff --git a/spec/unit/lib/services/service_brokers/v2/schema_spec.rb b/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
index 3ff746f20ef..bf0c522fb86 100644
--- a/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
+++ b/spec/unit/lib/services/service_brokers/v2/schema_spec.rb
@@ -8,6 +8,7 @@ module VCAP::Services::ServiceBrokers::V2
         schema.validate
         schema
       end
+
       let(:raw_schema) { { '$schema' => 'http://json-schema.org/draft-04/schema#', 'type' => 'object' } }
 
       context 'when attrs have an invalid value as type' do