Problem/Motivation

getQualifiedMapTableName in

\core\modules\migrate\src\Plugin\migrate\id_map\Node.php

is generating full qualified table names that are missing the SCHEMA part.

It will return {database}.{prefix}{tablename}

when it should return {database}.{schema}.{prefix}{tablename}

Looks like in mySQL you can cross database without using a schema, but this fails to work on other databases.

Beta phase evaluation

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug: database specific logic should only be present in database drivers.
Issue priority Major because: this breaks automated testing on SQL Server and possibly other database engines.
Unfrozen changes Unfrozen because it just moves a piece of logic into the database driver, so specific driver can override it.
Disruption Non disruptive: it just moves a piece of logic into the database drivers, where it belongs. Current implementation of that logic is kept exactly the same.

Proposed resolution

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

david_garcia’s picture

Status: Active » Needs review
FileSize
1.51 KB

Added schema information to the fully qualified name + moved the logic into the Schema class.

If it does not pass tests in mySQL, at least we should move the logic to the Schema class so that database drivers can have their own implementation.

Status: Needs review » Needs work

The last submitted patch, 1: d8-2388067-FullQualifiedTableName.patch, failed testing.

Damien Tournoud’s picture

Status: Needs work » Closed (works as designed)

David, this actually looks correct. What Drupal calls a database is actually a schema for PostgreSQL, i.e. a namespace that allows cross-namespace querying. Drupal doesn't have any knowledge of the PostgreSQL concept of database, and it doesn't have to. Multiple PostgreSQL databases map to multiple connections.

david_garcia’s picture

Status: Closed (works as designed) » Active
FileSize
1.48 KB

I don't want to be rude reopening this, but at least wan't to have it clear before doing so.

This issue is arising on SQL Server, not Postgre, during D6 migration tests execution.

Currently the database driver (drupal, not specific for any driver) manages 3 concepts:

['database']: this is supposed to be the "name" of the database.
['schema']: a database can have multiple schemas, defaults to dbo.
['prefix']: a prefix to append to tables to void collisions when running multiple things inside the same database.

In SQL Server, the correct way of globaly targeting a specific table in a database would be: database.schema.table (where table has the embedded prefix from drupal).

In SQL Server:

Yes, the primary purpose of SQL schema was -is- to facilitate security management: define who [which principals] can access what [which database objects]. This was made particularly easier starting with SQL 2005 when the schema stopped being directly tied to the owner.

Another use of schema is to serve as a namespace, that is preventing name clashes between objects from different schemas.
The original use of this was to allow multiple [interactive, i.e. ad-hoc like] users of a given database to create their own tables or stored procedures (or other objects), without having to worry about the existence of similarly named objects possibly introduced by other users.
The Namespace-like nature of schema can also be put to use in a planned database setting, i.e. one when a single architect designs the database structure in a way which provides distinct type of access, and indeed different behaviors, for distinct user groups.

The issue here isn't wether or not the FullyQualifiedName is properly generated (it does for mySQL), what I am trying to say is that the FullQualifiedName generated inside getQualifiedMapTableName() is not working on SQL Server because it needs the SCHEMA between database and table name, and that Drupal should delegate any database specific handling (such as generating a full qualified name) to the specific database drivers.

So I am proposing to move that logic into the Schema class, so that specific database drivers can override it if necessary.

I confess to not have properly honored the original implementation, in an attempt to generate something that was mySQL and SQL Server friendly. But tests clearly show I failed at that.

So, at least that logic can be moved into the Schema class to allow specific driver overriding.

david_garcia’s picture

Status: Active » Needs review
Damien Tournoud’s picture

Status: Needs review » Needs work

@david_garcia that actually makes sense.

   /**
+   * Get a fully qualified table name.
+   */

^ Please document the parameters and return value, and add a @see to getPrefixInfo.

+  function GetFullQualifiedTableName($table) {

^ This should be getFullyQualifiedTableName

+    $table_info = $this->getPrefixInfo($table, TRUE);
+    $info = $this->connection->getConnectionOptions();
+    return implode('.', array($info['database'], $table_info['table']));
+  }

^ The default implementation should be simply return $table_info['schema'] . '.' . $table_info['table']. You can override it in the SQL Server driver (but you probably actually have to, except if you want to support cross-database distributed queries).

The last submitted patch, 4: d8-2388067-FullQualifiedTableName.patch, failed testing.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

@Damien Tournoud

I know that the number of sites that will ever directly benefit from this change is exactly 0 (unless D7 to D8 migration will use cross database queries), but this change is needed in order too pass D6 migration tests in core with SQL Server.

We need to move any database specific logic into the drivers to be able to say that Drupal is truly database agnostic.

I cannot believe the patch failed because the stub database driver is not supporting getPrefixInfo!!

Rewrote the patch to not depend on that method.

Status: Needs review » Needs work

The last submitted patch, 8: d8-2388067-FullQualifiedTableName.patch, failed testing.

Damien Tournoud’s picture

@david_garcia: please just add getPrefixInfo to the stub driver. Your current code is just incorrect.

The default implementation needs to be exactly this::

$table_info = $this->getPrefixInfo($table, TRUE);
return $table_info['schema'] . '.' . $table_info['table'];
david_garcia’s picture

@Damien Tournoud: I think your proposed code is also broken, the original implementation is using 'database' (from connection options) instead of 'schema'.

I also agree that the stub schema should have the getPrefiInfo implemented, but I don't feel comfortable adding it because whoever wrote the stub schema class decided to leave nearly 95% of it's methods throwing a Not Supported exception, and I must confess as to not having a clue of what the stub schema is being used for.

Looks like last patch failed because at some point in the tests there is a Schema instance that does not have it's internal $connection property initialized, and the getQualifiedMapTableName depends on information provided by the connection itself (getConnectionOptions).

I'll wait to see if I can get my local testing to work, it's been broken for a while now and I am working blindfolded, combine that with a 5 minute long bursts dedication to Drupal and that translates in publishing broken patches over and over again :(

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: d8-2388067-FullQualifiedTableName.patch, failed testing.

david_garcia’s picture

Looks like the stub Schema is constructed without a reference to a Connection object, it's built with the database contents themselves:

  public function __construct(array &$database_contents) {
    $this->uniqueIdentifier = uniqid('', TRUE);

    // @todo Maybe we can generate an internal representation.
    $this->databaseContents = &$database_contents;
  }

Meaning that, on the stub schema, getQualifiedMapTableName() should not be supported or be implementad on such a way that it will not depend on the existence of a Connection object, as it depends on connection specific settings such as 'prefix', 'schema' and/or 'database'.

david_garcia’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

Let's try luck, still cannot test locally.

Moved getFullQualifiedTableName to Connection, where it makes more sense along with tablePrefix, and connectionOptions.

david_garcia’s picture

Issue summary: View changes
david_garcia’s picture

Title: getQualifiedMapTableName() is missing SCHEMA information » getQualifiedMapTableName is database specific logic out of the database driver
benjy’s picture

Assigned: Unassigned » Damien Tournoud

Would be good if Damien could review this again.

david_garcia’s picture

Assigned: Damien Tournoud » Unassigned
Priority: Major » Critical

Boosted to critical, I'm not sure it qualifies but the truth is that, without this, a D8 release will not be compatible (at least) with MS SQL because there's database specific logic out of the database driver. This bug is breaking (at least) continuous QA because it prevents core tests to pass on MSSQL.

dawehner’s picture

Priority: Critical » Major

Just ensured, we don't block a release because of MS SQL support, and so its not critical ... afaik you can even override the schema class anyway for it anyway.

david_garcia’s picture

Acknowledged. But I think the issue cannot be solved by overriding the schema class. That's the core of the problem, the "failing" logic is outside of the database abstraction layer, inside:

/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php

benjy’s picture

Acknowledged. But I think the issue cannot be solved by overriding the schema class. That's the core of the problem, the "failing" logic is outside of the database abstraction layer, inside:

/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php

You can replace the id_map plugin. This issue seems like something we could fix, but there could be further changes required in the future for MS SQL specific support.

  /**
   * {@inheritdoc}
   */
  public function getIdMap() {
    if (!isset($this->idMapPlugin)) {
      $configuration = $this->idMap;
      $plugin = isset($configuration['plugin']) ? $configuration['plugin'] : 'sql';
      $this->idMapPlugin = \Drupal::service('plugin.manager.migrate.id_map')->createInstance($plugin, $configuration, $this);
    }
    return $this->idMapPlugin;
  }

See this in the Migration entity. You can set $this->idMap['plugin'] on the migration and that will change the id map plugin to be created.

david_garcia’s picture

Makes no sense to support D6-D7 migrations in MSSQL because D6 did not support MSSQL. But sooner or later D7-D8 migrations will be there, and the sooner we start fixing this small issues, less problems there will be latter for the D7-D8 upgrade path.

At this moment the issue is just a matter of QA where tests are not passing (yes MSSQL users can disable the broken tests) but no matter how "mucho" simple the justification for disabling them is, the QA Audit guys don't like having tests disabled, specially if they are from core.

And overally, the solution to this issue is just shifting three lines of code to the place they belong: the database abstraction layer! Then MSSQL driver can override it to it's needs.

The chance of breaking anything with the patch in #15 is 0% because the code just just been moved from one place to another, the logic has not changed.

Crell’s picture

Someone in IRC pinged me about this issue. The general changes here seem reasonable at first glance. Database-specific logic belongs in database-specific classes. Is there any related cleanup that can/should be done at the same time? Do we need an overridden version of the new method for any of the core-supported databases?

david_garcia’s picture

1. I spent some time a few months ago testing (and implementing) the MSSQL driver for D8, I found a few things that needed fixes in core (some are already commited by now), but this was the only "database logic out of database layer" issue.

2. The issue shows up on automated core migration tests. I believe these tests must have been run on core supported databases, and if no one has complained then they are probably passing. But because the logic has not changed, if it didn't work before it still will not work, and if it did, it will continue working.

Thanks for bringing attention into this, fighting the MSSQL front on core feels like swimming upstream.

benjy’s picture

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -318,6 +318,21 @@ public function tablePrefix($table = 'default') {
+    // @see https://www.drupal.org/node/2388067

I think we could just remove this, then RTBC for me.

Would be good to checkout the Postgres test results after this goes in. #2160433: [policy, no patch] Move PostgreSQL driver support into contrib

david_garcia’s picture

benjy’s picture

Status: Needs review » Reviewed & tested by the community

OK Great, lets do it.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b79c650 and pushed to 8.0.x. Thanks!

  • alexpott committed b79c650 on 8.0.x
    Issue #2388067 by david_garcia: getQualifiedMapTableName is database...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.