sync _removed doesn't map ID to VARCHAR(32) as usual

[robshep]

My system supplies IDs in the javascript number type. It appears that persistence.js always maps an object's ID into VARCHAR(32) (when using SQL store at least)

persistence.sync.js doesn't remap the IDs when a '_removed' entity is encounted and attempts to use the raw encountered datatype

For example.

DELETE FROM `Entity` WHERE `id` IN ( ?,?,? ), [ 1,2,3 ]

instead of

DELETE FROM `Entity` WHERE `id` IN (?,?,?), [ "1", "2", "3" ]

Naturally, one would want to ensure the server-side data type was in the correct format, but I just got stung when persistence silently used the String type. I have a patch to prevent other people getting bitten by deletes silently failing, due to this caveat.

[miklschmidt]

This commit broke the sync adapter. group is undefined. Furthermore, existingItems.concat returns the concat'ed array it doesn't modify the one given. So it should be existingItems = existingItems.concat.... The current code doesn't work. Let me know if you want a PR to fix it.

[romlel]

Hello. i encountered this error too and I am wondering what is the best way to move on. For now, I commented out the 4 lines after /* ensure IDs var chars */, but I would like to get something more robust for heavy use later.

I'm new to this lib and don't want to make a mess in the code. Is the 115 fix important, and if so, how would it be possible to integrate it properly ?

Brest regards.

[AgDude]

Could someone please clarify what we are trying to accomplish with this block:

/* ensure IDs var chars */
var idVals = new Array();
for(var id=0;id<group.length;id++){
  idVals.push( persistence.typeMapper.entityIdToDbId(group[id]));
}

I understand the entityIdToDbId function, but this it does not appear to present in all backends. It also does not appear that idVals is ever referenced after being created here.

I am going to take the suggestion from @romlel and remove those four lines.

I suggest the "fix" earlier be removed entirely. I am not convinced that the initial bug presented here is a bug. PersistenceJS is designed around an id property which is a uuid, making an "int" id an non-typical use case.

[robshep]

It may not be a bug if Persistence.js is only designed to support self-generated UUID identifers

If the server-provided dataset has any other data type as an "id" fields then the system should hard fail.

As it happened - at the time of first reporting - everything worked apart from deleting, which was silently uneffective.

better docs will make this not a bug :)

Cheers.