other(find-all): throw on empty attributes #11867
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
papb
added
breaking change
|
Tests are breaking. I don't think it is a good idea, what if you got includes or scopes are injecting attributes. |
papb
force-pushed
the
throw-on-empty-attrs
branch
5 times, most recently
from
42b53bf
to
549088d
Compare
papb
force-pushed
the
throw-on-empty-attrs
branch
from
549088d
to
30fa13a
Compare
sushantdhiman
added
status: in discussion
Codecov Report
@@ Coverage Diff @@
## master #11867 +/- ##
=========================================
- Coverage 96.27% 90.08% -6.2%
=========================================
Files 94 93 -1
Lines 9217 9100 -117
=========================================
- Hits 8874 8198 -676
- Misses 343 902 +559
Continue to review full report at Codecov.
|
|
Hi @sushantdhiman, you are right, however I still think something could be done about these errors, so I worked on another approach. Tests are passing now, please review again, thanks! |
papb
added
status: awaiting maintainer
and removed
status: in discussion
| @@ -2019,7 +2020,26 @@ class QueryGenerator { | |||
| return { mainQueryOrder, subQueryOrder }; | |||
| } | |||
|
|
|||
| _ensureNonemptySelectClause(attributes, extraInfo = {}) { | |||
There was a problem hiding this comment.
Suggested change
| _ensureNonemptySelectClause(attributes, extraInfo = {}) { | |
| _throwOnEmptyAttributes(attributes, extraInfo = {}) { | |
| if (attributes.length > 0) return; | |
| const asPart = extraInfo.as && `as ${extraInfo.as}` || ''; | |
| const namePart = extraInfo.modelName && `for model '${extraInfo.modelName}'` || ''; | |
| const message = `Attempted a SELECT query ${namePart} ${asPart} without selecting any columns`; | |
| throw new sequelizeError.QueryError(message.replace(/ +/g, ' ')); | |
| } |
There was a problem hiding this comment.
@sushantdhiman Done, thanks 👍
sushantdhiman
added
status: awaiting response
papb
added
status: awaiting maintainer
and removed
status: awaiting response
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
Closes #11858
This provides a much better error message for the situation described in #11858.
However it is technically a breaking change because on Postgres running
SELECT FROM "Foos"does not throw (simply yielding an empty array). However from the Sequelize point of view, it should throw (IMO), since why would you run a SELECT query and fetch nothing?Note: The commit as in PR title
other(find-all): throw on empty attributesis not allowed by commitlint becauseotheris not a valid scope type. However, I am unsure of what choice to make among the valid options, which are build, ci, docs, feat, fix, perf, refactor, revert, style and test, so I leave it for you to decide @sushantdhiman. I ended up puttingfix(find-all)in the actual commit message, but on squash-and-merge that won't matter much.