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

[4.6] Bug: sync hasMany relationships with pointers to unloaded data do not recompute if the data loads later #8846

Open
patrick-auditboard opened this issue Sep 6, 2023 · 2 comments
Labels
🏷️ bug This PR primarily fixes a reported issue team-review

Comments

@patrick-auditboard
Copy link

patrick-auditboard commented Sep 6, 2023

This bug is present in at least 4.6.

Given the following setup: @hasMany('foo', { async: false, inverse: null }) foos;
And given an initial payload of relationships: { foos: { data: [ { type: 'foo', id: '1' } ] } } but which does not include resource data for foo:1

  • The cache and graph will enter the correct state

Possibly related to reproduction:

  • a notification will be sent to the record
  • the notification is discarded because the record is not yet materialized

Then

  • The parent record becomes materialized
  • The foos relationship is accessed on the parent record
  • the ManyArray is instantiated
  • retrieveLatest is called from init
  • retrieveLatest filters out the record as it has no resource data loaded
    for (let i = 0; i < jsonApi.data.length; i++) {
    // TODO figure out where this state comes from
    let im = this.store._instanceCache._internalModelForResource(jsonApi.data[i]);
    let shouldRemove = im._isDematerializing || im.isEmpty || !im.isLoaded;
    if (!shouldRemove) {
    identifiers.push(im.identifier);
    }

Later

  • The related record is loaded, nothing invalidates the ManyArray to resync

Unrelated to bug but of Note: in our app, when the related record is loaded we also reload the relationship data for the primary record; however, since the data is unchanged no change notification is sent so the ManyArray does not invalidate.

@runspired runspired added the Bug label Sep 6, 2023
runspired added a commit that referenced this issue Sep 10, 2023
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
@runspired
Copy link
Contributor

Some additional context here:

This bug can only be hit in production because its for unspecified behavior that occurs when an assert that would normally be triggered in development is removed.

but

This is a situation where we should probably attempt to not totally wreck the system when the error case is entered. How to handle that is unclear. The situation only presents itself when applications choose to give identifiers for relationships to records that are unloadable the user's lack of permission to see the records. Ostensibly the application should either be modeling this differently or using links instead of identifiers for managing relationship state.

@runspired
Copy link
Contributor

We added tests to codify what happens in 4.6 when we enter these situations in production, not because we consider this supported or the behavior good but simply because we do want to know when we've changed what happens.

Tests for 4.6: #9115
We have ported those tests to 4.12 where we used them to minimize the impacts of some changes: #9120
We will similarly port forward the 4.12 state to the main branch and 5.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue team-review
Projects
Status: Needs Planning
Development

No branches or pull requests

2 participants