orbitjs / orbit

Composable data framework for ambitious web applications.
https://orbitjs.com
MIT License
2.33k stars 134 forks source link

Need optional chaining in `MemoryCache::removeInverseRelationshipsSync()` #970

Closed bradjones1 closed 1 year ago

bradjones1 commented 2 years ago

https://github.com/orbitjs/orbit/blob/fd40512ee3b0b622f737786e03928186938f454a/packages/%40orbit/memory/src/memory-cache.ts#L332-L339

I believe this .get() should be ?.get() as I've discovered that relationships may be dangling after prior remove operations. The if (rels) below indicates that we anticipate this but don't account for it during the get.

This is a potential bug I've discovered while working through some situations where I'm trying to prune the cache of certain records to ensure I have an updated set from a coordinated remote source... However, it's proving difficult to remove records without also removing inverse relationships, which isn't my intent (I'm fine with them dangling.)

dgeb commented 2 years ago

@bradjones1 this._inverseRelationships is set in _resetInverseRelationships for every model in the schema. I don't think there's a normal code path by which this._inverseRelationships[SOME_TYPE] gets cleared. But if there is, then we should defend against this in a number of code paths, not just in removeInverseRelationshipsSync.

Are you saying that you're manually clearing out this._inverseRelationships[SOME_TYPE] during a cleanup process? Any chance you can provide a failing test that illustrates your particular scenario?

bradjones1 commented 2 years ago

@dgeb Thanks for taking a look. The scenario is that I want to ensure that I have a "complete" cache client side of a particular resource type. I store information in a non-Orbit flag that tells me if I think I need to basically clear my local cache of all instances of the resource, and then re-request from the remote, to ensure I have a complete set, and no stale resources that might have gone away from the remote. However, this resource is also linked to from other resource types. So when I iterate through all the resource IDs locally and delete them, all those references get deleted, as well. (I would prefer they just be dangling.) And thus I run into an error where the cache has inconsistent inverse relationships. In a way, I'd kinda just rather not track them, but I imagine this is core to Orbit's functionality.

I know that's clear as mud, but hopefully that provides some context until I can generate a failing test case, or you tell me that this is all a very bad idea and anti-pattern and breaks on purpose.

dgeb commented 2 years ago

In a way, I'd kinda just rather not track them, but I imagine this is core to Orbit's functionality.

@bradjones1 Tracking inverse relationships is the responsibility of the SyncCacheIntegrityProcessor. By default cache processors are set to [SyncSchemaConsistencyProcessor, SyncCacheIntegrityProcessor].

However, you can optionally leave this processor out when creating a cache like so:

const source = new MemorySource({ schema, cacheSettings: { processors: [SyncSchemaConsistencyProcessor] } });
bradjones1 commented 1 year ago

OK, thanks for the detailed reply. I think this is then more a +1 to document these various processors and their roles in the official docs, a la https://github.com/orbitjs/orbit/issues/955.