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

Options param support for create method #26

Merged
merged 1 commit into from
Mar 9, 2016

Conversation

rafalszemraj
Copy link
Contributor

Fix for #25

Options param support for create method
@ekryski
Copy link
Member

ekryski commented Mar 9, 2016

Thanks @rafalszemraj. Ideally we'd want to test for this. Since this is a such a benign change I'll add it and we can go back an add a specific test for passing options to Sequelize.

ekryski added a commit that referenced this pull request Mar 9, 2016
Options param support for create method
@ekryski ekryski merged commit 056ffeb into feathersjs-ecosystem:master Mar 9, 2016
@ekryski
Copy link
Member

ekryski commented Mar 9, 2016

I've published v1.1.3 that contains this fix.

@daffl
Copy link
Member

daffl commented Mar 9, 2016

I'm not sure if this is the best fix.There is a chance that you can mess directly with the database by passing arbitrary query parameters from the client.

Instead of using params directly it should probably use params.sequelize to set those options.

@rafalszemraj
Copy link
Contributor Author

@daffl Probably you're right. The reason I wanted to have this is to be able to create records with n:m association . I'm just starting with feathers and I can see that relation model form sequelize is not yet provided (maybe you do not have any plans to do so?). Anyway, starting this with change like this it's not best idea I guess - whole thing needs to be planned beforehand. I can leave without that, you can revert that if you do not feel this is right way to do it.

@ekryski
Copy link
Member

ekryski commented Mar 9, 2016

Ah shit! I missed that. Good catch @daffl. I think we should roll this change back then. Looks like we are missing documentation on the params.sequelize object and we really need an example of showing many-to-many relationships.

@daffl
Copy link
Member

daffl commented Mar 9, 2016

No need to roll it back, this is a good fix. We just need to change https://github.com/feathersjs/feathers-sequelize/blob/master/src/index.js#L83-L89 to

  create(data, params) {
    const options = params.sequelize || {};
    if (Array.isArray(data)) {
      return this.Model.bulkCreate(data, options).catch(utils.errorHandler);
    }

    return this.Model.create(data, options).catch(utils.errorHandler);
  }

Then one can set params.sequelize to whatever it needs to be e.g. in a before hook.

@ekryski
Copy link
Member

ekryski commented Mar 9, 2016

Ah ok cool. I'll fix that right away and do a new patch release.

@daffl
Copy link
Member

daffl commented Mar 10, 2016

@rafalszemraj I hope that works for you as well and many thanks again for the PR!

@rafalszemraj
Copy link
Contributor Author

👍

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

Successfully merging this pull request may close these issues.

3 participants