search

doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.93k stars 2.51k forks source link

Partial unique constraints should not be created as full unique constraints on MySQL #7270

Open BenjaminNolan opened 6 years ago

BenjaminNolan commented 6 years ago

Bug Report

We've been weighing up whether to report this one, but I've decided to go ahead with it as the behaviour is unexpected at the very least.

Q A
BC Break no
Version 2.6.1

Summary

When declaring an @Index or a @UniqueConstraint on a @Table, you can provide a where clause in the options parameter which will convert the index from a full index to a partial one. On DBMSes which don't support partial indices, such as MySQL, Doctrine simply drops the where clause from the statement that it generates. For @Index annotations, this is fine, as the fallback functionality is to create a full-table index rather than a partial one. However, for @UniqueConstraint annotations, dropping the where clause can cause incorrect behaviour in an application, and, in our case, errors when executing the statement.

In our application, a user has one or more email addresses linked to their account, one of which is marked as their primary address. They can have several non-primary addresses. We currently enforce this constraint in our code, however in the future we are likely to move to a DBMS which has support for partial indices and want to add the partial constraint annotation now. Unfortunately, when we do add this, it generates a query which would limit the user to a maximum of two addresses, one primary, and one secondary.

How to reproduce

The following two entities are simplified versions of the ones from our application.

/**
 * @ORM\Entity
 */
class User
{
    /**
     * @ORM\Column(type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
     * @var Collection<Email>
     *
     * @ORM\OneToMany(targetEntity="Email", mappedBy="user", cascade={"persist"})
     * @ORM\OrderBy({"isPrimary" = "DESC", "email" = "ASC"})
     */
    private $emails;
}

/**
 * @ORM\Entity
 * @UniqueEntity("email")
 * @ORM\Table(
 *      indexes={
 *          @ORM\Index(name="email_idx", columns={"email"})
*      }
 * )
 */
class Email
{
    /**
     * @ORM\Column(type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
     * @ORM\ManyToOne(targetEntity="User", inversedBy="emails")
     * @ORM\JoinColumn(name="user_id", referencedColumnName="id")
     */
    private $user;

    /**
     * @ORM\Column(type="string", length=255, unique=true)
     */
    private $email;

    /**
     * @ORM\Column(type="boolean", options={"default":false})
     */
    private $isPrimary = false;
}

Inserting a row into user, and then several rows into email linked to this user id where one only is marked as primary and at least two are marked as not-primary works correctly. Now modify the annotation for Email to this and run doctrine:schema:update --dump-sql:

/**
 * @ORM\Entity
 * @UniqueEntity("email")
 * @ORM\Table(
 *      indexes={
 *          @ORM\Index(name="email_idx", columns={"email"})
 *      },
 *      uniqueConstraints={
 *          @ORM\UniqueConstraint(
 *              name="unique_primary_email_per_user",
 *              columns={"is_primary", "user_id"},
 *              options={"where": "is_primary = 1"}
 *          )
 *      }
 * )
 */

When we dump the SQL from the schema tool on MySQL, it generates this UNIQUE query:

CREATE UNIQUE INDEX unique_primary_email_per_user ON email (is_primary, user_id);

Running this query with the data described above would result in an error.

Expected behavior

We believe that Doctrine should actually omit this query entirely on DBMSes which do not support partial unique constraints, as the query that is generated does not enforce the same logic as the partial constraint, and the fallback for omitting it entirely is less problematic than adding it.

Apologies for this being a bit rambly. I am very tired. :) Please let me know if you need any additional information.

Ocramius commented 6 years ago

On DBMSes which don't support partial indices, such as MySQL, Doctrine simply drops the where clause

@powerkiki this feature was built by you some time ago, and I don't know if it is a good idea to have it in ORM anymore (sorry!): Could you advise on how to move forward?

PowerKiKi commented 6 years ago

@BenjaminNolan, the doc clearly explains that options and also where is platform specific. IMHO you should not expect platform specific things to work for all platforms.

@Ocramius why would you think this should no longer be part or ORM ? AFAIK it is not the only thing that is platform specific, and it exposes useful features that would not have any workaround if we remove it ?

PowerKiKi commented 6 years ago

For reference the original PR was #1027.

BenjaminNolan commented 6 years ago

Whilst it does indeed explain that the where option is platform specific, the fallback behaviour for normal partial indices is fine, whereas the fallback behaviour for unique partial constraints results in potentially application-breaking behaviour. It's for this reason that I brought it up, as, generally, Doctrine tries to fail safe.

Perhaps we could add an additional option to indicate that Doctrine should drop the constraint entirely on platforms that don't support partial indices? But feel free to close this if you'd rather, I just wanted mainly to raise the fact that the fallback behaviour for one particular case can be problematic.

PowerKiKi commented 6 years ago

Unfortunately I think your expectations are too high. "fallback behaviour" is a nice way to say "don't do anything at all, because it's not even aware of the option potential existence".

I'd have to double check, but I'm pretty sure it's exactly the same behavior for other non supported, platform specific options. Meaning that they are entirely and silently ignored. Calling that a fallback is a bit generous.

I suppose we could technically throw exceptions in non supported platforms. But then it should be done for every single features, and I'm not sure if it's a direction doctrine would want to go.

For now, I wouldn't touch anything, or at most improve the docs. But I don't see a strong case for removing a working, tested and useful feature. And I don't really see a strong case either to change something in non supported platforms.

Ocramius commented 6 years ago

I suppose we could technically throw exceptions in non supported platforms. But then it should be done for every single features, and I'm not sure if it's a direction doctrine would want to go.

Currently, such indexes can lead to incorrect/misleading behaviour if silently skipped, so an exception is preferable over a silent skip.

Unsure about which other features get silently skipped, but that's dangerous and probably to be prevented by stopping execution (exception)

BenjaminNolan commented 6 years ago

An exception would definitely be an improvement in this particular instance. :)

PowerKiKi commented 6 years ago

According to grep -rPoh ">getOption\('[^']*'\)" .|sort -u ran on DBAL master, we have the following options that are platform specifics and that would need to throw exceptions in non-supported platforms:

So while we could probably hardcode a whitelist of options (and their values) for table, foreignKey and index for each platforms, it feels like a lot of maintenance work for something that is clearly documented as "fragile" to begin with.

While I understand that the current situation is not perfect, it is a documented, widespread and consistent pattern in the project. My opinion is still to do nothing about it.

PowerKiKi commented 6 years ago

Today I stumbled upon a feature request for partial indexes in MariaDB (and MySQL too). This might one day also land in their codebase. So that's one more reason to not remove our support for it.