PDO connector / Parameterised queries / SQL query building and generation

[Damian Mooyman]

I've been quite busy lately with work on a new database abstraction layer for SS 3.1. See http://open.silverstripe.org/ticket/7429.

The intent of this was to develop a parameterised framework for the Silverstripe back end, but as a result I seem to have implemented quite a few other refactors and features in the process.

I have rebased and pushed by latest work to the following branches:

https://github.com/tractorcow/sapphire/tree/3.1-pdo-connector

https://github.com/tractorcow/silverstripe-cms/tree/3.1-pdo-connector

The work is broken down into the below:

• Support for paramaterised queries both using PDO, and updating MySQLi to support this.
• Refactor of the database layer into a set of configurable dependencies including:
• Database controller (extends SS_Database)
• Database schema manager (extends DBSchemaManager)
• Database connector (extends DBConnector, subclasses PDOConnector and MySQLiConnector)
• SQL Query generator (default implementation DBQueryBuilder, but can be subclassed by other adaptors)
• Ability to configure database via injector. /_config/database.yml includes two default configurations for PDO and MySQLi as connector classes.
• Support for parameterised queries. As a result, the entire codebase has moved from using escaped conditions ("\"URLSegment\" = '" . Convert::raw2sql($text) . "'") to parameterised conditions (array('"URLSegment"' => $text)). I've invested a huge amount of time in developing the syntax, as well as migrating all existing conditions to using this. This also supports dynamic conditions that are evaluated at execution point (see SQLConditionGroup), backwards support for hard coded string conditions, and support for multi-parameter conditons e.g. array('\"Seconds\" > MAX(?, ?)" => array($yesterday, $tomorrow))
• Support for parameterised insert / update. These use a similar syntax to the above, but with a different methodology. See SQLAssignmentRow / SQLWriteExpression for the API to use here.
• Code cleanup. I'm sure I'll get some well deserved critique for this, but honestly, DataObject::write()... it was not the prettiest nor as concise as it should be. No child of mine is going out looking like that. Also replaced the magic numbers for the $changed flags to use named constants.
• Breaking changes. Mostly due to the fact that schema operations are handled via DB::getSchema instead of DB::getConn, but I have tried to maintain as much backwards compatibility as necessary to be practical. Database connector modules (SQLLite, MS SQL, Postgres) will need to be refactored before they will work, however.

Test cases for the new classes are however lacking. I still need to write PDOConnectorTest, SQLDeleteTest, SQLUpdateTest, SQLInsertTest, DBQueryBuilderTest, DBSchemaManagerTest (and others), update many existing test cases to use the new code, and fix the existing tests that break on SQL string comparison.

I need to do massive testing, but unfortunately this is going to be a time consuming process, so I'd appreciate any assistance others could lend.

[Daniel Hensby]

Wow wow wow. Great work, I've been dreaming of when we'd have parameterized database queries in ss, and pdo integration too!

I wish I had more time to help out but serious props to this.

I'll try to check out out and give it some testing on one of my projects tomorrow at the least.

How confident are you that this is stable?

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Damian Mooyman]

To be honest, I'm not confident at all. I've got it running, and passing the major test cases, but due to the fact that there are many missing test cases, and that the API has changed so much, it's still very much experimental at this stage I'm afraid.

To be honest, I'd be more confident pushing this out to 3.1.1, but the SS team might want to keep the large API changes to major releases.

I'd like some feedback from the SS team on my question from the ticket page (quoted from http://open.silverstripe.org/ticket/7429#comment:36)

 One of my issues is that I've taken it upon myself to write the DBQueryGenerator class in such a way that it semi-formats the resulting SQL query with indentation and newlines. This also means that test cases that were written with the old SQL generator code no longer pass, even if the SQL is correct in many cases (although there will be still cases where it's not correct).

Here is my question; Should I revert to unformatted SQL generation (no newlines, indentation, etc), should I rewrite the test cases to use the new format, or thirdly, should I write a SQL text comparison class that compares SQL strings accounting for things such as parameters, formatting, case difference, newlines, indentation, and the like? 

Anyone have any feelings on this?

[Daniel Hensby]

Can't you just replace white space with a single space before comparison? I guess it isn't very robust... but it'll work 99% of the time?

[Damian Mooyman]

Another issue is that two SQL strings with different parameters can be mistakenly treated as equivalent when their parameters differ. There is also the issue of how to treat brackets that may or may not be separated by whitespace.

E.g. how do you compare "WHERE ID = ? AND (URLSegment = ?)" and "WHERE ID = ? AND ( URLSegment = ? )".

Simply replacing repeating whitespace won't solve the problem of comparing no whitespace to whitespace. It might be enough for the sake of the test cases though. :)

Still, is it worth trying to format the SQL at all, or do we as developers prefer a wall of solid text when reading SQL, such as when viewed in an emailed error report?

[Daniel Hensby]

I think it's much nicer to have it formatted, but it does add length to the queries, thus slowing down queries,  increasing cache store sizes and memory usage. Perhaps only format if debugging?

[Damian Mooyman]

I'm not so certain whitespace adds any overhead to execution, but not having formatting does add overhead to debugging. :)

I'll have a look into writing an extension to SapphireTest to handle query comparison when I get some time.

[Daniel Hensby]

Would it be difficult to turn formatting on just for debugging and leave it without the rest of the time?

[Damian Mooyman]

Yeah, I think I could do that, and make it a configurable option (so you can set it to whatever your project prefers)

DBQueryBuilder:

  formatting: true # or false for live environment

[Damian Mooyman]

Hi Pigeon, I've added your suggestion.

https://github.com/tractorcow/sapphire/tree/3.1-pdo-connector

I've also added a new assertSQLEquals to SapphireTest and applied it to DataQueryTest as a proof of concept. Seems to work!

[Damian Mooyman]

Next job is to go through the test cases and replace any SQL comparison assertions with the new logic.

It'd be nice to at least fix the existing test cases before I go and add more. :)

[Ingo Schommer]

Exciting times, thanks a lot for your awesome work Damian :)

You mentioned 3.1.1 as a potential release target. Even if this doesn't require any API changes,

I feel its too big of a change for a patch release. Either we get it into 3.1.0, or it'd have to wait until 3.2.0.

A merge into 3.1.0 would require all supported DBs to be operational at the time of merge, otherwise we inhibit adoption of the new release.

While this is a highly desireable feature to have in core, we're already quite late in the 3.1 release cycle.

The changes and API would need to be rock solid for 3.1, in my opinion within the next couple of weeks,

in order to get some real world usage before we go into RC stage. Is that realistic?

Some thoughts on the changes:

- Overall, the separation of concerns and inheritance structure makes sense to me.

- MySQLSchema extends DBSchemaManager - We should keep class naming consistent and call this MySQLSchemaManager.

- SQLInsert etc. are pretty generic names - can you confirm we're not clashing with any PECL/PEAR libs here? I've done a quick search, looks like we're good though.

- Should we extend SQLQuery with an empty SQLSelect class for consistency?

- Could we make a create() shorthand for SQLUpdate/SQLInsert etc classes? In most cases an instance var is not required, right?
Before: $update = new SQLUpdate("..."); $update->execute(); - After: SQLUpdate::create("...")->execute();

No need to change existing usage, since that'd be a lot of effort - its more of a preferred API approach going forward.

Also has the added benefit of making the classes injectable (if we use Object::create()).

- It might pay off to add notable changes to an upgrading guide (in docs/en/changelogs/3.1.0.md) as you go, easy to forget something later on

- DataObject::write() is large and ugly, but also battle tested. You change it significantly (at least diff-wise). Can you list specific changes to make peer review easier? And maybe even which changed/new behaviour is not covered by tests?

I didn't even get halfway through the sapphire diff though, its ... enormous! But going in the right direction.

[Damian Mooyman]

Hi Ingo, it might be possible for us to fit it into 3.1 over the next few weeks, but as you said it would have to be rock solid. Maybe it can happen, as there is still a bit of time, but I'll leave that decision up to the devs.

I could add a SQLSelect class, but I'd probably do it in reverse order (SQLQuery extending SQLSelect) and with a deprecation on SQLQuery. How would this suit?

Good idea on the create syntax, but that's typically used for wrapping dependency injection. Would you like me to simply wrap the constructor (my preference), or actually make SQLExpression extend Object and use the built in object construction code? My concern here is the performance overhead of adding DI here.

I can document the changes around DataObject::write, if it makes it easier to review. I don't think any new tests are required here, as it still effectively behaves the same, other than using the new API.

How do you feel about the separation of classes in the database connection code? Have I correctly captured the requirements (Controller/Schema/Connector/QueryBuilder)?

Don't suppose you'll be going to the meetup tonight in Auckland?

:) Have a great day

[Ingo Schommer]

Hey Damian,

On 19/02/2013, at 9:23 PM, Damian Mooyman <damian....@gmail.com> wrote:

> I could add a SQLSelect class, but I'd probably do it in reverse order (SQLQuery extending SQLSelect) and with a deprecation on SQLQuery. How would this suit?

Makes sense.

>
> Good idea on the create syntax, but that's typically used for wrapping dependency injection. Would you like me to simply wrap the constructor (my preference), or actually make SQLExpression extend Object and use the built in object construction code? My concern here is the performance overhead of adding DI here.

I can't think of a reason you'd want to use DI on SQLExpression (the connector/builder classes are more likely for that),
so in general extending Object won't give us much benefit from an architecture perspective.
Wrapping the constructor is fine with me.

>
> I can document the changes around DataObject::write, if it makes it easier to review. I don't think any new tests are required here, as it still effectively behaves the same, other than using the new API.

Maybe it just looks like a lot in the diff, so if you say that its functionally equivalent, that's cool.

>
> How do you feel about the separation of classes in the database connection code? Have I correctly captured the requirements (Controller/Schema/Connector/QueryBuilder)?

Still need a bit of time to look through code. Would be cool if we can come up with use cases
where this architecture comes in handy, for a bit of real life validation. For example:
Does it make custom GIS types and DB-specific GIS features easier to implement?

>
> Don't suppose you'll be going to the meetup tonight in Auckland?

I would've left for the airport quite a while ago to still make the meetup (working from Germany) ;)

[Damian Mooyman]

> Don't suppose you'll be going to the meetup tonight in Auckland?
I would've left for the airport quite a while ago to still make the meetup (working from Germany) ;)

Hah, a man can dream can't he? Worth a shot. :)

[Damian Mooyman]

Hi Ingo, I've gone over your feedback as well as your feedback on Github and pushed up a new update.

https://github.com/tractorcow/sapphire/tree/3.1-pdo-connector

https://github.com/tractorcow/silverstripe-cms/tree/3.1-pdo-connector

- Renamed SQLQuery to SQLSelect, and added SQLQuery as a deprecated class

- Renamed MySQLSchema to MySQLSchemaManager for consistency

- Added SQLSelect::create() style construction for SQLSelect, SQLDelete, SQLUpdate, and SQLInsert

- Updated the .md documentation with new security notes

- Re-added in deleted API with the correct deprecation messages

- Updated some PHP doc to better clarify arguments

- Bugfixes from last night's work

- Bugs from last night's work (still in testing!)

I haven't updated the changelog yet... am going to attack the failing test cases for the time being. I'm trying to setup a new test environment where I can quickly run test cases without monopolising my development environment. It literally takes hours to finish the test cases on my current machine. =/

[Hamish Friedlander]

Hi Damian,

Thanks for taking care of this. I haven't had a look over the code yet, but the general structure you describe sounds good.

However realistically I think we should be targeting getting this into 3.2. It's just too big a change for 3.1 - while we are still changing some APIs in 3.1 we're trying to move it towards a stable release pretty rapidly now and a big restructuring like this has a pretty high chance of causing regressions.

Hamish Friedlander

[Damian Mooyman]

Hi Hamish, thanks for getting back to me.

That sounds ideal really. When do you think a 3.2 branch would be available?

[Simon J Welsh]

master is the current 3.2 branch.
---
Simon Welsh
Admin of http://simon.geek.nz/

[Ingo Schommer]

Thanks Damian! May I suggest that you incrementally build up new commits in the branch?

Given its large scope, this will make peer review a lot easier - at the moment its impossible

to see what you actually changed since I've last looked at it. The squashing will need to be done

later on (even if it causes a bit of overhead then). To be clear: Squashing changes into PRs

is usually preferred, but only for smaller PRs which are easy to review and merge.

[Damian Mooyman]

Hi Ingo, I apologise for that!

If it helps, I had merged a bunch of commits from the last 18 hours into a single commit, so if you are looking for the "new" changes they will be in there. The only work in there that you might have seen was the SapphireTest extensions and test case tweaks, but they were in fact broken (part of the reason I had squashed them subsequently).

https://github.com/tractorcow/sapphire/commit/0ce1f500973b456a5efa3a193c2e0719284ff8a4

I'm often moving around various locations so I will occasionally push up a change temporarily while I'm fixing issues. I should have used the working branch, but I was a bit hasty. :)

In future I'll try to keep my untested code to the working branches, especially while my code is being reviewed.

Thanks for your time, and please keep the critique coming. I really appreciate your help.

[Damian Mooyman]

Hi Ingo, just as a note, I did happen to spot a bug in the existing dataobject::write code, but it wasn't being captured by the test cases. Fortunately the refactor brought it out, so I updated the test case to demonstrate the issue.

Hope that's enough of a case to justify the cleanup. :)

I agree with you though, it's a very battle tested function, which makes it challenging to cleanup.

The fix: https://github.com/tractorcow/sapphire/commit/596b787012c30f74226fda9c41426dc14bf4c137

The test case (in case you want to test it against 3.1): https://github.com/tractorcow/sapphire/commit/939792c160882c146846bf47c3a228c3bfd7c2f7

Also, should I rebase this onto master now, or still good to keep it against 3.1?

On Wednesday, February 20, 2013 9:29:35 AM UTC+13, Ingo Schommer wrote> 

> I can document the changes around DataObject::write, if it makes it easier to review. I don't think any new tests are required here, as it still effectively behaves the same, other than using the new API.

[Damian Mooyman]

I can also confirm that all of the current test cases pass! Finally.

My next jobs in approximate order:

- Update postgresql module

- More test cases for new and updated classes

- Update deprecation messages to 3.2 instead of 3.1

- Update sqllite module

- Update ms sql module

 

[Ingo Schommer]

Hello Damian, yes please rebase onto master :)

[Pigeon Friend]

Hi Damian.

I've set this up on one of my sites and noticed there is no longer a set_connection_charset() function. I think this needs to still be there, but with a deprecation notice...

Other than that, I've been able to switch on a code base and it's been pretty seamless...

Ingo, you sure you don't want this in 3.1 ;)

Dan

[Pigeon Friend]

Damian,

Two other things:

1. ?showqueries now shows the queries with the '?' placeholders - understandable as these are prepared queries, but is there anyway to get the placeholders replaced with values when they're output as lawd only knows that the values are a ton of the debugging help....
2. For my site, the SiteTree in the CMS isn't showing any of the pages that are hidden from menus

Cheers,

Dan

[Damian Mooyman]

Yeah, I will re-visit the set_connection_charset. It was moved to the connection arguments ($databaseConfig) because it's really a per-connection argument, and I didn't feel it was correct to treat it as a static. I can easily re-add in the old api though with some deprecation.

and below,

1. We could do an unintelligent find and replace of all ? with the respective parameters, but that's not technically the correct way to deal with that. There is some code in Zend_Database that has to do with parsing of parameterised queries and does it quite well, but I didn't feel it was a problem big enough to warrant including another external dependency. Might be better instead to print out queries and then list the parameters separately?

2. I'll look into this.

Thanks for the feedback. :)

[Damian Mooyman]

I don't think it should be, since we need time to bring the non-mysql adaptors up to scratch, and 3.1 is enough of a change as it is.

I am also sure it would mean a lot of bugs blamed on me for many months to come. At least this way they'll be just in the development branch.

[Daniel Hensby]

I completely agree that the charset should be on the connection and not the database, I always found this a bit strange. But a deprecation notice would be nice.

1. Yes, I'm not sure how is best, though for this to be a useful debugging tool it really needs to return executable queries imo. A lot of the time I find errors in logic by copying the query from the debug to mysql query browser and then can see why I'm not getting expected results. At the moment, this is much more difficult and it's a much less useful debugging tool.

2. Thanks, I haven't had time to dig around at all, so it may be something obvious...

As for going into 3.1, I was just teasing, this looks good, but agree it needs more community testing and prep for going into a release. Let's save blaming bugs on you until next year...

Dan

[Damian Mooyman]

Hi all, I have a small update on the progress on this module.

First of all I have rebased my work against the master tree, with 3.2 as the target for the API changes. Please see below for the new branches. The 3.1-pdo-connector branches will be left stagnant as a reference.

https://github.com/tractorcow/sapphire/tree/3.2-pdo-connector

https://github.com/tractorcow/silverstripe-cms/tree/3.2-pdo-connector

This update includes a new API addition: Convert::id2sql. This will hopefully profile a replacement for manually escaping of table and column names, meaning that there are reasonably symmetric mechanisms for escaping (and quoting) both values and identifiers. Convert::id2sql("SiteTree.Title") outputs "SiteTree"."Title" (ANSI standard escape). I possibly could rename this function to something better.

Additionally I have been working on a postgres module:

https://github.com/tractorcow/silverstripe-postgresql/tree/3.2-pdo-connector

I had some issues with postgres working across multiple databases via other connectors (PDO for example) so I've attempted to resolve this by using a schema-as-database pattern, which is configurable via postgresql.yml. Fortunately in my original refactor of MySQLDatabase I left enough of the control in the base database class for this to be implemented without having to add special hacks to PDOConnector.

This module still doesn't work, so don't bother testing it. Will let you know how I get on...

[Damian Mooyman]

Well, it does kind of seem to work. I can build a database and browse the site.

One thing I've noticed across all the databases is that on the second dev/build the SiteTree->ClassName enum always loves to rebuild itself. I might see if I can fix that, but no biggie.

Will investigate the MS SQL / SQLLITE modules soon I hope. :)

Damian

[Damian Mooyman]

SQLLite3 version of the module is up at https://github.com/tractorcow/silverstripe-sqlite3/tree/3.2-pdo-connector. Am testing this now, and am beginning work on the MS SQL Server module.

While I'm pushing forward on this, and am doing less development, more testing, and preparation for deployment, are there any additional tasks that I'll need to make sure of before issuing a pull request?

I haven't yet looked into the Silverstripe installer and what work will need to be done there, but are there other modules I'll need to update?

I hope I'm not stumbling along in the wrong direction here. :)

[Sam Minnée]

While I'm pushing forward on this, and am doing less development, more testing, and preparation for deployment, are there any additional tasks that I'll need to make sure of before issuing a pull request?

You seem to have the bases covered; once you've issued a pull request we'll really start getting into the nitty gritty of reviewing the branch.  The main concern we have with major API changes / refactorings is upgradeability.  I would hope that if you take a 3.0/3.1 site and upgrade to this version, that either dev/build makes all the necessary changes, or there is another easily executed script.
 

I haven't yet looked into the Silverstripe installer and what work will need to be done there,

It will probably be minimal. 

 

but are there other modules I'll need to update?

No, I think you're all good.  It would be worth trying out some other modules - for example, blog, forum, advancedworkflow - and see if your branch breaks anything that you might need to list in the upgrade guide.

 

I hope I'm not stumbling along in the wrong direction here. :)

No, things are looking good :-) 

[Damian Mooyman]

That's great Sam, thanks for the reply.

I don't think there will be any breaking changes for most sites, unless they do things that work on a very low level with SQL or databases.

[Damian Mooyman]

Wow, thanks for the shout out at http://vimeo.com/60414528#t=20m0s. Was just watching this during lunch. :) 

Let's just hope that the thing works at the end of the day. :P

On Wednesday, March 13, 2013 12:41:33 PM UTC+13, Sam Minnée wrote:

[Damian Mooyman]

A further update on this, I have completed migration of all of the modules and finished testing (against existing and a few new test cases, more will still be to come). The MS SQL driver was the latest to be migrated. https://github.com/tractorcow/silverstripe-mssql/tree/3.2-pdo-connector

I have decided to, unfortunately, drop support for the mssql driver, as it's no longer actively maintained and doesn't have parametrised query support as the sqlsrv driver does (at least outside of stored procedures). I attempted to write a basic connector, but it would be the only driver that would require parameterised queries to be emulated, and it didn't sit well with me as being consistently robust and well implemented.

I'm currently working on updating the installer, including better support for multiple options within database connectors (Do you want that with or without PDO?).

It's a little tiring migrating in ongoing updates and pull requests to my working branch. It's been teaching me a lot about merging conflicts. :)

I hope to get the pull requests ready some time for next week, after which I will probably work on additional test cases as subsequent pull requests. I could hold back until then, but it's probably better at this point to get the code into the main trunk so that other developers can start getting to grips with the new API, and hopefully will result in some good early feedback. Does this sound agreeable to all?

[Damian Mooyman]

https://github.com/silverstripe/sapphire/pull/1360

All of the related pull requests can be found here.