Fix unscope not working when where by tripe dot range

[ippachi]

Motivation / Background

Fixes #48094
unscope not working when where by tripe dot range

Detail

Define fetch_attribute to activerecord/lib/arel/nodes/and.rb, because unscope uses fetch_attribute to determine whether to exclude predicate or not.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[skipkayhil]

The fix looks good to me. Can you add a CHANGELOG entry (and make sure you only have one commit after that)?

[nvasilevski]

We are doing something wrong here. I'm not familiar with the area to immediately tell what is wrong but in some cases .fetch_attribute is expected to return a boolean, for example:

rails/activerecord/lib/active_record/associations/join_dependency/join_association.rb

Line 60 in 1f773fa

!Arel.fetch_attribute(node) { |attr| attr.relation.name == table.name }

rails/activerecord/lib/arel/nodes/binary.rb

Line 116 in 1f773fa

left.fetch_attribute(&block) && right.fetch_attribute(&block)

rails/activerecord/lib/active_record/relation/where_clause.rb

Line 193 in 1f773fa

end || Arel.fetch_attribute(node) do |attr|

But by using .each here we will always return the value we enumerate over which is children and it'll always be truthy. It may or may not be the issue but I wanted to point out that this implementation of fetch_attribute doesn't seem to be consistent with how we expect to use it

because unscope uses fetch_attribute to determine whether to exclude predicate or not.

Could you elaborate on this a little bit? Because if it makes decision based on the return value then the implementation seems wrong as the return value will always be truthy

[ippachi]

I looked over the code
As you said, fetch_attribute should return a boolean value

fetch_attribute seems to be a method that determines whether the return value of the block is true for all attributes it owns

So it seems correct to use all? instead of each this time. What do you think?

[nvasilevski]

I think .all? seems to be more aligned with the expected behavior however be careful with [].all? being true.
We may want to additionally check for children.any?

So perhaps
children.any? && children.all? { |child| child.fetch_attribute(&block) }

[ippachi]

You are right

I don't know if there are cases where the children are empty, but in any case it would be better to take empty cases into consideration

I will amend it as such and at the same time add an entry in CHANGELOG

[ippachi]

@nvasilevski @skipkayhil
I fixed code and added CHANGELOG entry

[ghiculescu]

FYI #49463