Add order keywords whitelist #2919
Conversation
|
@fixe if 2nd argument is empty default to ASC probably? (and then revert tests) |
|
As for unit testing the query generator, #2690 |
| if (subQuery && (Array.isArray(t) && !(t[0] instanceof Model) && !(t[0].model instanceof Model))) { | ||
| subQueryOrder.push(this.quote(t, model)); | ||
| } | ||
| mainQueryOrder.push(this.quote(t, model)); | ||
| }.bind(this)); | ||
| } else { | ||
| if (Array.isArray(options.order)) { |
There was a problem hiding this comment.
This condition will never be true. When the order is sent as a string, we'll probably need to parse it and look for the ASC/DESC keywords no?
There was a problem hiding this comment.
You are right, that condition will never be reached.
I'm not sure about parsing, i prefer people use the correct syntax (but don't we all)
fixe
force-pushed
the
bugfix/whitelist-order-keywords
branch
from
a24bd6e to
a93a3a7
Compare
|
@mickhansen PR updated. I need more feedback on testing this though :-) |
| @@ -525,7 +525,7 @@ describe(Support.getTestDialectTeaser('Include'), function() { | |||
| ], | |||
| order: [ | |||
| User.rawAttributes.id, | |||
| [Product, 'id'] | |||
| [Product, 'id', 'ASC'] | |||
There was a problem hiding this comment.
Ideally this should still not be needed.
But not sure exactly how we would go about that.
|
@fixe something like: expect(
User.findAll({
order: [
['id', ';DELETE YOLO INJECTIONS']
]
})
).to.throw();And perhaps add one for using an include and ordering by that include too. |
|
Need to expand this to cover |
Fixes #2906.
Breaking change: it is mandatory to provide the order when using the array notation.
@mickhansen I think we should create an abstract query-generator test file and add the missing tests there. I can also check if the number of arguments in the array is 1 and keep the BC in that case. Thoughts?