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

raw: false, does not work with with Buzzard #184

Closed
musicformellons opened this issue Dec 28, 2017 · 6 comments
Closed

raw: false, does not work with with Buzzard #184

musicformellons opened this issue Dec 28, 2017 · 6 comments

Comments

@musicformellons
Copy link

musicformellons commented Dec 28, 2017

This hook on my server:

module.exports = function (options = {}) { // eslint-disable-line no-unused-vars
  return function (hook) {
    if (hook.params.query && hook.params.query.include) {

      const joinModel = hook.app.service('favorites_points').Model;
      const pointsModel = hook.app.service('points').Model;

      hook.params.sequelize = {
        raw: false,
        include: [{
          model: joinModel,
          as: 'FavPoints',
          include: [{
            model: pointsModel
          }]
        }]
      };

      delete hook.params.query.include;
    }

When called from client with (I am using websockets):
api.service('users').get(2, {query: {include: true}})
Gives this error (btw: when I leave out the inner include: [{ model: pointsModel }], same error remains):

index.js?3d97:235 Uncaught (in promise) Error: Maximum call stack size exceeded
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:28:20)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:35:11)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:56:59)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:56:59)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:56:59)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:56:59)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:56:59)
    at hasBinary (/home/usr/api_server/node_modules/has-binary2/index.js:56:59)
convert @ index.js?3d97:235
(anonymous) @ client.js?8dbe:71
Socket.onack @ socket.js?0183:312
Socket.onpacket @ socket.js?0183:236
(anonymous) @ index.js?cea2:21
Emitter.emit @ index.js?a675:133
Manager.ondecoded @ manager.js?0ad8:332
(anonymous) @ index.js?cea2:21
Emitter.emit @ index.js?a675:133
Decoder.add @ index.js?5f3d:241
Manager.ondata @ manager.js?0ad8:322
(anonymous) @ index.js?cea2:21
Emitter.emit @ index.js?a675:133
Socket.onPacket @ socket.js?5eac:456
(anonymous) @ socket.js?5eac:273
Emitter.emit @ index.js?a675:133
Transport.onPacket @ transport.js?64e8:145
Transport.onData @ transport.js?64e8:137
ws.onmessage @ websocket.js?7304:147
Promise rejected (async)
queryMe @ Nearby.vue?9ccf:48
boundFn @ vue.runtime.esm.js?ff9b:189
click @ Nearby.vue?dc3e:39
invoker @ vue.runtime.esm.js?ff9b:1979
Vue.$emit @ vue.runtime.esm.js?ff9b:2489
click @ quasar.esm.js?8bfb:3118
boundFn @ vue.runtime.esm.js?ff9b:188
invoker @ vue.runtime.esm.js?ff9b:1979
fn._withTask.fn._withTask @ vue.runtime.esm.js?ff9b:1777

When I comment out raw: false, it gives proper output.

This hook worked fine with raw: false, before upgrading to feathers Buzzard (with matching assistance libraries). My current package.json for the server has these:

    "@feathersjs/authentication": "^2.1.0",
    "@feathersjs/authentication-jwt": "^1.0.1",
    "@feathersjs/authentication-local": "^1.0.2",
    "@feathersjs/authentication-oauth2": "^1.0.2",
    "@feathersjs/configuration": "^1.0.1",
    "@feathersjs/errors": "^3.2.0",
    "@feathersjs/express": "^1.1.2",
    "@feathersjs/feathers": "https://github.com/feathersjs/feathers.git#master",
    "@feathersjs/socketio": "^3.0.1",
    "feathers-authentication-hooks": "^0.1.5",
    "feathers-hooks-common": "git://github.com/feathers-plus/feathers-hooks-common.git#master",
    "feathers-sequelize": "^3.0.0",
    "sequelize": "^4.28.6",

What is going on? Why does my query suddenly work without raw: false, ??

@musicformellons
Copy link
Author

musicformellons commented Dec 28, 2017

I checked, but in the 'before upgrade version' the query also works when I leave raw: false, out, however the output-structure is different (more nested) when its added, so makes me think that sequelize is simply circumvented and probably there is no eager-loading etc. Can you confirm?

So my question now is: why does it not work anymore with raw: false, ?

Btw, the package.json of the 'before upgrade version':

  "feathers": "^2.2.3",
  "feathers-authentication": "^1.3.0",
  "feathers-authentication-hooks": "^0.1.5",
  "feathers-authentication-jwt": "^0.3.2",
  "feathers-authentication-local": "^0.4.4",
  "feathers-authentication-oauth2": "^0.3.2",
  "feathers-blob": "^1.3.1",
  "feathers-configuration": "^0.4.2",
  "feathers-errors": "^2.9.2",
  "feathers-hooks": "^2.1.2",
  "feathers-hooks-common": "^3.10.0",
  "feathers-rest": "^1.8.1",
  "feathers-seeder": "^1.0.10",
  "feathers-sequelize": "^2.4.0",
  "feathers-socketio": "^2.0.1",
  "sequelize": "^4.23.1",

@musicformellons musicformellons changed the title raw: false, not necessary with Buzzard?! raw: false, does not work with with Buzzard Dec 28, 2017
@musicformellons
Copy link
Author

After adding a dehydrate hook (as an after hook) the output is as expected. Not clear to me why before upgrading this was not an issue...

@daffl
Copy link
Member

daffl commented Dec 29, 2017

I don't think anything in that regard should have changed, I'm wondering why that makes a difference. @DesignByOnyx did you try to upgrade already and maybe ran into this issue?

@musicformellons
Copy link
Author

Before upgrade I had these hooks:

const { authenticate } = require('feathers-authentication').hooks;
const commonHooks = require('feathers-hooks-common');
const { restrictToOwner } = require('feathers-authentication-hooks');
const { hashPassword } = require('feathers-authentication-local').hooks;
const addRelationship = require('./hooks/addRelationship');

const restrict = [
  authenticate('jwt'),
  restrictToOwner({
    idField: 'user_id',
    ownerField: 'user_id'
  })
];

module.exports = {
  before: {
    all: [addRelationship()],
    // all: [],
    find: [ authenticate('jwt'), ...restrict ],
    // get: [ ...restrict ],
    get: [ ],
    create: [ hashPassword() ],
    update: [ ...restrict, hashPassword() ],
    patch: [ ...restrict, hashPassword() ],
    remove: [ ...restrict ]
  },

  after: {
    all: [
      commonHooks.when(
        hook => hook.params.provider,
        commonHooks.discard('password')
      )
    ],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};

After upgrade:

const { authenticate } = require('@feathersjs/authentication').hooks;
const { hashPassword, protect } = require('@feathersjs/authentication-local').hooks;
// const hydrate = require('feathers-sequelize/hooks/hydrate');
const dehydrate = require('feathers-sequelize/hooks/dehydrate');
const addRelationship = require('./hooks/addRelationship');

const restrict = [
  authenticate('jwt')
];

module.exports = {
  before: {
    all: [],
    find: [ ...restrict, addRelationship()],
    get: [ ...restrict, addRelationship()],
    create: [ hashPassword()],
    update: [ ...restrict, hashPassword(), addRelationship()],
    patch: [ ...restrict, hashPassword(), addRelationship()],
    remove: [ ...restrict, addRelationship()]
  },

  after: {
    all: [
      dehydrate(),

      protect('password')
    ],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};

So protect ?

@daffl
Copy link
Member

daffl commented Dec 29, 2017

Probably. It expects a plain object and makes a shallow copy (via Object.assign). I think it should check for a toJSON first. I created a follow up issue in feathersjs-ecosystem/authentication-local#48

@DesignByOnyx
Copy link
Contributor

DesignByOnyx commented Feb 5, 2018

Just wanted to leave a comment here for some closure. By default, sequelize defaults to raw: true. This was a controversial decision, but the reason behind the decision was so that feathers-sequelize could interoperate with other hooks, especially feathers-hooks-commom. As soon as you set raw: false, feathers-sequelize should not work with common hooks.

You say that using raw: false did work before the upgrade, but if it did it was merely a fluke and you likely found an edge case where it worked. As it stands, here are the general rules with regard to feathers-sequelize.

  • For simple service calls (without associations, raw: true by default), everything should work out of the box, even with 3rd party (common) hooks.
  • When eager loading (using the include option), you should set raw: false. This will give you properly nested data (this is a shortcoming of sequelize).
  • If you ever use raw: false AND want to use 3rd party (common) hooks, you MUST deydrate() the data first as 3rd party hooks cannot work with sequelize instances.

Hope that helps anybody else coming here. If you still have questions, come to the "sequelize" room in Feather's Slack.

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

3 participants