Skip to content
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-generator): Generate INSERTs using bind parameters #9431

Merged
merged 1 commit into from May 30, 2018

Conversation

@gazoakley
Copy link
Contributor

@gazoakley gazoakley commented May 13, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This is a new spin of #9371 where I've removed the bindParams option and updated all the INSERT unit tests to match the new syntax. It's thrown up a few issues:

  • Data types that don't easily match up with built in JS data types (specifically GEOMETRY) cause issues since drivers can't identify them before sending them as bind parameters. They need to be sent using a correct CAST and providing the bind parameter in string form e.g. GeomFromText($1) with a valid bind param
  • Upsert needs updating - both PostgreSQL and Sqlite override the upsertQuery function in the query generator
@@ -725,6 +739,9 @@ GEOGRAPHY.prototype.escape = false;
GEOGRAPHY.prototype._stringify = function _stringify(value, options) {
return 'GeomFromText(' + options.escape(Wkt.convert(value)) + ')';
};
GEOMETRY.prototype.bindParam = function bindParam(value, options) {

This comment has been minimized.

@sushantdhiman

sushantdhiman May 21, 2018
Contributor

_bindParam to match _stringify etc stuff

@@ -989,11 +989,18 @@ class QueryGenerator {
return SqlString.escape(value, this.options.timezone, this.dialect);
}

bindParam(bind) {

This comment has been minimized.

@sushantdhiman

sushantdhiman May 21, 2018
Contributor

👍

@@ -56,14 +56,18 @@ describe(Support.getTestDialectTeaser('DataTypes'), () => {
});
});

const testSuccess = function(Type, value) {
const testSuccess = function(Type, value, useBindParam) {

This comment has been minimized.

@sushantdhiman

sushantdhiman May 21, 2018
Contributor

Third argument should be options with arbitrary configuration possibilities { useBindParam }

@codecov
Copy link

@codecov codecov bot commented May 23, 2018

Codecov Report

Merging #9431 into master will decrease coverage by <.01%.
The diff coverage is 96.8%.

@gazoakley
Copy link
Contributor Author

@gazoakley gazoakley commented May 25, 2018

@sushantdhiman Hit a bit of a snag doing upserts with Postgres. The current upsertQuery function generates SQL along the lines of:

CREATE OR REPLACE FUNCTION pg_temp.sequelize_upsert(OUT created boolean,OUT primary_key text)  AS $func$
BEGIN
INSERT INTO "users" ("id","username","zab","createdAt","updatedAt") VALUES (1,"john","foo","1970-01-01 00:00:00.000 +00:00","1970-01-01 00:00:00.000 +00:00"); created := true;
EXCEPTION WHEN unique_violation THEN
UPDATE "users" SET "id"=42,"username"='john',"updatedAt"='1970-01-01 00:00:00.000 +00:00' WHERE ("id" = 42); created := false;
END;
$func$ LANGUAGE plpgsql;
SELECT * FROM pg_temp.sequelize_upsert();

That's fine when not using bind parameters/prepared statements, but when I generate something along the lines of:

CREATE OR REPLACE FUNCTION pg_temp.sequelize_upsert(arg1 <argtype>,arg2 <argtype>,arg3 <argtype>,arg4 <argtype>,arg5 <argtype>,OUT created boolean,OUT primary_key text)  AS $func$
BEGIN
INSERT INTO "users" ("id","username","zab","createdAt","updatedAt") VALUES (arg1,arg2,arg3,arg4,arg5); created := true;
EXCEPTION WHEN unique_violation THEN
UPDATE "users" SET "id"=42,"username"='john',"updatedAt"='1970-01-01 00:00:00.000 +00:00' WHERE ("id" = 42); created := false;
END;
$func$ LANGUAGE plpgsql;
SELECT * FROM pg_temp.sequelize_upsert($1,$2,$3,$4,$5);

I get an error back from Postgres of cannot insert multiple commands into a prepared statement - I can't string together multiple statements if using prepared statements. I think the result of upsertQuery for Postgres is going to have to return multiple statements to accomplish what its trying to do. I'm thinking of having it return along the lines of:

[
    {
        "query": "CREATE OR REPLACE FUNCTION pg_temp.sequelize_upsert(arg1 <argtype>,arg2 <argtype>,arg3 <argtype>,arg4 <argtype>,arg5 <argtype>,OUT created boolean,OUT primary_key text)  AS $$func$$ BEGIN INSERT INTO \"users\" (\"id\",\"username\",\"zab\",\"createdAt\",\"updatedAt\") VALUES (arg1,arg2,arg3,arg4,arg5); created := true; EXCEPTION WHEN unique_violation THEN UPDATE \"users\" SET \"id\"=42,\"username\"='john',\"updatedAt\"='1970-01-01 00:00:00.000 +00:00' WHERE (\"id\" = 42); created := false; END; $$func$$ LANGUAGE plpgsql;",
        "bind": []
    },
    {
        "query": "SELECT * FROM pg_temp.sequelize_upsert($1,$2,$3,$4,$5);",
        "bind": [42, "john", "foo", "1970-01-01 00:00:00.000 +00:00", "1970-01-01 00:00:00.000 +00:00"]
    }
]

And have something further up treat an array of query elements as something that must be run as a sequence of statements that must be run one after each other. Does that seem reasonable to you?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented May 28, 2018

You are right, we can't use parameterized query with multiple statements. It seems we need to execute two queries with correct order, also keeping other upsert from affecting each other.

And have something further up treat an array of query elements as something that must be run as a sequence of statements that must be run one after each other. Does that seem reasonable to you?

Yeah I agree, we need to return multiple statements with their parameters.

In old approach we used to send one single query which will create / replace function and execute it at server level, but now we need to wrap Model.upsert in transaction. As two different statements are going to be executed, we dont want one upsert to be replacing other upsert function and return inconsistent results.

Another potential solution is to support ON CONFLICT but we still need to support this for Postgres < 9.5 so multiple statements solution still needs to be implemented.

So overall, we need to do this

  1. Return array of statements from query generator
  2. Execute them in order
  3. Wrap upsert in transaction, so there is no cross talk between other upsert trying to replace same function
@gazoakley
Copy link
Contributor Author

@gazoakley gazoakley commented May 28, 2018

The tests are now passing on everything apart from Postgres which is becoming increasingly complex. There's still some fixes needed to support RANGE correctly with bind params, and I've also spotted the Postgres specific code that is returned by the abstract insertQuery which needs the same treatment.

This PR is starting to get quite large too. So that I can split this work up better, would it be acceptable to have an option to turn off bindParams? This would let me:

  • Turn it off by default if insertQuery is passed options.exception as true
  • Turn it off when calling insertQuery from Postgres upsertQuery

I can then postpone having to deal with returning multiple queries and the further complexity that entails. There's also a good opportunity at that point to remove the duplicate code in insertQuery so that it calls fn/exceptionFn in postgres/query-generator.js

@janmeier
Copy link
Member

@janmeier janmeier commented May 28, 2018

Sounds good to me :)

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented May 28, 2018

Sounds good if we keep it as a private option, Use it internally so we can keep converting old queries to parameter based queries and then remove it eventually

@gazoakley gazoakley force-pushed the gazoakley:feat-insert-bind-iii branch 3 times, most recently from d3bb785 to 09fe634 May 28, 2018
@gazoakley
Copy link
Contributor Author

@gazoakley gazoakley commented May 29, 2018

@janmeier @sushantdhiman Any chance either of you could try running the tests yourselves? I can't reproduce the failures in Travis 😕

@janmeier
Copy link
Member

@janmeier janmeier commented May 29, 2018

Works locally for me as well....

@janmeier
Copy link
Member

@janmeier janmeier commented May 29, 2018

You need to rebase on top of latest master (specifically this commit b66f9dd has some tests that are now failing)

I was tipped up by the fact that my local tests said 1764 passing, while travis was 1764 passing, 4 failing so I figured my local copy was missing something

@gazoakley
Copy link
Contributor Author

@gazoakley gazoakley commented May 29, 2018

@janmeier Ooops! I'll try and sort it over lunch.

@gazoakley gazoakley force-pushed the gazoakley:feat-insert-bind-iii branch 3 times, most recently from 0fb148e to 8614879 May 29, 2018
@gazoakley
Copy link
Contributor Author

@gazoakley gazoakley commented May 29, 2018

Tests are all green 🎉

expectation: {
query: 'INSERT INTO `myTable` (`name`,`value`) VALUES ($1,$2);',
// TODO: Check
bind: ['foo', true]

This comment has been minimized.

@janmeier

janmeier May 29, 2018
Member

Should be fine (I'm pretty sure sqlite takes whatever you throw at it)

expectation: "INSERT INTO `myTable` (`name`,`value`) VALUES ('bar',NULL);"
expectation: {
query: 'INSERT INTO `myTable` (`name`,`value`) VALUES ($1,$2);',
// TODO: Check

This comment has been minimized.

@janmeier

janmeier May 29, 2018
Member

Hmm, not sure - There is another issue about ignoring undefined completely, but it should be fine for now

@gazoakley gazoakley force-pushed the gazoakley:feat-insert-bind-iii branch from 8614879 to bf9eb5e May 29, 2018
@gazoakley gazoakley changed the title WIP: feat(query-generator): Generate INSERTs using bind parameters feat(query-generator): Generate INSERTs using bind parameters May 29, 2018
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Looks good, some small changes

query = _.template(query, this._templateSettings)(replacements);
// Used by Postgres upsertQuery and calls to here with options.exception set to true
if (options.bindParam === false) {
return query;

This comment has been minimized.

@sushantdhiman

sushantdhiman May 30, 2018
Contributor

return an object in both cases, so all calls can just use .query. In case options.bindParam: false return no bind parameters.

This comment has been minimized.

@gazoakley

gazoakley May 30, 2018
Author Contributor

Done

@@ -169,7 +171,14 @@ class QueryGenerator {
}
}

if (_.get(this, ['sequelize', 'options', 'dialectOptions', 'prependSearchPath']) || options.searchPath) {

This comment has been minimized.

@sushantdhiman

sushantdhiman May 30, 2018
Contributor

Use full path sequelize.options.dialectOptions.prependSearchPath in _.get

This comment has been minimized.

@gazoakley

gazoakley May 30, 2018
Author Contributor

I can change it, but it's more efficient in Lodash to use the array path form. If you use the string path form (e.g. _.get(this, 'sequelize.options.dialectOptions.prependSearchPath')) Lodash first converts it back to the array path form which currently entails a bunch of regex operations to parse the path. For a single call like this it probably doesn't matter, but do it many times and there's a big difference in performance.

This comment has been minimized.

@sushantdhiman

sushantdhiman May 30, 2018
Contributor

@gazoakley No need to change it then :)

return this.User.create({
dates: []
}, {
logging(sql) {
expect(sql.indexOf('TIMESTAMP WITH TIME ZONE')).to.be.greaterThan(0);
expect(sql).not.to.contain('TIMESTAMP WITH TIME ZONE');
expect(sql).not.to.contain('DATETIME');

This comment has been minimized.

@sushantdhiman

sushantdhiman May 30, 2018
Contributor

assert presence of bind param?

@sushantdhiman sushantdhiman removed the wip label May 30, 2018
Copy link
Member

@janmeier janmeier left a comment

I think all the tests asserting on SQL can be covered with unit tests instead

@gazoakley gazoakley force-pushed the gazoakley:feat-insert-bind-iii branch from bf9eb5e to 8fc477a May 30, 2018
@sushantdhiman sushantdhiman merged commit bce3d9a into sequelize:master May 30, 2018
4 checks passed
4 checks passed
codecov/patch 96.8% of diff hit (target 95.97%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +0.83% compared to b05abdd
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gazoakley
Copy link
Contributor Author

@gazoakley gazoakley commented May 30, 2018

Now we just need to make UPDATE, SELECT and DELETE use bind params 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants