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

Only request returning attribute if client supports returning #1770

Merged
merged 3 commits into from Feb 21, 2018

Conversation

Projects
2 participants
@larryprice
Contributor

larryprice commented Feb 21, 2018

Introduction

knex 0.14.4 has been updated such that a warning is logged when using the returning attribute in a SQL dialect that does not support it. bookshelf always requests a returning attribute during an insert, which causes a flood of unwarranted warning messages whenever sqlite3 is used, and there is no way to tell bookshelf not to specify returning.

Motivation

When running my tests, I see warning messages like this flood my logs every time I call an insert:

console.log node_modules/knex/lib/helpers.js:90
  Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.
console.log node_modules/knex/lib/helpers.js:90
  Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.
console.log node_modules/knex/lib/helpers.js:90
  Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.

Proposed solution

My solution is to check if the current client supports returning and request the idAttribute if and only if the support is available.

Current PR Issues

N/A

Alternatives considered

Also considered updating the client within knex to be able to do this logic. However, due to the fact that this change was implemented on the knex side, I have to assume that this is intended behavior.

@ricardograca

Thanks for this, but the test suite must pass before this gets merged.

@ricardograca ricardograca added the change label Feb 21, 2018

@ricardograca ricardograca added this to To Do in Version 0.13.0 via automation Feb 21, 2018

@larryprice

This comment has been minimized.

Contributor

larryprice commented Feb 21, 2018

@ricardograca Thanks, I've updated the PR to pass the tests.

@ricardograca

This is something that probably should be abstracted away in knex, but for now we'll handle it.

@ricardograca ricardograca merged commit c08fdf4 into bookshelf:master Feb 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Version 0.13.0 automation moved this from To Do to Done Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment