THREESCALE-9973: Bullet unused eager loading offences

[Madnialihussain]

What this PR does / why we need it:

Fixes unused_eager_loading offences in the Bullet safelist. The safelist is reduced from ~40 unused_eager_loading entries down to 10 intentional ones.

https://issues.redhat.com/browse/THREESCALE-9973

Verification steps:

• Run the test suite: all unused_eager_loading Bullet offences pass without safelist entries

The (Metric :owner, Metric :parent, UsageLimit :metric) are intentional, the includes are needed to prevent N+1 but Bullet cannot detect their use because they are passed as arguments to polymorphic_url

The remaining 7 entries are conditional use cases will be investigated in separate PRs:

Notes for reviewer:
32a1a0c: Fix ApiDocs::Service :service false positive, test setup used service: nil, updated to use a real service
7f3546e: Move application_plans include to plans_for_filter and make pricing_rules conditional on finance.allowed?
431a175: Remove unused [:original] from bought_plans, Plan#xml_builder never accesses it
a6abedc: Remove .includes(:issuer) from ApplicationPlansController#collection, only plan.issuer_id (a column) is ever read
18739a7: Remove plan: [:service] from ApplicationPlanLimitsController, plan is already in memory via Rails inverse association
9656bfc: Decouple application finder from applications collection in BuyersApplicationsController, single-record actions were inheriting collection-level includes unnecessarily
86f5457: Fix Invoice unused eager loading, remove buyer_account, decouple invoice finders, use cached line_items sum instead of SQL aggregate
6d38f08: Make admin_user include conditional on buyer context, remove ServicePlan :pricing_rules entry (added back again), deduplicate Metric entries
b744991: admins.first → admin_user in ApiDocs::ProviderData#user_ids: admins.first goes through the has_many :admin_users association, bypassing the preloaded has_one :admin_user cache. top_level_metrics → metrics.top_level to avoid implicit :children include.
0b61aaf: Reverted Finance::Builder::XmlMarkup to original (removed .to_f), Added back 3 safelist entries: Cinstance :user_account, Invoice :line_items, and Invoice:provider_account ( being investigated in separate PRs)
27f1bee: Just update the safelist ordering to avoid false addition
dcf53df: Restore [:original] include for bought_plans, I tested it, and it causes n+1 queries so reverted back the test and changes, will create a separate PR

[qltysh]

❌ 2 blocking issues (3 total)

Tool	Category	Rule	Count
reek	Lint	Cinstance#self.serialization_preloading is controlled by argument 'format'	1	❌
rubocop	Lint	Block has too many lines. [146/25]	1	❌
qlty	Structure	Function with high complexity (count = 5): index	1

[akostadinov]

Your work looks great! Could you please stop extending this PR and file new ones. It will become impossible to review if many exceptions are handled at the same time. It is already too big with 21 files. Ideally we would have 1 removed exception per PR. I'm looking at this PR now.

[akostadinov]

How did you verify these were false positives?

[Madnialihussain]

No, these were not false positives. I added them back after reading your previous comment because they would have required changes to more files.

I am also updating the PR description. Right now, there are 9 unused eager load entries in test.rb, and to make the description more relevant to the current changes

[Madnialihussain]

10 now, reverted one more after some more testing. I agree with your comment about creating a separate PR for each exception, I should have done that 🫠

[jlledom]

There are a total of 30 items removed from the list. For each one, could you please tell me an example of a test that failed before and works now? I think it's the only way to review this.

[Madnialihussain]

On it

[akostadinov]

I think it makes more sense to run the tests without code changes and only the bullet exceptions removed. Then we see the failures and the failures will represent some bullet exception. Otherwise we would need to run the tests 30 times for each bullet exception.

[jlledom]

I think already happened: https://app.circleci.com/pipelines/github/3scale/porta/34155/workflows/f9d44ad9-c75a-4530-8e81-38604c2e4de6

Maybe what we need is for the git log history to make more sense. I took 32a1a0c, I saw only tests were fixed instead of production, I saw the changes are reverted later, and I checked that removing the Bullet exception doesn't make that modified test fail anyway.

After that, I think we need this to be easier to review, e.g. one commit per bullet exception or similar

[Madnialihussain]

Yes about 32a1a0c. Initially I thought it was a false positive, the test was creating an ApiDocs::Service with service: nil, so the :service association was being preloaded but never accessed. I thought that was a test setup issue and changed the test to use a real service so the association would be accessed. But then I realised that service: nil is a valid, so I reverted it. And you are right, it makes the commit message irrelevant and messes up the git history

[Madnialihussain]

@akostadinov @jlledom what i am thinking is do a git reset master to unstage all the changes, then recommit them cleanly, one commit per Bullet exception, with clear messages or close this one and open a fresh PR one per exception 🫠

[jlledom]

I think that would be great. No need to close the PR, just force push the new commits IMO.