Proposal: bypass the AR association for more efficient DB queries

[tompave-zen]

This started from my observation that accessing one single property from a "property set" loads all entries from the DB [1], and that updating one will check if all the others already exist [2] because of a uniqueness validation (?).

I saw that something similar was attempted before (#79), but that was rejected because (if I follow correctly) it would modify the behaviour of the has_many association in an inconsistent way. I think that there is some merit in that line of reasoning.

For that reason I tried a different approach. I based it on the idea that a has_many ActiveRecord association is not the best tool to manage this sort of data. It comes with too much baggage, and its interface provides too many ways to interact with the data. Keeping the property_set entries in the DB as individual records might be ok, but I thought that they could be accessed directly without going through the has_many association.

Summary of Contents

1. The main result of these changes is that code like account.settings.last_login will only SQL-SELECT that one record instead of everything. And that code like account.settings.save will only SQL-UPDATE the records that actually changed, rather than loading and validating them all.
2. Rather than refactoring the gem, I've introduced a separate AssociationWrapper class. Most of the new logic is in there, and I've modified the existing code with what I needed to inject the new wrapper.
3. I haven't done it in this PR, but I can see a way to make the new wrapper class opt-in, so that it will only be used if explicitly enabled. This could be controlled with an Arturo flag. This is not in the PR because I wanted to keep things simple for now.
4. If we think that this approach makes sense, I think that we shouldn't keep both the legacy implementation (based on the AR has_many) and my new wrapper. The two alternative implementations are kept sepatate for now because it makes it easier to review the changes (they're mostly isolated in their own file), and because it would make it safer to roll this out (feature flags). I am not suggesting that the code in this PR should be final.
5. I've extracted the logic to read a value (checking the type, casting the raw value, deserializing it if necessary) into a helper class, because I wanted to use it in two places.
6. For now I focused on making the existing tests for this repo pass, and I haven't added new tests. This makes me fairly confident that the contract of the gem is preserved. I plan to do add unit tests for the new components after the initial discussion. It looks like the tests are failing for some combinations of older Ruby+Rails; I need to double check.
7. I've tested these changes on Classic. It works. I'm just getting some test failures because of some code that is reaching into the internals (i.e. @ivars) of the AR associations.
8. I haven't yet properly investigated how this will interfere with our AR caching layers (Kasket, and the default Rails caching that is replacing it). My gut feeling, briefly discussed with the team, is that a simpler property_set implementation that doesn't rely on automagic patching of the AR has_many should be easier to cache.

---

[1]: e.g. SELECT * from account_settings WHERE account_id = ?
[2] e.g. SELECT 1 AS one FROM account_settings WHERE account_settings.name = ? AND (account_settings.id != ?) and account_settings.account_id = ?, for each record that is part of the property_set.

[tompave-zen]

This logic has been moved into PropertySets::ValueReader, but I haven't changed it.

[tompave-zen]

This is an example of things that I've left in the code just to make it easier to follow the changes, and that should be cleaned up before this can move forward.

[tompave-zen]

These were the only changes needed in the test files. I couldn't find a way around adding #to_a. I'm not a big fan of how an AR:Relation can automatically be reified to behave like an Enumerable when required, so I didn't try to implement it in the new AssociationWrapper.

On the #reload, this is necessary because this test uses account.update_attributes!(:texts_attributes => [...]), which bypasses the property_set gem and goes directly to ActiveRecord. I couldn't find a convenient hook to invalidate the cache automatically.

Compare this to the next test below (it "be updateable as a nested structure"), that instead uses account.update_attributes!(:settings => { ... }). That is intercepted by the property_set gem, and technically there the #reload is not necessary (even though the test was calling it 🤷 ).