scottwrobinson / camo

A class-based ES6 ODM for Mongo-like databases.
556 stars 80 forks source link

Inconsistent aliasing of `id` #20

Closed ijsf closed 8 years ago

ijsf commented 8 years ago

There are some inconsistencies with the _id to id field aliasing.

When I perform a load call (e.g. loadMany with {} empty query), camo returns objects with the _id field aliased to id:

{
  ...
  "id": "5669e44f5d531cbd00e7d106"
}

However, when specifying an actual input query to these load calls, I am unable to specify the id field, and instead have to specify MongoDB's original _id field, as using the id field yields no results at all:

MyModel.loadMany({
  "id": "5669e44f5d531cbd00e7d106"
});

In other words, for queries, the id field is not aliased back to _id, which is inconsistent with the load call result behavior, and requires the use of additional workarounds in the application using camo.

b22n commented 8 years ago

+1

scottwrobinson commented 8 years ago

Thanks for bringing this up!

I'll probably drop the id alias and just use _id for everything. Should be able to get out a fix tomorrow.

Scott

scottwrobinson commented 8 years ago

Ok so now I remember why I did this. You should be able to access the ID via document.id (the alias) and document._id (the original property). So then you can both access and query the ID with _id.

The alias .id is just supplemental and was added because Mongoose had it. I thought it would be convenient to have when switching ODMs.

Thoughts? Still think we should remove the .id alias?

Scott

ijsf commented 8 years ago

I thought it was a good addition at first, but it has to be consistent all the way through. Perhaps it will be better to remove it in favor of consistency with MongoDB.

Though, I'm not completely sure.

scottwrobinson commented 8 years ago

I've deprecated the id alias, and will fully remove it in the next major version, so I'm closing the issue.

Thanks again!