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

Uint8Array data (e.g. Postgres "bytea" type) erroneously considered "undefined" by recent 0.11.7 update. #1601

Merged
merged 2 commits into from Jul 31, 2016

Conversation

@mashaalmemon
Copy link
Contributor

@mashaalmemon mashaalmemon commented Jul 30, 2016

Hi There,

Currently using bookshelf in a project. Upon update to knex v0.11.7, noticed that an update was failing with error "Undefined binding(s) detected when compiling RAW query: ...".

I understand that any data field with value undefined will cause this type of error.

I looked into my code and found that as part of my query, no data to be bound was "undefined". Digging deeper I found that input values of type "Uint8Array" were failing the deep inspection test on data to be bound in helpers.js:

export function containsUndefined(mixed) {
  let argContainsUndefined = false;

  if(mixed && isFunction(mixed.toSQL)) {
    //Any QueryBuilder or Raw will automatically be validated during compile.
    return argContainsUndefined;
  }

  if(isArray(mixed)) {
    for(let i = 0; i < mixed.length; i++) {
      if(argContainsUndefined) break;
      argContainsUndefined = this.containsUndefined(mixed[i]);
    }
  } else if(isObject(mixed)) {
    for(const key in mixed) {
      if(argContainsUndefined) break;
      argContainsUndefined = this.containsUndefined(mixed[key]);
    }
  } else {
    argContainsUndefined = isUndefined(mixed);
  }

  return argContainsUndefined;
}

The Uint8Array was failing the check because it fails the lodash "isArray" check, and so is evaluated as an object; it has an internal property "parent" which was undefined. It should be noted that data returned from "pg" for Postgres "bytea" columns are passed back to knex as a "Uint8Array". In my use case, I was updating a record with data originally passed back from a previous query. Thus my value was not "undefined"

To fix this case, I propose that the lodash "isTypedArray" check also be used alongside the "isArray" check. Then a Uint8Array will be treated as an Array (with index 0-n and property length) thus not being considered "undefined" when it is not truely so:

export function containsUndefined(mixed) {

...
  if(isArray(mixed) || isTypedArray(mixed)) {
    ...

Please advise if this simple proposal is acceptable, if you have any additional feedback or approaches. I hope this change can be merged into the knex code base for the next release.

… such as Uint8Arrays are treated as "Arrays" vs as "Objects". For example, pgsql "bytea" type column data is passed back as Uint8Arrays by the node pgsql driver "pg" and without this modification, such data will fail the "containsUndefined" test.
@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented Jul 30, 2016

@mashaalmemon nice catch. Actually it doesn't appear possible for a typed array to contain undefined:

new Uint8Array([undefined]);
// -> [0]

So there's no need to check the elements if a typed array is provided. You can simplify this check to:

if (isTypedArray(mixed)) return false;
… such as Uint8Arrays are automatically treated as "not undefined" (vs as "Array" which will result in unecessary testing of Array contents). For example, pgsql "bytea" type column data is passed back as Uint8Arrays by the node pgsql driver "pg" and without this modification, such data will fail the "containsUndefined" test with previous implementation.
@mashaalmemon
Copy link
Contributor Author

@mashaalmemon mashaalmemon commented Jul 30, 2016

@rhys-vdw , thanks for the quick feedback. This will result in a much quicker "containsUndefined" check for typed arrays. I've made made adjustments to the PR as requested.

On a side note, when might this change make it to a release?

@rhys-vdw rhys-vdw merged commit 831ba2f into knex:master Jul 31, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented Jul 31, 2016

On a side note, when might this change make it to a release?

@mashaalmemon not sure. I'll put it on my to do list.

tgriesser added a commit that referenced this pull request Aug 9, 2016
* master:
  release 0.11.10
  Prep for release
  Add CHANGELOG.md (#1615)
  Added padding to avoid header sticking to the example usage.
  Updated docs to note schema builder is getter.
  Remove unused files
  Uint8Array data (e.g. Postgres "bytea" type) erroneously considered "undefined" by recent 0.11.7 update. (#1601)
  Revert "[docs] document multi-column order-by"
  [docs] document multi-column order-by
  MSSQL columnInfo() had hard coded schema name (#1585)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants