dbal icon indicating copy to clipboard operation
dbal copied to clipboard

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

Open bogdan-dubyk opened this issue 1 year ago • 12 comments

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 avatar Nov 25 '24 15:11 bogdan-dubyk

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

bogdan-dubyk avatar Nov 25 '24 17:11 bogdan-dubyk

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.

derrabus avatar Nov 27 '24 08:11 derrabus

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 avatar Nov 27 '24 08:11 bogdan-dubyk

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

bogdan-dubyk avatar Nov 27 '24 08:11 bogdan-dubyk

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.

derrabus avatar Nov 27 '24 08:11 derrabus

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

bogdan-dubyk avatar Nov 27 '24 09:11 bogdan-dubyk

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

derrabus avatar Nov 27 '24 10:11 derrabus

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');

bogdan-dubyk avatar Nov 27 '24 10:11 bogdan-dubyk

@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.                                                      

berkut1 avatar Nov 30 '24 02:11 berkut1

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 avatar Dec 18 '24 11:12 bogdan-dubyk

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 ?

bogdan-dubyk avatar Dec 18 '24 11:12 bogdan-dubyk

For what it's worth, I decided to create a custom schema manager to intercept the _getPortableTableColumnDefinition call and choose the correct type. Hope it will help somebody: https://gist.github.com/ruudk/10fd5dc1325492df97b33682b3423f34

ruudk avatar Jan 16 '25 13:01 ruudk