WIP - delay_touching on relationship table may cause "update where NULL" query

[oehlschl]

This PR introduces a failing spec to replicate issue described in #19. This branch can be used as the source branch for fixes, but shouldn't be merged until specs pass.

[krainboltgreene]

Supplied a first attempt approach at solving.

[mtuckergh]

I think this is a good start. However, since we're skipping the actual update for non-persisted records, it seems we should also skip calling run_callbacks on those records on line 101, right? In the case where these records didn't persist, and thus the ID falls back to null, I think we're also in the same boat on identifying those records in the Set, right? To that end, there's little value in calling state.updated for those records, as they cannot be identified from the Set for removal (same core problem as #14).

Presuming the above is correct, does it make sense to pre-filter records to only persisted records, and reference that updated collection in this method?

# only persisted records need to be updated
records.select!(&:persisted?)

#... remainder of references left as-is, since `records` was pre-filtered
klass.unscoped.where(klass.primary_key => records).update_all(changes)
# ... and so on

The unfortunate thing here is that delay_touching class now has awareness of this AR persistence problem, where we managed to isolate that concern to the state class in the infinite loop problem. I'm not sure there's a way to have State handle this scenario, though.

[oehlschl]

@mtuckergd I know you're not thrilled by the performance, but I think one way of doing the filtering upstream is the old implementation: https://github.com/godaddy/activerecord-delay_touching/pull/17/files#diff-c3d5a91eaad03c43d84f142a4473a06dR56

I think that would prevent unpersisted records from ever getting into this touch_records method and keep the persistence logic well encapsulated within the State. FWIW, with that you might even be able to remove the if persisted? (previously unless destroyed?) check here.

[oehlschl]

Also thanks for your continued support on this.

[mtuckergh]

Given that this is a performance-focused library, I think compromising clean encapsulation to favor performance is probably the right direction, if we can't find a way to preserve both.

The old implementation introduces a 3N iteration overhead (all guaranteed full iterations) of the collection of records, where the final merged implementation adds at most a 1N iteration (possibly less, since there's a shortcut on first persisted record). Even introducing the iteration in the select!, added to the merged solution, leaves us at worst with 2N overhead.

Also notable: the old implementation still introduced state awareness to delay_touching because it had to trigger the state object to remove unpersisted records, so I don't think it saves us much in the leaky abstraction problem.

What do you guys think about the proposal above, adding the records.select!(&:persisted?) call? Open to other suggestions, as well.
@oehlschl ☝️

[mtuckergh]

Seems we've stalled out on this conversation. Sorry, I've had a lot of distractions recently. Unless I hear otherwise, I'll probably forge ahead with my recommendation above. Still open to suggestion.