Entity: adapter.addOrUpdate

[ghost]

I'm submitting a...

[ ] Regression
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request

What is the current behavior?

Sometimes when receiving a full entity from the server, you don't know if it already exists. At the moment you need to check for that manually and choose to update instead of add.

    case user.ADD_USER: {
      const changes = action.payload.user;
      const id = changes.id;

      if (state.ids.indexOf(id) > -1) {
        return adapter.updateOne({ id, changes }, state);
      }
      
      return adapter.addOne(changes, state);
    }

Expected behavior:

The adapter provides a method that adds the entity when it does not exists, but merges (or replaces as it's not a partial entity) the entity into the existing one when it does exist.

    case user.ADD_USER: {
      return adapter.addOrUpdateOne(action.payload.user, state);
    }

    case user.ADD_USERS: {
      return adapter.addOrUpdateMany(action.payload.users, state);
    }

[Silthus]

This should be added for many entities as well: addOrUpdateMany.

[karptonite]

This is called "Upsert" in some databases. There could also be call for "Replace", which would Add or replace rather than than Add or merge; this would be important if, for example, an entity with an optional field has that field unset (and not returned by the server).

[dinvlad]

+1, though another possible workaround is to simply compose the reductions:

case user.ADD_OR_UPDATE: {
  const changes = action.payload.user;
  const id = changes.id;
  state = adapter.addOne({ id, changes }, state); // safe, either create or do nothing
  return adapter.updateOne(changes, state); // also safe, now it exists, we apply a (possibly dummy) update
}

Obviously, a more native check would avoid additional calculations.

[brandonroberts]

Adding upsertOne and upsertMany seems reasonable.

[dinvlad]

Btw, in

platform/modules/entity/src/unsorted_state_adapter.ts

Lines 71 to 90 in aa7172a

	function updateOneMutably(update: Update<T>, state: R): boolean {
	const index = state.ids.indexOf(update.id);
	if (index === -1) {
	return false;
	}
	const original = state.entities[update.id];
	const updated: T = Object.assign({}, original, update.changes);
	const newKey = selectId(updated);
	if (newKey !== update.id) {
	state.ids[index] = newKey;
	delete state.entities[update.id];
	}
	state.entities[newKey] = updated;
	return true;
	}

why do we call computationally expensive indexOf when index only has to be used if the id has changed (and the check for index === -1 can be easily replaced with !(update.id in state.entities))? It may be wise to move it inside if (newKey !== update.id) { to avoid possible quadratic time penalty when calling updateOneMutably from updateManyMutably.

Same goes for @jobvanstiphout's comment above - if we are to implement upsertOne, we need to make sure not to unnecessarily call indexOf.

[dinvlad]

Also, calling updateOneMutably from updateManyMutably, with an internal indexOf in

platform/modules/entity/src/sorted_state_adapter.ts

Line 81 in aa7172a

const index = state.ids.indexOf(update.id);

leads to quadratic time on average. But we could do an N log N by sorting the "new" array and then copy-merging it with the "old" one (which is already sorted).

[MikeRyanDev]

@dinvlad I will gladly accept PRs that improve perf 👍

[dinvlad]

sounds good, I can take a look into it

[dinvlad]

I've updated methods to improve performance of collection operations. Let me know if you'd consider a PR for upserts too.

[brandonroberts]

Sure

[progral]

I would suggest "upsert" instead of "addOrUpdate". First many db systems do so and seconds it sounds better to say "upsertMany" instead of "addOrUpdateMany". It is definitly a must have for the adapter!

[rupeshtiwari]

I agree with @xipheCom .

[dinvlad]

Sorry got a bit distracted, can draft it over the weekend

[progral]

Any progress @dinvlad ?

[dinvlad]

Submitted a PR. Sorry for the delay.

I kept the signature for upsert() methods identical to that of update(). Otherwise (i.e. for the signature of add()), we get a type error for the id because string | number determined by selectId is not assignable to Update id for some reason, even though Update id is defined for both string and number. I think we can fix that, if you prefer the signature of add().

[dinvlad]

More specifically,

Type '{ id: string | number; changes: any; }' is not assignable to type 'Update<T>'.
    Type '{ id: string | number; changes: any; }' is not assignable to type 'UpdateNum<T>'.
      Types of property 'id' are incompatible.
        Type 'string | number' is not assignable to type 'number'.
          Type 'string' is not assignable to type 'number'. (2345)

So to use add() signature for upserts, we need to change the signature of Update to

export type Update<T> = {
  id: string | number;
  changes: Partial<T>;
};

[dinvlad]

But then we won't be able to change id with upsert(), so it's a double-edged sword...

[progral]

we should use the signature of Update for Upsert. If we think in a Database System update and upsert have both a query. They are very similar. An Insert (or Add) function has another task.

Add: Add something
Update: Update something with some condition
Upsert: Update something with some condition, in case it does not exist add it.

I think you have done it right! Any idea of the team when the PR will be accepted?

[georgeF105]

Nice work on the PR @dinvlad. One issue I can see with keeping the Upsert() type the same is Update is that this will mean partial objects can be pushed to the cache. In a project I am working on we created an Upsert() type function with a signature similar to update but without the partial:

export type FullUpdate<T> = {
  id: string | number;
  changes: T:
}

Changing the signature to be the same as Add would a good alternative.

[dinvlad]

Hmm, one thing I've just realized is that the current PR (and test fixtures) don't take into account the situation when selectId is different from entity => entity.id. So in the current method for insertion, I just assign the id of the inserted entity to be update.id:

platform/modules/entity/src/unsorted_state_adapter.ts

Lines 130 to 133 in 9826793

	added.push({
	...update.changes,
	id: update.id,
	});

But if the effective id is named differently (e.g. selectId = entity => entity.title), then this simple approach will fail.

[dinvlad]

Or as @georgeF105 noted, we can use

function upsertManyMutably(updates: FullUpdate<T>[], state: R): boolean;
...
if (update.id in state.entities) {
  updated.push(update);
} else {
  // now each change must be a full entity
  // containing the new effective id.
  added.push(update.changes);
}

That will enable complete replacement of an entity, including its effective id, and will prevent addition of partial entities.

[mcblum]

@dinvlad Thank you for your work on this. I'm about to refactor a pretty giant redux mess and I'm wondering if a merge is on the horizon? Your point about needing to respect the defined selectorId is good, though, we're using that a lot to make it not the entity.id prop.