[Uid] Add UidFactory to create Ulid and Uuid from timestamps and randomness/nodes #39507

fancyweb · 2020-12-14T22:58:58Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

Ref #36097

When you migrate an existing resource identifier to an uid, you might want to choose the timestamp so that it is coherent with the creation date of the existing resource. (eg: I have a row in a table with id=1, created_at=2018-12-11 19:00:00, I would like to use that timestamp to create the resource Ulid).

It can also be useful to choose the time of the Ulid or the node of the Uuid.

This PR provides new services and corresponding autowiring aliases and implementations:

ulid.factory <=> Symfony\Component\Uid\Factory\UlidFactory
uuid.factory <=> Symfony\Component\Uid\Factory\UuidFactory
time_based_uuid.factory <=> Symfony\Component\Uid\Factory\TimeBasedUuidFactory
random_based_uuid.factory <=> Symfony\Component\Uid\Factory\RandomBasedUuidFactory
name_based_uuid.factory <=> Symfony\Component\Uid\Factory\NameBasedUuidFactory

All factories have a create() method that returns a different kind of AbstractUid.

UuidFactory::create() returns either a time-based Uuid, or a random one. It defaults to a time-based UuidV6, because that's what is most favorable to DB indexes.

This default is configurable using semantic config. E.g. this switches to UuidV4 by default:

framework:
  uid:
    default_uuid_version: 4

Semantic configuration also allows configuring a default "node" for UuidV1/V6 and a default "namespace" for UuidV3/V5.

A UuidGenerator is also provided, that can be used to generate the "id" of entities. See also doctrine/DoctrineBundle#1284 for a sidekick PR.

By using these factories instead of creating UUIDs/ULIDs inline, one will be able to centralize the definition of e.g. the UUID version that should be used for time/name-based UUIDs, or to define the same "node" per php-fpm instance, to easily identify the origin of all time-based UUIDs, while not leaking MAC info nor reducing the uniqueness.

And of course, these factories also allow easy mocking of their return values in tests.

src/Symfony/Component/Uid/UuidV4.php

jderusse

I'm not sure for ULID, but this is exactly what address UUID V5: $predectible = Uuid::v5('my entity', $id);

src/Symfony/Component/Uid/UidFactory.php

src/Symfony/Component/Uid/Ulid.php

src/Symfony/Component/Uid/UuidV1.php

src/Symfony/Component/Uid/UuidV4.php

stof · 2020-12-16T10:12:27Z

I guess it can also be useful to choose the randomness of the Ulid

Choosing randomness does not make sense. It is not random anymore then.

src/Symfony/Component/Uid/UuidV4.php

nicolas-grekas · 2020-12-16T16:35:34Z

I think this class is going to be useful as a service (it should NOT be final btw as that kills extensibility).

What about adding the $time and $node arg to Uuid::v1()? That could be done in a separate smaller PR.

But to be useful as a service, I think the class should have a constructor, with the following arguments:

the node id (MAC-address-like) to generate UUIDv1 & UUIDv6

the default namespace to generate UUIDv3 & UUIDv5

Thinking about this, we could merge the two arguments in one: passing a single UUID to the constructor is enough to derive both a node id for v1&6 and a namespace for v3&5, WDYT?

About Ulid, I think we should remove the $randomness argument.
About V4, I don't think we should allow any arguments at all.

And maybe also as I proposed in #36097:

the time source as a callable

the entropy source as a callable

Unlike what I did in #36097, these callables should be provided only as constructor arguments - not on the methods themselves.

About this part, maybe leave it for a later PR. No need to do it unless one has the need for it.
This would be the way to take control of values for V4 and ULIDs' randomness btw.

Also, I'm wondering, what about giving meaningful names to the factory methods?

createNamed($name, $ns = null, $version = null) => defaults to v3/5 depending on a constructor arg, which itself defaults to v5
createTimed($time = null, $version = null) => defaults to v1/6 depending on a constructor arg, which itself defaults to v1 to keep max portability? or to v6 to help with DB indexes by default? (portability is an additional and uncommon concern, isn't it?) - would allow generating ULID also?
createRandom() => v4

fancyweb · 2020-12-24T11:51:39Z

I pushed a new version taking into account the various comments. Tell me what you think.

I prefer to split UidFactory and UlidFactory since the timestamp meaning is not the same.

nicolas-grekas

Thanks, I like it. Here are some more comments.

What about adding the $time and $node arg to Uuid::v1()? That could be done in a separate smaller PR.

WDYT about this one? Should be easy thanks to the new generate() methods, don't you think?

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

src/Symfony/Component/Uid/UidFactory.php

src/Symfony/Component/Uid/UlidFactory.php

src/Symfony/Component/Uid/UuidV1.php

src/Symfony/Component/Uid/UuidV6.php

src/Symfony/Component/Uid/CHANGELOG.md

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

src/Symfony/Component/Uid/Ulid.php

src/Symfony/Component/Uid/UlidFactory.php

src/Symfony/Component/Uid/UuidFactory.php

nicolas-grekas · 2021-01-27T21:59:59Z

Let's rebase on top of #40008 now

Done :)

I've replaced all float arguments by DateTimeInterface|null.
Please take over :)

@tgalopin

…s-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [Uid] Add RFC4122 UUID namespaces as constants | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Discussed with @tgalopin Will play well with #39507 Commits ------- 4e73aeb [Uid] Add RFC4122 UUID namespaces as constants

…s-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [DoctineBridge] Remove UuidV*Generator classes | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - There is no benefit in using these classes over creating the UUIDs inline. Let's keep things simple. For `UlidGenerator`, I'm keeping it as it will provide something useful once #39507 and doctrine/DoctrineBundle#1284 are finished. Commits ------- 3c296bd [DoctineBridge] Remove UuidV*Generator

src/Symfony/Component/Uid/UuidFactory.php

nicolas-grekas

After many iterations, I'm happy to say that this is ready.

src/Symfony/Bridge/Doctrine/IdGenerator/UuidGenerator.php

src/Symfony/Bridge/Doctrine/Tests/IdGenerator/UlidGeneratorTest.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Uid/BinaryUtil.php

…aces and nodes

nicolas-grekas · 2021-02-11T17:51:18Z

Thank you @fancyweb.

carsonbot added Status: Needs Review Uid Feature labels Dec 14, 2020

fancyweb force-pushed the uid/uid-factory branch from f0b803d to 8745fcb Compare December 14, 2020 22:59

fancyweb commented Dec 14, 2020

View reviewed changes

src/Symfony/Component/Uid/UuidV4.php Outdated Show resolved Hide resolved

fancyweb changed the title ~~[Uid] Add UidFactory to create Ulid and Uuid from hardcoded values~~ [Uid] Add UidFactory to create Ulid and Uuid from timestamps and randomness/nodes Dec 14, 2020

fancyweb force-pushed the uid/uid-factory branch from 8745fcb to 421431c Compare December 14, 2020 23:06

jderusse reviewed Dec 15, 2020

View reviewed changes

jderusse added this to the 5.x milestone Dec 15, 2020

nicolas-grekas reviewed Dec 16, 2020

View reviewed changes

src/Symfony/Component/Uid/UuidV4.php Outdated Show resolved Hide resolved

fancyweb force-pushed the uid/uid-factory branch 4 times, most recently from 27bffd6 to e6ec19f Compare December 24, 2020 11:46

nicolas-grekas reviewed Jan 19, 2021

View reviewed changes

fancyweb force-pushed the uid/uid-factory branch 2 times, most recently from 311c10b to c03a9c6 Compare January 22, 2021 14:01