chado_db_query must be improved

[bradfordcondon]

Feature Request/Discussion

• This feature [does ] attempt to solve an existing problem with Tripal
• This feature is [somewhat URGENT]

edit: i wrote this issue thinking we need to fix the lack of join support but really i think the base class is just broken. see this basic test which is failing right now.

Description

the chado_db_query() function returns a ChadoDatabaseConnection see here. I'm using it as per our discussion in the #774 instead of db_select('chado.table_name').

    $connection = chado_db_select('pub', 'p');
    $pub = $connection->fields('p')->condition('p.uniquename', $uname)->execute()->fetchObject();

However, what if i need to join to another chado table?

$connection = chado_db_select('pub', 'p');
$connection->join('analysis_pub', 'ap');
    $pub = $connection->fields('p')->condition('ap', $analysis_id)->execute()->fetchObject();

this wont work. It should function similarly to chado_query where the chado table names go in a {} and are replaced by the API.

consider this failing test:

  /**
   * @group sad_panda
   */
  public function testChadoInsertRecordCanJoin(){

    $pub = factory('chado.pub')->create();
    $analysis = factory('chado.analysis')->create();

    chado_insert_record('analysis_pub', ['pub_id' => $pub->pub_id, 'analysis_id' => $analysis->analysis_id]);
    $connection = chado_db_select('pub', 'p');
    $connection->join('analysis_pub', 'ap', 'ap.pub_id = p.pub_id');
//  $connection->join('{analysis_pub}', 'ap', 'ap.pub_id = p.pub_id');
    $connection->fields('p');
    $connection->condition('ap.pub_id', $analysis->analysis);
    $result = $connection->execute()->fetchObject();

  // this assertion fails
    $this->assertNotFalsel($result);

// so does this one obviously
    $this->assertEquals($result->pub_id, $pub->pub_id);

  }

I can think of other problems. How do i join across public and chado schema with this method? What if i want to start from a public table and join into a chado table?

additional problems?

switching to chado_db_select breaks my functionality but perhaps i'm using it wrong.
it looks like db_query returns a SelectQuery object https://api.drupal.org/api/drupal/includes%21database%21select.inc/class/SelectQuery/7.x
whereas chado_db_select extends a DatabaseConnection_pgsql object

Observe below:

    $query = chado_db_select('analysis', 't');
    $analysis = $query->fields('t')
      ->condition('analysis_id', $id)
      ->execute()
      ->fetchObject();
    $querytwo = db_select('chado.analysis', 't');

      $actual_analysis = $querytwo
      ->condition('analysis_id', $id)
      ->fields('t')
      ->execute()
      ->fetchObject();

    var_dump([$analysis, $actual_analysis]);
###### dump info
######
######

array(2) {
  [0] =>
  bool(false)
  [1] =>
  class stdClass#127 (10) {
    public $analysis_id =>
    string(2) "84"
    public $name =>
    string(6) "wgs.5d"
    public $description =>
    string(0) ""
    public $program =>
    string(14) "a method, v1.0"
    public $programversion =>
    string(14) "a method, v1.0"
    public $algorithm =>
    string(0) ""
    public $sourcename =>
    string(12) "SAMN03704235"
    public $sourceversion =>
    string(0) ""
    public $sourceuri =>
    string(0) ""
    public $timeexecuted =>
    string(19) "2015-10-22 00:00:00"
  }
}

as you can see using a plain db_query specifying chado retrieves the analysis,, but using the chado_db_select does not.

UPDATE: I've figured it out. chado_db_select is NOT compatible with Tripal Test Suite transactions whereas db_select is. If you turn off transactions, this failing test will pass.

proposal

This method should accept table names in {} and [] like chado_query does to differentiate between chado and public. It should accept them whenever you put in tables instead of aliases (join and select at least).

[bradfordcondon]

we have a PR up at #867 . In making it we uncovered a problem that we need a decision on.

chado_db_query and chado_query do not work the same way.

• Chado_query uses {} for chado tables and [] for Drupal tables.
• Chado_db_query uses no prefixes in the input. It does support both chado and public, but not if the two tables have the same name.

I think its confusing to have the two APIs behave differently. I think its cleaner if we factor out the table string replacement so that both methods can use it.

However, requiring chado_db_query to have table prefixes would be a breaking change. Its only used in one place in core's code, so easy fix there. however, other modules might be using it. presumably outside of a db transaction because within one the function doesnt work.

[almasaeed2010]

I think I have a solution that is not a breaking change.

We can do the following:

• If {} or [] is provided in select/join, we use the replace method
• Otherwise, revert to the old implementation of figuring where the table comes from

If we do it this way, every one is happy!

[laceysanderson]

I vote for consistency (i.e. keeping the table prefixing for both) especially if it can be a non-breaking change

[almasaeed2010]

So I guess to make sure it is none breaking we have to implement it as I suggested above. We should also add a deprecation notice to the old way of doing things and make people aware that this will change in the future. Agreed?

[spficklin]

We are not different than Drupal's API. db_select does not require brackets around tables, but db_query does. So, do we want consistency in our own API or consistency with Drupal's API?

But there is still the problem of accessing non-Chado tables.

[spficklin]

An idea....
So, we always know what the current Chado and Drupal schema names are using these two API functions calls:
chado_get_schema_name('chado')
chado_get_schema_name('drupal')

My preference would be to keep chado_db_select as similar to Drupal's db_select as possible. So I would vote not to use the brackets ({}, []) around table names in the chado_db_select function. What if instead we just kept the prefixing as public. and then in the chado_db_select we change any table with a public. using the second function call above.

One thing I'm not sure of is will using public. confuse or help people? For folks who use Drupal/Tripal in a single-Chado instance with Drupal in public it's really easy. For those who don't then they have to remember that public. does not mean the actual public schema (which all postgres databases have). Alternatively we could use a place holder like DRUPALBASE.user to reference tables in the Drupal schema.

[almasaeed2010]

A quick update on this. Over a discussion on Slack, it's been decided that public. will be replaced with the current public schema if different. Same goes for chado.. Also, if no schema is provided, the function will revert back to it's previous behavior of attempting to find out whether the given table is a chado table or a public one then add the schema to it. However, if an unknown schema is given (anything other than chado or public), it will be left as is.

[laceysanderson]

The associated PR has been merged so I'm closing this issue. If there was anything unresolved by the PR please comment and we will re-open!