Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TEST] - Unload record clears the current state of a has many relationship #6147

Closed
wants to merge 1 commit into from
Closed

[TEST] - Unload record clears the current state of a has many relationship #6147

wants to merge 1 commit into from

Conversation

lsg-braymon
Copy link
Contributor

This is a test case for issue: #6146

@runspired
Copy link
Contributor

Roughly speaking this is an interesting artifact of a very long standing issue that I have a (reasonable) amount of hope that I'll be able to fix in the next few months combined with an expected behavior.

I'll start with a short explanation of the long-standing issue: there's some additional info about that in this long twitter thread: https://twitter.com/Runspired/status/1058536977597775872

Roughly the issue is that how we currently cache and operate on the state of the graph is too lossy, and leads to situations where we are both unable to apply patches correctly and unable to preserve local state when updating information from a remote source is required. This is something we can largely fix with a good bit of refactoring, and for the cases we can't eventually we will expose capabilities to users that allow them to signal to use enough information to get it right 100% of the time.

Now, for the expected behavior:

unloadRecord moves a record into a "never-loaded" state. It does not purge references to that record from the cache unless there are no retainers (relationships, especially async relationships, are retainers).

When a record is unloaded, for async relationships this causes us to trigger a re-fetch to re-load if the record is still needed and the relationships is accessed.

For sync relationships we do a (bit of an odd) thing by treating it as a signal that we can fully purge the record. We do this not because of correctness (it's not a correct thing to do) but because for a long time buggy behavior in unload effectively allowed users to treat it as such and it became widely depended upon. When we apply this "canonical delete", we end up doing a re-sync of our remaining "known good remote state" to our current state. This is where the buggy behavior mentioned above causes this issue for sync relationships. For async relationships the buggy behavior would manifest only once the re-fetch completes.

If you convert this test to a TODO (there are other examples in the code-base to pull from) I'll merge it as the test looks solid. I don't think we'll be able to address this for a matter of months however.

@lsg-braymon
Copy link
Contributor Author

@runspired The test has been converted to a TODO. Thanks!

@runspired
Copy link
Contributor

I suspect all the refactoring I did in the relationship layer may have resolved this, I'll make sure to get this test merged next week to check.

runspired added a commit that referenced this pull request Sep 2, 2022
@runspired
Copy link
Contributor

I didn't have permissions to rebase the original so I've reopened this in a new PR #8160

@runspired runspired closed this Sep 2, 2022
runspired added a commit that referenced this pull request Sep 2, 2022
* port test from #6147

* promote to active test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants