Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THREESCALE-11139 simplify application queries #3845

Merged
merged 14 commits into from
Oct 11, 2024

Conversation

akostadinov
Copy link
Contributor

@akostadinov akostadinov commented Jul 10, 2024

Check JIRA for issue description https://issues.redhat.com/browse/THREESCALE-11139

Note to reviewer. As an additional optimizations, I also fixed N+1 queries in this call. I removed the bullet ignores to see which one were triggered by the tests and removed 4 of the ignores. This had the side effect of also causing unrelated tests to fail, so I fixed some other N+1 queries as well.

So wherever you see includes changes, you can largely ignore them because these were changed to make tests pass and there should be no functional change there.

@akostadinov akostadinov self-assigned this Jul 10, 2024
@applications ||= begin
cinstances = current_account.provided_cinstances.where(service: accessible_services)
if (service_id = params[:service_id])
cinstances = cinstances.where(service_id: service_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already being done by scope_search in #index. I was thinking whether this scope_serach call should be moved here inside #applications because #application also calls #applications.

But I think it doesn't make sense for #application because there a single app is selected by other parameters.

Copy link
Contributor

@mayorova mayorova Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix for the query in JIRA, right? The rest are just N+1 improvements?
UPDATE: together with the provided_by refactoring...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provided_by is the main thing. This is just further optimization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think it doesn't make sense for #application because there a single app is selected by other parameters.

I actually don't agree with this.

We do have service_id as a filtering parameter in the API docs. And we actually need to conserve it, because we allow applications with the same app ID under different services. So, we should be able to find the application that belongs to the specified service.

application-find

I tried these three tests (in test/integration/user-management-api/applications_test.rb). They all pass in master but the 2nd and the 3rd fail in the branch.

  test 'find by user_key with correct service_id' do
    @service.backend_version = '1'
    @service.save!

    get find_admin_api_applications_path(format: :xml), params: { user_key: @application.user_key,
                                                                   provider_key: @provider.api_key,
                                                                   service_id: @service.id }

    assert_response :success
    assert_application(@response.body,
                       { id: @application.id,
                         user_key: @application.user_key })
  end

  test 'find by user_key with incorrect service_id' do
    @service.backend_version = '1'
    @service.save!

    another_service = FactoryBot.create(:service, account: @provider)

    get find_admin_api_applications_path(format: :xml), params: { user_key: @application.user_key,
                                                                   provider_key: @provider.api_key,
                                                                   service_id: another_service.id }

    assert_response :not_found
  end

  test 'find by user_key when there are two apps with the same user key' do
    @service.backend_version = '1'
    @service.save!

    another_service = FactoryBot.create(:service, account: @provider, backend_version: '1')
    another_app = FactoryBot.create(:cinstance,
                                    service: another_service,
                                    plan: FactoryBot.create(:application_plan, issuer: another_service),
                                    user_key: @application.user_key)

    get find_admin_api_applications_path(format: :xml), params: { user_key: @application.user_key,
                                                                  provider_key: @provider.api_key,
                                                                  service_id: another_service.id }

    assert_application(@response.body,
                       { id: another_app.id,
                         service_id: another_service.id,
                         user_key: another_app.user_key })
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for catching this!

I added your tests and updated controller to add the service condition when searching by user_key. See fc5f1e7 . Is this enough? app_id should always be unique as far as I understand so it doesn't need such restriction. Correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's actually the same for app_id :) we allow having the same ID (and the app key too) as long as the apps are under different services.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😵‍💫 ok

      scope.where.has { (service.backend_version == '1') & (user_key == param_key) }.first!

                   when app_id = params[:app_id]
      scope.where.has { (service.backend_version != '1') & (application_id == app_id) }.first!

                     else
      scope.find(params[:application_id] || params[:id])

So it sounds like application_id and app_id are not the same. How about application_id, do we need it to be scope.find(params[:application_id] || params[:id]) or applications.find(params[:application_id] || params[:id]) is sufficient (in case this finally is the primary key and can't be anything else)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, right, application_id seems to the be id of the Cinstance (aka primary key). It seems a bit weird using it as a search query parameter... But well. To answer your question I'd say we need to check the current behavior. To me it seems logical to apply find to the already filtered scope. So, if you have the following:

service: 1
  application: 
    id: 11
    app_id: a1
    
service: 2
  application: 
    id: 22
    app_id: a2

searching by application_id=11&app_id=a1&service_id=1 should return the first application, while any other combination should return a 404, e.g.:
application_id=22&app_id=a2&service_id=1
application_id=22&app_id=a1&service_id=2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have app_id then application_id appears to be ignored. Historically using the wrong service_id would still return nothing though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL

app/models/cinstance.rb Outdated Show resolved Hide resolved
@@ -200,7 +201,7 @@ def self.by_service(service)
# maybe move both limit methods to their models?

def self.serialization_preloading
includes(:application_keys, :plan, :user_account,
includes(:plan, :user_account,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a Bullet error without this.

Bullet::Notification::UnoptimizedQueryError: user: avalon
GET /admin/api/applications
AVOID eager loading detected
  Cinstance => [:application_keys]
  Remove from your query: .includes([:application_keys])

FYI this can be worked around by:

Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :application_keys

but this is not the point. More interesting is why it didn't appear previously... maybe because things were obtained by JOIN, not by separate queries.

Either way, seems unused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many tests fail because of this error, I assume it's already fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more, not only this. But this was hiding the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All related should be fixed now. I've spent a lot of time to make sure this calls preloads everything. While I've just fixed whatever failed for the rest.

@akostadinov akostadinov requested a review from a team July 11, 2024 12:29
@jlledom
Copy link
Contributor

jlledom commented Jul 16, 2024

Could you please add a brief PR description to explain which was the problem and how you've solved it? it would help the review process and make the reviewer save a lot of investigation time.

app/models/cinstance.rb Outdated Show resolved Hide resolved
Comment on lines 150 to 151
def self.provided_by(account, services_association: Service.unscoped)
where(plan_id: ApplicationPlan.unscoped.where(issuer_type: "Service", issuer_id: Service.unscoped.select(:id).of_account(account).merge(services_association)))
Copy link
Contributor

@jlledom jlledom Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my tests, it seems to me the more efficient query is what @mayorova suggested in the issue comment

SELECT
  COUNT(*)
FROM
  (
  SELECT
      DISTINCT `cinstances`.`id`
  FROM    `cinstances`
  INNER JOIN `services` ON
      `services`.`id` = `cinstances`.`service_id`
  WHERE
      `cinstances`.`type` = 'Cinstance'
      AND `services`.`account_id` = 2445579871111
      AND `services`.`state` != 'deleted') AS whatever;

And I can get a pretty similar query by using this code:

Suggested change
def self.provided_by(account, services_association: Service.unscoped)
where(plan_id: ApplicationPlan.unscoped.where(issuer_type: "Service", issuer_id: Service.unscoped.select(:id).of_account(account).merge(services_association)))
def self.provided_by(account, services_association: Service.unscoped)
joins(:service).where(services: { account_id: account.id }).merge(services_association)

Which results in:

SELECT
	COUNT(*)
FROM
	`cinstances`
INNER JOIN `services` ON
	`services`.`id` = `cinstances`.`service_id`
WHERE
	`cinstances`.`type` = 'Cinstance'
	AND `services`.`state` != 'deleted'
	AND `services`.`account_id` = 2445579871111

Why make it more complicated adding a join to plans?

You mentioned here that is better to get the services from the plans, but why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but we spoke that getting through Plans is more reliable. I'm not sure why service_id exists in the model, maybe for service contracts. The original intention seems to be obtaining service through the plans.

This replicates that intent.

I'm fine if your investigation shows this would not be an issue though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, your changes generate a query which has an execution cost of 10.66, while the suggested changes generate a query costing 3,23.

However, I'm not sure how relevant is the execution cost for this case, since the original query had a cost of 2,72... and took more than one second to execute. Do you know the cause of this contradiction between execution time and cost? It should be the less cost the faster...

CC @mayorova

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but we spoke that getting through Plans is more reliable. I'm not sure why service_id exists in the model, maybe for service contracts. The original intention seems to be obtaining service through the plans.

Understood, if the query is much faster now and we can be more confident it behaves as before, LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jlledom I have no idea, unfortunately, how the cost is calculated, and how it is related to the time of execution 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akostadinov I pulled the last changes from this branch and now this is the generated query:

SELECT
	COUNT(*)
FROM
	`cinstances`
WHERE
	`cinstances`.`type` = 'Cinstance'
	AND `cinstances`.`plan_id` IN (
	SELECT
		`plans`.`id`
	FROM
		`plans`
	WHERE
		`plans`.`type` = 'ApplicationPlan'
		AND `plans`.`issuer_type` = 'Service'
		AND `plans`.`issuer_id` IN (
		SELECT
			`services`.`id`
		FROM
			`services`
		WHERE
			`services`.`state` != 'deleted'
			AND `services`.`account_id` = 2445579871111))

Which is exactly the same as before the PR, but removing the spurious subquery. In my tests, there's not performance improvement, both old and new queries take the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I changed back to using a join with Services and just add to the services conditions instead of refering to plans and using sub-queries. Submitting to the old decision to keep service and plan->issuer in sync.

@@ -200,7 +201,7 @@ def self.by_service(service)
# maybe move both limit methods to their models?

def self.serialization_preloading
includes(:application_keys, :plan, :user_account,
includes(:plan, :user_account,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many tests fail because of this error, I assume it's already fixed?

lib/tasks/multitenant/tenants.rake Outdated Show resolved Hide resolved
app/models/account/provider_methods.rb Outdated Show resolved Hide resolved
Comment on lines 150 to 151
def self.provided_by(account, services_association: Service.unscoped)
where(plan_id: ApplicationPlan.unscoped.where(issuer_type: "Service", issuer_id: Service.unscoped.select(:id).of_account(account).merge(services_association)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akostadinov I pulled the last changes from this branch and now this is the generated query:

SELECT
	COUNT(*)
FROM
	`cinstances`
WHERE
	`cinstances`.`type` = 'Cinstance'
	AND `cinstances`.`plan_id` IN (
	SELECT
		`plans`.`id`
	FROM
		`plans`
	WHERE
		`plans`.`type` = 'ApplicationPlan'
		AND `plans`.`issuer_type` = 'Service'
		AND `plans`.`issuer_id` IN (
		SELECT
			`services`.`id`
		FROM
			`services`
		WHERE
			`services`.`state` != 'deleted'
			AND `services`.`account_id` = 2445579871111))

Which is exactly the same as before the PR, but removing the spurious subquery. In my tests, there's not performance improvement, both old and new queries take the same time.

app/models/cms/group.rb Outdated Show resolved Hide resolved
@akostadinov akostadinov marked this pull request as draft August 28, 2024 17:36
Comment on lines +34 to +35
can %i[read show edit update], Cinstance, Cinstance.permitted_for(user).where_values_hash
# can %i[read show edit update], Cinstance, user.accessible_cinstances do |cinstance|
# cinstance.plan&.issuer_type == "Service" && cinstance.plan.issuer.account == user.account &&
# (!user.forbidden_some_services? || user.member_permission_service_ids.include?(cinstance.plan.issuer.id))
# end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between the new one, the old one and the commented one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the commented out uses a block which is not recommended. I believe more description is in the commit messages. But for the time being, I'm looking at other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 fix user can access Cinstance

The `#where_values_hash` method does not support joins and sub-queries.

Originally the `account.provided_cinstances` part was ignored because it
was JOINs. With the update to sub-queries, it turned into `plan_id: nil`
which is incorrect and broke the logic.

So now we keep logic as previously by resorting only to the
`Cinstance.permitted_for` part of the query.

This relies on the fact that `Cinstance.plan.issuer` is set as
`Cinstance.service` when that issuer is a service.

Also relies on the fact that `User.member_permission_service_ids` will
not set to ids of services that don't belong to the account.

Which may not be ideal but allows for permission checking without extra
database queries.

@akostadinov akostadinov force-pushed the tests branch 2 times, most recently from 634f3d1 to ef6760b Compare September 11, 2024 21:38
@akostadinov akostadinov marked this pull request as ready for review September 11, 2024 23:12
@akostadinov akostadinov force-pushed the tests branch 3 times, most recently from 4fd70f8 to 53d0222 Compare September 13, 2024 21:49
Comment on lines +151 to +153
# we can access service through plan but also keep service.id in sync with plan.service.id
# this is a simpler way to do the query used historically
joins(:service).where.has { service.sift(:of_account, account) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main fix point (UPDATED). But there are further optimizations in other files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akostadinov Do you have any idea why the ogiginal query had readonly(false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea, probably itdidn't work without it at some point. I think it shouldn't be needed anymore

https://stackoverflow.com/a/33360519/520567

@@ -110,6 +115,12 @@ def buyer_account
@buyer_account ||= buyer_accounts.find(params[:id])
end

def to_present(accounts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name it something like preload_associations to make the purpose more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which associations? I wanted it to be clear that this is making it good for being (re)presented. It is not clear which associations it means. But I'm open for another way of thinking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whatever associations that need to be preloaded 😬

to me to_present is too vague and from the name I don't know what the method does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preload_associations is more vague. How about preload_to_present?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is this preloading needed specifically for presentation? In this case, preload_to_present might be fine. Better than just to_present in any case, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is this preloading needed specifically for presentation?

This was what I was trying to explain :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@@ -110,6 +115,12 @@ def buyer_account
@buyer_account ||= buyer_accounts.find(params[:id])
end

def to_present(accounts)
# ActiveRecord::Associations::Preloader.new(records: Array(accounts), associations: [:annotations, {bought_plans: %i[original]}]).call # Rails 7.x
ActiveRecord::Associations::Preloader.new.preload(Array(accounts), [:annotations, {bought_plans: %i[original]}])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method ever used for arrays? if not, maybe we should rename the argument as account to make it clear it's accepts and returns a single object?

Otherwise (to make it more generic), it can be left as it is, but then I guess it should be Array(accounts).flatten in case accounts is actually an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need flatten.

[1] pry(main)> Array([1,2,3])
=> [1, 2, 3]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh OK, nice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the first time I see Preloader, what's the advantage over .includes? Why is this method supposed to receive "accounts" but instead it's called with buyer_account?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are no advantages over #includes. Issue is that #includes only operates with Associations/queries that are not already performed. If your method receives a bunch of active record objects, then you can't call #includes on them.

end

def application
@application ||= case

when user_key = params[:user_key]
when param_key = params[:user_key]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why rename user_key to param_key ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because underneath there is a baby_squeel expression (user_key == param_key). I thought that even if it worked, it would be highly confusing to read (user_key == user_key)

account_id == account.id
end

scope :of_account, ->(account) { where.has { sift(:of_account, account) } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using sifter any better than a plain scope?...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the sifter in a join query while I don't think you can use a scope like that.

Comment on lines +210 to +213
if format == "xml"
service_includes << :default_application_plan
plan_includes << :original
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only for XML responses?... JSON/UI don't need these fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For JSON it complained that we are eager loading stuff that is not later accessed. UI uses another controller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this too much... but OK, if that makes the queries more efficient - let's keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise these are queried once per entry displayed in the XML. So we should have it. But if you have a better idea how to do, that would be cool.

@@ -125,7 +125,7 @@ def raw_applications
def applications
@applications ||= raw_applications.scope_search(search)
.order_by(*sorting_params)
.preload(:service, user_account: %i[admin_user], plan: %i[pricing_rules])
.includes(plan: %i[pricing_rules])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my laziness - what is the main difference between preload and includes ? 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU #preload simply preloads what was requested with separate queries. While #includes is supposed to be smarter and extract objects from joins as well. Also it accumulates with other includes. There is a caveat that if in the join some associated objects are filtered, they are not seen by #includes.

In this case I don't want to mix them because IIRC there was some includes used in a chain and probably that will play better together with it. I'm not sure whether preload plays together well with includes. But also see no reason to experiment. includes seems to be the primary documented way. Although some people are concerned about possible surprises with it due to filtering of related objects in joins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the explanation!

User.any_instance.expects(:member_permission_service_ids).returns([@service.id]).at_least_once
get admin_api_applications_path, params: { access_token: token.value, service_id: @service.id + 1 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why @service.id + 1 ? 🤔

what is this testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a service ID that doesn't exist or user has no access to. Requesting a single such service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should try with both options? Or this is fine for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this should be service_2.id, I don't see how using a random other ID make sense.

User.any_instance.expects(:member_permission_service_ids).returns([@service.id, service_2.id]).at_least_once
get admin_api_applications_path, params: { access_token: token.value }
assert_response :success
assert_select "applications/application", 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the number of returned applications?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, how many times the selection matched

@josemigallas josemigallas changed the title THREESCALE-11139 simplify applications query THREESCALE-11139 simplify application queries Sep 26, 2024
@@ -544,7 +544,7 @@ def on_trial?
# Grabs the support_email if defined, otherwise falls back to the email of first admin. Dog.
def support_email
se = self[:support_email]
se.presence || admins.first&.email
se.presence || first_admin&.email
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many occurrences of admins.first, should we change all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be evaluated. Maybe but I'm not sure it can be a simple sed command because it involves caching.

I would rather leave this for further PRs. This is big enough already.

```
Bullet::Notification::UnoptimizedQueryError: user: localuser
GET http://admin.foo.3scale.localhost:39073/p/admin/applications/16
AVOID eager loading detected
  ApplicationPlan => [:plan_metrics]
  Remove from your query: .includes([:plan_metrics])
```
Fixes issue with keys.feature:Regenerate provider key

Bullet::Notification::UnoptimizedQueryError: user: avalon
PUT http://master-account.3scale.localhost:42285/p/admin/applications/4/change_user_key
AVOID eager loading detected
  ApplicationPlan => [:original]
  Remove from your query: .includes([:original])
```
Bullet::Notification::UnoptimizedQueryError:
  user: default
  GET /admin/api/applications.json?provider_key=***&inactive_since=2014-05-05
  AVOID eager loading detected
    Service => [:default_application_plan]
    Remove from your query: .includes([:default_application_plan])
```
The `#where_values_hash` method does not support joins and sub-queries.

Originally the `account.provided_cinstances` part was ignored because it
was JOINs. With the update to sub-queries, it turned into `plan_id: nil`
which is incorrect and broke the logic.

So now we keep logic as previously by resorting only to the
`Cinstance.permitted_for` part of the query.

This relies on the fact that `Cinstance.plan.issuer` is set as
`Cinstance.service` when that issuer is a service.

Also relies on the fact that `User.member_permission_service_ids` will
not set to ids of services that don't belong to the account.

Which may not be ideal but allows for permission checking without extra
database queries.
Historically it was a conscious decision to optimize access to cintance
-> service by denormalizing the database. So we can access service
directly and not through the plan.

We also keep these two in sync with model callbacks.

So this commit further simplifies the queries to fully rely on this
fact.
@akostadinov akostadinov merged commit fedeb89 into 3scale:master Oct 11, 2024
17 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants