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

Add UUID version 7 as the default guid generator #3249

Merged
merged 9 commits into from
Sep 1, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,6 @@ public override bool TrySelect(IProperty property, ITypeBase typeBase, out Value
=> property.ClrType.UnwrapNullableType() == typeof(Guid)
? property.ValueGenerated == ValueGenerated.Never || property.GetDefaultValueSql() is not null
? new TemporaryGuidValueGenerator()
: new GuidValueGenerator()
: new UUid7ValueGenerator()
roji marked this conversation as resolved.
Show resolved Hide resolved
: base.FindForType(property, typeBase, clrType);
}
59 changes: 59 additions & 0 deletions src/EFCore.PG/ValueGeneration/UUid7ValueGenerator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
using System.Runtime.InteropServices;

namespace Npgsql.EntityFrameworkCore.PostgreSQL.ValueGeneration;

/// <summary>
/// Generates sequential <see cref="Guid" /> values according to the UUID version 7 specification.
/// Will be updated to use Guid.CreateVersion7 when available.
/// </summary>
public class UUid7ValueGenerator : ValueGenerator<Guid>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The .NET team has chosen to stick to avoid using multiple synonyms, hence Guid.CreateVersion7. For consistency, I would go with GuidVersion7ValueGenerator.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: All generators in the current package are prefixed with Npgsql. Should we follow that convention or not, @roji?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, they are marked as internal APIs. Should we rather be consistent with them or with EF Core's own GuidValueGenerator?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EF's GuidValueGenerator is fully public since it's meant to be referenceable by external providers, i.e. it's infrastructure; whereas types inside EFCore.PG aren't... So I think it's indeed better to make the generator pubternal (by moving it into the Internal namespace) - we don't want people to start using it for whatever purpose, it's just an internal implementation detail of the provider.

Given that it would be pubternal, the naming doesn't matter too much. I have a slight preference for UUID7, since that's the definition of that particular format; the .NET type which holds UUIDs (of any sort) happens to be Guid, but the actual format is UUID7 I think. But no strong feelings.

Re the Npgsql prefix, no strong feelings there either (again, it would be a pubternal type anyway). Since there's nothing particular Npgsql-specific about it - it just generates UUID7s, I'd personally leave it out.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have the prefix, for consistency with similar existing types in the package, and to reinforce the pubternal-ness some more.

{
private const byte Variant10xxValue = 0x80;
private const ushort Version7Value = 0x7000;
private const ushort VersionMask = 0xF000;
private const byte Variant10xxMask = 0xC0;

/// <summary>
/// Gets a value to be assigned to a property.
/// </summary>
/// <param name="entry">The change tracking entry of the entity for which the value is being generated.</param>
/// <returns>The value to be assigned to a property.</returns>
public override Guid Next(EntityEntry entry)
{
Span<byte> guidBytes = stackalloc byte[16];
var succeeded = Guid.NewGuid().TryWriteBytes(guidBytes);
var unixms = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds();
Span<byte> counterBytes = stackalloc byte[sizeof(long)];
ChrisJollyAU marked this conversation as resolved.
Show resolved Hide resolved
MemoryMarshal.Write(counterBytes, in unixms);

if (!BitConverter.IsLittleEndian)
{
counterBytes.Reverse();
}

//unix ts ms - 48 bits (6 bytes)
guidBytes[00] = counterBytes[2];
guidBytes[01] = counterBytes[3];
guidBytes[02] = counterBytes[4];
guidBytes[03] = counterBytes[5];
guidBytes[04] = counterBytes[0];
guidBytes[05] = counterBytes[1];

//UIDv7 version - first 4 bits (1/2 byte) of the next 16 bits (2 bytes)
var _c = BitConverter.ToInt16(guidBytes.Slice(6, 2));
_c = (short)((_c & ~VersionMask) | Version7Value);
BitConverter.TryWriteBytes(guidBytes.Slice(6, 2), _c);

//2 bit variant
//first 2 bits of the next 64 bits (8 bytes)
guidBytes[8] = (byte)((guidBytes[8] & ~Variant10xxMask) | Variant10xxValue);
return new Guid(guidBytes);
}

/// <summary>
/// Gets a value indicating whether the values generated are temporary or permanent. This implementation
/// always returns false, meaning the generated values will be saved to the database.
/// </summary>
public override bool GeneratesTemporaryValues
=> false;
}
Loading