$AlterTable->dropConstraint() Throw error on MySQL,There is no support for "DROP CONSTRAINT"

[solody]

I'm dropping a constraint on mysql5.6, code:

$AlterTable = new Ddl\AlterTable($test_table_name);
$AlterTable->dropConstraint('text_UniqueKey');

echo $sql->buildSqlString($AlterTable,$adapter);

$adapter->query(
    $sql->buildSqlString($AlterTable,$adapter),
    $adapter::QUERY_MODE_EXECUTE
);

And this code output :

ALTER TABLE `test` DROP CONSTRAINT `text_UniqueKey`

Here is the problem, Mysql nerver support the Syntax "DROP CONSTRAINT", so it throw an error :

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CONSTRAINT `text_UniqueKey`' at line 2

Note: $adapter config array is

'db' => array(
    'driver' => 'Pdo_Mysql',
    'hostname' => 'localhost',
    'username' => 'root',
    'password' => 'abc123',
    'database' => 'easypay',
)

Help ! What to do now ???

[nanawel]

Interested in this issue too. I guess there should be a way to do this through the MySQL AlterTable decorator.

[alextech]

Yes, you can see example in my MSSQL PR at https://github.com/zendframework/zend-db/pull/231/files and more specifically https://github.com/zendframework/zend-db/pull/231/files#diff-1c04a65ef544d7ab52700dcd9818209e

Generally speaking:

1. Extend existing decorator into your project, and use that decorator (or modify sources in your own zend db fork)
2. Modify inherited $specifications property to provide a template for what syntax should be with printf style placeholders
3. add processWhatever function, which matches the key of $specifications template you added, or trying to modify. It should return array that will be able to fill placeholders from step 2.

Did not realize some syntax does not work for mysql here. Usually its the other way around. What is appropriate mysql way of doing this?

[nanawel]

Did not realize some syntax does not work for mysql here. Usually its the other way around. What is appropriate mysql way of doing this?

In MySQL, DROP CONSTRAINT is replaced by DROP FOREIGN KEY for foreign key constraints only.
Edit: there's no DROP CONSTRAINT at all on this platform, you must use the type of the constraint when dropping.

My problem here is that I don't want to break the abstraction layer by using explicitly a MySQL-oriented class in my application (it should work the same on MySQL and SQLite for example).

So I can't add a new method dropForeignKey() because it would break that abstraction, and moreover I use an AlterTable object, which does not provide this method before being decorated (which happens later, when the SQL string is generated).

[alextech]

This is not a first time I ran into problem of constraints and foreign keys used interchangeably in syntax generation. @ezimuel Should version 3 or maybe even next 2.x consider providing separate API for constraint vs FKs? (not easiest question, because some DB engines do use them interchangeably)

A horrible suggestion for now to keep abstraction layer between your MySQL and SQLite engines same, is on dropConstraint() call do a meta data query against DB engine to see if its a constraint or FK XD

[nanawel]

Yeah that's probably what I'll have to do, but that's an ugly workaround.
My idea was also to provide a generic additional (and optional) argument to those methods to allow passing more details at call time. Those parameters would be stored with the rest, and decorators would be able to use them.

In that very case, you could call

$alterTable = new AlterTable();
$alterTable->dropConstraint('MY_FK', ['type' => 'foreign_key']);

and then \Zend\Db\Sql\Platform\Mysql\Ddl\AlterTableDecorator would be able to generate the right SQL string via processDropConstraints() method.
Others decorators might just ignore them, and behavior would be handled by the parent method.

[alextech]

Your idea looks fine for temporary work around. Would vote against including something like that in library, because optional parameters create unmaintainable logical path branches that future contributors often forget to keep in mind.

[nanawel]

You're right.
Do you think a

public function dropConstraint($name, $type = null) {}

would be more acceptable? It's still an optional parameter, but now at least it's clearly identified.

Still searching for a clean way to do this, but I can't see anything with the current architecture.

[alextech]

Its the same problem. There are many bugs in PostgreSQL adapter as primary example from top of my head because of inconsistency of optional parameter handling few function calls in. Only time its considered acceptable if it modifies call in some trivial non-branching way.

Another potential solution maybe to use strategy pattern, where syntax is generated based on parameter type. Currently, dropConstraint() only supports string as parameter, which in itself is bad because it is inconsistent with other parts where Identifier metadata object is often used. addConstraint() can take in Constraint identifier, IIRC. So if I have a nice object oriented schema definition, I cannot use that metadata object definition on constraint removal. Not only is that annoying from DDL usage perspective, it would make this problem easier to solve. Constraint metadata object could generate its own code. That is, AlterTable decorator would not be the one generating constraint syntax, but ConstraintDecorator would.

So dropConstraint signature can be changed to take in Constraint type object instead of string. Constraint can be overriden using decorator, same way everything else can be decorated, which would allow relatively simpler

processDroppedConstraint(Constraint $constraint) {
   return $constraint->getSqlString($this->adapter);
}

and dropConstraint would just add $constraint to removal queue.

[alextech]

More specifically,

$constraint = new Constraint();
$fk = new ForeignKey();
$table->dropConstraint($constraint); // should work
$table->dropConstraint($fk); // also would work.

[nanawel]

That's much better indeed. I've thought of using typed objects as parameters (and specifically Constraints subclasses) but that meant so much code rewrite that I prefered focusing on finding a quicker way without touching to the signature.

Your solution is really attractive, but personally I can't imagine the whole implementation with the associated validation tests that would be necessary...

[nanawel]

The other problem is that replacing the current string parameter by a Constraint object would break the backward compatibility. Do you think the new implementation has to support both?

[nanawel]

Currently the method addConstraint() is defined as follows:

/**
     * @param  Constraint\ConstraintInterface $constraint
     * @return self Provides a fluent interface
     */
    public function addConstraint(Constraint\ConstraintInterface $constraint)
    {
        $this->addConstraints[] = $constraint;

        return $this;
    }

$constraint being a \Zend\Db\Sql\Ddl\Constraint\ConstraintInterface.

IMHO and if I understand what you wrote correctly, using this very class for dropping is not the right way to go, but instead use \Zend\Db\Metadata\Object\ConstraintObject. Am I right?
That seems logical to me if for example you want to iterate over an existing structure you've just retrieved by analyzing the DB.

[nanawel]

@alextech I have an implementation based on what you suggested, with unit tests and all. I'll push a PR soon, but I doubt it could be accepted on the main branch, even though it fixes a currently broken implementation.