Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate AUTO identifier generator strategy #8893

Open
greg0ire opened this issue Aug 7, 2021 · 24 comments · Fixed by #8931
Open

Deprecate AUTO identifier generator strategy #8893

greg0ire opened this issue Aug 7, 2021 · 24 comments · Fixed by #8931

Comments

@greg0ire
Copy link
Member

greg0ire commented Aug 7, 2021

This strategy allows to pick the preferred strategy given the current platform. It allows full portability because you are guaranteed to have a working strategy no matter which RDBMS you use.

The issue is that a given strategy might be optimal at some point in time, then letter better strategies emerge.

For instance, for PostgreSQL, AUTO translates to using sequences, but, as mentioned in doctrine/dbal#5614 , that might no longer be the best strategy.

Since we cannot really release a new major version of the ORM every time this happens, I suggest we deprecate the AUTO strategy and let people explicitly pick one.

The obvious drawback is that full portability is lost.

@derrabus
Copy link
Member

derrabus commented Aug 8, 2021

An alternative could be that we make the ID generator strategy mapping configurable. The defaults would remain as-is for eternity, but if someone started a new app, they could decide that AUTO on Postgres means "identity columns" and not "sequences".

@greg0ire
Copy link
Member Author

greg0ire commented Aug 8, 2021

They would have to know about all this, though, wouldn't they? The scenario I fear might happen then is:

  1. You start a new app.
  2. Once it's in production, you hear about this.
  3. You now have to deal with that technical debt by doing a migration that might not be so easy, or just ignore it.
  4. You resent doctrine/orm for making bad choices for you.

@derrabus
Copy link
Member

derrabus commented Aug 9, 2021

They would have to know about all this, though, wouldn't they?

Yes. In Symfony apps, we could solve this with a recipe. In general, we could deprecate not configuring this mapping, so that we can throw in 3.0 when AUTO is used without a configured strategy for the database that is used.

Maybe there's a better solution, I'm just brainstorming.

You resent doctrine/orm for making bad choices for you.

I don't know if for instance sequences on Postgres are really the inferior choice, tbh. The idea is to allow the dev to make that choice.

The AUTO strategy is really used a lot. It's currently the default and the reasonable choice if we want the application to be portable. Sacrificing it sounds like a massive burden for apps using the ORM.

@beberlei
Copy link
Member

beberlei commented Aug 9, 2021

Postgres now also has an identity / autoincrement like ID, I talked with Sergei about it a while ago, but forgot what it was exactly. It might be necessary to add this to DBAL and then we could really think about removing AUTO and making this explicit. Good DX Idea!

@greg0ire
Copy link
Member Author

greg0ire commented Aug 9, 2021

@beberlei I think it might be the syntax mentioned in doctrine/dbal#5614

CREATE TABLE color (
    color_id INT GENERATED ALWAYS AS IDENTITY,
    color_name VARCHAR NOT NULL
);

It does indeed not seem supported in the DBAL yet, so this is probably too early.

Note that there is already some sort of support for autoincrement with the use of SERIAL thought (which is just a shortcut to a sequence, like GENERATED ALWAYS | BY DEFAULT AS IDENTITY seems to be)?

Sacrificing it sounds like a massive burden for apps using the ORM.

How so? I think if we implement the above in Postgres, then switching to IDENTITY would make the app fully portable, I think it would just be not supported on Oracle (which seems in fact wrong nowadays too)?

@morozov
Copy link
Member

morozov commented Aug 9, 2021

The obvious drawback is that full portability is lost.

Would it be possible to introduce an API where the consumer could actually prefer the way to generate identity values if the target platform supports both? The defaults in ORM 2.x could correspond to the logic of the prefers* methods in DBAL 2.x. The actual AUTO strategy would still exist but would fail if more than one method is available for the current platform (hence, requiring user intervention). If a consumer changes their preference, they are responsible for the migration.

In ORM 3.0, the defaults could be removed.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 9, 2021

The actual AUTO strategy would still exist but would fail if more than one method is available for the current platform (hence, requiring user intervention

I don't understand that part. Since the ORM would have defaults, the strategy would know what method to pick, wouldn't it? Or are you talking about ORM 3 where there would be no defaults?

@morozov
Copy link
Member

morozov commented Aug 9, 2021

The AUTO strategy may exist even in ORM 3 but it would have to require user preferences for those platforms that support multiple strategies. This way, the application remains portable with little to no configuration (depending on whether the platforms supporting multiple strategies are used).

@greg0ire
Copy link
Member Author

greg0ire commented Aug 9, 2021

Sounds like a good plan. That way, Symfony application could be configured with a recipe to have "auto" mean "identity", and only newly-created apps would be affected.

Configuration is accessible from the CMF where we need to intervene:

$this->driver = $this->em->getConfiguration()->getMetadataDriverImpl();

@derrabus @beberlei what do you think?

@greg0ire
Copy link
Member Author

Meanwhile, I created an issue on the DBAL to talk about identity columns: doctrine/dbal#4744

@beberlei
Copy link
Member

beberlei commented Aug 15, 2021

@greg0ire its a good idea! We would have a new configuration option accessible as Configuration::get/setIdentityGenerationAutoStrategy($name) (better name TBD) and use defaults for each platform. Then users can change the strategies implementation.

@greg0ire
Copy link
Member Author

Ok, so if somebody uses AUTO and PostgreSQL, they would get deprecation messages telling them that AUTO is defaulting to SEQUENCE, and that they:

  1. should call Configuration::setIdentityGenerationAutoStrategy([PostgreSQLPlatform::class => ClassMetadata::GENERATOR_TYPE_SEQUENCE]) explicitly;
  2. would probably be better off using identity, like this: Configuration::setIdentityGenerationAutoStrategy([PostgreSQLPlatform::class => ClassMetadata::GENERATOR_TYPE_IDENTITY]).

there would be a map called $backwardsCompatibleDefaults that would stay the same forever, and another called $recommendedDefaults that would be used in the deprecation message above.

better name TBD

setIdentityGenerationPreference()?

@greg0ire
Copy link
Member Author

greg0ire commented Aug 20, 2021

This way, the application remains portable with little to no configuration (depending on whether the platforms supporting multiple strategies are used).

I think the vast majority of platforms support several strategies (NONE or UUID come to mind). I now the the right think to do would be to deprecate reliance on the defaults iff those diverge from what we currently recommend, and I think we should recommend IDENTITY no matter the platform for all platforms except Oracle.

@morozov
Copy link
Member

morozov commented Aug 21, 2021

For the end-user of the ORM, whether it's an identity column or a sequence-based implementation, the result is the same: the value in the column is auto-incremented. Why should they bother about the implementation details? This is different than say UUID where the resulting generated value is psudo-random.

What I'm saying is, we could probably rethink the granularity of this API and not expose IDENTITY/SEQUENCE as two different values. Those are just platform-specific implementation details.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 21, 2021

Why should they bother about the implementation details?

If I believe doctrine/dbal#5614 , the user might want to use the ORM in some scenarios, and use SQL in others (to perform inserts), and then it becomes more cumbersome to use a sequence than to use SERIAL.

What I'm saying is, we could probably rethink the granularity of this API and not expose IDENTITY/SEQUENCE as two different values. Those are just platform-specific implementation details.

If we do that, how will the user control if a SEQUENCE or SERIAL or in the future, GENERATED BY DEFAULT AS IDENTITY is used? Shouldn't they be able to specify it somehow? It's not only platform-specific if a platform supports multiple methods. If it does, it means either we have to take a decision and stick to it forever, or put the control in the hands of the user.

@morozov
Copy link
Member

morozov commented Aug 21, 2021

It looks like this discussion heavily overlaps with #8850 (specifically, #8850 (comment)). I agree that SERIAL should be preferred to use if it's available on a given platform since this is eventually what users want. But if the platform only supports sequences, proper implementation will make it indistinguishable from SERIAL both at the ORM and SQL levels. That's why I'm saying that at the end, the end-user shouldn't care about the implementation.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 22, 2021

@morozov are you saying that what I have done in #8931 for AUTO should also be done for IDENTITY, and that we should remove SEQUENCE? Meaning when not configured IDENTITY would always result in the situation we have right now, but that the user could configure it to mean SERIAL or SEQUENCE or GENERATED BY DEFAULT AS IDENTITY?

@morozov
Copy link
Member

morozov commented Aug 24, 2021

I probably confused SERIAL for IDENTITY in the context of PostgreSQL above.

we should remove SEQUENCE?

As an ORM-level API, yes. If a user chooses IDENTITY, the ORM will use either identity an column, if supported by the target platform, or a sequence. Whether to name this IDENTITY or AUTOINCREMENT it's up to the ORM but the latter seems to be more applicable. Whether it's an identity column or a sequence is a platform-specific implementation detail.

Meaning when not configured IDENTITY would always result in the situation we have right now, but that the user could configure it to mean SERIAL or SEQUENCE or GENERATED BY DEFAULT AS IDENTITY?

At least as an upgrade path, yes. Eventually, the ORM could just choose the default implementation for the platform.

@stof
Copy link
Member

stof commented Dec 4, 2021

To me, AUTO has a nice benefit: it is something that can be used on all platforms (as the ORM takes care of choosing a supported id generator). this is especially useful when writing open-source packages (a bunch of Symfony bundles are shipping with entity mapping) where the platform is not known by the developer of the bundle (and so they intent their mapping to work on all platforms supported by the ORM)

@stof
Copy link
Member

stof commented Dec 4, 2021

Note that if IDENTITY is the one working on all platforms by switching to sequence internally, this is also fine for that case

@norkunas
Copy link
Contributor

In one project I'm switching from MySQL to Postgres and now getting this deprecation, but none of my entities use generated ID's so a little bit strange to get this deprecation :)

@Mepcuk
Copy link

Mepcuk commented Jan 5, 2024

I have the same deprication, but how to fix it?

@adrienbrault
Copy link
Contributor

Hey, I just ran into the following deprecation in a new app:

[info] User Deprecated: Relying on non-optimal defaults for ID generation is deprecated, and IDENTITY
results in SERIAL, which is not recommended.
Instead, configure identifier generation strategies explicitly through
configuration.
We currently recommend "SEQUENCE" for "Doctrine\DBAL\Platforms\PostgreSqlPlatform", so you should use
$configuration->setIdentityGenerationPreferences([
    "Doctrine\DBAL\Platforms\PostgreSqlPlatform" => ClassMetadata::GENERATOR_TYPE_SEQUENCE,
]); (ClassMetadataFactory.php:755 called by ClassMetadataFactory.php:629, https://github.com/doctrine/orm/issues/8893, package doctrine/orm)

To fix the deprecation, I used a compiler pass to call setIdentityGenerationPreferences:

<?php

// src/Kernel.php

declare(strict_types=1);

namespace App;

use Override;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
use Doctrine\ORM\Mapping\ClassMetadata;
use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel
{
    use MicroKernelTrait;

    #[Override]
    protected function build(ContainerBuilder $container): void
    {
        $container->addCompilerPass(new class() implements CompilerPassInterface {
            public function process(ContainerBuilder $container): void
            {
                $container->getDefinition('doctrine.orm.default_configuration')
                    ->addMethodCall(
                        'setIdentityGenerationPreferences',
                        [
                            [
                                PostgreSQLPlatform::class => ClassMetadata::GENERATOR_TYPE_SEQUENCE,
                            ],
                        ]
                    );
            }
        });
    }
}

hippothomas added a commit to hippothomas/hippolyte-thomas-back that referenced this issue Jan 31, 2024
hippothomas added a commit to hippothomas/hippolyte-thomas-back that referenced this issue Feb 3, 2024
mo-bou pushed a commit to mo-bou/foodora that referenced this issue Mar 25, 2024
…ine/orm#8893]

+ Symfony.lock update (forgotten on a previous commit)
@madonzy
Copy link

madonzy commented May 8, 2024

@adrienbrault thanks it helped, but actually in my case, I could avoid it by updating DBAL to version 4:

composer require doctrine/dbal:^4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants