@ngrx/data - mergeQuerySet does not use PreserveChanges strategy

[AdditionAddict]

Minimal reproduction of the bug/regression with instructions:

see mergePreserveChanges() in app.component.ts in this stackblitz

after some setup the state (1 x added, 1 x updated, 1 x deleted) is:

    const result = {
      entityCache: {
        Hero: {
          ids: [2, 3, 4, 5, 8, 6],
          entities: {
            "2": { id: 2, name: "Thor", power: 5 },
            "3": { id: 3, name: "Hulk", power: 6 },
            "4": { id: 4, name: "Ant-Man", power: 7 },
            "5": { id: 5, name: "Iron Man", power: 9 },
            "6": { id: 6, name: "Thanos", power: 11 },
            "8": { id: 8, name: "Magneto", power: 10 }
          },
          entityName: "Hero",
          filter: "",
          loaded: true,
          loading: false,
          changeState: {
            "1": {
              changeType: 2,
              originalValue: { id: 1, name: "Spiderman", power: 1 } // deleted
            },
            "6": {
              changeType: 3,
              originalValue: { id: 6, name: "Thanos", power: 10 } // updated
            },
            "8": { changeType: 1 } // added
          }
        }
      }
    };

I'm dispatching a mergeQuerySet with MergeStrategy.PreserveChanges strategy

    this.entityCacheDispatcher.mergeQuerySet(
      querySet,
      MergeStrategy.PreserveChanges
    );

Here I've updated the power to -1:

    const querySet = {
      Hero: [
        { id: 1, name: "Spiderman", power: -1 }, // locally deleted
        { id: 2, name: "Thor", power: -1 },
        { id: 3, name: "Hulk", power: -1 },
        { id: 4, name: "Ant-Man", power: -1 },
        { id: 5, name: "Iron Man", power: -1 },
        { id: 6, name: "Thanos", power: -1 } // locally updated
        // id: 8 locally added
      ]
    };

Expected behavior:

Reference https://ngrx.io/guide/data/entity-change-tracker#merge-strategies

2. PreserveChanges - Updates current values for unchanged entities. For each changed entity it preserves the current value and overwrites the originalValue with the merge entity. This is the query-success default.

Expected state:

    const resultPreserveChanges = {
      entityCache: {
        Hero: {
          ids: [2, 3, 4, 5, 8, 6],
          entities: {
            "2": { id: 2, name: "Thor", power: -1 },
            "3": { id: 3, name: "Hulk", power: -1 },
            "4": { id: 4, name: "Ant-Man", power: -1 },
            "5": { id: 5, name: "Iron Man", power: -1 },
            "6": { id: 6, name: "Thanos", power: 11 }, // preserves the current value
            "8": { id: 8, name: "Magneto", power: 10 }
          },
          entityName: "Hero",
          filter: "",
          loaded: true,
          loading: false,
          changeState: {
            "1": {
              changeType: 2,
              originalValue: { id: 1, name: "Spiderman", power: -1 } // deleted // overwrites the originalValue
            },
            "6": {
              changeType: 3,
              originalValue: { id: 6, name: "Thanos", power: -1 } // updated // overwrites the originalValue
            },
            "8": { changeType: 1 } // added
          }
        }
      }
    };

Actual state:

    const resultPreserveChanges = {
      entityCache: {
        Hero: {
          ids: [1, 2, 3, 4, 5, 6, 8],
          entities: {
            "1": { id: 1, name: "Spiderman", power: -1 },
            "2": { id: 2, name: "Thor", power: -1 },
            "3": { id: 3, name: "Hulk", power: -1 },
            "4": { id: 4, name: "Ant-Man", power: -1 },
            "5": { id: 5, name: "Iron Man", power: -1 },
            "6": { id: 6, name: "Thanos", power: -1 },
            "8": { id: 8, name: "Magneto", power: 10 }
          },
          entityName: "Hero",
          filter: "",
          loaded: true,
          loading: false,
          changeState: {
            "1": {
              changeType: 2,
              originalValue: { id: 1, name: "Spiderman", power: 1 }
            },
            "2": {
              changeType: 3,
              originalValue: { id: 2, name: "Thor", power: 5 }
            },
            "3": {
              changeType: 3,
              originalValue: { id: 3, name: "Hulk", power: 6 }
            },
            "4": {
              changeType: 3,
              originalValue: { id: 4, name: "Ant-Man", power: 7 }
            },
            "5": {
              changeType: 3,
              originalValue: { id: 5, name: "Iron Man", power: 9 }
            },
            "6": {
              changeType: 3,
              originalValue: { id: 6, name: "Thanos", power: 10 }
            },
            "8": { changeType: 1 }
          }
        }
      }
    };

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

ngrx 8.6.0
angular 8

Other information:

I'll look through source and see if I can figure out the cause.

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[AdditionAddict]

Full chain following the source code:

mergeQuerySet
→ this.dispatch(new MergeQuerySet(querySet, mergeStrategy, tag)); (entity-cache-dispatcher.ts)
→ this.mergeQuerySetReducer(entityCache, action as MergeQuerySet); (entity-cache-reducer.ts)
→ this.applyCollectionReducer(entityCache, act); (entity-cache-reducer.ts)

Where applyCollectionReducer is run for each entityName with entityOp UPSERT_MANY + PreserveChanges is passed in action

The problem with this is that in upsertMany shown below, this.adapter.upsertMany(entities, collection); isn't filtering out changed entities and trackUpsertMany (and others like trackAddMany, trackAddOne) isn't making any use of mergeStrategy like mergeServerUpserts (which is why GET_ALL works with mergeStrategy demo) but this doesn't.

  protected upsertMany(
    collection: EntityCollection<T>,
    action: EntityAction<T[]>
  ): EntityCollection<T> {
    // <v6: payload must be an array of `Updates<T>`, not entities
    // v6+: payload must be an array of T
    const entities = this.guard.mustBeEntities(action);
    const mergeStrategy = this.extractMergeStrategy(action);
    collection = this.entityChangeTracker.trackUpsertMany(
      entities,
      collection,
      mergeStrategy
    );
    return this.adapter.upsertMany(entities, collection);
  }

I'm generally curious if a refactor where we keep entityOriginalCache and entityChangedCache as separate normalised states and make heavier use of @ngrx/entity if this would prevent some these bugs. Obviously this would be breaking for anyone using changeState directly but the overarching API could be kept?

[AdditionAddict]

within mergeQuerySetReducer all current tests pass changing from const entityOp = EntityOp.UPSERT_MANY; to const entityOp = EntityOp.QUERY_ALL_SUCCESS; and this uses the happier path mergeQueryResults → mergeServerUpserts