search

1602 / jugglingdb

Multi-database ORM for nodejs: redis, mongodb, mysql, sqlite3, postgresql, arango, in-memory...
http://1602.github.io/jugglingdb/
2.05k stars 241 forks source link

Requiring ID to be Number causes issues #382

Closed jvskelton closed 8 years ago

jvskelton commented 10 years ago

in commit https://github.com/1602/jugglingdb/commit/77d65bb20ff4a305bfb49d990f59f77c23eaaaef

forcing the id to be a Number does fix querying an undefined id, but forces a conversion using anywhere model.find is used(in my our case anyway)

There are two concerns here, one, having to call parseInt on any request variable from the client because tedious, and lets face it, dirty. Two, there are instances where the id may not be a Number(for example it could be a guid).

It seems to me that in fixing the initial issue the DB architecture is now limited to using a number as a primary key.

One suggestion might be to provide an option to ignore this particular validation.

peralmq commented 10 years ago

:+1:

I use UUID and Postgres in my usecase for the id field.

The escapeId function (https://github.com/1602/jugglingdb/blob/97c326cb5f157894abf12755137b7d9d7e633d41/lib/sql.js#L45) and the queries using it need to support ids of other types than Number.

1602 commented 8 years ago

It is configurable per model and by default managed by adapter, so for example arango adapter may specify default type for id as a string or in you case you can just do it on model-level. By the way in mysql adapter we built in uuid as a setting, it could be a good idea to move it to meta-level (not database specific).