Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent aliasing of id #20

Closed
ijsf opened this issue Dec 10, 2015 · 5 comments
Closed

Inconsistent aliasing of id #20

ijsf opened this issue Dec 10, 2015 · 5 comments
Labels

Comments

@ijsf
Copy link

ijsf commented Dec 10, 2015

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
Copy link

b22n commented Dec 11, 2015

+1

@scottwrobinson
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

ijsf commented Dec 28, 2015

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
Copy link
Owner

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

Thanks again!

bvkimball pushed a commit to bvkimball/crypsis that referenced this issue Jan 15, 2016
* upstream/master: (41 commits)
  Bumped version, updated CHANGELOG, and updated README
  Fixed serialization test for MongoDB IDs
  Deprecated 'id' alias on document object for issue scottwrobinson#20
  Removing some inconsistencies with accessing the ID. Partially covers issue scottwrobinson#20
  Consolidated hook code so it can be re-used throughout Document methods
  Moved collectionName method to BaseDocument. Fixes scottwrobinson#26
  Added new ValidationError object, fixed some validation tests, fixed min/max validation, and fixed validation for array of embedded documents
  Fixed issue with running 'canonicalize' tests on travis-ci and bumped version
  Removed unused harmony-reflect dependency, updated CHANGELOG, and bumped version
  Updated CHANGELOG, README, and bumped version
  Removed --harmony-proxies flag from tests now that Proxy isn't used
  Added sanity check to 'required' tests
  Updated PR scottwrobinson#19 to conform to new collection naming
  Changed how you name collections
  Removed need for Proxy and harmony-reflect dependency
  Added a required key value validation.
  Adding .npmignore file
  Moved jshint settings out of package.json and in to .jshintrc file. Updated .gitignore for .jshintrc
  Removed custom harmony-reflect script and adding harmony-reflect as dependency
  Added tests for canonicalizing dates
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants