DRAFT: retire parent_data via replayed-model recording#67
Draft
navidemad wants to merge 1 commit into
Draft
Conversation
Exploration / alternative to Gusto#61. Instead of lazily reading the parent's cached content when a child is saved, remove the reason the child reads it: the parent_data parameter. ActiveRecordCoder#mount now records the models it replays into a per-generate @replayed_models set; ActiveRecordCoder#generate folds that set into its captured models after the block runs. This works because the runner reuses one coder instance and the parent is mounted inside the child's generate block. Cache#evaluate no longer reads parent.cache.content, and the Coder contract drops the parent_data keyword. I lean against merging this — it trades an explicit parameter for hidden, ordering-dependent instance state and changes the public Coder contract, without buying a real simplification (per-model INSERT tagging would require de-batching the one-execute_batch-per-connection call a spec pins). Gusto#66 documents why parent_data stays. Opened as a draft so the alternative is concrete. Includes a propagation test matrix (basic cross-process, STI collapse, second connection pool, delete-only nil sql) that drives the real parent.mount -> @replayed_models path, and rewrites the coder's old parent_data merge spec against the new mechanism.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft / for discussion. I lean against merging this; see "Trade-offs".
What this explores
An alternative to #61. Instead of fixing the
data_for for nilcrash by lazily reading the parent's cached content, this removes the reason the child reads the parent's content during save at all: theparent_dataparameter.Today
Cache#evaluatelooks upfixture.parent.cache.content.data_for(coder.class)and passes it tocoder.generate(parent_data:). The coder mergesparent_data.keysinto its captured model set, because the parent's rows are replayed (viaparent.mount) under the log tag"FixtureKit Insert", which thesql.active_recordsubscriber can't attribute to a model.This branch drops
parent_dataand has the coder record what it replays instead:ActiveRecordCoder#mountaddsdata.keysto a per-generate@replayed_modelsset.ActiveRecordCoder#generatefolds@replayed_modelsinto the captured set after the block runs.It works because the runner reuses one coder instance and the parent is mounted inside the child's
generateblock. One side effect worth noting: the child no longer readsparent.cache.contentduring save, so the #61 crash can't occur on this path at all.Trade-offs (why I lean against it)
parent_datais a visible input togenerate.@replayed_modelsis an instance variable populated as a side effect ofmount, with an implicit "mount runs inside generate's block" contract. Harder to reason about, easier to break.Codercontract.generate(parent_data:)becomesgenerate(&block), so any custom coder that usedparent_datanow has to implement the record-on-mount, fold-on-generate dance itself.execute_batchper connection thatactive_record_coder_spec.rbpins. Worse trade.So
parent_dataisn't cruft, it's the clean version. #66 documents why it stays; this branch is here so the alternative is concrete rather than hypothetical.Test matrix
spec/unit/fixture_cache_spec.rbgets a propagation matrix that drives the realparent.mountto@replayed_modelspath (the existing inherited specs stub mount, so they don't):CartoVehicle)ActivityLogon the analytics DB)nil, still carried forwardThe old
parent_datamerge test inactive_record_coder_spec.rbis rewritten against the new mechanism.Test plan
bundle exec rspec(183/183)