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

Don't query salesforce for a .first call if query already has records #77

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

CharlesMassry
Copy link
Contributor

Every time the .first method is called it will query the api. this is bad, because if we already fetched the records we need, it will make a new api call, resulting in many extra queries. For example, given a model Post

posts = Posts.where(published: true)
post = posts.first

will make 2 requests instead of making one, then peering into the already fetched records to retrieve the first item

@CharlesMassry
Copy link
Contributor Author

The previous PR broke everything because ActiveForce::ActiveQuery inherits from ActiveForce::Query and ActiveForce::Query#first calls limit if @records isn't populated. This call to limit is actually a call to ActiveForce::ActiveQuery#limit and not a call to ActiveForce::Query#limit. ActiveForce::ActiveQuery#limit has a different API than ActiveForce:Query#limit because ActiveForce::ActiveQuery#limit will return a collection if there is more than one record, but the underlying instance if there is only one record. ActiveForce::Query#limit will always return a collection. In general usage however, we never use ActiveForce::Query as everything will be an instance of ActiveForce::ActiveQuery.

end

it 'returns a single record when the api was already queried' do
active_query.to_a
Copy link

@mikelarkin mikelarkin Nov 30, 2023

Choose a reason for hiding this comment

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

I'm not sure this spec proves anything?

expect(active_query.first.id).to eq("0000000000AAAAABBB") is called in both, it's unclear what the to_a is supposed to prove here?

And the .first method calls super.to_a already so what's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

over here, to_a is a way to get the instance variable @records to be populated on active_query. The to_a that runs via super.to_a is actually a different call, as that method is run on a separate cloned object that has the @records variable set

Choose a reason for hiding this comment

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

Hmm, ok, I see yeah. Maybe a comment to explain this for those of us that don't know active_force that well or doesn't remember that to_a does what you said?

@bethanymuir bethanymuir merged commit 4ddfb81 into main Dec 19, 2023
5 checks passed
@bethanymuir bethanymuir deleted the activeforce-first-performance-inhancement-take-two branch December 19, 2023 01:38
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.

3 participants