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

Private method _get() not processing parameters #22

Closed
kulakowka opened this issue Feb 29, 2016 · 12 comments
Closed

Private method _get() not processing parameters #22

kulakowka opened this issue Feb 29, 2016 · 12 comments
Assignees

Comments

@kulakowka
Copy link

In method .get() it is possible to pass parameters to the query.

get(id, params) {
  return this._get(id, params);
}

For example, so:

app.service('api/v1/users').get('kulakowka', {
  query: {
    $select: {'username': 1}
  }
})

But it does not work!

I found the cause. Private method _get() only takes one parameter id:

_get(id) {
  return this.Model.findById(id).then(instance => {
    if(!instance) {
      throw new errors.NotFound(`No record found for id '${id}'`);
    }

    return instance;
  })
  .catch(utils.errorHandler);
}

But it must take two parameters id and params.

@daffl
Copy link
Member

daffl commented Feb 29, 2016

What would params do? It doesn't look like Model.findById takes options like that.

@ekryski
Copy link
Member

ekryski commented Feb 29, 2016

@daffl I think we used to use params for some of the special params like $select specifically. However, you can easily do that with hooks too so we probably should just remove the params option from get. It shouldn't really be there.

@daffl
Copy link
Member

daffl commented Feb 29, 2016

Well what @kulakowka is saying that it is removed in _get but mainly because the model call inside does not use it. We could change it to a find adds the id and takes those params and returns only one item.

@ekryski
Copy link
Member

ekryski commented Feb 29, 2016

@daffl I don't think we should do that. it's over complicating things. Unless we have a case where you really need to pass params to a get, it should just take an id and we should remove the `params attribute on these lines: https://github.com/feathersjs/feathers-sequelize/blob/master/src/index.js#L79-L80

@kulakowka
Copy link
Author

I use it for populate creator when i get tutorial.

app.service('/api/v1/tutorials').after({
  get (hook) {
      const creator = hook.result.creator
      const userService = hook.app.service('api/v1/users')

      return userService.get(creator, {
        query: {
          $select: {'username': 1}
        }
      }).then((user) => {
        let result = hook.result.toJSON()
        result.creator = user
        hook.result = result
        return hook
      })
    }
})

I think that we should be able to specify parameters for the .get () method. for example in order to be able to specify which fields should be returned from the database.

@daffl
Copy link
Member

daffl commented Feb 29, 2016

I agree. We just need to make sure that this works the same for all database adapters. Basically the only relevant field would be $select right?

@kulakowka
Copy link
Author

@daffl yes. Other params do not have sense for get() method. Now the functionality is very limited. We can get the record by id and that's all. And what to do if you want to get the record where username === 'user1' OR 'user2'?

I have a situation where I need to get the record for two keys. Look this for example:

Project.findOne({platform: 'npm', name: 'feathers'})

Maybe you can redesign the interface for method .get()? I would like to be able to get a single record with some conditions, not only ID.

For example so:

app.service('projects').get({ platform: 'npm', name: 'feathers' })

app.service('todos').get({ id: 123 })

app.service('todos').get(123)

@kulakowka
Copy link
Author

Hmmm... maybe we can make it so:
When id param is number - we can use getById().
But when id param is object - we can use findOne().

@ekryski
Copy link
Member

ekryski commented Feb 29, 2016

@kulakowka in your case you could easily do a find with a $limit of 1. I think we're over thinking it here. Looking at mongoose (which is where this originated I think) we are using it for $populate. We're not even using it for $select.

I think .get should support params but only for the sake of $populate and $select along with an id. Otherwise you should be using .find().

@lopezjurip
Copy link

I'm not able to use sequelize.include from http://docs.feathersjs.com/help/faq.html#the-orm-way

@lopezjurip
Copy link

It's easy to fix: https://github.com/feathersjs/feathers-sequelize/blob/master/src/index.js#L58-L59

_get(id, params) {
    return this.Model.findById(id, params.sequelize).then(instance => { ...

@daffl
Copy link
Member

daffl commented Jun 10, 2016

I published a fix for @mrpatiwi's issue. The other issue discussed can now be solved by adding a before hook for get:

app.service('myservice').before({
  get(hook) {
    if(typeof hook.id === 'object') {
      return this.find(hook.id).then(result => {
        const data = result.data || result;

        hook.result = data[0];
        return hook;
      });
    }
  }
});

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