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

[#997] Evolutions splitting fix (Play 2.0 version) #134

Closed
wants to merge 34 commits into from

Conversation

Wilhansen
Copy link

The same fix for https://play.lighthouseapp.com/projects/57987-play-framework/tickets/997 "ported" to Play 2.0 (but still Java version). Note that the main code is now located in play.core.utils instead of play.db as in Play 1.2.x

@fcanedo
Copy link

fcanedo commented Mar 6, 2012

I see this tries to split the SQL statements properly. However, is there a specific reason why they should be split at all? Can't we just send the whole thing in one Statement?

@Wilhansen
Copy link
Author

Actually, that's what I thought too: http://groups.google.com/group/play-framework/browse_frm/thread/72de8ed20c552568/6c76c5443b50e6d5?lnk=gst&q=sql+split#6c76c5443b50e6d5
this is also how we temporarily fixed this issue with our app running on PostgreSQL (by just sending the entire thing).

But apparently (or rather, probably as I haven't emperically tested it), different DBs have different behaviors. Some might probably disallow it to prevent SQL-injection attacks (since you can't run more than 2 statements per exec).

@fcanedo
Copy link

fcanedo commented Mar 6, 2012

That's unfortunate, if that's the case. So far, I've tested with postrgresql and H2; they're both fine with multiple SQL statements in one execute(). I guess it all depends on how the driver developers interpret the API docs: http://docs.oracle.com/javase/6/docs/api/java/sql/Statement.html#execute(java.lang.String)

Can "an unknown SQL string" be interpreted as a string with one or more SQL statements and have they? None of the relevant documentation that I could find makes it clear what's supposed to be allowed. I do think, though, that having this limited to one statement, seems a bit artificial.

@Wilhansen
Copy link
Author

I'm not a MySQL user but it seems that the MySQL JDBC driver will have problems accepting multiple statements per exec unless you set "allowMultiQueries=true" in the connection string: http://dev.mysql.com/doc/refman/5.6/en/connector-j-reference-configuration-properties.html and some people say they placed the limitation to minimize SQL injection attacks...

OT: sorry again for screwing up the pull request.... just cherry pick the first and last commit if ever. (I still need to get a hang of this)

@fcanedo
Copy link

fcanedo commented Mar 7, 2012

Like I said, unfortunate :(

But, I've tested your splitter with the use-case that I ran into this problem with and it works.

Thanks for the fix.

@ghost ghost assigned guillaumebort Mar 23, 2012
@guillaumebort
Copy link
Contributor

I'm going to fix this, but we have to find a really good solution for that. And to write a good parser.

@adamhooper
Copy link

Is there another bug filed somewhere on this issue? I can't find one, aside from http://play.lighthouseapp.com/projects/82401/tickets/212 which is marked as a duplicate of some as-yet-unnamed bug....

My 2p: since what's valid SQL changes from RDBMS to RDBMS, shouldn't Play use RDBMS-specific logic? That might take less effort than writing a good parser, because many RDBMSs allow sending the entire evolution in one Statement.

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

10 participants