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

InsertOrUpdate now returns the autoincremented ID #1262

Merged
merged 1 commit into from
Jul 7, 2017
Merged

InsertOrUpdate now returns the autoincremented ID #1262

merged 1 commit into from
Jul 7, 2017

Conversation

matthiaz
Copy link
Contributor

InsertOrUpdate is a great method that is currently only used by the dbsessionhandler. It allows you to remove a query to determine if you want to insert or update because mysql will do this for you.
Unfortunately, when you use this method, you didn't get the LAST_INSERT_ID on insert. I added this, plus when there is a duplicate, qcubed will return the id found in the update.

To simplify the INSERT statement I also used the alternative INSERT syntax. INSERT INTO tablename SET col=val in stead of INSERT INTO tablename (col) values(val)

I'm running dbbackedsessionhandler in production.and it sees no difference between the old and new function. So there shouldn't be an impact on the rest of qcubed.

InsertOrUpdate is a great method that is currently only used by the dbsessionhandler. It allows you to remove a query to determine if you want to insert or update because mysql will do this for you. 
Unfortunately, when you use this method, you didn't get the LAST_INSERT_ID on insert. I added this, plus when there is a duplicate, qcubed will return the id found in the update. 

To simplify the INSERT statement I also used the alternative INSERT syntax. INSERT INTO tablename SET col=val in stead of INSERT INTO tablename (col) values(val)

I'm running dbbackedsessionhandler in production.and it sees no difference between the old and new function. So there shouldn't be an impact on the rest of qcubed.
@spekary
Copy link
Member

spekary commented May 17, 2017

The (col) values(val) syntax was chosen because not all sql servers support the other syntax. That might have changed though. I don't know which are compliant and which are not.

Remember that currently all the sql we generate must work with all sql engines. For selects, its pretty standard across all of them. Updates and inserts are where a lot of sql engines diverge.

On the roadmap for v5 is changing things so that database drivers create their own sql based on the QQuery structure, which will allow whatever sql works for that driver to be used.

I suggest you do a quick look to see if all the modern sql engines support the new syntax, and then report back. Otherwise, you will need to change it to the old syntax for now.

@matthiaz
Copy link
Contributor Author

I didn't modify the generated INSERT statements. I considered it, but as you point out, the sql has to work for all sql engines. What I did change was the specific InsertOrUpdate method in QMySqliDatabase.class.php to make it a little more efficient. So that shouldn't have an impact on anything really.

I did take up your advice and looked up the syntax for a couple of other databases (postgres, sql server,..). At first glance, it seems that only Mysql has support for this alternative syntax. So adding this to the generated code is a no go, I agree.

But I don't see the harm in changing the InsertOrUpdate method specifically for mysql.

@spekary spekary merged commit 976502e into qcubed:v3.2 Jul 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants