catfan / Medoo

The lightweight PHP database framework to accelerate the development.
https://medoo.in
MIT License
4.83k stars 1.15k forks source link

Medoo quoting table cause query to fail with MSSQL server 14.0.3029.16 #824

Closed klodha closed 3 years ago

klodha commented 5 years ago

Describe the bug Using medoo library to connect to a MySQL database and MSSQL database. It works perfectly with MySQL database but with MSSQL simple select query are failing. This is observed when MSSQL database has collection, schema and table.

In this case, medoo using method tableQuote adds " before and after table name, making select query to fail / return empty results. Details are shared below.

Information

Detail Code

Name of database is medoo_test

It has a collection named game and that collection has table named users.

So in MSSQL I can execute this query:

USE medoo_test
GO
SELECT TOP 10 user_id FROM [game].[users]
GO

OR

USE medoo_test
GO
SELECT TOP 10 user_id FROM game.users
GO

But SELECT TOP 10 user_id FROM "game.users" will raise error in MSSQL.

In Medoo code, after connecting to database successfully, following line of code is executed:

$users = $db_connection->select('game.users', '*');

But found $users array was empty. So used debug() method to get generated query like:

$query = $db_connection->debug()->select('game.users', '*'); 
echo $query . "\n";

and output show:

SELECT * FROM "game.users"

I found that tableQuote method in Medoo.php file and changed it to

protected function tableQuote($table)
{
        return $this->prefix . $table;
//  return '"' . $this->prefix . $table . '"';
}

and then executed same code and it produced query without " character surrounding table name, and I also got results as expected.

Expected output I should get results from MSSQL database table users into my php array.

catfan commented 5 years ago

If you configured the connection option database_type as mssql correctly, you should get output from debug() with MSSQL default quotation identifier [abc].

What's your connection option?

klodha commented 5 years ago
 $this->db =  new Medoo([
            'database_type' => 'mssql',
            'database_name' => 'medoo_test',
            'server' => '192.168.100.5',
            'username' => 'sa',
            'password' => 'somepasswordnottoshared'
        ]);

This is how connection is configured, and yet debug() with MSSQL returning table name quoted with " character, making it invalid for MSSQL.

And it is using php_pdo_sqlsrv driver as MSSQL is running on a Linux server (a docker container). Let me know if any further details are required about this case.

klodha commented 5 years ago

@catfan also in Medoo.php I couldn't find any check in tableQuote method which handles quote differentially based on database server type. If I am missing something here.

catfan commented 5 years ago

Should be $database->select("users", "*");. No schema prefix on table name.

klodha commented 5 years ago

Sorry but $this->db->select("users", "*"); (i.e. not mentioning schema name) doesn't work. This generates MSSQL query SELECT * FROM [users] and that won't run in SQL Studio either.

SELECT * FROM [users]

Msg 208, Level 16, State 1, Line 1
Invalid object name 'users'.
Total execution time: 00:00:00.043

Perhaps this might work if there is only one schema in database. But my MSSQL database has 3 different schemas. If I specify schema name like SELECT * FROM [game].[users] it works fine in SQL Studio. So my understanding is to get same query generated via my application.

While looking around for fix for generating a query with [ ] instead of ", I found that tableQuote method is defined as protected in Medoo class. So I created a new class in my code DatabaseConnection extending the Medoo class. then I override the tableQuote method like below:

protected function tableQuote($table)
{
    switch ($this->type)
    {
        case 'mssql': 
            $parts = explode('.', $table);
            $toReturn = '';
            foreach($parts as $part)
            {
                $toReturn = $toReturn  . '[' . $part . '].';
            }
            $toReturn = substr($toReturn, 0, strlen($toReturn) - 1);
            return $toReturn;
        default:
            return '"' . $this->prefix . $table . '"';
    }
}

This is not optimal code, but my purpose was just to test the fix. In my application I used new DatabaseConnection to create db object and

$query = $db_connection->debug()->select('game.users', '*'); 
echo $query . "\n";

Generated exact query as I can run in SQL studio with this change. Then I executed $users = $db_connection->select('game.users', '*'); and got all 400 records of my table dumped into $users array in PHP.

I can confirm that at least in my case missing [ ... ] for schema, table name is the issue.

klodha commented 5 years ago

In addition to above fix, another option is not to add " character. In that case, query will be like: select top 1 * FROM game.user.

For that tableQuote function might look similar to:

protected function tableQuote($table)
{
    switch ($this->type)
    {
        case 'mssql': 
            return  $this->prefix . $table;
        default:
            return '"' . $this->prefix . $table . '"';
    }
}

This works as well. But then having object notation is better than dot notation in MSSQL table names.

However given that no one else faced this problem ( I didn't find any Github issue in project with this kind of error), I am still hoping that I am missing something somewhere, and really need not to override this class/method.

Please suggest.

Thanks for your support and wonderful library, this seems only block for me as of now and unless we can find an optimal solution, I can perhaps go with this overridden method approach.

catfan commented 5 years ago

Using multiple schema will have a lot of compatibility problem (database support, table prefixing, table joining, authorization, and more...). As for most project, using one schema (aka the database_name on initialization) with multiple table is enough.

We currently have no plan to support cross schema. You can simply create the database with one schema and multiple table for your project.

klodha commented 5 years ago

Thanks for the revert. Given that I have no control over MSSQL database (its a vendor database) and can't merge schemas, I have to use this existing structure only.

So in that case given the approach I took for overriding tableQuote method with a check for MSSQL database type working with both MySQL and MSSQL database in my case, I will go for that. As to me it seems easier than switching away from this wonderful database library.

If you suggest, I can make a fork of repo, make this change particular change. So that if anyone else faces similar issue, can use that.

lemoatom commented 5 years ago

same question

cameronepstein commented 5 years ago

Yeah I'm having a similar issue.

Running this

$database = new medoo([
    'database_type' => 'pgsql',
    'database_name' => xxxx,
    'server' => xxxx,
    'port' => xxxx,
    'username' => xxxx,
    'password' => xxxx,
     // [optional] Enable logging (Logging is disabled by default for better performance)
    'logging' => true,
  ]);

$datas = $database->select(schema.my_table, "*");

Doesnt work. Even if I remove the schema. It seems this is because Medoo is adding unnecessary " marks to the table reference

klodha commented 5 years ago

@cameronepstein @lemoatom my solution was to extend Meedo class and then override method tableQuote like below:

protected function tableQuote($table)
{
    switch ($this->type)
    {
        case 'mssql': 
            $parts = explode('.', $table);
            $toReturn = '';
            foreach($parts as $part)
            {
                $toReturn = $toReturn  . '[' . $part . '].';
            }
            $toReturn = substr($toReturn, 0, strlen($toReturn) - 1);
            return $toReturn;
        default:
            return '"' . $this->prefix . $table . '"';
    }
}

I am using this to connect MySQL and MSSQL both same time.