Skip to content

Commit

Permalink
Use DISTINCT only when required
Browse files Browse the repository at this point in the history 
As part of PR cloudfoundry#3620 [1] it has been validated that there is no overlap
between the different sources (UNIONed datasets). But as these single
datasets are created by JOINing several tables, there might be
duplicates depending on the cardinality of those relations. Thus the
overall DISTINCT is replaced by DISTINCTs added when JOINing a table
with a one-to-many relation. Calling '.distinct' multiple times does not
have any negative effect as this simply sets a flag on the Sequel
dataset.

This change should improve performance of requests to the following
endpoints whenever those JOINs are not needed (e.g. depending on request
parameters):
- /v3/service_offerings
- /v3/service_plans

Tests now ensure that there are:
- multiple plans per offering
- multiple service instances per plan
- multiple visibilities per plan
- multiple spaces per organization

[1] cloudfoundry#3620
  • Loading branch information
@philippthun
philippthun committed Feb 23, 2024
1 parent b9f2933 commit 066d3ff
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 64 deletions.
11 changes: 7 additions & 4 deletions app/fetchers/base_service_list_fetcher.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_
end.from_self(alias: klass.table_name)
end

dataset.distinct.eager(eager_loaded_associations)
dataset.eager(eager_loaded_associations)
end

def readable_by_plan_org(dataset, readable_orgs_query)
Expand Down Expand Up @@ -156,7 +156,8 @@ def join_services(dataset)

def join_plan_org_visibilities(dataset)
dataset = join_service_plans(dataset)
join(dataset, :inner, :service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id])
dataset = join(dataset, :inner, :service_plan_visibilities, service_plan_id: Sequel[:service_plans][:id])
dataset.distinct # service plans can have multiple visibilities
end

def join_service_brokers(dataset)
Expand All @@ -181,12 +182,14 @@ def join_broker_orgs(dataset)

def join_plan_spaces(dataset)
dataset = join_plan_orgs(dataset)
join(dataset, :inner, Sequel[:spaces].as(:plan_spaces), organization_id: Sequel[:plan_orgs][:id])
dataset = join(dataset, :inner, Sequel[:spaces].as(:plan_spaces), organization_id: Sequel[:plan_orgs][:id])
dataset.distinct # organizations can have multiple spaces
end

def join_service_instances(dataset)
dataset = join_service_plans(dataset)
join(dataset, :inner, :service_instances, service_plan_id: Sequel[:service_plans][:id])
dataset = join(dataset, :inner, :service_instances, service_plan_id: Sequel[:service_plans][:id])
dataset.distinct # service plans can have multiple instances
end

def join(dataset, type, table, on)
Expand Down
3 changes: 2 additions & 1 deletion app/fetchers/service_offering_list_fetcher.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ def filter(message, dataset, klass)
end

def join_service_plans(dataset)
join(dataset, :inner, :service_plans, service_id: Sequel[:services][:id])
dataset = join(dataset, :inner, :service_plans, service_id: Sequel[:services][:id])
dataset.distinct # services can have multiple plans
end
end
end
Expand Down
90 changes: 59 additions & 31 deletions spec/unit/fetchers/service_offering_list_fetcher_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module VCAP::CloudController

describe 'visibility of offerings' do
let!(:public_offering_1) { make_public_offering }
let!(:public_offering_2) { make_public_offering }
let!(:public_offering_2) { make_public_offering(number_of_plans: 2) }

let!(:private_offering_1) { make_private_offering }
let!(:private_offering_2) { make_private_offering }
Expand All @@ -32,6 +32,7 @@ module VCAP::CloudController
let!(:org_restricted_offering_2) { make_org_restricted_offering(org_2) }
let!(:org_restricted_offering_3) { make_org_restricted_offering(org_3) }
let!(:org_restricted_offering_4) { make_org_restricted_offering(org_3) }
let!(:org_restricted_offering_5) { make_org_restricted_offering(org_1, org_3) }

let(:space_1) { Space.make(organization: org_1) }
let(:space_2) { Space.make(organization: org_2) }
Expand All @@ -41,6 +42,14 @@ module VCAP::CloudController
let!(:space_scoped_offering_3) { make_space_scoped_offering(space_3) }
let!(:space_scoped_offering_4) { make_space_scoped_offering(space_3) }

it 'is tested with multiple plans per offering' do
expect(public_offering_2.service_plans.count).to be > 1
end

it 'is tested with multiple visibilities per plan' do
expect(org_restricted_offering_5.service_plans.first.service_plan_visibilities.count).to be > 1
end

context 'when no authorization is specified' do
it 'only fetches public plans' do
service_offerings = fetcher.fetch(message).all
Expand All @@ -63,7 +72,8 @@ module VCAP::CloudController
org_restricted_offering_1,
org_restricted_offering_2,
org_restricted_offering_3,
org_restricted_offering_4
org_restricted_offering_4,
org_restricted_offering_5
)
end
end
Expand All @@ -84,7 +94,8 @@ module VCAP::CloudController
public_offering_2,
org_restricted_offering_1,
org_restricted_offering_3,
org_restricted_offering_4
org_restricted_offering_4,
org_restricted_offering_5
)
end
end
Expand All @@ -106,7 +117,8 @@ module VCAP::CloudController
space_scoped_offering_3,
space_scoped_offering_4,
org_restricted_offering_3,
org_restricted_offering_4
org_restricted_offering_4,
org_restricted_offering_5
)
end
end
Expand All @@ -116,20 +128,34 @@ module VCAP::CloudController
let(:org_1) { Organization.make }
let(:org_2) { Organization.make }
let(:org_3) { Organization.make }
let(:space_1) { Space.make(organization: org_1) }
let(:space_1_1) { Space.make(organization: org_1) }
let!(:space_1_2) { Space.make(organization: org_1) }
let(:space_2) { Space.make(organization: org_2) }
let(:space_3) { Space.make(organization: org_3) }

let!(:public_offering) { make_public_offering }
let!(:public_offering) { make_public_offering(number_of_plans: 2) }

let!(:org_restricted_offering_1) { make_org_restricted_offering(org_1) }
let!(:org_restricted_offering_2) { make_org_restricted_offering(org_2) }
let!(:org_restricted_offering_3) { make_org_restricted_offering(org_3) }
let!(:org_restricted_offering_4) { make_org_restricted_offering(org_1, org_2) }

let!(:space_scoped_offering_1) { make_space_scoped_offering(space_1) }
let!(:space_scoped_offering_1_1) { make_space_scoped_offering(space_1_1) }
let!(:space_scoped_offering_2) { make_space_scoped_offering(space_2) }
let!(:space_scoped_offering_3) { make_space_scoped_offering(space_3) }

it 'is tested with multiple spaces per organization' do
expect(org_1.spaces.count).to be > 1
end

it 'is tested with multiple plans per offering' do
expect(public_offering.service_plans.count).to be > 1
end

it 'is tested with multiple visibilities per plan' do
expect(org_restricted_offering_4.service_plans.first.service_plan_visibilities.count).to be > 1
end

describe 'organization_guids' do
context 'omniscient' do
it 'can filter plans by organization guids' do
Expand All @@ -139,7 +165,8 @@ module VCAP::CloudController

service_offerings = ServiceOfferingListFetcher.fetch(message, omniscient: true).all

expect(service_offerings).to contain_exactly(org_restricted_offering_1, org_restricted_offering_2, public_offering, space_scoped_offering_1, space_scoped_offering_2)
expect(service_offerings).to contain_exactly(org_restricted_offering_1, org_restricted_offering_2, org_restricted_offering_4,
public_offering, space_scoped_offering_1_1, space_scoped_offering_2)
end

it 'only shows public plans when there are no matches' do
Expand Down Expand Up @@ -168,7 +195,7 @@ module VCAP::CloudController
readable_spaces_query:
).all

expect(service_offerings).to contain_exactly(org_restricted_offering_1, public_offering)
expect(service_offerings).to contain_exactly(org_restricted_offering_1, org_restricted_offering_4, public_offering)
end
end

Expand All @@ -192,10 +219,9 @@ module VCAP::CloudController
end

context 'when the user only has access to some spaces in an org' do
let(:space_1a) { Space.make(organization: org_1) }
let!(:space_scoped_offering_1a) { make_space_scoped_offering(space_1a) }
let!(:space_scoped_offering_1_2) { make_space_scoped_offering(space_1_2) }
let(:readable_orgs) { [org_1] }
let(:readable_spaces) { [space_1] }
let(:readable_spaces) { [space_1_1] }

it 'only shows space-scoped plans for the readable spaces' do
message = ServiceOfferingsListMessage.from_params({
Expand All @@ -208,7 +234,7 @@ module VCAP::CloudController
readable_spaces_query:
).all

expect(service_offerings).to contain_exactly(public_offering, org_restricted_offering_1, space_scoped_offering_1)
expect(service_offerings).to contain_exactly(public_offering, org_restricted_offering_1, org_restricted_offering_4, space_scoped_offering_1_1)
end
end
end
Expand All @@ -217,10 +243,11 @@ module VCAP::CloudController
context 'omniscient' do
it 'can filter by space guids' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid, space_2.guid].join(',')
space_guids: [space_1_1.guid, space_2.guid].join(',')
}.with_indifferent_access)
service_offerings = ServiceOfferingListFetcher.fetch(message, omniscient: true).all
expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, space_scoped_offering_2, org_restricted_offering_2, public_offering)
expect(service_offerings).to contain_exactly(space_scoped_offering_1_1, org_restricted_offering_1, org_restricted_offering_4,
space_scoped_offering_2, org_restricted_offering_2, public_offering)
end

it 'only shows public plans when there are no matches' do
Expand All @@ -234,35 +261,35 @@ module VCAP::CloudController

context 'only some spaces are readable (SPACE_DEVELOPER, SPACE_AUDITOR etc)' do
let(:readable_orgs) { [org_1] }
let(:readable_spaces) { [space_1] }
let(:readable_spaces) { [space_1_1, space_1_2] }

it 'shows only plans for readable spaces and orgs' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid, space_2.guid].join(',')
space_guids: [space_1_1.guid, space_1_2.guid, space_2.guid].join(',')
}.with_indifferent_access)
service_offerings = ServiceOfferingListFetcher.fetch(
message,
readable_orgs_query:,
readable_spaces_query:
).all
expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, public_offering)
expect(service_offerings).to contain_exactly(space_scoped_offering_1_1, org_restricted_offering_1, org_restricted_offering_4, public_offering)
end
end

context 'when a filter contains a space guid that the user cannot access' do
let(:readable_orgs) { [org_1, org_2] }
let(:readable_spaces) { [space_1] }
let(:readable_spaces) { [space_1_1] }

it 'ignores the unauthorized space guid' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid, space_2.guid].join(',')
space_guids: [space_1_1.guid, space_2.guid].join(',')
}.with_indifferent_access)
service_offerings = ServiceOfferingListFetcher.fetch(
message,
readable_orgs_query:,
readable_spaces_query:
).all
expect(service_offerings).to contain_exactly(space_scoped_offering_1, org_restricted_offering_1, public_offering)
expect(service_offerings).to contain_exactly(space_scoped_offering_1_1, org_restricted_offering_1, org_restricted_offering_4, public_offering)
end
end

Expand All @@ -272,7 +299,7 @@ module VCAP::CloudController

it 'shows only public plans' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid, space_2.guid].join(',')
space_guids: [space_1_1.guid, space_2.guid].join(',')
}.with_indifferent_access)

service_offerings = ServiceOfferingListFetcher.fetch(
Expand All @@ -290,16 +317,16 @@ module VCAP::CloudController
context 'omniscient' do
it 'results in the overlap' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid, space_2.guid].join(','),
space_guids: [space_1_1.guid, space_2.guid].join(','),
organization_guids: [org_2.guid, org_3.guid].join(',')
}.with_indifferent_access)
service_offerings = ServiceOfferingListFetcher.fetch(message, omniscient: true).all
expect(service_offerings).to contain_exactly(space_scoped_offering_2, org_restricted_offering_2, public_offering)
expect(service_offerings).to contain_exactly(space_scoped_offering_2, org_restricted_offering_2, org_restricted_offering_4, public_offering)
end

it 'only shows public plans when there are no matches' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid].join(','),
space_guids: [space_1_1.guid].join(','),
organization_guids: [org_3.guid].join(',')
}.with_indifferent_access)
service_offerings = ServiceOfferingListFetcher.fetch(message, omniscient: true).all
Expand All @@ -309,11 +336,11 @@ module VCAP::CloudController

context 'when only some spaces and orgs are visible' do
let(:readable_orgs) { [org_1, org_2] }
let(:readable_spaces) { [space_1] }
let(:readable_spaces) { [space_1_1] }

it 'excludes plans that do not meet all the filter conditions' do
message = ServiceOfferingsListMessage.from_params({
space_guids: [space_1.guid, space_2.guid].join(','),
space_guids: [space_1_1.guid, space_2.guid].join(','),
organization_guids: [org_2.guid, org_3.guid].join(',')
}.with_indifferent_access)
service_offerings = ServiceOfferingListFetcher.fetch(
Expand Down Expand Up @@ -456,9 +483,9 @@ module VCAP::CloudController
end
end

def make_public_offering
def make_public_offering(number_of_plans: 1)
service_offering = Service.make(label: "public-#{Sham.name}")
ServicePlan.make(public: true, active: true, service: service_offering)
number_of_plans.times { ServicePlan.make(public: true, active: true, service: service_offering) }
service_offering
end

Expand All @@ -473,10 +500,11 @@ def make_space_scoped_offering(space)
Service.make(service_broker: service_broker, label: "space-scoped-#{Sham.name}")
end

def make_org_restricted_offering(org)
def make_org_restricted_offering(org1, org2=nil)
service_offering = Service.make(label: "org-restricted-#{Sham.name}")
service_plan = ServicePlan.make(public: false, service: service_offering)
ServicePlanVisibility.make(organization: org, service_plan: service_plan)
ServicePlanVisibility.make(organization: org1, service_plan: service_plan)
ServicePlanVisibility.make(organization: org2, service_plan: service_plan) unless org2.nil?
service_offering
end
end
Expand Down
Loading

0 comments on commit 066d3ff

Please sign in to comment.