tripal/t4d8

Table prefixes in the TripalDBXConnection (Formerly BioDB).

spficklin opened this issue · 6 comments

spficklin commented

This issue is regarding the problem of tests not passing because the Chado tables are given a prefix (causing problems described in issue #190). But more generally, it's a problem with any testing that uses the ChadoConnection class (extends from TripalDBXConnection).

When the Drupal unit/functional testing is run, the default database configuration is dynamically setup to use a "test" prefix on all Drupal tables so that all tables are recreated with that prefix and won't interfere with existing tables. The TripalDBXConnection class needs to know that prefix because it is possible to do joins with tables in the default Drupal schema (i.e., public). However, there seems to be a problem with recognition of Chado tables.

Functional tests will fail with this type of query:

$query = $this->chado->select('db', 'db')

It will give this type of error message because the default Drupal prefix is added to the Chado tables:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42P01]: Undefined table: 7 ERROR:  relation "test89952494db" does not exist
LINE 3: "test89952494db" "db"
        ^: SELECT "db"."name" AS "name"
FROM
"test89952494db" "db"
WHERE "db"."name" = :db_condition_placeholder_0; Array
(
    [:db_condition_placeholder_0] => GO
)

We don't ever add a prefix to Chado tables, in functional testing we create a test schema instead and repopulate it with Chado tables. So, there's no need for prefixing of tables.

If I change the code to the following then the tests work.

$query = $this->chado->select('1:db', 'db')

The TripalDBXConnection recognizes via the 1: prefix that this table does not belong to the default schema.

So, this makes me think something is broken. Shouldn't the ChadoConnection recognize when a Chado table is being called and add the 1: prefix automatically if it's not specified? I don't think I should have to hardcode the 1: prefix for all of my queries in Tripal.

Valentin @guignonv do you have thoughts on what might be wrong?

guignonv commented

Well, it is a bit more complicated than that when it is coded. :-) Th design I choose was to "guess" when the default schema (ie. no prefixing) is Drupal's (public) or "chado". It's a bit complicated to get into the details and explain the "why" and the "how" but I believe there should be a way to fix that. I'll have a look into it in the coming weeks. I've currently a "top priority" to handle that is not Drupal/Tripal related but I'll do my best to see this issue and come back to you with some fix or explanations on what's going on.

spficklin commented

Thanks @guignonv. In the meantime, I've added a 1: prefix to all of the queries. I've noticed this has been done elsewhere too so maybe it's okay and this isn't that urgent. I'm just not used to that, but on second thought it may be good to keep it around because it leads people to ask "why am I doing this" and that makes them cognizant that Tripal can handle multiple databases.

spficklin commented

On second thought, we shouldn't hard-code the Chado to use in our code... so this should be addressed.

guignonv commented

Could you provide the full piece of test code you are using? How did you instanciate "$this->chado" ?
If you can point me to the related test file of a given git branch, I could try and see what's happening. It might be related to a Drupal upgrade.
The design is to have this working:

  $dbxdb = \Drupal::service('tripal_chado.database');
  $query = $dbxdb->select('feature', 'x');
  $query->condition('x.is_obsolete', 'f', '=');
  $query->fields('x', ['name', 'residues']);
  $query->range(0, 10);
  $result = $query->execute();

So it should work as you expect it in the tests. If it does not, it might be either a bug or because the tests uses classes that are not usual maybe. Maybe some classes are mocked and that's the problem but I need to see what's going on.
You might also be interested in the TripalDbxConnection method "useTripalDbxSchemaFor()".
Also, to run tests under a Chado database, I created a base class to simplify the process:
https://github.com/guignonv/t4d8/blob/9.x-4.x/tripal_chado/tests/src/Functional/ChadoTestBase.php
I don't know if Lacey saw it and what are her plans regarding that approach.
[edit]: that class is obsolete regarding current branch in t4d8 but the approach behind might still be useful and adaptable.

laceysanderson commented

Updating here based on a conversation I had with @guignonv in cofest today.

You might also be interested in the TripalDbxConnection method "useTripalDbxSchemaFor()".

Some insight into how Tripal DBX chooses to use Chado vs. Drupal as the default database:

  • Especially in testing, Drupal often uses the TripalDBX connections for it's own queries. As such, it was important to make Drupal always the default if the additional syntax (i.e. 1:tablename) is not provided.
  • In order to still allow us to use Tripal DBX for chado without the additional syntax, you use "useTripalDbxSchemaFor()" to add our classes to a whitelist of class names to use Chado as the default for.
  • For example, if we want to ensure Chado is the default for connections coming out of tests then we need to add the ChadoTestBase to the whitelist using "useTripalDbxSchemaFor()"
  • Valentin believes this should work through inheritance as well (that's how it was designed)
  • Another example is that we would want to add the base chado importer class to this whitelist so that all Chado Importers use Chado by default.

I have started a branch to address this issue and will be working on tests specific to the Chado implementation of Tripal DBX. I will specifically test the table prefixing and will see if the approach above fixes the difficulties we have come across. If so then I will document this in the Tripal DBX API readthedocs so that we know do to this in the future :-)

laceysanderson commented

Added a test to show exactly what we were experiencing:

  /**
   * Tests table prefixing by the ChadoConnection + TripalDbxConnection classes.
   *
   * NOTE:
   * In Drupal you can execute queries directly using CONNECTION->query()
   * or you can use the various query builders: CONNECTION->select(),
   * CONNECTION->update(), CONNECTION->merge(), CONNECTION->upsert(), CONNECTION->delete().
   *
   * This test is focusing on CONNECTION->query() since a code analysis shows
   * that the other options are simply preparing a query and then executing it
   * using CONNECTION->query().
   *
   * That said, at some point we may want to add additional tests to show that
   * the query builders are building queries appropriately but because these
   * are Drupal functionalities and our differences come during execution
   * and not at the query building stage, we are currently going to assume that
   * the Drupal testing is sufficient for the query builders.
   */
  public function testDefaultTablePrefixing() {

    // Open a Connection to the default Tripal DBX managed Chado schema.
    $connection = \Drupal::service('tripal_chado.database');
    $chado_1_prefix = $connection->getSchemaName();

    // Create a situation where we should be using the core chado schema for our query.
    $query = $connection->query("SELECT name, uniquename FROM {1:feature} LIMIT 1");
    $sqlStatement = $query->getQueryString();
    // We expect: "SCHEMAPREFIX"."feature" but since the quotes are not
    // necessary and could be interchanged by Drupal, we use the following pattern.
    $we_expect_pattern = str_replace('SCHEMAPREFIX', $chado_1_prefix, '/["\']+SCHEMAPREFIX["\']+\.["\']+feature["\']+/');
    $this->assertMatchesRegularExpression($we_expect_pattern, $sqlStatement,
      "The sql statement does not have the table prefix we expect."); 

    // Test the API realizes that chado is the default schema for this query.
    $query = $connection->query("SELECT name, uniquename FROM {feature} LIMIT 1");
    $sqlStatement = $query->getQueryString();
    // We expect: "SCHEMAPREFIX"."feature" but since the quotes are not
    // necessary and could be interchanged by Drupal, we use the following pattern.
    $we_expect_pattern = str_replace('SCHEMAPREFIX', $chado_1_prefix, '/["\']+SCHEMAPREFIX["\']+\.["\']+feature["\']+/');
    $this->assertMatchesRegularExpression($we_expect_pattern, $sqlStatement,
      "Unable to determine that chado is the default schema because the sql statement does not have the table prefix we expect.");
  }

Results are as we have experienced: the first assert (when {1:feature} is used) passes and the second assert (when {feature} is used) fails. As shown in the error output it applies the Drupal table prefixing rather then Chado.

docker exec -it --workdir=/var/www/drupal9/web/modules/contrib/tripal t4d8-dbx phpunit
--group "TripalDBX Chado"
PHPUnit 9.5.25 #StandWithUkraine

Testing
E 1 / 1 (100%)

Time: 00:15.898, Memory: 14.00 MB

There was 1 error:

  1. Drupal\Tests\tripal_chado\Functional\ChadoConnectionTest::testDefaultTablePrefixing
    Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "test75213645feature" does not exist
    LINE 1: SELECT name, uniquename FROM "test75213645feature" LIMIT 1
    ^: SELECT name, uniquename FROM "test75213645feature" LIMIT 1; Array
    (
    )

/var/www/drupal9/web/core/lib/Drupal/Core/Database/ExceptionHandler.php:79
/var/www/drupal9/web/core/lib/Drupal/Core/Database/Connection.php:985
/var/www/drupal9/web/core/modules/pgsql/src/Driver/Database/pgsql/Connection.php:195
/var/www/drupal9/web/modules/contrib/tripal/tripal_chado/tests/src/Functional/Database/ChadoConnectionTest.php:54
/var/www/drupal9/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Caused by
PDOException: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "test75213645feature" does not exist
LINE 1: SELECT name, uniquename FROM "test75213645feature" LIMIT 1
^

/var/www/drupal9/web/core/lib/Drupal/Core/Database/StatementWrapper.php:145
/var/www/drupal9/web/core/lib/Drupal/Core/Database/Connection.php:945
/var/www/drupal9/web/core/modules/pgsql/src/Driver/Database/pgsql/Connection.php:195
/var/www/drupal9/web/modules/contrib/tripal/tripal_chado/tests/src/Functional/Database/ChadoConnectionTest.php:54
/var/www/drupal9/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

ERRORS!
Tests: 1, Assertions: 8, Errors: 1.

Now to start working on @guignonv's suggestion of adding the test class using useTripalDbxSchemaFor() to see if that resolves the error.