Avoid unnecessary identity notification when record is saved

[robbytx]

Description

Since the elimination of InternalModel in 4.7.0, every time an existing record is updated, the Record Data/Cache (by invoking InstanceCache.setRecordId) notifies that the identity of the record has changed, even if the id was not changed, which is the typical case.

This behavior leads to unnecessary invalidation (and therefore recomputation) of properties that are subscribed to model.id, leading to worse performance and possible bugs due to the unexpected behavior.

Notes for the release

N/A?

[robbytx]

@runspired I feel like the IdentifierCache.updateRecordIdentifier method (or each caller of it) should be responsible for notifying the identity property, so that the notifications are consistently sent precisely when the id changes.

An alternative is to revert to the earlier behavior where the cache tracks the id itself so it can detect when that changes.

A possible third alternative is to eliminate IdentifierCache.updateRecordIdentifier call that precedes Cache.didCommit, so that it can actually observe the id change.

[runspired]

I feel like the IdentifierCache.updateRecordIdentifier method (or each caller of it) should be responsible for notifying the identity property, so that the notifications are consistently sent precisely when the id changes.

Possibly, though I still think we want the cache to determine the right time. The issue here is the legacy-handler, were it not for the legacy support the cache would be what calls updateRecordIdentifier.

An alternative is to revert to the earlier behavior where the cache tracks the id itself so it can detect when that changes.

I think the 4.4 behavior was due to some craziness in how InternalModel specifically buffered things.

A possible third alternative is to eliminate IdentifierCache.updateRecordIdentifier call that precedes Cache.didCommit, so that it can actually observe the id change.

broad strokes yes. If we can make this happen today I'm for it, but I suspect the issue with this is legacy support / deprecated code-path support.

There's another angle here: you shouldn't actually be allowed to change the ID once it is set, but this is not a constraint of the identity-manager but rather a constraint of the cache. So the cache should throw an error if an id exists and a new id is returned.

Today, we warn in the cache when a new ID is returned

    if (id !== undefined) {
      let newId = coerceId(id);

      if (identifier.id !== null && identifier.id !== newId) {
        // here we warn and ignore, as this may be a mistake, but we allow the user
        // to have multiple cache-keys pointing at a single lid so we cannot error
        warn(
          `The 'id' for a RecordIdentifier should not be updated once it has been set. Attempted to set id for '${wrapper}' to '${newId}'.`,
          false,
          { id: 'ember-data:multiple-ids-for-identifier' }
        );
      }
    }

We should also only update the id on the identifier if none existed before or if the configured update handler tells us too, e.g. this appears too greedy

   // upgrade the ID, this is a "one time only" ability
  // for the multiple-cache-key scenario we "could"
  // use a heuristic to guess the best id for display
  // (usually when `data.id` is available and `data.attributes` is not)
  if ((data as ExistingResourceObject).id !== undefined) {
    identifier.id = coerceId((data as ExistingResourceObject).id);
  }

[robbytx]

There's another angle here: you shouldn't actually be allowed to change the ID once it is set, but this is not a constraint of the identity-manager but rather a constraint of the cache. So the cache should throw an error if an id exists and a new id is returned.

I'm hesitant to introduce that change, since this has not been a hard requirement in the past, and it strikes me as reasonable to permit model ids to change over time, at least in well-defined ways that have worked in the past.

If the behavior has worked, and it serves a use case, then I feel like it go through an official deprecation + major release before being removed.

I suspect the issue with this is legacy support / deprecated code-path support.

I'm reading this and I see that the file is called "legacy", but I don't see any replacement code path that invokes didCommit, so to me this is the primary/non-legacy code path for all intents and purposes, until a working alternative is available.

The issue here is the legacy-handler, were it not for the legacy support the cache would be what calls updateRecordIdentifier.

But the cache does call it! The legacy handler invokes cache.didCommit, which invokes InstanceCache.setRecordId (via its store wrapper), which invokes IdentifierCache.updateRecordIdentifier.

So why can't we remove the legacy handler's up-front call to IdentifierCache.updateRecordIdentifier?

1. Is it because there are existing, supported cache implementations that don't call setRecordId? This would be surprising to me, but I suppose possible. In this case, we could vary behavior based on whether the cache is default.
2. Is it because we need to handle the merge before passing to the cache, so the cache updates the right target? In this case, I don't see how an alternate code path could avoid doing the same.
3. Is it because the legacy handler needs the actualIdentifier in order to do store.peekRecord(actualIdentifier)? Why can't it ask the identifier cache to translate the original identifier to whatever the merge result might have been?
4. Is it because there's a timing issue with handing it off to the cache that I'm overlooking?

[runspired]

@robbytx

I'm hesitant to introduce that change, since this has not been a hard requirement in the past, and it strikes me as reasonable to permit model ids to change over time, at least in well-defined ways that have worked in the past.

This actually was a hard constraint going very far back. It was accidentally relaxed when we removed InternalModel, InternalModel would not have allowed this to occur.

I'm reading this and I see that the file is called "legacy", but I don't see any replacement code path that invokes didCommit, so to me this is the primary/non-legacy code path for all intents and purposes, until a working alternative is available.

That's because the alternative is literally "this thing does not need to exist" 😅

So why can't we remove the legacy handler's up-front call to IdentifierCache.updateRecordIdentifier?

We may not need to anymore, this needs investigated. In the recent past the answer would have been a combination of reasons (2) (3) and (4).

Your solution in (3) sounds correct, but I think this shows that didCommit should be changed to return the response document similar to put. I'll put up a PR that makes that addition (and amend the RFC)

e.g.

didCommit<T>(
    identifier: StableRecordIdentifier,
    data: StructuredDataDocument<T>
  ): void;

should be

didCommit<T>(
    identifier: StableRecordIdentifier,
    data: StructuredDataDocument<T>
  ): ResourceDocument;