-
Notifications
You must be signed in to change notification settings - Fork 209
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
Invalid column name errors #358
base: master
Are you sure you want to change the base?
Conversation
ee69b68
to
0e2618a
Compare
@@ -1225,23 +1225,23 @@ module.exports = Nodal => { | |||
children__is_favorite: true, | |||
children__license: null, | |||
pets__name: 'Oliver', | |||
pets__alive: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write a new test case here instead of adapting an old one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually an issue with the existing tests since the column active
doesn't exist (it's is_active
). I had to update them because my new changes would break the tests otherwise. This column doesn't seem to affect the result of the test anyway since they still pass like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Makes sense then
Parent.query() | ||
.join('pets') | ||
.where({ active: true }) | ||
.end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the style consistent with other tests (use the callback and catch the error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I wrote the code is such that it throws an error outside of the end()
call. I did this is because it's more of a developer error, not a DB one. If I did it inside the end call then it might be less obvious.
Would you prefer me to have it throw the error inside the end()
call?
name: 'Zoolander', | ||
pets__animal__in: ['Cat'] | ||
}, | ||
{ | ||
children__is_favorite: true, | ||
children__license__not_null: true, | ||
pets__name: 'Oliver', | ||
pets__alive: true, | ||
pets__is_alive: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test for comparators too (pets__name__ilike
or __lt
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I could add those if you like but this change was just to fix the bad column name used in the original test.
This PR will make the composer throw errors if a query tries to use invalid column names.
Reasoning
Queries currently ignore invalid column names which makes it easy to write queries that break unexpectedly.
This can lead to errors such as when a 'where' clause returns more records than it should. This change will ensure the developer is aware of a bad query and fix the issue.
Example
author_id
andis_admin
Model.query().where({ user_id: user.id, is_admin: false }).destroyAll(...)
user_id
. This doesn't exist on the model so Nodal silently ignores it. This query then selects and deletes all users by accident.