Improve handling of schemas in SQL Server >= 2008 #3020

stlrnz · 2018-02-12T15:38:05Z

Table listings do not consider schema names for the SQL Server platform.
This fix adds the schema name to SQL Server >= 2008 table listingss using SQL Sever built-in function SCHEMA_NAME() applied to column uid.

Funtion reference: https://docs.microsoft.com/en-us/sql/t-sql/functions/schema-name-transact-sql
Column reference: https://docs.microsoft.com/en-us/sql/relational-databases/system-compatibility-views/sys-sysobjects-transact-sql

Without this fix doctrine will always try to create the whole schema, even if you only triggered an update. This is because the state of the current schema stored in database is not read correctly.

Note: This fix might also be necessary for SQL Server platforms older than 2008. Unfortunately, I have no possibility to test it.
If anyone wants to backport this fix, be aware that the built-in function SCHEMA_NAME is only available since SQL Server 2008. Maybe the function USER_NAME will help: https://social.msdn.microsoft.com/Forums/sqlserver/en-US/135d51e5-2c22-4ce9-a308-fbb958f3362e/how-to-get-schema-name-from-uid-column-in-dbosysobjects-in-sql-server-2000?forum=transactsql

stlrnz · 2018-03-01T07:35:53Z

The fix now adds support for custom schemas in table columns, too.
Previously the schema 'dbo' was always used.

I'll try to add some tests in the next days.

stlrnz · 2018-03-03T07:19:42Z

Seems like Travis had some trouble yesterday:
https://www.traviscistatus.com/incidents/69gf0dn51g0v
https://www.traviscistatus.com/incidents/f4j8x61l4qgc

@Majkl578 Are you able to rerun the build? Seems like they just killed it.
What do you think about this PR? Does it have a chance to get merged?

morozov · 2018-03-04T03:43:04Z

@stlrnz I restarted the build. Please see the code standards violations.

morozov · 2018-03-04T03:47:18Z

@stlrnz is it possible to test the patch in a functional way? IMO, testing what SQL looks like is the least important thing.

stlrnz · 2018-03-05T15:14:33Z

I've added a test for the modified table listing.

I don't really know which functional test to add, because this PR isn't changing any functionality. It just makes some existing features available for schema-based SQL Server environments (like I use). The only thing I could do is duplicate some existing tests and add a schema name. But does this really makes sense?

stlrnz · 2018-03-09T12:29:30Z

@morozov do you have some more feedback to this PR?
Is there anyone here who could review it?

morozov · 2018-03-09T17:53:18Z

@stlrnz sorry, I missed the update. I was asking for a functional test case which would have failed before the change and would describe the issue you're solving.

Table listings do not consider schema names for the SQL Server platform.

As far as I understand, this ↑ is the root cause which is fixed in SQLServer2008Platform::getListTablesSQL().

Without this fix doctrine will always try to create the whole schema, even if you only triggered an update. This is because the state of the current schema stored in database is not read correctly.

And this ↑ is the actual problem.

Is it possible to create a functional test (the one which interacts with a real DB without mocking it) which initializes the schema, applies some changes to it and makes sure only the required changes are applied, not the entire schema is recreated? Ideally, this test should be applicable not only to SQL server but to an abstract DB platform, since we expect the schema updates to work the same on all platforms.

morozov · 2018-03-09T17:54:38Z

tests/Doctrine/Tests/DBAL/Schema/SQLServerSchemaManagerTest.php

+        $platform   = $this->createMock(SQLServerPlatform::class);
+        $connection = $this
+            ->getMockBuilder(Connection::class)
+            ->setMethods(['fetchAll'])


IMO, a test like this doesn't bring much value. Mocking a DB connection means that we know for sure what a DB will return in our case which is often not true, especially given the variety of the supported platforms and their versions.

I agree with your opinion in general.
However, this tests shows one special thing: The schema name "dbo" is getting ignored. This is done to increase compatibility to the old behaviour.

I see your point. On the other hand, the part about not ignoring the non-dbo namespace is already covered by the functional test. If ignoring the dbo namespace is implemented in the same functional test, it will have two benefits:

The logic of handling dbo and non-dbo namespaces is tested in a cohesive way which make the test much more informative.

The test doesn't need to depend on the DB internal names. It will only test if a table was created in a non-default namespace, it will be returned with the namespace, otherwise, the namespace will be omitted. At this point, dbo will remain a platform-specific detail which the test will not care about.

morozov · 2018-03-09T17:55:29Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php

@@ -579,14 +579,83 @@ public function testGetCreateSchemaSQL()
        self::assertEquals('CREATE SCHEMA ' . $schemaName, $sql);
    }

+    /**
+     * @group DBAL-42


Where does the 42 come from?

All tests relatet to column comments in AbstractPlatformTestCase.php are annotated with this group. I thought it would be important to keep it grouped. However, if thats not true I will remove it.

stlrnz · 2018-03-12T07:51:11Z

@morozov thank you for your feedback. I got your point and will try to implement a test like this.

stlrnz · 2018-03-12T14:04:26Z

@morozov I pushed another test. I don't think it is 100% what you asked for, but let me explain the initial problem to make clear why it's hard for me to write a test.

I use doctrine2 as ORM component inside a symfony application in combination with a SQL Server database. The database is organized using schemas and created by using the command doctrine:schema:create. Everything works fine until this point.
When I call doctrine:schema:update right after creation, my expectation is that there is nothing to update (since I did not change anything on the entity). Instead, doctrine2 tries to create the whole schema again. This is because doctrine2 "knows" that my tables should be defined in a schema but dbal does not report it back. Consequently, doctrine2 finds no matching table for the entity and tries to create it again.

So IMO the real problem is in the communication between doctrine2 and dbal. I fixed this by modifying the behaviour of dbal. In my (production) scenario this works very well.

I think a good test scenario would consider the following steps:

define a model entity (using schema)
create the schema
calculate the diff of the schema (with unmodified entity)
test that nothing has changed

IMO this is impossible to test in dbal because it involves doctrine2 functionality at least in step 3.

morozov · 2018-03-14T00:14:21Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        $this->_sm->alterTable($tableDiff);
+
+        $tableDetails = $this->_sm->listTableDetails('alter_table');


Should alter_table be testschema.my_table instead? Otherwise, the last assertion fails since there's no such table at all.

OMG you are right! This assertion only did not fail in my test because one test before exactly this table was is created and modified in dbo.

morozov · 2018-03-14T00:16:33Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        }
+
+        $myTable = $this->createTestTable('testschema.my_table');
+        self::assertTrue(in_array('testschema.my_table', $this->_sm->listTableNames(), true));


This should be self::assertContains().

morozov · 2018-03-14T00:21:28Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+            $this->markTestSkipped('Schema definition is not supported by this platform.');
+        }
+
+        if (! in_array('testschema', $this->_sm->listNamespaceNames(), true)) {


Instead of checking if the schema already exists, could the test create the schema unconditionally and drop it on teardown? Otherwise, if a previous test execution didn't exit cleanly (e.g. the schema was created but the table wasn't), the subsequent executions will always fail.

I trusted on the fact that the database is dropped on the beginning of the test. This would remove the schema, too. However, I'll try to implement it more specific.

morozov · 2018-03-14T00:33:32Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

@@ -570,6 +572,44 @@ public function testAlterTableScenario()
        }
    }

+
+    public function testAlterTableInNamespace()


I think the functional testing could be reduced to making sure that if the platform supports schemas, then the table names returned by listTableNames() are fully qualified (i.e. are formatted as schema-name.table.name). The same seems already true for PostgreSQL.

At the same time, for the SQL Server specifically, if a table belongs to the dbo schema, it should be returned without the schema prefix for BC.

Does it fairly describe the fix?

Yes, that's correct. I'll implement it in that way.

morozov · 2018-03-14T00:40:10Z

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

@@ -706,12 +726,21 @@ protected function getAlterColumnCommentSQL($tableName, $columnName, $comment)
     */
    protected function getDropColumnCommentSQL($tableName, $columnName)
    {
+        if (strpos($tableName, '.') !== false) {
+            list($schema, $table) = explode('.', $tableName);
+            $schema               = $this->quoteStringLiteral($schema);


It's better to name the variables as $tableSQL and $schemaSQL since they are not names but SQL expressions.

morozov · 2018-03-14T00:42:35Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        $myTable = $this->createTestTable('testschema.my_table');
+        self::assertTrue(in_array('testschema.my_table', $this->_sm->listTableNames(), true));
+


After this point, the rest of test could be removed as not really relevant to the fix. Covering dbo and non-dbo tables will be enough to test the changes.

morozov · 2018-03-14T00:53:36Z

tests/Doctrine/Tests/DBAL/Schema/SQLServerSchemaManagerTest.php

+        $platform   = $this->createMock(SQLServerPlatform::class);
+        $connection = $this
+            ->getMockBuilder(Connection::class)
+            ->setMethods(['fetchAll'])


I see your point. On the other hand, the part about not ignoring the non-dbo namespace is already covered by the functional test. If ignoring the dbo namespace is implemented in the same functional test, it will have two benefits:

The logic of handling dbo and non-dbo namespaces is tested in a cohesive way which make the test much more informative.

The test doesn't need to depend on the DB internal names. It will only test if a table was created in a non-default namespace, it will be returned with the namespace, otherwise, the namespace will be omitted. At this point, dbo will remain a platform-specific detail which the test will not care about.

stlrnz · 2018-03-16T11:13:34Z

@morozov thank you for your patience and the helpful feedback. I've updated the tests according to your comments.

However, there is one nasty thing. The SchemaDiff does not handle dropping of namespaces. So for now, i had to hardcode the SQL.

Majkl578 · 2018-03-16T15:34:52Z

@stlrnz Hi, can you please rebase onto current master? We added CI testing for SQL Server recently in #2617 & #3050 so your functional tests will be checked against live SQL Server on CI now.

morozov · 2018-03-17T02:05:12Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+    {
+        parent::tearDown();
+
+        if ($this->getName() !== 'testTableInNamespace' || ! $this->_sm->getDatabasePlatform()->supportsSchemas()) {


As far as I understand, this is in order to prevent the exception from dropTable() if the table doesn't exist. This is not the best solution since the table may not exist if the test failed and didn't manage to create it.

Please use $this->_sm->tryMethod() and get rid of the conditional logic.

Actually, this is done to reduce the attempts to drop the table / the namespace.
However, I think performance is no requirement on functional tests. So, I will remove it.
But there will be a special handling for the DROP SCHEMA left.

morozov · 2018-03-17T02:05:41Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        //foreach ($diff->toSql($this->_sm->getDatabasePlatform()) as $sql) {
+        //    $this->_conn->exec($sql);
+        //}


Please remove commented out lines.

stlrnz · 2018-03-17T09:04:25Z

@Majkl578 I merged the changes. However, it seems there are some problems with the AppVeyor tests.

@morozov I've improved the tearDown() according to your feedback.

Ocramius · 2018-03-17T09:10:17Z

@stlrnz we don't merge upstream - only rebases are really allowed here.

Also, we now merged 7.2-only on AppVeyor, so maybe the results may be different :)

morozov · 2018-03-17T18:54:39Z

The SchemaDiff does not handle dropping of namespaces. So for now, I had to hardcode the SQL.

This is okay. We can fix it later if needed.

@Ocramius does schema manager not support dropping schemas by design or it's a missing feature?

Ocramius · 2018-03-18T16:09:06Z

@Ocramius does schema manager not support dropping schemas by design or it's a missing feature?

It's something that @deeky666 started handling, but it leads to a lot of tricky scenarios where a DB connection still needs an existing DB (depends on the DB, but take SQLite, for example), so it was likely never improved.

Ocramius

🚢 thanks @stlrnz

Ocramius · 2018-03-18T16:10:43Z

@stlrnz some failures in https://ci.appveyor.com/project/doctrine/dbal/build/1.0.15/job/rjfn6vs2bmmlox3y - can you check if they are related?

Ocramius · 2018-03-18T16:12:18Z

Meanwhile: restarted the build.

stlrnz · 2018-03-19T08:57:29Z

Thank you for approving!
@Ocramius should I try to do a rebase on the current master?

I seems that AppVeyor randomly fails on SQL Server 2008 because of "Cannot open database "doctrine_tests" requested by the login. The login failed.". I cannot see any regularity.
This never happened on my local SQL Server 2008. There was even one good run on AppVeyor: https://ci.appveyor.com/project/doctrine/dbal/build/1.0.14/job/ny1mc1f4nfs1l0c5

So I think its a Problem on AppVeyors side. SQL Server 2012 and 2017 never fails the test.

morozov · 2018-03-22T06:14:28Z

SQLSTATE [42000, 4060]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Cannot open database "doctrine_tests" requested by the login. The login failed.

I could've sworn I've seen this multiple times but the only other place I can find is build 1.0.22 where the actual error was (see #3061 (comment)):

SQLSTATE[HY000]: General error: user-supplied statement class must be derived from PDOStatement

Builds 1.0.6 and 1.0.36 have the same symptoms.

I wouldn't be surprised if this was a manifestation of some other inconsistently reproducible issue (e.g. caused by an unlucky sequence of the tests which is non-deterministic in our case). One more restart fixed the build.

Majkl578 · 2018-03-28T22:20:00Z

SQL Server issue should be fixed by #3071.

Majkl578 · 2018-03-29T14:06:15Z

I did a rebase & squased those 25 commits. Original tree is kept in my fork for now if needed: https://github.com/Majkl578/doctrine-dbal/tree/stlrnz/mssql-schema

Majkl578 · 2018-03-29T14:09:52Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+
+        $this->_sm->tryMethod('dropTable', 'testschema.my_table_in_namespace');
+
+        //TODO: SchemaDiff does not drop removed namespaces?


Does this need adressing before merge?

No, it’s OK.

Majkl578 added Missing Tests Improvement labels Feb 27, 2018

stlrnz changed the title ~~fix for missing schema in table listing for sql server >= 2008~~ Improve handling of schemas in SQL Server >= 2008 Mar 1, 2018

morozov reviewed Mar 9, 2018

View reviewed changes

morozov reviewed Mar 14, 2018

View reviewed changes

morozov added the SQL Server label Mar 15, 2018

morozov mentioned this pull request Mar 15, 2018

Drop support for SQL Server older than 2008 #3052

Closed

Majkl578 removed the Missing Tests label Mar 16, 2018

morozov reviewed Mar 17, 2018

View reviewed changes

morozov previously approved these changes Mar 17, 2018

View reviewed changes

Ocramius added this to the 2.7.0 milestone Mar 18, 2018

Ocramius self-assigned this Mar 18, 2018

Ocramius previously approved these changes Mar 18, 2018

View reviewed changes

Majkl578 dismissed stale reviews from Ocramius and morozov via a909a53 March 29, 2018 14:05

Majkl578 force-pushed the mssql-schema branch from a2761ee to a909a53 Compare March 29, 2018 14:05

Majkl578 reviewed Mar 29, 2018

View reviewed changes

Improve handling of schemas in SQL Server >= 2008

3f0fd62

Majkl578 force-pushed the mssql-schema branch from a909a53 to 3f0fd62 Compare March 29, 2018 19:47

Majkl578 requested review from morozov and Ocramius March 29, 2018 20:09

morozov approved these changes Mar 29, 2018

View reviewed changes

Ocramius approved these changes Mar 29, 2018

View reviewed changes

morozov merged commit 61c5e4e into doctrine:master Mar 29, 2018

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2022


		$this->_sm->alterTable($tableDiff);

		$tableDetails = $this->_sm->listTableDetails('alter_table');


		$myTable = $this->createTestTable('testschema.my_table');
		self::assertTrue(in_array('testschema.my_table', $this->_sm->listTableNames(), true));


		$this->_sm->tryMethod('dropTable', 'testschema.my_table_in_namespace');

		//TODO: SchemaDiff does not drop removed namespaces?

Improve handling of schemas in SQL Server >= 2008 #3020

Improve handling of schemas in SQL Server >= 2008 #3020

Conversation

stlrnz commented Feb 12, 2018 • edited Loading

stlrnz commented Mar 1, 2018

stlrnz commented Mar 3, 2018

morozov commented Mar 4, 2018

morozov commented Mar 4, 2018

stlrnz commented Mar 5, 2018

stlrnz commented Mar 9, 2018

morozov commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlrnz commented Mar 12, 2018

stlrnz commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlrnz commented Mar 16, 2018

Majkl578 commented Mar 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlrnz commented Mar 17, 2018

Ocramius commented Mar 17, 2018

morozov commented Mar 17, 2018

Ocramius commented Mar 18, 2018

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Mar 18, 2018

Ocramius commented Mar 18, 2018

stlrnz commented Mar 19, 2018

morozov commented Mar 22, 2018 • edited Loading

Majkl578 commented Mar 28, 2018

Majkl578 commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlrnz commented Feb 12, 2018 •

edited

Loading

morozov commented Mar 22, 2018 •

edited

Loading