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

option.id ignored in _get #58

Closed
robotnic opened this issue Feb 20, 2016 · 10 comments
Closed

option.id ignored in _get #58

robotnic opened this issue Feb 20, 2016 · 10 comments

Comments

@robotnic
Copy link

Here is a query to _id even if this.id is set to an other parameter.

https://github.com/feathersjs/feathers-mongoose/blob/58020126684aa06ab12c653d9acfdd372c147254/src/service.js#L97

Desired functionality:
Use the id specified in option.id

@marshallswain
Copy link
Member

It seems like that shouldn't even be an option. Mongo DB doesn't allow you to use a different ID property. http://stackoverflow.com/questions/15854693/mongodb-how-to-set-another-field-different-from-id-as-id-of-mongo-document

@daffl
Copy link
Member

daffl commented Feb 20, 2016

I already kicked this out of NeDB (see feathersjs-ecosystem/feathers-nedb#11). We should remove it here as well and take it out of the docs.

@ekryski
Copy link
Member

ekryski commented Feb 20, 2016

@marshallswain you are right mongo does require each doc to have an _id field but you can obviously use another attribute as the "id". I agree though that this should be removed like NeDB.

@ekryski
Copy link
Member

ekryski commented Feb 20, 2016

So... looking at the code. I don't think we should actually remove this option. If someone wants to index a certain attribute and use that as their primary id they should be able to. It's not a very common case but I could see that happening. Say if you wanted to look people up by a slug, uuid, or incremental id.

In fact, in order to stay database agnostic for your data model, it's actually a good practice to assign uuid's to all of your records rather than use the default database id.

So I'm just going to fix it so that the queries use the options.id attribute.

ekryski added a commit that referenced this issue Feb 20, 2016
get should use the options.id attribute. Closes #58
@robotnic
Copy link
Author

Thanks for quick response and bugfix!

Now I have one more problem. It's not a bug, but I don't know how to handle this.

I'm working with the "ep_votes.json.xy" file from
http://parltrack.euwiki.org/dumps/

The file contains votes of EU Parliament Members.
I wan't to find all votings a MEP participated.

The query doesn't look that difficult:
?For.groups.votes.ep_id=124935

BUT it doesn't work, because the "ep_id" is a number in the json file.
The query doesn't match and now I'm searching for a solution for this problem.

@marshallswain
Copy link
Member

@robotnic you can create a before hook to access hook.params.query.ep_id and change it to the type you want.

@daffl
Copy link
Member

daffl commented Feb 27, 2016

Looks like @marshallswain beat me to it. This is how it could look:

const hook = require('feathers-hooks');

app.configure(hooks());

app.service('members').before({
  find(hook) {
    const epId = hook.params.groups.votes.ep_id;
    if(epId) {
      hook.params.groups.votes.ep_id = parseInt(epId, 10);
    }
  }
});

@ekryski
Copy link
Member

ekryski commented Feb 27, 2016

lol I was about to respond now too but @marshallswain and @daffl beat me to it. Nice work team! 😄

There is a chance that you might need to convert your query to a nested object instead of using the dot notation. Report back if the dot notation works. I haven't tried it yet.

@robotnic
Copy link
Author

That syntax works

let myHook = function(options) {
    return function(hook) {
      console.log('My custom hook ran!',hook.params.query['For.groups.votes.ep_id']);
         const epId = hook.params.query['For.groups.votes.ep_id']
            if(epId) {
              hook.params.query['For.groups.votes.ep_id'] = parseInt(epId, 10);
            }
    }
  }

@ekryski
Copy link
Member

ekryski commented Feb 27, 2016

👍. Thanks @robotnic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants