Add GUID as secondary order parameter when available

This change resolves an issue with inconsistent ordering that CAPI users
were experiencing when resources were created within the same second as
each other.  Resources that have a GUID field in the CAPI DB will now
order by GUID as a secondary order parameter, which should cause the
ordering of resources to be consistent.

#2249

Co-authored-by: Jenna Goldstrich <jgoldstrich@pivotal.io>
Co-authored-by: Sarah Weinstein <sweinstein@pivotal.io>

app/models/runtime/organization_auditor.rb

@@ -6,7 +6,7 @@ class OrganizationAuditor < Sequel::Model(:organizations_auditors)
     def_column_alias :guid, :role_guid
 
     def before_create
-      self.guid = SecureRandom.uuid
+      self.guid ||= SecureRandom.uuid
     end
 
     def validate

app/models/runtime/space_auditor.rb

@@ -6,7 +6,7 @@ class SpaceAuditor < Sequel::Model(:spaces_auditors)
     def_column_alias :guid, :role_guid
 
     def before_create
-      self.guid = SecureRandom.uuid
+      self.guid ||= SecureRandom.uuid
     end
 
     def validate

app/models/runtime/space_manager.rb

@@ -6,7 +6,7 @@ class SpaceManager < Sequel::Model(:spaces_managers)
     def_column_alias :guid, :role_guid
 
     def before_create
-      self.guid = SecureRandom.uuid
+      self.guid ||= SecureRandom.uuid
     end
 
     def validate

lib/cloud_controller/paging/sequel_paginator.rb

@@ -11,7 +11,14 @@ def get_page(sequel_dataset, pagination_options)
       table_name = sequel_dataset.model.table_name
       column_name = "#{table_name}__#{order_by}".to_sym
       sequel_order = order_direction == 'asc' ? Sequel.asc(column_name) : Sequel.desc(column_name)
-      query = sequel_dataset.extension(:pagination).paginate(page, per_page).order(sequel_order)
+
+      if sequel_dataset.model.columns.include?(:guid)
+        guid_column_name = "#{table_name}__guid".to_sym
+        guid_sequel_order = Sequel.asc(guid_column_name)
+        query = sequel_dataset.extension(:pagination).paginate(page, per_page).order(sequel_order, guid_sequel_order)
+      else
+        query = sequel_dataset.extension(:pagination).paginate(page, per_page).order(sequel_order)
+      end
 
       PaginatedResult.new(query.all, query.pagination_record_count, pagination_options)
     end

spec/request/roles_spec.rb

@@ -808,7 +808,6 @@
 
     let!(:space_auditor) do
       VCAP::CloudController::SpaceAuditor.make(
-        guid: 'space-role-guid',
         space: space,
         user: other_user,
         created_at: Time.now - 5.minutes,
@@ -817,7 +816,6 @@
 
     let!(:organization_auditor) do
       VCAP::CloudController::OrganizationAuditor.make(
-        guid: 'organization-role-guid',
         organization: org,
         user: other_user,
         created_at: Time.now,
@@ -1070,6 +1068,21 @@ def make_space_role_for_current_user(type)
 
       it_behaves_like 'permissions for list endpoint', ALL_PERMISSIONS
     end
+    context 'listing roles with overlapping timestamps' do
+      let!(:user_jeff) { VCAP::CloudController::User.make(guid: 'jeff-guid') }
+      let!(:role_one) { VCAP::CloudController::OrganizationAuditor.make(guid: '1', user: user_jeff, organization: org, created_at: '2019-12-25T13:00:00Z') }
+      let!(:role_two) { VCAP::CloudController::SpaceAuditor.make(guid: '2', user: user_jeff, space: space, created_at: '2019-12-25T13:00:00Z') }
+      let!(:role_three) { VCAP::CloudController::SpaceManager.make(guid: '3', user: user_jeff, space: space, created_at: '2019-12-25T13:00:00Z') }
+      it 'sorts the the roles on a secondary key and keeps the same order between calls' do
+        get('/v3/roles', nil, admin_header)
+        expect(last_response).to have_status_code(200)
+
+        parsed_response = MultiJson.load(last_response.body)
+        expect(parsed_response['resources'][0]['guid']).to match('1')
+        expect(parsed_response['resources'][1]['guid']).to match('2')
+        expect(parsed_response['resources'][2]['guid']).to match('3')
+      end
+    end
 
     context 'listing roles with include' do
       let(:other_user_response) do

spec/support/fakes/blueprints.rb

@@ -20,7 +20,7 @@
   uaa_id              { |index| "uaa-id-#{index}" }
   domain              { |index| "domain-#{index}.example.com" }
   host                { |index| "host-#{index}" }
-  guid                { |_| "guid-#{SecureRandom.uuid}" }
+  guid                { |_| SecureRandom.uuid.to_s }
   extra               { |index| "extra-#{index}" }
   instance_index      { |index| index }
   unique_id           { |index| "unique-id-#{index}" }

spec/unit/controllers/runtime/stagings_controller_spec.rb

@@ -487,7 +487,7 @@ def upload_droplet
           expect(last_response.status).to eq(200)
 
           droplet.reload
-          expect(last_response.headers['X-Accel-Redirect']).to eq("/cc-droplets/gu/id/#{droplet.blobstore_key}")
+          expect(last_response.headers['X-Accel-Redirect']).to match("/cc-droplets/.*/#{droplet.blobstore_key}")
         end
       end
 

spec/unit/lib/cloud_controller/paging/sequel_paginator_spec.rb

@@ -60,6 +60,29 @@ module VCAP::CloudController
 
         expect(paginated_result.total).to be > 0
       end
+
+      it 'orders by GUID as a secondary field when available' do
+        options = { page: 1, per_page: 2, order_by: 'created_at', order_direction: 'asc' }
+        app_model1.update(guid: '1', created_at: '2019-12-25T13:00:00Z')
+        app_model2.update(guid: '2', created_at: '2019-12-25T13:00:00Z')
+
+        pagination_options = PaginationOptions.new(options)
+        paginated_result = paginator.get_page(dataset, pagination_options)
+        expect(paginated_result.records.first.guid).to eq(app_model1.guid)
+        expect(paginated_result.records.second.guid).to eq(app_model2.guid)
+      end
+
+      it 'does not order by GUID when the table has no GUID' do
+        options = { page: 1, per_page: per_page }
+        pagination_options = PaginationOptions.new(options)
+        request_count_dataset = RequestCount.dataset
+        RequestCount.make.save
+        paginated_result = nil
+        expect {
+          paginated_result = paginator.get_page(request_count_dataset, pagination_options)
+        }.not_to raise_error
+        expect(paginated_result.total).to be 1
+      end
     end
   end
 end