updateEntitiesIds weird behavior

[strigefleur]

Which @ngneat/elf-* package(s) are the source of the bug?

entities

Is this a regression?

No

Description

If you try to feed updateEntitiesIds with non-existing (or just negative, idk) id it would update entities collection by adding undefined: {} object and nothing to ids collection.

I expect it to ignore update if the id provided doesn't exist. In my case it looked like

updateEntitiesIds(-1, model.id)

where I wanted to update temporary entity (if there's any) with real id.

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in

No response

Anything else?

No response

Do you want to create a pull request?

No

[NetanelBasal]

@DmitryEfimenko

[DmitryEfimenko]

We have a guard in place that assures that the new entity id does not already exist.

It would make sense to add a guard that assures that the entity id you want to update actually exists. Following the logic above, it could throw an error if the entity id does not exist to make the develop explicitly aware of a potential issue in the app.

However, I'd like to hear more from you about a potential use-case to ignore the non-existing entity rather than to throw. @strigefleur , could you please describe your use-case?

[NetanelBasal]

I wonder how you have a case where you pass an id that doesn't exist.

[strigefleur]

The entity is created on the frontend, whereas the real entity id is database-side step. So until the backend process the entity I have -1 as temp id. Backend is requested with a complex object that may or may not contain that kind of temp entity, it responds with another complex object that always contains related entity with it's real id.

So I was going to shorten the code and reassign the id in any case - skipping the explicit check for it's existence in the store: if the id was -1 it's updated to real, if it was real, nothing changes.

[DmitryEfimenko]

do you mind showing some pseudo-code?

[strigefleur]

// object with temporary id
interface Foo {
   id: number;
   // rest
}

// update object that is sent to backend
interface FooBarUpdateRequest {
   foo?: Foo;
   // rest
}

// response object that backend responses with
interface FooBarUpdateResponse {
   foo: Foo;
   // rest
}

// we create some entity with temp id
addNewFoo() {
   store.update({
      addEntities({
         id: -1,
         somedata: '',
      })
   })
}

// some service logic here
// foo is sent to backend only if it's temporary
const hasTempFoo = store.pipe... entities.some(x => x.id === -1)...
const request: FooBarUpdateRequest = {
   foo: hasTempFoo ? foo : undefined;
   // rest
};
http.post<FooBarUpdateResponse>('ourawesomeurl', request).pipe(
   tap((update) => {
      store.update(
          // rest
          // here case 1 is when temporary id exists, we update it to new id
          // case 2 is when no temporary id exists - it means the store already has same id inside entities
          updateEntitiesIds(-1, update.foo.id)
      )
   })
)

[strigefleur]

Currently I workaround it by simply doing full check before update:

const storeHasTempId = store.pipe... entities.some(x => x.id === -1)...
if (storeHasTempId) {
   store.. updateEntitiesIds(-1, update.foo.id)
} else {
  //...
}

so it's up to you to decide what is the correct behavior in that case. Anyway it should not create undefined: {}.

[NetanelBasal]

We can ignore them, not throw an error

[DmitryEfimenko]

Errors are an effective way to communicate to the developer about unexpected behavior. This actually made me look at the current implementation of the updateEntities and I see that it would throw an error if you're trying to update an entity by an id that does not exist like so:

const idThatDoesNotExist = 5;
store.update(updateEntities(idThatDoesNotExist, entity => ({ ...entity, name: 'name' })));

And it is a good thing that it would throw an error. The developer would see it and would know what the issue is. It's much worse if the Error is not thrown... a potential bug is then introduced in the application and the developer is not aware of it.

Case in point, take a look what's going to happen in the following code

const idThatDoesNotExist = 5;
store.update(updateEntities(idThatDoesNotExist, { name: 'name' }));

Even though there's no entity with id 5 and there's nothing to update, the Elf would insert a new entity to the store: { id: 5, name: 'name' }. However, it would not add the new id to the list of ids. The store state would be messed up.

I'd argue that we should throw an error in both cases, the updateEntitiesIds and updateEntities when the store does not have an expected id.

[NetanelBasal]

I have no idea how these cases arise. I've never experienced it. There is no need to protect each line of code in the library.

[DmitryEfimenko]

In my experience, these situations are rare now. However, when I was starting out as a developer they would be more often. Mostly it's a result of some sort of a race condition.

Imagine calling the functionality to addItem which makes requests to the server multiple times quickly. If handled properly, there would be no issue, but if asyncroneous nature is not taken into account and id -1 is used like in the example above, it would certainly be a bug.

I'd be curious to hear opinions from the authors of libraries out there. I'll try soliciting feedback on Twitter. Although I don't have a big following there.