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

Projects
None yet
2 participants
@mashaalmemon
Contributor

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.

mashaal
Made modification so that attributes with data stored in typed arrays…
… 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 rhys-vdw added bug PR please and removed PR please labels Jul 30, 2016

@rhys-vdw

This comment has been minimized.

Collaborator

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;
mashaal
Made modification so that attributes with data stored in typed arrays…
… 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

This comment has been minimized.

Contributor

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 tgriesser:master Jul 31, 2016

1 check passed

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

This comment has been minimized.

Collaborator

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

Merge branch 'master' into gh-pages
* 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