SqlServer & Database Refactoring

[MrPickles]

Hello, I was working on a SQL Server implementation about a year ago. I had it working pretty solid, the only piece remaining was adding UI support to the windows form & testing. I ran into an incident that rendered my laptop unusable. I didn’t take any backups and my only option was to re-image it so I lost everything. I was too upset to rewrite it from scratch, but now, a year later I’m ready to get back on it. One thing I noticed, the code based changed significantly. It looks so much better now! Good work Leodagan & Kypdurron, it is much cleaner and intuitive! Is the refactoring complete? It looks complete but I want to eventually pitch the SQL Server implementation as a contribution but I don’t want to get too carried away if it still has work that needs to be done to it. I’m trying to keep everything in the same style/pattern as SqlLiteObjectDatabase & MySqlObjectDatabase.
One concern I have is I don’t want to bastardize SqlObjectDatabase by littering it with a bunch of if (connectionType == ConnectionType.DATABASE_MYSQL) do this, if (connectionType == ConnectionType.DATABASE_SQLSERVER) do that like the old way. It looks like a lot of that mess was cleaned up. I noticed this was everywhere in the old version so I wasn’t too concerned. However, now it’s been cleaned up, do you guys have any recommendations for keeping it clean? Because unfortunately, the only way around something like this is to check the connection type and fire off a completely new block, eg:

Code: Select all

var command = string.Format("UPDATE `{0}` SET {1} WHERE {2}", tableHandler.TableName, string.Join(", ", columns.Select(col => string.Format("{0} = {1}", col.ColumnName, col.ParamName))), string.Join(" AND ", primary.Select(col => string.Format("{0} = {1}", col.ColumnName, col.ParamName))));

the “`” doesn’t exist in SQL server so it would have to get ugly again by doing this:

Code: Select all

var command = string.Empty; if(connectionType == ConnectionType.DATABASE_MYSQL || connectionType == ConnectionType.DATABASE_SQLITE)) { command = string.Format("UPDATE `{0}` SET {1} WHERE {2}", tableHandler.TableName, string.Join(", ", columns.Select(col => string.Format("{0} = {1}", col.ColumnName, col.ParamName))), string.Join(" AND ", primary.Select(col => string.Format("{0} = {1}", col.ColumnName, col.ParamName)))); } else { command = string.Format("UPDATE {0} SET {1} WHERE {2}", tableHandler.TableName, string.Join(", ", columns.Select(col => string.Format("{0} = {1}", col.ColumnName, col.ParamName))), string.Join(" AND ", primary.Select(col => string.Format("{0} = {1}", col.ColumnName, col.ParamName)))); }

To me, littering SqlObjectDatabase with all these checks on what type of connection it is seems to defeat the purpose of the refactoring you guys did as connectionType isn’t even exposed to this class anymore (can’t confirm this at the moment, just getting started). What recommendations do you have for keeping things clean? Or, is there really just no way around it. I know I can make it work by simply doing what was done in the above example but since all the effort was put into tidying it up, I don’t want to undo the progress. Thank you in advance

[Leodagan]

Welcome back MrPickles !

Thanks for you interest in this Database Implementation Revamp, I must admit I made it all the way thinking about your troubles for implementing MS SQL Server back then

The implementation is pretty much finished, I spent a lot of time testing it and trying different rewrite to get most of the DB engine features implemented in their own class, there may be some code missing around "Default Values" when migrating table schema but nothing that prevent running a fresh server with this implementation !

Your main goal now is to build a subclass of SQLObjectDatabase (something like MSSQLObjectDatabase : SQLObjectDatabase) and override anything that don't work as expected in MS SQL Server...

I admit that the "backtick" => ' ` ', is pretty much everywhere in current implementation, I thought this was common SQL Language...

What is the way to protect field names or table names in MS SQL ?
Could there be a switch to make MS SQL use them correctly ?
If the backtick has no meaning AT ALL in MS SQL language, you could use string replace at the lowest Query Execution methods...

Maybe this could be implemented by using a virtual method for protecting field name, this way it could be overriden by different databases implementations !

The current implementation is not "static", we mostly need to keep the DB Interface as is but there can be new logic implemented if drivers are handling things differently.

Anyway, we should never go back to connection type checking, there isn't anymore reference to the ConnectionType enum

Worse Case Scenario you need to subclass from ObjectDatabase and rewrite SQL logic for MS SQL... but we should have easy way to prevent this

Edit:
After some research protecting identifiers is really database dependent, I will probably try to implement a virtual function for this

[Leodagan]

Double post to explain a bit more after some night of sleep

You forced me to document a bit more about connector and SQL Language standards to be able to make a program that can use some SQL syntax without worrying of the underlying Database...

It seems like the default char for protecting field from SQL syntax is the double quote ->> "

Code: Select all

SELECT "key" from "table";

This can work in MySQL using

Code: Select all

SET SESSION SQL_MODE='ANSI';

For SQLite it already support double quote as protecting char, but it has ambiguous meaning, if the double quoted field does not match any table or column it behave as a literal string...

In fact SQLite support most of other DB Engine protect char for compatibility and for the purpose of being used as a drop-in replacement for any Database Implementation...

----

Appart from the SQLObjectDatabase Implementation, there is a lot of "user defined queries" inside Game Server Code or even Game Server Scripts that we have no knowledge of...

I can walk through most of the "Core" Code and change backtick to double-quotes, this way MS SQL would be compatible with fresh setups, but I need to search for a method to enforce SQL_MODE for MySQL...

[MrPickles]

Thanks for the quick reply! The way I got around the protected fields on my first try was by wrapping each field into brackets. For Instance:

Code: Select all

SELECT [Key], [Name], [Id] FROM Account;

This wasn't necessary as not all fields needed it, but it did no harm in doing it. There were a lot of other gotchas I ran into last time. I did not document them, this go-around, I will be. Once I get it to the state I had it last time, meaning I can get the server up and running off of Sql Server, place mobs, etc, I'll bring my findings back to here to see if work-around I did could potentially break anything if you wouldn't mind looking them over. Since it's SQL server specific, it wouldn't break anything existing.

Maybe this is a bad idea, I don't know, I'm a poor man's architect but what do you think about having a string extension method that has the connection type exposed to it strip out back-tics and clean up the SQL based on the type of server?If it's MySQL or SQLLite, nothing changes but if it's SQLServer, clean up the command string. This way, we're not checking for what type like the old way. I'm not too crazy about this idea as it seems a bit unnessicary to call a "CleanSqlCommand" method on each sql string command. So, I'm open to suggestions. Like you said, worse case scenario, I inherit from it, make them virtual, override with the valid MSSSql but I would like to avoid that, it's definitely goes against the DRY pricincipal .

I'm pretty tied up this weekend and next weekend with wrapping up a project for work but after that I'll have quite a bit of free time to work on this.

Thank you.

[Leodagan]

No problem, like I said I was awaiting for someone trying to implement another driver to have some feedback

To sum up :

• You need to subclass "DOL.Database.SQLObjectDatabase", put it in "DOL.Database.Handlers" namespace (for log4net config and dependencies)
• Implement Properties : ConnectionType (which is arbitrary for displaying purpose only...)
• Implement Methods : CheckOrCreateTableImpl(), ExecuteSelectImpl(), ExecuteNonQueryImpl(), ExecuteScalarImpl()
• Override if needed : Escape(), DatabaseSetValue(), FillSQLParameter() (for type conversions)
• Update Factory ObjectDatabase.GetObjectDatabase() (this design is clearly not long-term and is there for compatibility)
• To be able to launch DOL or any existing Server Objects/Scripts, use a Regex.Replace() in ExecuteSelectImpl(), ExecuteNonQueryImpl(), ExecuteScalarImpl() to search for PAIRS of "backticks" in query and replace them with "brackets" (only expected side-effect is if someone use backticks in a literal string it will be stored with bracket instead, but literal string should be banned from game query and use query parameters)
• Try to run Unit Tests, create duplicates for each unit test files that is prefixed by "MySQLDB", rename them accordingly, and update their namespace and content to match SQL Server initialization sequence...

When you have a working "Proof of Concept" we'll try to see what can be done for field protection (brackets, quotes, backticks...), anyway you'll need a workaround to make your tests to begin with

I insist that fields must be protected in DOL queries, some table have fields that use SQL reserved Names and would fail without the special chars protecting them, even if we could try to prevent use of SQL Reserved Names we have no way of guessing future english common names that could be sql reserved in newer database updates (SQL Server could have a new version where "Level" is a reserved name, SQLite or MySQL could have new versions where Region is a reserved name etc...)

Have Fun