search

active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.19k stars 178 forks source link

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

Closed pfeiffer closed 1 year ago

pfeiffer commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

pfeiffer commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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.

flavorjones commented 1 year ago

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