Refactor `Relation` and conditions to fix mutating scopes issues #268

pfeiffer · 2022-10-18T06:18:18Z

This PR adresses the same issues as #266, #265, #257, #193 and #261 at once, by adressing the 'root' of the issue - namely the mutating methods on Relation.

While #266 and #265 also adresses this in isolation, this PR is an alternative addressing the kinda messy structure of Relation and WhereChain, by extracting the condition predicate matching to their own Condition class. The conditions are evaluated lazily when filtering on the records of a relation should happen.

By doing it like this, it can open up for supporting more of the Rails scoping API such as #reorder and eg. #invert_where. Both are included here in the PR.

kbrock · 2023-03-09T03:48:40Z

lib/active_hash/base.rb

+        relation = ActiveHash::Relation.new(self, @records || [])
+        relation = relation.where!(options[:conditions]) if options[:conditions]
+        relation


is there a reason to change this block?

I suppose the Relation could be initialized with the conditions directly.

The initializer has:

self.conditions = Conditions.wrap(conditions || [])

I wondered if there was a reason to not use that.
The new code passes a nil (so it gets and empty array)
and the where! then adds to that conditions.

Is this to ensure we don't modify the options[:conditions]?

I kinda like the original way better but if this has a reason then 👍

kbrock · 2023-03-09T03:49:51Z

lib/active_hash/condition.rb

+
+  def matches_value?(value, comparison)
+    return comparison.any? { |v| matches_value?(value, v) } if comparison.is_a?(Array)
+    return comparison.include?(value) if comparison.is_a?(Range)


could you squash the third commit into this commit.

no reason to introduce a bug to then fix it in a later commit.

Sure, but if this PR is eventually merged via a single squashed commit, I think it's good to keep the individual commit in this PR for future clarity.

Happy to squash if you guys plan merging via merge-commits and not squash and merge strategy.

kbrock

Thanks @pfeiffer (especially for submitting a more tame PR and this implementation)

This approach may seem complicated but it feels similar to the way that rails has implementing the chaining.

For that reason, I like this implementation.

I do worry that it may be confusing for people.

kbrock · 2023-03-09T03:52:47Z

lib/active_hash/condition.rb

+    return comparison.include?(value) if comparison.is_a?(Range)
+    return comparison.match?(value) if comparison.is_a?(Regexp)
+
+    normalize(value) == normalize(comparison)


Could you talk more on this?

The first thing I tested was:

(1.0).to_s == 1.to_s

But cases like this work great (which are probably more relevant since this will fix the very common find_by(id: "4"):

"4".to_s == "4".to_s

Yes; this is to make ActiveHash behave similar to how ActiveRecord behaves for querying.

ActiveRecord (or rather the DB) does a similar typecasting when querying:

# AR User.find("2").id # => 2 User.find_by(id: "2").id # => 2 User.find_by(username: :john).username # => "john"

The normalize with typecasting to string here makes ActiveHash behave similar:

# AH City.find("2").id # => 2 City.find_by(id: "2").id # => 2 City.find_by(country_code: :gb).country_code # => "gb"

.. without this normalization, no records would be returned in the examples above for AH, while it would for AR.

kbrock · 2023-03-09T03:54:34Z

lib/active_hash/conditions.rb

+class ActiveHash::Relation::Conditions
+  attr_reader :conditions
+
+  delegate_missing_to :conditions


trying to wrap my mind around this one.
(the future commit does a little more explaining)

I guess real active record does something similar any time you run methods on relations.

Can you just put a comment to explain this a little more?

Sure. This class is an delegator to an array of conditions. It delegates methods to the array of conditions, but also implements #matches? which is a short-hand for array_of_conditions.all? { |c| c.matches?(record) } and makes it a bit cleaner to work with in the Relation.

It also adds .wrap which is similar to Array.wrap to 'wrap' an array of conditions (or return itself if already wrapped).

Happy to add comments to the code, if you believe this would make it clearer.

kbrock · 2023-03-09T03:57:57Z

lib/active_hash/relation.rb

-    def where(query_hash = :chain)
-      return ActiveHash::Base::WhereChain.new(self) if query_hash == :chain
+    def spawn
+      self.class.new(klass, all_records, conditions, order_values)


is this ok to not dup conditions or order_calues?

I think all_records is fine since this is not modifying that.

thinking out loud (and not a request):
Also feels like we should be able to avoid all this passing around all_records.

thinking out loud (and not a request): Also feels like we should be able to avoid all this passing around all_records.

Passing around all_records is to support invert_where and other unscoping methods.

In theory, it could probably be possible to re-load all the records from the main class when a relation is unscoped, but I have a feeling that it would make the behavior of calling reload on a chained relation behave unexpected.

Let me know if this is something I should look into.

thanks. if it ain't broke...

kbrock · 2023-03-09T03:59:35Z

lib/active_hash/relation.rb

+    def records
+      @records ||= begin
+        filtered_records = apply_conditions(all_records, conditions)
+        ordered_records = apply_order_values(filtered_records, order_values) # rubocop:disable Lint/UselessAssignment


Seems it would have been easier to remove ordered_records than add the rubocop line

Is this for future debugging or something?

You're right, probably had a breakpoint here at one point to inspect :-)

kbrock · 2023-03-09T04:01:22Z

lib/active_hash/relation.rb

      self
    end

    def all(options = {})
-      if options.has_key?(:conditions)
+      if options.key?(:conditions)


You did not write this code, so probably ignore this but...

is there a reason to have the conditional here instead of an ||?

where(options[:conditions] || {})

There would be a difference when passing an explicit false-y (nil, false, ..) value as the :condition, right?

But not sure what effect it has for the finders though.

kbrock · 2023-03-09T04:07:36Z

@flavorjones Do you have thoughts on this implementation? (I think you are also pretty familiar with rails internals)

Coming up with the simplest solution that works well is the goal.
Unfortunately, I am not sure that there is a simpler solution that will meet our needs.

I feel comfortable with most of this solution, but I do worry how supportable it will be for the active-hash collaborators

kbrock · 2023-03-09T20:30:09Z

@pfeiffer does this potentially fix #271 as well, or is that different?
(not asking to test, just asking if you think it may overlap that PR]

pfeiffer · 2023-03-09T20:31:48Z

Yup, that issue would be addressed as well by this PR.

pfeiffer · 2023-03-09T20:46:20Z

Thanks for taking the time to review this rather aged PR 😀

We've been running this in production for a while now, and I would love to see it merged.

I'll add some more context and reply to the individual review comments tomorrow. I'm aware that it's a major rewrite and not as surgical as the other PRs - I do however believe that it's the way to go and if I remember correctly (on my phone right now), all public interfaces are kept while the buggy and unexpected behaviors are fixed.

kbrock · 2023-03-10T01:31:20Z

@pfeiffer sorry for the delay.
I guess this is a big change, and probably intimidating, BUT...
It feels so similar to what I remember of ActiveRecord internals, so it really isn't so bad.

I do need some feedback from another maintainer. This changes the approach to the problem and want to make sure others are comfortable with this alternate style.

I did notice that people were hoping for the smaller changes (there are a few PRs that all seem to be taking a whack at this issue).

pfeiffer · 2023-03-10T08:17:08Z

@pfeiffer sorry for the delay. I guess this is a big change, and probably intimidating, BUT... It feels so similar to what I remember of ActiveRecord internals, so it really isn't so bad.

You're right - the patterns are lifted from ActiveRecord and it provides a similar interface for chaining and lazily-evaluating conditions on the scopes. Method names etc. are also lifted from AR to keep the code and patterns similar for developers familiar with Rails.

The general idea is to make relations returned by where(..), order(..), reorder(..), not(..) etc. immutable, which is how AR also works. By doing so, it solves a lot of fundamental issues with the previous code and opens up for supporting the newer AR methods such as invert_where, reorder etc., which would be impossible to support with the existing code, as the scopes mutate the original collection of records.

I do need some feedback from another maintainer. This changes the approach to the problem and want to make sure others are comfortable with this alternate style.

I did notice that people were hoping for the smaller changes (there are a few PRs that all seem to be taking a whack at this issue).

I've seen that as well and contributed a few more surgical PRs as well as I see others have also been doing (#271, #266, #255, #261, #197). However, while these might fix the most obvious issues and bugs, they do not address the underlying issues of mutating scopes, and implementing eg. invert_where would be close to impossible without a refactoring of the scopes like this.

Happy to jump on a chat or call to discuss if needed :-)

kbrock · 2023-03-11T23:00:58Z

@flavorjones Do you find this approach reasonable?
This will fix many of our outstanding issues around sorting and modifying data.

I like how it follows rails patterns but want a second opinion before I merge

flavorjones · 2023-03-14T14:29:56Z

Sorry, I've been underwater for a few days. I'll make time to read the code this week. I'm not an expert in Rails internals but I appreciate you asking for a gut check.

kbrock

I'd like to move forward with this.
Bunch of issues around this central issue.
Going the route of following Rails is a great direction.

This simplified Hash we had before sure works well for simplicity, but it breaks down. And going forward with some of the rails 7 features (thanks for highlighting them), we really need to go this route.

I'll leave this in approve for a little bit, waiting for comment. But will merge if no one comments this week.

flavorjones · 2023-03-30T06:12:31Z

@kbrock No objections to merging this, sorry for not doing a more detailed review or getting back to you earlier.

pfeiffer added 3 commits October 6, 2022 11:24

Refactor Relation and restructure condition matching

aa512c4

Add invert_where method as in AR 7 to invert all conditions

b35b81b

Use #cover? instead of #include? for Range

fbbc057

This was referenced Oct 18, 2022

Bugfix: Ensure that using id: [..] as condition returns unique records #266

Closed

Relation: Make where return a fresh relation to avoid leaking scopes #265

Closed

kbrock reviewed Mar 9, 2023

View reviewed changes

kbrock approved these changes Mar 27, 2023

View reviewed changes

kbrock merged commit e1bbfda into active-hash:master Mar 30, 2023

flavorjones mentioned this pull request Jun 18, 2023

Condition explicitly uses attribute values and not public_send #281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Relation` and conditions to fix mutating scopes issues #268

Refactor `Relation` and conditions to fix mutating scopes issues #268

pfeiffer commented Oct 18, 2022

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock Mar 14, 2023

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock left a comment

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock Mar 14, 2023

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock Mar 9, 2023

pfeiffer Mar 10, 2023

kbrock commented Mar 9, 2023

kbrock commented Mar 9, 2023

pfeiffer commented Mar 9, 2023 •

edited

Loading

pfeiffer commented Mar 9, 2023

kbrock commented Mar 10, 2023

pfeiffer commented Mar 10, 2023

kbrock commented Mar 11, 2023

flavorjones commented Mar 14, 2023

kbrock left a comment

flavorjones commented Mar 30, 2023

Refactor Relation and conditions to fix mutating scopes issues #268

Refactor Relation and conditions to fix mutating scopes issues #268

Conversation

pfeiffer commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Mar 9, 2023

kbrock commented Mar 9, 2023

pfeiffer commented Mar 9, 2023 • edited Loading

pfeiffer commented Mar 9, 2023

kbrock commented Mar 10, 2023

pfeiffer commented Mar 10, 2023

kbrock commented Mar 11, 2023

flavorjones commented Mar 14, 2023

kbrock left a comment

Choose a reason for hiding this comment

flavorjones commented Mar 30, 2023

Refactor `Relation` and conditions to fix mutating scopes issues #268

Refactor `Relation` and conditions to fix mutating scopes issues #268

pfeiffer commented Mar 9, 2023 •

edited

Loading