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

Custom datetime with support of microseconds not properly works after update of dbal from 3.6 to 4.2.1 #6631

Open
bogdan-dubyk opened this issue Nov 25, 2024 · 11 comments

Comments

@bogdan-dubyk
Copy link

bogdan-dubyk commented Nov 25, 2024

Bug Report

Q A
Version 4.2.1
Previous Version if the bug is a regression 3.6

Summary

I have a custom type to support microsecond precision with Postgres

final class DateTimeImmutableMicrosecondsType extends VarDateTimeImmutableType
{
    private const FORMAT = 'Y-m-d H:i:s.u';

    /**
     * @param T $value
     *
     * @return (T is null ? null : string)
     *
     * @throws ConversionException
     *
     * @template T
     */
    public function convertToDatabaseValue(mixed $value, AbstractPlatform $platform): ?string
    {
        if (
            $value instanceof DateTimeImmutable &&
            $platform instanceof PostgreSQLPlatform
        ) {
            return $value->format(self::FORMAT);
        }

        return parent::convertToDatabaseValue($value, $platform);
    }

    /** @param mixed[] $column */
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        if ($platform instanceof PostgreSQLPlatform) {
            return 'TIMESTAMP(6) WITH TIME ZONE';
        }

        return $platform->getDateTimeTzTypeDeclarationSQL($column);
    }
}

all was good on version 3.6, doctrine migration generated the SQL like updated_at TIMESTAMP(6) WITH TIME ZONE and custom doctrine comments added 'COMMENT ON COLUMN products_data_api_schema.product.updated_at IS \'(DC2Type:datetime_immutable)\'', but now after I update a DBAL version to 4.2.1 and run migrations diff, I see doctrine trying to generate new migration

        $this->addSql('ALTER TABLE public.table ALTER updated_at TYPE TIMESTAMP(6) WITH TIME ZONE');
        $this->addSql('COMMENT ON COLUMN public.table.updated_at IS \'\'');

but type is already TIMESTAMP(6) and even if I execute that migration, doctrine again generates the same migration, and what is more interesting in the migration down section is generating ALTER TABLE public.table ALTER update_at TYPE TIMESTAMP(0) WITH TIME ZONE trying to set precision to 0 for some reason.

Current behavior

Doctrine always trying to generate migration like TIMESTAMP(6 but that type is already set

Expected behavior

Doctrine should not generate any schema changes to that field

How to reproduce

You need to have a field with type TIMESTAMP(6) in our Postgres table, have that custom doctrine type from above, and be on dbal version 4.2.1 and run doctrine:migrations:diff. Though I'm not sure, maybe it's an issue of the migrations library, but I'm on the most supported version which is 3.8.2 and did not change migration version, I only increase the dbal version

@bogdan-dubyk
Copy link
Author

bogdan-dubyk commented Nov 25, 2024

Started debugging and I can see that Doctrine\DBAL\Schema\Comparator class while comparing the current scheme with code (possible schema) return such info for problematic field
current schema

"updated_at" => Doctrine\DBAL\Schema\Column^ {#205
          #_name: "upadted_at"
          #_namespace: null
          #_quoted: false
          #_type: Doctrine\DBAL\Types\DateTimeTzType^ {#421}
          #_length: null
          #_precision: null
          #_scale: 0
          #_unsigned: false
          #_fixed: false
          #_notnull: true
          #_default: null
          #_autoincrement: false
          #_values: []
          #_platformOptions: []
          #_columnDefinition: null
          #_comment: "(DC2Type:datetime_immutable)"
        }

new schema

 "updated_at" => Doctrine\DBAL\Schema\ColumnDiff^ {#1205
        -oldColumn: Doctrine\DBAL\Schema\Column^ {#205}
        -newColumn: Doctrine\DBAL\Schema\Column^ {#992
          #_name: "updated_at"
          #_namespace: null
          #_quoted: false
          #_type: App\Types\DateTimeImmutableMicrosecondsType^ {#442}
          #_length: null
          #_precision: null
          #_scale: 0
          #_unsigned: false
          #_fixed: false
          #_notnull: true
          #_default: null
          #_autoincrement: false
          #_values: []
          #_platformOptions: array:1 [
            "version" => false
          ]
          #_columnDefinition: null
          #_comment: ""
        }
      }

so for some reason, Column diff sees the column in DB as type Doctrine\DBAL\Types\DateTimeTzType instead of my custom App\Types\DateTimeImmutableMicrosecondsType, and all the time trying to change it.

NOTE: I have custom type registered like this:

doctrine:
    dbal:
            datetime_immutable: App\Types\DateTimeImmutableMicrosecondsType

and in entity XML mapping

        <field name="updatedAt" type="datetime_immutable" column="updated_at" nullable="true"/>

So for some reason after the update of DBAL, during the read of the current schema state doctrine transformed the existing timestamp(6) field into an incorrect type which is Doctrine\DBAL\Types\DateTimeTzType and trying to set it to the correct type App\Types\DateTimeImmutableMicrosecondsType, and as output generating same SQL all the time

@derrabus
Copy link
Member

This is a known problem. DBAL does not support changing the precision of datetime fields. This is a planned feature and attempts have been made to contribute this, but so far nobody has brought it over the finish line.

If you need a custom type for your datetime fields, you will have to override the comparator to enable it to detect the case that your custom type covers. However, if you want this behavior for every datetime field in your application, you might be better off overriding the platform instead.

@bogdan-dubyk
Copy link
Author

bogdan-dubyk commented Nov 27, 2024

I do not think the issue is with my custom type, I tried with the original DateTimeTzImmutableType, and the issue is the same, here is the dump from comparison:

"updated_at" => Doctrine\DBAL\Schema\Column^ {#205
          #_name: "upadted_at"
          #_namespace: null
          #_quoted: false
          #_type: Doctrine\DBAL\Types\DateTimeTzType^ {#421}
          #_length: null
          #_precision: null
          #_scale: 0
          #_unsigned: false
          #_fixed: false
          #_notnull: true
          #_default: null
          #_autoincrement: false
          #_values: []
          #_platformOptions: []
          #_columnDefinition: null
          #_comment: ""
        }

and

 "updated_at" => Doctrine\DBAL\Schema\ColumnDiff^ {#1205
        -oldColumn: Doctrine\DBAL\Schema\Column^ {#205}
        -newColumn: Doctrine\DBAL\Schema\Column^ {#992
          #_name: "updated_at"
          #_namespace: null
          #_quoted: false
          #_type: Doctrine\DBAL\Types\DateTimeTzImmutableType^ {#442}
          #_length: null
          #_precision: null
          #_scale: 0
          #_unsigned: false
          #_fixed: false
          #_notnull: true
          #_default: null
          #_autoincrement: false
          #_values: []
          #_platformOptions: array:1 [
            "version" => false
          ]
          #_columnDefinition: null
          #_comment: ""
        }
      }

So for some reason comparison now not use Doctrine comments like 'COMMENT ON COLUMN updated_at IS \'(DC2Type:datetime_immutable)\'' but taking type directly from the platform, and of course platform see type TIMESttAMP WITH TIME ZONE, and convert it to DateTimeTzType as it can't know that I want to use immutable version or not. Though on DBAL 3 everything worked as expected even with custom type :(

@bogdan-dubyk
Copy link
Author

so if I understand it correctly it always generates migration because it sees that type in DB is Doctrine\DBAL\Types\DateTimeTzType but in mapping it's Doctrine\DBAL\Types\DateTimeTzImmutableType and trying to generate SQL with changes, but in reality it generate same SQL over and over again

@derrabus
Copy link
Member

I do not think the issue is with my custom type

Okay, but it is.

I tried with the original DateTimeTzImmutableType, and the issue is the same, here is the dump from comparison:

I'm not sure what I should be reading from these dumps, but if it's about a different type being found during introspection, this is usually fine if both types generate the same DDL.

So for some reason comparison now not use Doctrine comments

The reason is that the support for those comments has been removed. You might want to read doctrine/migrations#1441 for issues with custom types following that removal.

@bogdan-dubyk
Copy link
Author

bogdan-dubyk commented Nov 27, 2024

Okay, but it is.

I just mentioned that I removed my custom type, and still doctrine generating all the time SQL but now of course TIMESTAMP(0) WITH TIME ZONE without precision. So I remove usage of my custom type, I run diff, it altered table to remove precision, I run that migration, run diff again and it altering table again with code TIMESTAMP(0) WITH TIME ZONE that is already in database

@derrabus
Copy link
Member

Can you provide a minimal piece of code that proproduces your issue?

@bogdan-dubyk
Copy link
Author

bogdan-dubyk commented Nov 27, 2024

Entity (I'll skip all other fields)

class Product implements AggregateRoot
{
    public function __construct(
        private string $id,
        private DateTimeImmutable $updatedAt
    ) {
    }
}

mapping (same, skipping all fields)

<?xml version="1.0" encoding="UTF-8"?>
<doctrine-mapping xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                  xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping"
                  xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping
                          https://www.doctrine-project.org/schemas/orm/doctrine-mapping.xsd">

    <entity
            name="App\Entity\Product"
            table="product"
            schema="products_data_api_schema">

        <id name="id" column="id" type="string" />
        <field name="updatedAt" type="datetimetz_immutable" column="updated_at" nullable="true"/>
    </entity>
</doctrine-mapping>

at a point of migration from DBAL 3.6+ to 4.2.1 I had custom type registered, initial migration looked like this

        $this->addSql('CREATE TABLE my_schema.product (id VARCHAR(255) NOT NULL, updated_at TIMESTAMP(6) WITH TIME ZONE DEFAULT NULL, PRIMARY KEY(id))');
        $this->addSql('COMMENT ON COLUMN my_schema.product.created_at IS \'(DC2Type:datetimetz_immutable)\'');

Now I have updated to DBAL 4.2.1 and as mentioned above, also removed custom type, mapping, and entity are the same (only change is removing the custom type which I'll not describe here), running migrations:diff, it generates SQL like this

 $this->addSql('ALTER TABLE my_schema.product ALTER updated_at TYPE TIMESTAMP(0) WITH TIME ZONE');
 $this->addSql('COMMENT ON COLUMN my_schema.product.updated_at IS \'\'');

I run it and run migrations:diff, it generate the same

 $this->addSql('ALTER TABLE my_schema.product ALTER updated_at TYPE TIMESTAMP(0) WITH TIME ZONE');

@berkut1
Copy link
Contributor

berkut1 commented Nov 30, 2024

@bogdan-dubyk
I tried to reproduce the issue on my side using attributes (I’m not using XML), but I didn’t find any problems. It’s unlikely, but could this (XML) be the cause?
My test:

$this->addSql('CREATE TABLE audit_logs (
            "id"            UUID NOT NULL,
            "date"          TIMESTAMP(6) WITH TIME ZONE DEFAULT NULL,
            PRIMARY KEY(id))');
        $this->addSql('COMMENT ON COLUMN audit_logs.date IS \'(DC2Type:datetimetz_immutable)\'');
#[ORM\Table(name: "audit_logs")]
#[ORM\Entity]
class AuditLog
{
    #[ORM\Column(name: "date", type: Types::DATETIMETZ_IMMUTABLE, nullable: true)]
    private \DateTimeImmutable $date;
}

diff

public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE audit_logs ALTER date TYPE TIMESTAMP(0) WITH TIME ZONE');
        $this->addSql('COMMENT ON COLUMN audit_logs.date IS \'\'');
    }

    public function down(Schema $schema): void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE audit_logs ALTER date TYPE TIMESTAMP(0) WITH TIME ZONE');
        $this->addSql('COMMENT ON COLUMN audit_logs.date IS \'(DC2Type:datetimetz_immutable)\'');
    }

After doctrine:migrations:migrate --no-interaction I run diff again:

  No changes detected in your mapping information.                                                      

@bogdan-dubyk
Copy link
Author

bogdan-dubyk commented Dec 18, 2024

Maybe, still did not solve that, simply editing migrations all the time, no time to debug. But I also found another issue, here is the fresh version fo custom type

final class DateTimeImmutableMicrosecondsType extends VarDateTimeImmutableType
{
    private const FORMAT = 'Y-m-d H:i:s.u';

    /**
     * @param T $value
     *
     * @return (T is null ? null : string)
     *
     * @throws ConversionException
     *
     * @template T
     */
    public function convertToDatabaseValue(mixed $value, AbstractPlatform $platform): ?string
    {
        if (
            $value instanceof DateTimeImmutable &&
            $platform instanceof PostgreSQLPlatform
        ) {
            return $value->format(self::FORMAT);
        }

        return parent::convertToDatabaseValue($value, $platform);
    }

    /** @param mixed[] $column */
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        if ($platform instanceof PostgreSQLPlatform) {
            return 'TIMESTAMP(6) WITH TIME ZONE';
        }

        return $platform->getDateTimeTzTypeDeclarationSQL($column);
    }

    public function convertToPHPValue(mixed $value, AbstractPlatform $platform): ?DateTimeImmutable
    {
        if ($value === null || $value instanceof DateTimeImmutable) {
            return $value;
        }

        if ($platform instanceof PostgreSQLPlatform) {
            assert(is_string($value));

            try {
                $dateTime = DateTimeImmutable::createFromFormat([Settings::DOCTRINE_DATE_FORMAT](self::FORMAT), $value);
            } catch (Throwable) {
                throw InvalidFormat::new(
                    $value,
                    self::class,
                    [Settings::DOCTRINE_DATE_FORMAT](self::FORMAT),
                );
            }

            if ($dateTime !== false) {
                return $dateTime;
            }

            throw InvalidFormat::new(
                $value,
                self::class,
                Settings::DOCTRINE_DATE_FORMAT,
            );
        }

        return parent::convertToPHPValue($value, $platform);
    }

}

and issue is that convertToPHPValue method is not triggered when the entity manager getting data from DB and building an entity but convertToDatabaseValue is triggered.

This leads to an issue that UOW sees changes all the time. In example

$product = $this->entityManager->getRepository(Product::class)->find($id);
$this->entityManager->persiti($product)
 $this->unitOfWork->computeChangeSets();

 $this->unitOfWork->getEntityChanges($product);

will show changes like this

  "updatedAt" => array:2 [
    0 => DateTimeImmutable @1734520477 {#12690
      date: 2024-12-18 11:14:37.763499 UTC (+00:00)
    }
    1 => DateTimeImmutable @1734520477 {#13027
      date: 2024-12-18 11:14:37.763 +00:00
    }
  ]

even if there are no actual changes, and I assume it's because convertToPHPValue is not triggered and some default format without microseconds is used

@bogdan-dubyk
Copy link
Author

I tried to add dumps to other default types convertToPHPValue methods, and I see those also not triggered, so how does doctrine currently figure out how to transform value to expected PHP without triggering convertToPHPValue ?

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

No branches or pull requests

3 participants