(entity) Upsert Add overrides the id property

[fmalcher]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request

What is the current behavior?

The upsert methods of the EntityStateAdapter override the id property, even if this is not the primary entity key. This only happens when adding with upsert.

Expected behavior:

ngrx/entity should not make any changes to the added entity. It should leave the id property as is OR should change the property we assigned with the selectId function.

Explanation / reproduction

DEMO: https://stackblitz.com/edit/ngrx-upsert-bug-r96kbx
(thanks to @miniplus for the StackBlitz skeleton)

Imagine an entity like a book whose "primary key" is not named id but isbn.
However, this entity also has an id property which is not the primary key.

export interface Book {
  isbn: string,
  title: string,
  id: string;
}

(We might discuss why there is an id property which is not the ID, but that's not the point here at all 😉 )

To make ngrx/entity use the isbn property as key, we set the selectId selector function accordingly:

export const adapter: EntityAdapter<Book> = createEntityAdapter<Book>({
  selectId: book => book.isbn
})

Now, in the reducer, we're using the new upsertOne/upsertMany methods from the StateAdapter to add a new entity which is not yet in the entity list:

const book: Book = {
    isbn: '123',
    name: 'My Book',
    id: 'book-abc'
}
const change: Update<Book> = { id: book.isbn, changes: book }
return adapter.upsertOne(change, state);

(Note: The Update object seems to be necessary here, but shouldn't, see #818 )

This leads to the following entities object in the state:

'123': {
    isbn: '123',
    name: 'My Book',
    id: '123' // <-- THIS CHANGED 💥
}

...where it actually should be

'123': {
    isbn: '123',
    name: 'My Book',
    id: 'book-abc'  // <-- ✅
}

Notice, when doing an upsert as update (i.e. for an entity that already exists), the id property is set correctly.

Where's the error?

The issue might be in this line:
https://github.com/ngrx/platform/blob/master/modules/entity/src/sorted_state_adapter.ts#L125

for (const update of updates) {
    if (update.id in state.entities) {
        updated.push(update);
    } else {
        added.push({
            ...update.changes,
            id: update.id, // <-- overrides the id property
        });
    }
}

This sets/overrides the id property, regardless of whether this is the actual ID or not.

Possible solution

Remove the line completely.
update.changes contains the complete object to be added and there is no need to set the ID.
If we want to make sure that update.id and the ID from the entity are the same we should check this here using the selectId() function.

I'd like to submit a PR but wanted to discuss this first.

Thanks!

Affected versions

@ngrx/entity 5.1.0

[fmalcher]

I worked on fixing this, however this one is tightly coupled to #818 .
Without changing the API of the upsert methods, we can't easily fix this one here.

Look at this test case (from the actual unit tests):

    const firstChange = { title: 'Zack' };
    const secondChange = { title: 'Aaron' };
    const withMany = adapter.addAll([TheGreatGatsby], state);

    const withUpserts = adapter.upsertMany(
      [
        { id: TheGreatGatsby.id, changes: firstChange },
        { id: AClockworkOrange.id, changes: secondChange },
      ],
      withMany
    );

It uses upsertMany to do partial changes to existing entities.
In this scenario the change objects dont have an id property (or any other that could be the primary key). Thus, the line[1] mentioned above is actually necessary.

However, in my opinion, the upsert methods should take a complete entity as argument, as proposed in #818 and described in the docs.
It should never be used to apply partial changes to existing entities, because that's what the update methods are for.

The upsert methods should:

• take complete entities as argument
• if not exists: add them with the add methods
• if exists: create Update object and use update methods to update the entity

What do you think? @MikeRyanDev @brandonroberts @miniplus

[1] https://github.com/ngrx/platform/blob/master/modules/entity/src/sorted_state_adapter.ts#L125

[sandangel]

there was a long discussion about this: #421

[fmalcher]

Thanks for this reference! However, I dont really see a discussion about this topic there.
There was nobody who really cared about the side of "Upsert is an add that also updates and should tehrefore have the same API like add".
Seeing this way described in the docs and in Brandon's Medium article looks like adapter.upsertOne(entity, state) is far more intuitive than the other way.

Do you see a way to fix this issue here without revolutionizing the whole concept again?
I could live with adapter.upsertOne({ id: entity.id, changes: entity }, state), but overriding the id property seems to me to be an unwanted behavior.

[sandangel]

agree. I love the add signature for upsert too. Hope @ngrx team will accept a PR address this because this will be a breaking changes and they definitely don't want that

[fmalcher]

Decisions were made and I think, this has been discussed thoroughly. There seem to be reasons and I'm sure they're valid.

---

What remains is that upsertMany overrides the id property which is still a thing we need to talk about. How can we change this so that we can still:

• have entities with a primary key different from id
• upsert complete entities with { id: entity.myKey, changes: entity }
• upsert partial entities with { id: entity.myKey, changes: { foo: bar } } without having the id key set

This is the thing @dinvlad talked about in #421 (comment)

[MikeRyanDev]

In NgRx 6 will introduce a breaking change that updates the upsert methods to have a signature that looks like this:

upsertOne<T>(upsert: { id: string | number, entity: T }, state: EntityState<T>);
upsertMany<T>(upserts: { id: string | number, entity: T ][], state: EntityState<T>);

[fmalcher]

This is great news! How can we fix the id property issue until then?

[timdeschryver]

I'll provide a PR for the 6.x change.

@fmalcher I think that this is unsolvable right now (with my limited knowledge).

The only way of a 'kinda fix' that I can think of is the following - but this would require having the id in changes. (see my branch)

if (update.id in state.entities) {
  updated.push(update);
} else {
  // get the id value on the changes
  const id = selectId(update.changes);
  // find the key of the id, this can be wrong if multiple keys have the same value
  // for example {xxx: 123, isbn: 123, ...}
  const key = id ? Object.keys(update.changes).find(p => update.changes[p] === id) : null;
  if (key) {
    added.push({
      ...update.changes,
      [key]: update.id,
    });
  } else {
    added.push(update.changes);
  }
}

If you need the fix right now, I believe the only option would be to use a fork (with the fix) or to create an own adapter.

[goelinsights]

@MikeRyanDev appreciate the heads up on the breaking change.
Do you know if there's a good example of either updateOne or upsertOne as it relates to using this format in entities and then applying them with effects?

I'd previously been using the pattern of AddOne from the example app from the post request (which means using (result: Model) instead of (result: {model: Update<Model>}). Is there a template for making the added elements play well together in effects?

This is my current best guess for updating a photo object...anything you'd suggest to clean it up or make it future proof (v6)? (It looks like I would rename 'changes:' to 'entity:' )

@Effect()
  updatePhoto$ = this.actions$.ofType(PhotoActionTypes.UpdatePhoto)
    .pipe(
      map( (action: UpdatePhoto) => action.payload),
      switchMap(photo => {
        return this._photoService.updatePhoto(photo)
        .pipe(
          map( (results: Photo) => new UpdatePhotoSuccess({photo: {id: results.id, changes: results}})),
          catchError(err => of(new UpdatePhotoFail(err)))
        )
      })
    )

[brandonroberts]

Fixed via a0f45ff