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

Feature/fix chaining methods v2 #10

Merged
merged 17 commits into from
Aug 8, 2023
Merged

Conversation

sbelknap-bf
Copy link
Contributor

@sbelknap-bf sbelknap-bf commented Mar 3, 2023

The general idea of this PR is that if I instantiate a query, such as

first_active_query = active_query.where("Text_Label = 'foo'")

any subsequent queries chaining off the initial query, should not update the initial query.

second_active_query = first_active_query.where("Checkbox_Label = true")
expect(second_active_query).not_to eq first_active_query
expect(second_active_query.to_s).not_to eq first_active_query.to_s

but should instead leave the initial query untouched, and generate a new query instance and return the result of the new query.
Without the changes in this PR, the two expectations above would fail.

The reason for this change, is currently this type of modifying behavior can have unexpected results, in cases such as

selected_tradelines = Af::OpportunityTradeline.where(is_selected_flag: true, opportunity_id: '00605000009ynJ6AAI')
# returns 3 tradelines
selected_tradelines.all.map(&:is_conditional_debt_flag)
=> [false, false, false] 
conditional_tradelines = selected_tradelines.where(is_conditional_debt_flag: true)
# returns the same 3 tradelines...
conditional_tradelines.all.map(&:is_conditional_debt_flag)
=> [false, false, false]
# this should return empty
# if we inspect the query
selected_tradelines.to_s
=> "SELECT ... FROM Opportunity_tradeline__c WHERE (Is_Selected_Flag__c = true) AND (Opportunity_Id__c = '00605000009ynJ6AAI') AND (Is_Conditional_Debt_Flag__c = true)"
# two things have happened here. 
# first, the initial selected_tradelines query object has had its query string updated to include the 'Is_Conditional_Debt_Flag__c' = true condition
# and second, the conditional_tradelines query is not returning the expected results
conditional_tradelines.to_s == selected_tradelines.to_s
=> true
# so the conditional_tradelines query has the correct query string...
# but because the query object already has values assigned to @records, those @records are being returned instead of the new query results
selected_tradelines.object_id == conditional_tradelines.object_id
=> true
# the two queries are the same object

To fix this, using any query methods on an existing query will now return a new instance of a query object, and leave the original untouched.

asedge
asedge previously requested changes Mar 6, 2023
Copy link
Collaborator

@asedge asedge left a comment

Choose a reason for hiding this comment

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

Noticed that the cached records are being returned on the new ActiveQuery object. Please have a look at the discussion in Slack.

@asedge asedge self-requested a review March 9, 2023 14:18
@spacechurro
Copy link

@sbelknap-bf what's the scoop w/ this PR?

@rferg rferg reopened this May 11, 2023
@rferg rferg marked this pull request as draft May 11, 2023 17:31
@rferg rferg marked this pull request as ready for review May 11, 2023 18:48
@rferg
Copy link
Contributor

rferg commented May 11, 2023

Fixes #47

@rferg rferg requested a review from JeffLuckett May 15, 2023 20:27
@asedge asedge dismissed their stale review May 16, 2023 16:12

changes were made

@JeffLuckett
Copy link
Contributor

@rferg - what is the status of this? Are you dev complete? Can we get a run through QA on this?

@rferg
Copy link
Contributor

rferg commented Jun 5, 2023

@rferg - what is the status of this? Are you dev complete? Can we get a run through QA on this?

@JeffLuckett Yes this is dev complete as far as I know.

@spacechurro
Copy link

@sbelknap-bf @rferg what's the latest here? do we just need to get some QA eyes on it?

@rferg
Copy link
Contributor

rferg commented Jun 13, 2023

@sbelknap-bf @rferg what's the latest here? do we just need to get some QA eyes on it?

@spacechurro Yep. CI passed and I did some quick spot checks.

@spacechurro
Copy link

@sbelknap-bf @rferg what's the latest here? do we just need to get some QA eyes on it?

@spacechurro Yep. CI passed and I did some quick spot checks.

Cool. @sbelknap-bf what feature or card can we attach it to so that we can get some QA on it?

@sbelknap-bf
Copy link
Contributor Author

@sbelknap-bf @rferg what's the latest here? do we just need to get some QA eyes on it?

@spacechurro Yep. CI passed and I did some quick spot checks.

Cool. @sbelknap-bf what feature or card can we attach it to so that we can get some QA on it?

@rferg is there a trello card for the feature that originally identified the bug?

Copy link
Collaborator

@asedge asedge left a comment

Choose a reason for hiding this comment

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

This looks solid to me. Nice work guys!

@asedge asedge merged commit b646f50 into main Aug 8, 2023
@asedge asedge deleted the feature/fix_chaining_methods_v2 branch August 8, 2023 20:36
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.

[BUG] has_many caches mutated queries causing subsequent queries to be incorrect
5 participants