db_index_exists accepts only the name of an index. What happens if there are two different tables with indexes of the same name? Is there a way to select the table? Should index names be unique? There is nothing in db_add_index to say indexes should have unique names and db_add_index accepts $table, $name.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

peterx’s picture

Component: documentation » database system
Priority: Normal » Critical

db_index_exists accesses schema()->indexExists($name);
indexExists is:

  public function indexExists($table, $name) {
    return $this->connection->query('SHOW INDEX FROM {' . $table . "} WHERE key_name = '$name'")->fetchField();
  }

D6 -> D7 conversion is failing in block because block deletes a non existent index. db_index_exists is needed to make block #7003 work. The function should be:

function db_index_exists($table, $name) {
  return Database::getConnection()->schema()->indexExists($table, $name);
}

I will test with alpha2.

peterx’s picture

Tested in http://drupal.org/node/722920. The change works.

ctmattice1’s picture

Noticed this also when lookinG at the filter.update.

Besides index_exist was having problems with db_add_unique_key function.

could this possibly be related in some way to #720620: indexExists() for pgsql and sqlite does not prefix tablename with mysql silently failing

andypost’s picture

Priority: Critical » Normal

This is a good hit! Also related #718016: DatabaseSchema_pgsql::renameTable() needs to rename the indexes

The explanation could be: Mysql share index-name in namespace of the table but sqlite and pgsql index-name are global identifiers.

Also this is not critical

Crell’s picture

Hm. Irritating. Is there a way this can be handled without an API change, or do we just have to suck it up?

andypost’s picture

Priority: Normal » Critical

OMG! db_add_index() and db_drop_index() uses table-name so this change is reasonable!!!

We could not proceed without changing API because:
- for mysql we should know real table-name to check index!
- for sqlite and pgsql we should prefix index with full table-name before check!

We we won't changing api:
- all db_index_exists should be changed to {table}_{index} format
- change mysql schema.inc to make index names prefixed by table-name like pgsql and sqlite now!

Anyway all core should be checked for using db_index_exists and contib also...

This is really critical!

chx’s picture

If this is really critical can someone make a real summary which does not require us to dig out the signatures of the functions thrown around? "this is what we have (functions with signatures inlined). this does not work because (copypaste from above). The new signature should be because ...."

andypost’s picture

Title: Documentation problem with db_index_exists » db_index_exists() must conform schemaAPI indexExists()
FileSize
856 bytes

This is a required API change because

- signature of db_index_exists() is different from called schema->indexExists()
- we have no place in core that uses this function

Also all db-dependent implementations of indexExists() are different and does not conform abstract class

This is a first patch to make this change

andypost’s picture

@Crell because this function is not used in core suppose you should decide to return value

includes/database/database.inc
db_index_exists()
 * @return
 *   TRUE if the given index exists, otherwise FALSE.


includes/database/schema.inc
   * @return
   *   Index name if the table exists. Otherwise FALSE.
   */
  abstract public function indexExists($table, $name);


// Mysql implementation
  public function indexExists($table, $name) {
    return $this->connection->query('SHOW INDEX FROM {' . $table . "} WHERE key_name = '$name'")->fetchField();
  }

// pgsql implementation
  public function indexExists($table, $name) {
    $index_name = '{' . $table . '}_' . $name . '_idx';
    return $this->connection->query("SELECT COUNT(indexname) FROM pg_indexes WHERE indexname = '$index_name'")->fetchField();
  }

// sqlite implementation
  public function indexExists($table, $name) {
    return ($this->connection->query('PRAGMA index_info({' . $table . '}_' . $name . ')')->fetchField() != '');
  }

Right now only sqlite works as described!!!

Also we already have tests for this but they are using schema api:
already in #720620: indexExists() for pgsql and sqlite does not prefix tablename
and waiting #718254: Fieldname in tests are too long - this could be converted to db_index_exists() to cover this api

andypost’s picture

Status: Active » Needs review
FileSize
4.79 KB

This patch changes php-docs, result type for all db engines also includes modified test to strict check for result of API function!

andypost’s picture

Docs extended for each database

andypost’s picture

And this patch changes return value to Index-name as it was described in indexExists()
Tests also fixed.

chx’s picture

Why do you need identical FALSE, why assertFalse is not enough? If we are working on this why not do security hardening? It seems we are throwing strings against the DB without any escaping. There is a table escape function already and it feels appropriate to use it for index as well I think.

andypost’s picture

@chx I use assertIdentical() is required to check for TRUE and FALSE in #11 to be sure that result is boolean. So in #12 I just change a values. About security - suppose escaping should be done in calling module not this function because escaping could change the name, also other functions does not use escaping.

Anyway, #11 and #12 just a two possible ways to solve issue.

peterx’s picture

There is db_escape_table and that might also work on column names or field names. I personally think Drupal should protect against stuff passed in from outside. If all db functions implement the same escape for all table, column, and index names, people cannot create a column or field or index that cannot be tested by db_index_exists. Developers will be warned at creation time instead of later.

Plus db_escape_table is aimed at stopping the use of names that will not work in other databases. Again we should warn people when they go to create an index named xx;yy that it will not work with some databases and, even if the database accepts it, the file system might not.

Perhaps one of the Drupal overlords, Webchick?, will document the line where we protect databases from developers who want to decorate identifiers with corinthian capitals.

Crell’s picture

One of the key goals for the DB layer is to provide a unified API for all databases so module developers can support all supported databases without realizing they're doing so; if that means we block anything but ASCII alphanumeric in table and index names just to be safe, so be it.

I'm fine with using $connection->escapeTable() (not the function wrapper) to filter index names.

@andypost: I believe $this->assertFalse() is more correct in this case.

I'll try to review the code tonight if I can. Sorry, about to leave town for a client meeting. :-(

Dries’s picture

I'm asking Crell to sign-off on the final patch. I'm comfortable making API changes given that (i) it is a critical bug and (ii) the API doesn't seem to be used widely yet. Let's fix this properly.

andypost’s picture

There's a big difference in patches from #11 and #12 - first returns only booleans and second returns boolean and string index name!!!
So I thing that explicit type checking provided by assertIdentical() is required.

About using $connection->escapeTable() - suppose only Crell could decide on this because it require changes in all db-related functions...

peterx’s picture

I suggest the immediate need is to make db_index_exists work the same as db_column_exists, something that is already in use, because there are other issues waiting on the ability to specify the table name. If there is a need to change the way all db functions return values then it should be a separate issue.

Crell’s picture

I talked this over with andypost in IRC and we reached the following conclusion:

1) There's columnExists(), tableExists(), and indexExists(). Right now they all return different things, and columnExists()'s return doesn't make the slightest bit of sense anyway. I suspect it was a copy/paste error from findTables().

2) We agreed that we should normalize all three operations, across all databases, to return the name of the thing being checked if it exists and boolean FALSE if not. Given that, I think assertIdentical() for the FALSE check is OK, since the variable being returned could be multi-type. That is, the approach in #12, although it needs to fix up columnExists() too while we're at it.

3) Upon further investigation, it looks like we're already being horribly inconsistent in escaping of table names. We escape them in all built queries, but not on table creation and not in static queries. Gah, how'd we miss that? :-) That's an entirely separate issue on its own, so I am going to say we should not deal with that here, do no additional escaping, and open a new issue to decide what to do with the table/index escaping.

4) I also noticed several places in schema where we're using db_query() instead of $connection->query(). I don't know how those snuck in. That's a separate issue as well. Just noting it here since it impacts the same code in places.

I think that covers everything.

andypost’s picture

Also inform that pgsql still have no columnExists() and tableExists() implementations - DamZ said that default works fine.

And another issue should be extended tests for db_* functions

Talked with DamZ on IRC - he thinks as me that TRUE/FALSE as result enough because:

- returning result as string name could be confusing - pgsql and sqlite are prefix index-names with table name so patch from #12 returns same $name as $name argument, but this seems useless because calling function already knows $name
- this require additional operations and memory for strings

chx’s picture

it's silly to return a string in a function called exists. noone expects that AND it's indeed a known string. if you'd look for an unknown string then it'd be find or search or something but not exists.

Crell’s picture

I am not picky whether we normalize to name/false or true/false, as long as it does get normalized and documented as such.

andypost’s picture

@Crell I see no docs about return value at api.d.o at all

http://api.drupal.org/api/function/db_column_exists/7
http://api.drupal.org/api/function/db_table_exists/7
http://api.drupal.org/api/function/db_index_exists/7 (this said TRUE/FALSE)

Drupal 6 versions also have no php-doc

EDIT: Different return values require different implementations and tests so I think we should decide on that first!

Crell’s picture

I was looking at the code directly. The docblocks for it are all AFU. Those should get fixed as part of this issue.

andypost’s picture

New patch:

1) fixed all php-docs for db_*_exists() functions and their schema()->*Exists() analogs

2) return value unified to boolean TRUE/FALSE (I inspect all places in core - all of them use return value as boolean)

3) tests in schema.test are fixed to assertIdentical() for purpose that other possible db-drivers have ability to be tested for explicit return value of their schema implementations.

4) tests in database_test.test are grouped - they are about db_*_exists() functions same as 3)

5) As Crell pointed at #20-4) all db_query() (4) and one db_select() replaced with $connection->query() analogs

Crell’s picture

Status: Needs review » Needs work

And it's even extensively documented inline, too!

I hadn't intended to fix up the db_query() calls here, as there's already a patch for that here: #726344: Database system uses wrapper functions internally. I'll mark that duplicate, though, since it does clash with this patch on one line so I'm fine with doing it at the same time.

Everything here looks good except for the unit test. db_table_exists(), tb_index_exists(), and db_column_exists() are three separate operations so should get three separate test methods. (Some would argue 6 methods, one for each assert, but I won't go quite that far. :-) )

Once those tests get broken up, this is RTBC.

andypost’s picture

I put all tests together because it seems too expensive to test each function with clean install, I thick we should keep tests are optimized too!

andypost’s picture

Status: Needs work » Needs review

Re-roll agianst changes #582948: Improve safety of schema manipulation

Also tests are separated back to different methods as Crell proposed.

andypost’s picture

FileSize
10.79 KB

And patch

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for carrying this home, andy!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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