balderdashy / waterline-sequel

A SQL generator for use in Waterline Adapters
MIT License
16 stars 61 forks source link

bad query should probably throw exception rather than generate invalid sequel #86

Closed kevinburkeshyp closed 8 years ago

kevinburkeshyp commented 8 years ago

We wrote a query that ended up sending the following data as part of a .find()

  it('generates a query without the word "undefined"', function() {
    var sequel = new Sequel(userSchema, {});
    var val = sequel.find('users', { balance: { 'in': [ 1, 2 ] } });
    console.log(val);
  });

Note this works: sequel.find('users', { balance: [ 1, 2 ] }). The user schema is

var userSchema = {
  users: {
    identity: 'users',
    tableName: 'users',
    attributes: {
      id: {
        primaryKey: true,
        type: 'text',
      },
      email: {
        type: 'text',
      },
      balance: {
        type: 'integer',
        required: true,
      },
    },
  }
};

This triggers the CriteriaProcessor.prototype.and branch (instead of the _in branch) and generates the following SQL:

{ query: [ 'SELECT "users"."id", "users"."email", "users"."balance", "users"."pickupCount" FROM "users" AS "users"  "users"."balance" undefined  ' ],
  values: [ [] ] }

I think that should probably throw there (or check for a 'in' property) instead of generating invalid SQL. I'm also not sure where the undefined is coming from, I don't think it's exploitable.

sailsbot commented 8 years ago

@kevinburkeshyp Thanks for posting, we'll take a look as soon as possible. In the meantime, if you haven’t already, please carefully read the issue contribution guidelines and double-check for any missing information above. In particular, please ensure that this issue is about a stability or performance bug with a documented feature; and make sure you’ve included detailed instructions on how to reproduce the bug from a clean install. Finally, don’t forget to include the version of Node.js you tested with, as well as your version of Sails or Waterline, and of any relevant standalone adapters/generators/hooks.

Thank you!

kevinburkeshyp commented 8 years ago

@sailsbot this should be pretty easily reproducible!

kevinburkeshyp commented 8 years ago

I believe this is happening because there's no default case in prepareCriterion, at the top of the function var str is defined and then this can get appended to the string even if it's still undefined

kevinburkeshyp commented 8 years ago

^^ Our fix is available at the link. I'm not sure if that gets converted into a WLValidationError, probably depends on the adapter.

particlebanana commented 8 years ago

@kevinburkeshyp great! Would you mind submitting a PR with that commit?