-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat(query): bind parameters #10284
feat(query): bind parameters #10284
Conversation
05d6e1d
to
fdbe127
Compare
Codecov Report
@@ Coverage Diff @@
## master #10284 +/- ##
==========================================
+ Coverage 96.25% 96.35% +0.09%
==========================================
Files 97 97
Lines 9250 9213 -37
==========================================
- Hits 8904 8877 -27
+ Misses 346 336 -10
Continue to review full report at Codecov.
|
cbae767
to
9f34283
Compare
dc24966
to
92f65a3
Compare
Still a little concerned with the amount and complexity of the involved changes, is there any way to split it into smaller chunks? |
I think it will be quite some extra work, but I will give it a try. Each commit should be able to pass tests. However some commits will have a temporary negative impact on test coverage. The reason is, the commits will provide latent functionality that is only enabled and tested once other commits are ready. Also, some commits may temporarily introduce "duplicated" functionality |
This is amazing. Cant wait to see this merged. |
Looks like this PR has stalled because of the request to split into smaller chunks (which sounds like a humongous task to me). This sucks. Sequelize needs this. |
@javiertury This PR has conflicts, can you please rebase? |
@papb this feature is very time consuming. I want to get it merged, rather than to stall completely. But before investing more hours, we should first have a clear plan. Last time it was decided to merge this little by little into a new branch, sequelize:feat/compositions. However it is stalling. What if instead, I update and merge everything at once into a new branch(e.g. The idea is to allow everyone to work simultaneously on the full implementation at the same time. If enough people commit to this plan, we can have multiple reviewers and multiple coders working simultaneously and autonomously. Reviewer will be free to create as many issues as they need, and everyone will be able to fix them. After it becomes sufficiently polished, we can merge it into |
@javiertury Makes sense to me - but this kind of thing is up to @sushantdhiman 😬 By the way, thank you so much for the great work with Sequelize 👍 it wouldn't be the same without your help. |
Anyone, anyone, Buehler, Buehler? |
@sushantdhiman as this PR flounders, it's getting more and more conflicts with the master 👎 Really disappointed in the way this crucial change is being ignored. I remember the day when the CTO from my client saw the string interpolation on sequelize. He practically exploded and wanted to throw sequelize out then and there. |
@mschipperheyn I'm sorry... Our time is really short. Hopefully we will be able to pick up on this soon... |
Pull Request check-list
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
Issue #10285. All user-facing queries use binded parameters in all dialects, except select queries on mysql dialect.
Changes
node-mysql2
mariadb-connector-nodejs
can be used as an alternativeComposition
object that holds strings (sql commands),Slot
objects(binded parameters) and placeholders. This object is converted to a query in the very end.Composition
objectssequelize.query()
accepts aComposition
object as argument.Composition
objects provide guarantees that makeformatBindParameters()
unnecessary. No need to reparse query with regex.default
dialectComposition
objectsQB.composeQuery()
and or escaping withQB.composeString()
searchPath
query is run separately if there are values to bindignoreDuplicates
instead of exception handlingsequelize.slot()
andsequelize.placeholder()
can be combined withsequelize.composition()
to bind values and create placeholders.->>
operator didn't allow to bind the json path. Changed tojson_unquote(json_extract(...))