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

[8.x] SqlServer Grammar: Bugfixes for hasTable and dropIfExists / support for using schema names in these functions #37280

Merged
merged 2 commits into from
May 6, 2021
Merged

Conversation

arjenvanoostrum
Copy link
Contributor

This PR has 2 bugfixes and 1 enhancement:

  • Bugfixes and enhancement for dropIfExists and hasTable when using a schema in table name in SqlServer.
  • Shorter version for compileColumnListing, no changed behaviour

Consider a database in SqlServer named laravel. By default, all tables created in this database will be created in the dbo schema which is the default schema in SqlServer. When other schemas are created in the database (e.g. a schema called test_schema), tables with the same name can be created in both schema's. So a valid example listing of tables would be:

laravel.dbo.jobs
laravel.test_schema.jobs
laravel.test_schema.users

Current implementation of Schema::hasTable() will return:

Schema::hasTable('jobs'); // true

While the output of the function is correct, the underlying count here is 2 as both jobs tables are found

Schema::hasTable('dbo.jobs'); // false
Schema::hasTable('test_schema.jobs'); // false

Ok, now we really enter the subject of this PR here. Should this return true or false. SqlServer differs from other database engines such as MySQL / Postgres with the ability of using schemas (for security / seperation reasons) within the database.
Whether the output is correct here is determined by the question if the schema name is considered to be a part of the table name.

In Schema (Schema::create('test_schema.test_table, ...)), Eloquent (protected $table = 'test_schema.test_table';) and Query Builder (DB::table('test_schema.test_table')) the use of the schema name as part of the table name is valid and results in expected behaviour (CRUD on table in schema test_schema).
Returning false in statements above is therefore not consistent.

Schema::hasTable('users'); // true

At first glance a correct output as there is indeed a table users. However it apparently has no notion of the schema it lives in. When a statement is run in SqlServer without specifying the schema, the interpreter will assume the defaullt schema (dbo). So, when running select * from users here, SqlServer will answer with invalid object name 'users'. Which is very confusing when using it like:

if(Schema::hasTable('users')) { -> ok, so we have this table available?
$result = DB::table('users')->get(); // ERROR invalid object name 'users' -> Wait what?
}

Returning true here is therefore kind of strange

Proposed implementation of Schema::hasTable() will return:

Schema::hasTable('jobs'); // true

As no schema is provided, the object_id() function will assume the default schema (dbo) and finds the table.

Schema::hasTable('dbo.jobs'); // true
Schema::hasTable('test_schema.jobs'); // true

When schema name is provided, expected results are returned

Schema::hasTable('users'); // false

The default schema does not have a table users, however

Schema::hasTable('test_schema.users'); // true

The test_schema schema does have one.

Current implementation of Schema::dropIfExists() will return:

Schema::dropIfExists('jobs'); // null

SqlServer will drop the table jobs in the default schema dbo. Expected behaviour.
However, how will we drop the jobs table in the test_schema schema ?

Schema::dropIfExists('test_schema.jobs'); // null

No error here until we use it like:

$table = 'test_schema.jobs';
Schema::dropIfExists($table);
Schema::create($table, ...); // ERROR -> There is already an object named 'jobs' in the database.

So, the table was not dropped, instead considered to not exist.

Schema::dropIfExists('users'); // ERROR -> Cannot drop the table 'users', because it does not exist

Here, the if returns true, however the drop statement fails.. As the schema name is omitted, the drop statement will look for a table to drop in the default schema. The if check has no notion of schemas and finds the table in the test_schema schema.

Proposed implementation of Schema::dropIfExists() will return:

Schema::dropIfExists('jobs'); // null

SqlServer will drop the table jobs in the default schema dbo. Expected behaviour.

Schema::dropIfExists('test_schema.jobs'); // null

SqlServer will drop the table jobs in the schema test_schema. Expected behaviour.

Schema::dropIfExists('test_schema.jobs');
Schema::create('test_schema.jobs', ...); // null

So, the table was indeed really dropped

Schema::dropIfExists('users'); // null

No error, nothing is dropped as there is no users table in the default schema.

Schema::dropIfExists('test_schema.users'); // null

Table is dropped

Impact for users

It is higly unlikely that current users of these functions will have to change their code. Currently both functions only work as expected when tables are used without specifying a schema name, which will continue to work with proposed implementation.

It does however enable the use of the schema name as part of the table name, consistent with other functions in available in the framework.

One thing to consider is the use of both a table prefix on the connection and specifying a schema name in these functions. The prefix is, well, prefixed to the $table parameter and will as such essentially prefix the schema name of the table. This is however consistent with the behaviour of the other parts of the framework where the $table parameter is used.

@taylorotwell taylorotwell merged commit cf3fe08 into laravel:8.x May 6, 2021
@bestlong
Copy link

This change makes it impossible to support MSSQL 2000.

@driesvints
Copy link
Member

@bestlong only SQL Server 2017 and up are supported: https://laravel.com/docs/8.x/database#introduction

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.

5 participants